ApiAnnotations : AST version of nested forall loses forall annotation
ClosedPublic

Authored by alanz on Apr 10 2015, 8:30 AM.

Details

Summary

When parsing

{-# LANGUAGE ScopedTypeVariables #-}

extremumNewton :: forall tag. forall tag1.
                   tag -> tag1 -> Int
extremumNewton = undefined

the parser creates nested HsForAllTy's for the two forall statements.

These get flattened into a single one in HsTypes.mk_forall_ty

This patch removes the flattening, so that API Annotations are not lost in the
process.

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
alanz edited the test plan for this revision. (Show Details)Apr 10 2015, 8:30 AM
alanz added a reviewer: simonpj.
alanz updated the Trac tickets for this revision.
alanz added a comment.Apr 10 2015, 8:32 AM

This version currently fails in stage 2 with

compiler/hsSyn/HsTypes.hs:71:1: Warning:
    The import of ‘Data.Monoid’ is redundant
      except perhaps to import instances from ‘Data.Monoid’
    To import instances alone, use: import Data.Monoid()

This warning is spurious. I do not understand how it originates from the change to the HsForAllTy.

simonpj requested changes to this revision.Apr 13 2015, 4:33 AM
simonpj edited edge metadata.

I think we previously had a (perhaps undocumented) invariant that HsForAllTy was never immediately nested. Have we lost that invariant? Why?

compiler/hsSyn/HsTypes.hs
555

Comment!!!!!

705

Ugh. How can this nested situation happen? What if there's an HsPar in the middle?

compiler/typecheck/TcHsType.hs
192 ↗(On Diff #2719)

How can this nested situation happen? At very least you should use splitLHsForAllTy

This revision now requires changes to proceed.Apr 13 2015, 4:33 AM
alanz updated this revision to Diff 2738.Apr 13 2015, 8:58 AM
alanz edited edge metadata.

The Monoid import issue is resolved, but now getting bad warning/errors in the tests.

alanz updated this revision to Diff 2867.May 4 2015, 2:21 PM
alanz edited edge metadata.

Sorted out remaining problems, now passes all tests

alanz marked an inline comment as done.May 4 2015, 3:00 PM

The phab tests now pass, I would appreciate feedback on this.

compiler/hsSyn/HsTypes.hs
542–543

The f :: forall a. ((Num a) => Int) signature already generates a nested HsForAllTy in GHC 7.10.1

(SigD
   (TypeSig
      [ L rnfail058.hs:4:1  { VarName = f } ]
      (L rnfail058.hs:4:6-31
         (HsForAllTy
            Explicit
            Nothing
            (HsQTvs [] [ L rnfail058.hs:4:13 (UserTyVar  { TvName = a }) ])
            (L rnfail058.hs:4:17-23
               [ L rnfail058.hs:4:17-23
                   (HsParTy
                      (L rnfail058.hs:4:18-22
                         (HsAppTy
                            (L rnfail058.hs:4:18-20 (HsTyVar  { TcClsName = Num }))
                            (L rnfail058.hs:4:22 (HsTyVar  { TvName = a })))))
               ])
            (L rnfail058.hs:4:28-30 (HsTyVar  { TcClsName = Int }))))

So I think the comment is outdated snd should be removed.

compiler/typecheck/TcHsType.hs
192 ↗(On Diff #2867)

I'm not sure how I would apply splitLHsForAllTy here.

alanz updated this revision to Diff 2868.May 4 2015, 3:31 PM

Added test from D833

alanz updated the Trac tickets for this revision.May 5 2015, 1:38 PM
alanz updated this revision to Diff 2902.May 8 2015, 7:56 AM

Rebase against master

alanz updated this revision to Diff 2920.May 11 2015, 2:21 PM

Final preparation.

  1. Rebase master
  2. Remove outdated comment
  3. use splitLHsForAllTy in tc_inst_head
alanz updated this revision to Diff 2921.May 11 2015, 2:30 PM

Restore test accidentally merged out

alanz updated this revision to Diff 2923.May 11 2015, 2:43 PM

Explain that a nested Qualified HsForAllTy becomes Implicit when flattened
to the top level.

simonpj requested changes to this revision.May 13 2015, 4:58 AM
simonpj edited edge metadata.

Various bits unclear ot me

compiler/hsSyn/HsTypes.hs
555

Comment still not good. The point is that mkImplicitHsForAllTy is called when we encounter

f :: type

and then we want to wrap around a HsForallTy if one is not there already.

705

Again this nested situation is obsure to me. Needs a decent commetn or a clean up

compiler/parser/RdrHsSyn.hs
641

What is going on here. It looks ugly. At least it needs a serious comment; preferably a clean up

compiler/rename/RnTypes.hs
137

What are all these nested cases doing? At least it needs a major comment; preferably a fix.

This revision now requires changes to proceed.May 13 2015, 4:58 AM
alanz updated this revision to Diff 2937.May 13 2015, 1:06 PM
alanz edited edge metadata.
alanz marked 4 inline comments as done.

Update in response to the latest comments from @simonpj.

alanz added a comment.May 13 2015, 2:01 PM

An update referencing the note from all affected locations is coming soon...

alanz updated this revision to Diff 2939.May 13 2015, 2:13 PM
alanz edited edge metadata.

Reference [Nested HsForAllTy] from all affected locations

alanz updated this revision to Diff 2944.May 15 2015, 3:11 AM

Rebase against master now that it builds again

simonpj requested changes to this revision.May 15 2015, 6:50 AM
simonpj edited edge metadata.

I'm still bamboozled. More comments above

compiler/hsSyn/HsTypes.hs
699

Can you say exactly what this function is supposed to do, in the presence of nested HsPar, HsForAllTy?

708

This is terribly puzzling to me. Why only for Explicit? Why only for an empty outer ctx field? Why do you simply discard the tvs of the inner HsForAllTy? What if there is further nesting?

To me this seems deeply fragile and dependent on unstated assumptions about the input HsType.

728

What is going on here? The Note seems irrelevant. After all, there was NO HsForAllTy handling here before. Why only Implicit?

Moreover, the HsPar case seems entirely redundant, as is the nested pattern match. Probably just

HsForAllTy Implict _ _ _ ty -> checkl ty args

would do.

compiler/parser/RdrHsSyn.hs
641

Surely you should use splitHsForAllTy (or something like it) here, to avoid this horrible repeated nested pattern matching?

compiler/rename/RnTypes.hs
137

Could you use splitHsForAllTy (or something like it) here, to avoid this horrible repeated nested pattern matching?

This revision now requires changes to proceed.May 15 2015, 6:50 AM
alanz updated this revision to Diff 2955.May 17 2015, 9:18 AM
alanz edited edge metadata.

Rework to use a function to flatten the top level HsForAllTy

alanz updated this revision to Diff 2956.May 17 2015, 10:43 AM
alanz edited edge metadata.

Add back the tests. Still a problem with mkGadtDecl

alanz updated this revision to Diff 2959.May 18 2015, 5:11 AM

Move GADT decl parens issue to Trac #10399

alanz added a comment.May 18 2015, 5:56 AM

This is now ready for review. It is a lot cleaner than before, the main effect is that the flattening is deferred to the renamer, so the ParsedSource reflects the code as parsed, and from the RenamedSource on it is as before.

There is one niggle with extra parens in a GADT declaration, but I have kicked it out to Trac #10399 as it is not directly related to this patch.

simonpj requested changes to this revision.May 20 2015, 3:12 AM
simonpj edited edge metadata.

I think there's an easy improvement to be had here. Instead of flattenHsForAllTy :: HsType -> HsType, write splitHsForAllTy :: HsType -> Maybe (tyvars, extra, ctxt, type), which is a smart pattern-matcher. It has all the same logic as flattenHsForAllTy.

Then in the cases you need it, instead of having rnTyKi and rnTyKi', you can just say

rrnTyKi ty
  | Just (tvs, extra, ctxt, ty) <- splitHsForAllTy ty
 = ....the forall case...

This pattern is used over and over in the compiler, and saves having to use the concept of "a HsType that has been flattened at top level", which the true argument type of rnTyKi' at the moment.

Easy to do.

Simon

compiler/hsSyn/Convert.hs
248

Why are you specifically NOT calling mkImplicitHsForAllTy here? I suppose it's unnecessary, but still.

compiler/hsSyn/HsTypes.hs
568–569

In what way smart? I think it precisely and only deals with the "extra-constraints wildcard", right?

580

Please, I beg you, write a comment to explain what this function does. What invariants hold about its result?

599

I'm 99% sure that this call to mkHsForAllTy is redundant and misleading. The 'ctxt' in a HsForAllTy is "clean" in the sense that is has no wildcards. (This should be a comment on the HsForAllTy data constructor.) So mkHsForAllTy is guaranteed not to do anything here.

This revision now requires changes to proceed.May 20 2015, 3:12 AM
alanz marked 3 inline comments as done.May 20 2015, 2:38 PM

I can understand the splitHsForAllTy function for general use, but I am
not sure how using it in a guard would help converge rnTyKi and
rnTyKiForAll.

The reason I broke it out is that once the HsForAllTy is flattened
rnTyKiForAll pattern matches on the Explicit,Qualified and Implicit
cases and does slightly different processing for each. This has the
indirect effect of documenting in the code what the differences between
these three cases is.

The body of the guarded rnTyKi case would have to do the three different
kinds of processing, so would effectively be the same as it is now, or
would be a murkier piece of code to manage all the cases as one.

In D836#24368, @alanz wrote:

I can understand the splitHsForAllTy function for general use, but I am
not sure how using it in a guard would help converge rnTyKi and
rnTyKiForAll.

Well you could just say

rnHsTyKi ty
  | Just (exp_flag, extra, tvs, cxt, ty) <- flattenHsForAllTy ty
  = rnHsForAllTyKi exp_flag extra tvs cxt ty

and away you go with three equations for rnHsForAllTyKi

The reason I broke it out is that once the HsForAllTy is flattened
rnTyKiForAll pattern matches on the Explicit,Qualified and Implicit
cases and does slightly different processing for each. This has the
indirect effect of documenting in the code what the differences between
these three cases is.

The body of the guarded rnTyKi case would have to do the three different
kinds of processing, so would effectively be the same as it is now, or
would be a murkier piece of code to manage all the cases as one.

alanz added a comment.May 21 2015, 2:49 AM

I think there are two different things going on here.

In my mind this patch is purely about moving the top level flattening of an HsForAllTy from the parser to the renamer. This means applying it in just two places, so that the renamed source is then the same as it was before this patch. It needs to be applied in only these two places because if it is applied every time a HsForAllTy is referenced it flattens too much, and leads to errors in compilation.

There is a secondary concern relating to the breaking apart of HsForAllTys further on in the compilation pipeline. Since the flattening has already taken place during renaming, the same function should not form the basis for it.

compiler/hsSyn/Convert.hs
248

mkImplicitHsForAllTy no longer takes a context as a parameter.

simonpj accepted this revision.May 21 2015, 4:05 AM
simonpj edited edge metadata.

I didn't really understand that; but never mind... just go ahead and commit. I can always refactor a bit if necessary. It's basically fine.

Simon

This revision is now accepted and ready to land.May 21 2015, 4:05 AM
alanz updated this revision to Diff 2974.May 21 2015, 6:33 AM
alanz edited edge metadata.

Minor fixes as requested by @simonpj and rebase

This revision was automatically updated to reflect the committed changes.