Work in Progress
Needs RevisionPublic

Authored by johnleo on Sep 28 2017, 11:54 AM.

Details

Reviewers
goldfire
bgamari
alanz
austin
Trac Issues
#14268
Summary

Current state for review with Richard Eisenberg

johnleo created this revision.Sep 28 2017, 11:54 AM
johnleo updated this revision to Diff 14184.Sep 28 2017, 12:00 PM

Adding previous commits.

alanz added inline comments.Sep 28 2017, 3:48 PM
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.

johnleo added inline comments.Sep 28 2017, 5:28 PM
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.

bgamari requested changes to this revision.Oct 3 2017, 10:12 AM

Glad to see you back, @johnleo! Bumping out of review queue for now. Let me know if there's any way I can help.

This revision now requires changes to proceed.Oct 3 2017, 10:12 AM
johnleo updated this revision to Diff 14532.Nov 1 2017, 7:18 PM
  • save work before merge
  • type families work
  • parse RULE forall
  • ready for second review
bgamari requested changes to this revision.Nov 2 2017, 10:51 AM

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.

This revision now requires changes to proceed.Nov 2 2017, 10:51 AM

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 {

johnleo marked 9 inline comments as done.Nov 3 2017, 3:37 PM

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.

goldfire added inline comments.Nov 3 2017, 4:11 PM
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.

austin resigned from this revision.Nov 9 2017, 5:40 PM

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

johnleo marked 5 inline comments as done.Nov 10 2017, 7:09 AM

What's the status on this, John? It looks nearly there.

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.

goldfire added inline comments.Nov 10 2017, 9:48 AM
compiler/rename/RnSource.hs
749

Looks good to me. Go with that.

johnleo updated this revision to Diff 14701.Nov 15 2017, 5:17 PM
johnleo marked 7 inline comments as done.
  • 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.

johnleo marked an inline comment as done.Nov 15 2017, 5:34 PM
bgamari requested changes to this revision.Jan 18 2018, 5:22 PM

Bumping out of review. Let me know if you need anything from me, @johnleo.

This revision now requires changes to proceed.Jan 18 2018, 5:22 PM