Associate pattern synonyms with types in module exports
ClosedPublic

Authored by mpickering on Sep 20 2015, 7:16 PM.

Details

Summary

This patch implements Trac #10653.

It adds the ability to bundle pattern synonyms with type constructors in export lists so that users can treat pattern synonyms more like data constructors.

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

@gridaphobe, I do not understand your comments to view the problem as a satisfiability problem. It may well be the right solution but when it comes to an implementation I am not sure at all what you are suggesting. Are you suggesting calling emitImplication directly with the kind of implications you are describing?

I have started a page with some specification of which synonyms should be allowed to be associated with which types. It would be useful if you could both read and improve the section if you think anything is unclear. There are some examples in a later section as well.

https://ghc.haskell.org/trac/ghc/wiki/PatternSynonyms/AssociatingSynonyms#Typing

@gridaphobe, I do not understand your comments to view the problem as a satisfiability problem. It may well be the right solution but when it comes to an implementation I am not sure at all what you are suggesting. Are you suggesting calling emitImplication directly with the kind of implications you are describing?

I'm suggesting the satisfiability problem as a guiding principle for how to decide if a pattern can be associated with a type. It may or may not be the right implementation strategy, but it gives a crisp specification, ie given

module M ( T(P) ) where

data T
pattern P :: ProvTheta => ReqTheta => ... -> T_p

the associated export T(P) is valid iff ReqTheta => T ~ T_p is satisfiable.

I believe this encompasses the three cases you've listed in the typing section of the wiki, but it doesn't deal with any gritty details of what form T or T_p might take, thus it's easier to convey to others (including our future selves!). It's also nice to have a high-level specification so you can describe the implementation as a refinement of the spec, and thus build confidence in the implementation.

But there are still some issues to resolve with this specification, in particular how do we deal with type classes and open type families? Do we want to allow associations when there's an insoluble class constraint or an irreducible type family application (both of which could be solved by declaring an instance in a downstream module)? I'm inclined to say no, we should only work with the context of the current module. But you might argue otherwise.

I have started a page with some specification of which synonyms should be allowed to be associated with which types. It would be useful if you could both read and improve the section if you think anything is unclear. There are some examples in a later section as well.

https://ghc.haskell.org/trac/ghc/wiki/PatternSynonyms/AssociatingSynonyms#Typing

Thanks, it's great to have so many examples! I think we definitely need a guiding principle for the Typing section, the current separation of cases is far too gritty for an introduction in my opinion. Better would be an example showing why we even care about typing the associated export, e.g.

module M ( T( P, Q ) ) where

data T
pattern P :: T
pattern Q :: S

Associating P with T is fine, but Q should not be associated with T since Q :: S. Then present the guiding principle for determining the validity of an associated export. Then you can examine specific cases and describe how to handle them, and how they relate to the guiding principle.

Also, I would probably present the examples alongside the specification (at least a few of them), e.g.

# Specification
## Export
...
### Example
## Import
...
### Example
## Typing
...
### Example
mpickering updated this revision to Diff 4354.Sep 30 2015, 9:29 AM
  • Add another failing test
  • Implement Simon's more lax suggestion
mpickering updated this revision to Diff 4355.Sep 30 2015, 9:46 AM
  • Cleanup and validate
mpickering updated this revision to Diff 4357.Sep 30 2015, 11:39 AM
  • Add note about typing exports
  • Update failing tests
  • Small improvement to error message
mpickering updated this revision to Diff 4360.Sep 30 2015, 5:36 PM
  • Make the test work when running on multiple cores
  • Mark two tests as broken
  • whitespace

@bgamari, I think the Parent datatype isn't too bad. It's only purpose is to say which Names can be exported with which types, if a name corresponds to a pattern synonym then it can be associated with any constructor (during renaming) or to put it another way, it can have any parent. So maybe renaming the PatternSynonym constructor to AnyParent would be better?

Matthew, Ben says you are stuck on this, and need help from me. What exactly are you stuck on?

For myself, I'm still trying to be sure that I understand the specification (only then can I really understand the code). Have you come to a consensus about the typing part? The "typing" part of the spec looks incomplete.

Thanks

Simon

@bgamari, I think the Parent datatype isn't too bad. It's only purpose is to say which Names can be exported with which types, if a name corresponds to a pattern synonym then it can be associated with any constructor (during renaming) or to put it another way, it can have any parent. So maybe renaming the PatternSynonym constructor to AnyParent would be better?

Yeah, on re-reading the code it actually does seem rather appropriate. I retract my original statement.

Matthew, Ben says you are stuck on this, and need help from me. What exactly are you stuck on?

For myself, I'm still trying to be sure that I understand the specification (only then can I really understand the code). Have you come to a consensus about the typing part? The "typing" part of the spec looks incomplete.

Thanks

Simon

I think this patch is pretty much finished now. I tried to implement what you suggested on the ticket, I can update the wiki to match later today.

The patch which is truly in limbo is D1152 so please take a look at that rather than this if you only have a limited amount of time.

I think this patch is pretty much finished now.

I'm still concerned about the spec for the typing check. I've updated the wiki page. In short, I think we should do the really easy (and less expressive) thing and just allow a pattern synonym to be associated when the heads are identical.

Has anyone reviewed this patch in depth? I haven't -- just looked at type stuff.

Btw this now implements the simpler idea that both Simon and Richard proposed. I also need to update the wiki to reflect that.

I request that we don't call this feature "associated pattern synonyms", as I'd like to reserve that for the not-yet-specified-or-implemented feature of associating pattern synonyms with classes. This would be in parallel to associated type families. Instead, how do you feel about "bundled pattern synonyms"?

How does this interact with D1152? Specifically, will you be able to bundle pattern synonym record selectors? With what syntax?

This will also need a nice section in the manual.

Once again, I found myself concentrating on the type-checking parts. I know next to nothing about imports/exports, so I leave it to others there.

compiler/hsSyn/HsImpExp.hs
169

So the syntax for an IEThing with parentheses following it is to have a sequence of names, with 0 or 1 instances of .. in it? It definitely seems simpler to have IEThingWith [Located name] Bool where the Bool says whether or not there's a ... The API Annotations stuff can store the location of the .., I think.

compiler/parser/Parser.y
637–638

Does this do all the API Annotations stuff for round-tripping source code? It doesn't look like it to me, but I'm not an expert.

compiler/parser/RdrHsSyn.hs
1352

Do we want a new language extension here? I favor a new extension. Perhaps ask around?

Why a new extension? This might make sense in the absence of pattern synonyms. Right now, record selectors are bundled with datatypes. If a refactoring made a datatype lose its record selectors, the library author might still want to provide function accessors that are bundled. No pattern synonyms in sight. And it would be sad to need -XPatternSynonyms here.

compiler/typecheck/TcRnDriver.hs
2160

I think you can issue a pretty good error message here -- you know the mismatching tycons. By avoiding unifyType, you can then dispense with the captureConstraints and simplifyTop stuff, which is a Good Thing.

I request that we don't call this feature "associated pattern synonyms", as I'd like to reserve that for the not-yet-specified-or-implemented feature of associating pattern synonyms with classes. This would be in parallel to associated type families. Instead, how do you feel about "bundled pattern synonyms"?

How does this interact with D1152? Specifically, will you be able to bundle pattern synonym record selectors? With what syntax?

This will also need a nice section in the manual.

Once again, I found myself concentrating on the type-checking parts. I know next to nothing about imports/exports, so I leave it to others there.

D1325 provides the updated documentation.

D1152, I intend that you will be able to bundle pattern synonym record selectors in the same way. The syntax stays the same, the implementation changes very little. You just need to make sure that record synonym selectors get the right Avail.

It is interesting you propose a separate extension - it seems a sensible idea but I don't want the scope to creep too much. I have little time for specifying and implementing at the moment.

compiler/hsSyn/HsImpExp.hs
169

I agree that is simpler when working in the compiler. However, it would not work for ghc-exactprint as storing the location of .. doesn't tell you which position (1st, 2nd, 3rd etc) it was original in.

For example, with your scheme it is impossible to perform a refactoring which normalises import lists by placing the wildcard in the first position. With my scheme that is very easy to do.

compiler/parser/Parser.y
637–638

I think so but anyway, I am the one who did the roundtrip testing and will be the one doing it again before 8.0 so I can fix it up if it turns out to be wrong.

compiler/typecheck/TcRnDriver.hs
2160

Good suggestion. A month later, I can see that giving a custom error message will be much clearer.

I wasn't suggesting a larger scope. Just room to grow if we decide to later on. -XBundles, anyone?

For record selectors, let's look at a concrete example:

module A (???) where

data T = MkT { a :: Bool }
pattern P :: Bool -> T
pattern P { b } = MkT b

Which of the following are legal replacements for ???:

  1. T(P)
  2. T(P, b)
  3. T(P(b))
  4. T(b)

I think (1), (2), and (4) make sense, but (4) is a bit odd. I still think it would need to be there for full parallelism with how normal record selectors work.

compiler/hsSyn/HsImpExp.hs
169

It's not impossible. You have the locations of all the names, the locations of the commas, and the location of the ... It's not terribly hard to figure it all out. But it's annoying.

Here's a compromise: Instead of storing a Bool, we could store a Maybe Int, where the number denotes the 0-indexed element in the list before which the .. appears. So (A, .., B) would have an index of 1 and (A, B, ..) would have an index of 2. It still all seems much simpler having just one disjunct of IEThing in place of 3 and storing the information about .. directly instead of having a [Located (Maybe name)] with an invariant that no more than one element of the list is Nothing.

compiler/parser/Parser.y
637–638

Ah. Good to know.

mpickering updated this revision to Diff 4803.Oct 30 2015, 2:56 PM
  • partial merge
  • Fix merge
mpickering updated this revision to Diff 4835.Oct 31 2015, 8:20 PM
  • partial merge
  • Fix merge
  • Rebase
  • Refactor to just use IEThingWith
  • Add test
  • Use custom error message

I just think I need to fix the parser now to pass through annotations information correctly and then it should be fine.

mpickering updated this revision to Diff 4837.Oct 31 2015, 8:53 PM
  • Pass around annotation info
mpickering updated this revision to Diff 4848.Nov 1 2015, 4:52 AM
  • partial merge
  • Fix merge
  • Rebase
  • Refactor to just use IEThingWith
  • Add test
  • Use custom error message
  • Pass around annotation info
mpickering updated this revision to Diff 4857.Nov 1 2015, 10:59 AM
  • Update haddock submodule
  • update tests
mpickering updated this revision to Diff 4956.Nov 6 2015, 1:49 PM
  • partial merge
  • Fix merge
  • Rebase
  • Refactor to just use IEThingWith
  • Add test
  • Use custom error message
  • Pass around annotation info
  • Update haddock submodule
  • update tests
mpickering updated this revision to Diff 4957.Nov 6 2015, 2:40 PM
  • Update submodule
mpickering updated this revision to Diff 4958.Nov 6 2015, 2:45 PM
  • Update submodule
mpickering updated this revision to Diff 4994.Nov 9 2015, 5:22 PM
  • partial merge
  • Fix merge
  • Rebase
  • Refactor to just use IEThingWith
  • Add test
  • Use custom error message
  • Pass around annotation info
  • Update haddock submodule
  • update tests
  • Update submodule
  • Cleanup and typecheck record sels
  • Formating
  • Cleanup

If this validates then I think it is ready to merge.

mpickering updated this revision to Diff 4996.Nov 9 2015, 5:31 PM
  • Fix build

There's still nothing in the manual about this. And have we agreed to just lump this into -XPatternSynonyms? (I'm still partial to a new extension -XBundles but won't try hard to defend this.)

And I don't think anything in this patch has been reviewed except the type-checking stuff... and I just don't know enough about imports/exports to be an effective reviewer of that stuff.

There's still nothing in the manual about this. And have we agreed to just lump this into -XPatternSynonyms? (I'm still partial to a new extension -XBundles but won't try hard to defend this.)

And I don't think anything in this patch has been reviewed except the type-checking stuff... and I just don't know enough about imports/exports to be an effective reviewer of that stuff.

The documentation is in D1325.

I submitted the patch nearly two months ago now. In that time, the only bit which has significantly changed is the typechecking. I don't have an infinite amount of time to keep waiting for reviews and making small changes, Really I think ample time has passed for people to make comments, it seems Ben did a review on Sept 21st (at least in part) and there has been a lot of other discussion on the ticket. It is incredibly frustrating to be asked to make endless changes to a revision weeks later. I understand people are busy but more clarity about the status of patches would be much appreciated. I was certainly under the impression that this was going to be the last round of review but that doesn't appear to be the case.

mpickering updated this object.Nov 9 2015, 7:48 PM
mpickering updated the Trac tickets for this revision.
bgamari accepted this revision.Nov 11 2015, 3:48 AM

I understand people are busy but more clarity about the status of patches would be much appreciated. I was certainly under the impression that this was going to be the last round of review but that doesn't appear to be the case.

I'm sorry to hear it has been so frustrating! I hope we can do better in the future. These more subtle patches have indeed posed a challenge for us; I know I, for one, am quite guilty of sitting on reviews for far too long (and Phabricator's automatic persistence of in-progress comments perhaps makes this quite easy). We'll strive for faster turn-arounds going forward.

Thanks again @mpickering!

This revision is now accepted and ready to land.Nov 11 2015, 3:48 AM
This revision was automatically updated to reflect the committed changes.

I understand people are busy but more clarity about the status of patches would be much appreciated. I was certainly under the impression that this was going to be the last round of review but that doesn't appear to be the case.

I'm sorry to hear it has been so frustrating! I hope we can do better in the future. These more subtle patches have indeed posed a challenge for us; I know I, for one, am quite guilty of sitting on reviews for far too long (and Phabricator's automatic persistence of in-progress comments perhaps makes this quite easy). We'll strive for faster turn-arounds going forward.

Thanks again @mpickering!

I am partly to blame here, but I have been utterly submerged, and I'm content that Ben has gone ahead and committed without me. I still want to go back to look at it, mind you, but I should not be the blocking factor unless I specifically say "please wait till I've looked at this".

Simon