Current state for review with Richard Eisenberg
Diff Detail
- Repository
- rGHC Glasgow Haskell Compiler
- Branch
- forall
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
compiler/parser/Parser.y | ||
---|---|---|
1172 | Once this all stabilises, you will need to also put in mj AnnForall $1 and mj AnnDot $3. And similar for all other places where the new keywords appear. |
compiler/parser/Parser.y | ||
---|---|---|
1172 | Thanks! I talked with Richard Eisenberg this afternoon (I created this diff so we'd have something to look at together) and he pointed out the same thing. |
Glad to see you back, @johnleo! Bumping out of review queue for now. Let me know if there's any way I can help.
Would you like a review of this @johnleo? I'd be happy to say what I can, but it would be great to have an up-to-date summary of the differential.
Bumping out of the review queue pending further instructions.
Thanks @bgamari for the offer! Sorry I wasn't clear but this is still a work in progress and I posted the latest diff to give Richard a chance to see it and comment. However I'm happy to get feedback from anyone else too. I'll be clear when it's ready for "official" review. The hope is to get this done in time to make it into 8.4.
This is looking great. I've made a bunch of comments throughout, but this is surely working in the right direction.
compiler/deSugar/DsMeta.hs | ||
---|---|---|
582 | To do so, you will have to update the Template Haskell AST. | |
compiler/hsSyn/Convert.hs | ||
695 | When we're done fixing other bugs, you should augment the TH AST to handle the new foralls, and then you'll have something to put here other than Nothing. | |
compiler/hsSyn/HsDecls.hs | ||
1567 | The pprHsForAllTvs function does not print forall . when the list is empty. But we should in this case. Looking at this all a little further, I think pprHsForAllTvs is just wrong. It should always print the forall. The function is called in two other places: the ppr_mono_ty case for HsForAllTy, where the user has written a forall (so we should print it), and pprHsForAllExtra. The latter function takes a [LHsTyVarBndr pass] -- but it should actually take a Maybe [LHsTyVarBndr pass], solely to distinguish the forall . case from printing nothing. I think you'll find that propagating this change through to be easy. | |
1856 | This datatype needs more documentation. Specifically, there needs to be a comment that says that after typechecking, all RuleBndrs are made with the RuleBndr constructor, never the RuleBndrSig constructor. You also may want to describe that the quantified tyvars move from your new field to the list of RuleBndrs. I was originally thinking that the type-checked tyvar binders should stay in your new field, but there's no support for a LHsTyVarBndr Id -- the type checker just spits out TyVars. (I want to change this, eventually.) | |
1861 | Clarify that these are the type variables. The next field also contains forall'd binders. You may also want to include a link to the accepted proposal, because someone unfamiliar with that proposal may find the double-forall odd. | |
compiler/parser/Parser.y | ||
1178 | That's a question for @alanz. There are indeed tests in the testsuite about these. | |
1615 | I'm not sure. It might be due to the shift/reduce error, which means that the parser will take a guess somewhere, and perhaps it's taking the wrong guess. More generally, I think this could be refactored to be cleaner. But the double-forall rules in rule_forall, not in rule. Also, why use tv_bndrs1 instead of tv_bndrs? Is it bad to have an empty list? (Maybe a user just wants to make it very clear that the other list is term-level variables.) And I'm surprised the forall . case for term-level binders has to be treated specially. Could rule_var_list be modified to allow an empty list of term-level binders? There's a chance that the refactoring would get rid of the misbehavior, but I don't know happy well enough to be sure. | |
compiler/rename/RnSource.hs | ||
701 | Not your doing, but please document that an empty list of patterns is possible, but that the loc won't be used in this case. | |
713 | I wrote Maybes.whenIsJust :: Monad m => Maybe a -> (a -> m ()) -> m () just for this purpose. | |
736 | This whole separation of the payload vars from other vars seems suspect. For example, why reassign the location to the pattern variables but not the payload variables? I think that's wrong. Instead, this would seem cleaner if we arrange to have pat_vars contain variables from both the LHS and RHS, and then just call newTyVarNameRn (with reassigned locations) once. As it is, I'm left wondering if there is some difference in the treatment of LHS vars and RHS vars (there isn't). To be clear, this problem also isn't yours, but it distracted me when trying to understand your code. | |
749 | This would mean that type instance forall a. F Int = Bool wouldn't get a warning. I think we do indeed want a warning here, so you should pass a Just (text "...") here. | |
751 | Isn't this forall_kity_vars? | |
798 | This error is what we want (so I do think nonquant_var_names is fine here) -- but it should be rephrased. | |
808 | I personally would find mb_bndrs *> Just bndrs' easier to read. | |
830 | Type family default equations can mention only type variables, not type patterns on the LHS. So they're much simpler. See https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/glasgow_exts.html#assoc-decl-defs But, bizarrely, the type variables may be different from the variables used in the class head, so we still need to allow the possibility of an explicit forall. So you'll have to update this function, but it shouldn't be nearly as complex as what you've already done. | |
compiler/typecheck/TcTyClsDecls.hs | ||
1259 | This function needs to bring the bndrs into scope if the constructors are in H98 syntax. | |
1408 | Perhaps tcExplicitTKBndrs should check for an empty list and do the simple thing in that case. Might be an overall speedup elsewhere, too. | |
1414 | No need for the do { |
Thanks for the excellent comments. I've handled some of the easier changes (but not pushed the code yet) and answered a couple questions, but no further feedback is needed at this point.
compiler/rename/RnSource.hs | ||
---|---|---|
749 | As noted in the comment on lines 707-708, I did this to be consistent with type signatures. For example data D a where MkD :: forall x . D Int also gives no warning. Or should we give a warning in both cases? | |
751 | No, that has type [Located RdrName] whereas we need type [LHsTyVarBndr GhcPs]. | |
798 | Please check if the new error message is okay. | |
compiler/typecheck/TcTyClsDecls.hs | ||
1408 | I checked and the overhead of calling with an empty list is quite minimal so seems okay to leave as is. |
compiler/rename/RnSource.hs | ||
---|---|---|
749 | I think we should give the warning (assuming the user has enabled the relevant warning flag) in both cases, yes. |
What's the status on this, John? It looks nearly there.
compiler/hsSyn/HsDecls.hs | ||
---|---|---|
1857 | Please can we use record field names for this data constructor, both in the declaration and in pattern matching? I know it's orthogonal to this patch but it's a good moment to do it. Make it a separate patch if you prefer |
I will push in more code changes today. I'm very close to done with all the non-RULE work, but there's still a lot related to RULE that needs to be done. I'm not sure I can finish it by the 8.4 freeze.
compiler/hsSyn/HsDecls.hs | ||
---|---|---|
1857 | Sure, I will do this as part of this work. | |
compiler/rename/RnSource.hs | ||
749 | You're right that type signatures do in fact give a warning if the flag is enabled. I've fixed this for type instances but am not sure the message is good enough. Here's an example output: Unused quantified type variable ‘(m ∷ Nat)’ In the type family instance with LHS ‘∀ (m ∷ Nat) (n ∷ Nat). Plus 'Zero n’ | 31 | type instance forall (m ∷ Nat)(n ∷ Nat). Plus 'Zero n = n | ^^^^^^^^^ I don't output the RHS since it's not known to be outputable (we could add a constraint) but on the other hand the variable has to be used in the LHS so maybe this is better anyway. |
- save work before merge
- type families work
- parse RULE forall
- ready for second review
- save work before merge
- handle multiple foralls in a rule
- fix merge problems
- further work
Further changes added for Richard's review, but I need to talk with him directly about a few issues.
compiler/parser/Parser.y | ||
---|---|---|
1615 | Richard and I discussed this via email, and I decided on a simple solution to allow any number of foralls in the parser (this will be useful later on when that is officially allowed), and let the renamer do the work to ensure there is exactly one forall with only term variables or two foralls the first with type and the second with term variables. This fixes the parser issues. |