[TTG: Handling Source Locations] Foundation and Pat
Needs ReviewPublic

Authored by shayan-najd on Aug 2 2018, 4:28 AM.

Details

Reviewers
bgamari
alanz
simonpj
Trac Issues
#15495
Summary

This patch removes the ping-pong style from HsPat (only, for now), using the plan laid out at https://ghc.haskell.org/trac/ghc/wiki/ImplementingTreesThatGrow/HandlingSourceLocations (solution A).

  • the class HasSrcSpan, and its functions (e.g., cL and dL), are introduced
  • some instances of HasSrcSpan are introduced
  • some constructors L are replaced with cL
  • some patterns L are replaced with dL view pattern
  • XPat is renamed to NewPat
  • some type annotation are necessarily updated updated (e.g., Pat p --> Pat (GhcPass p))
  • (there was a bug in an earlier version of this patch related to using functor on Located things that is fixed)
Test Plan
  • GHC and the related code (e.g., Haddock) fully compile on my Linux system
  • the patch passes the tests and ./Validate
shayan-najd created this revision.Aug 2 2018, 4:28 AM
shayan-najd retitled this revision from [TTG: Handling Source Locations] - introduced class `HasSrcSpan`, and its functions (e.g., `cL` and `dL`) - instances of `HasSrcSpan` is introduced - some constructors `L` replaced with `cL` - some patterns `L` replaced with `dL` - `XPat` renamed... to [TTG: Handling Source Locations] Foundation and Pat.Aug 2 2018, 4:34 AM
shayan-najd edited the summary of this revision. (Show Details)
shayan-najd edited reviewers, added: simonpj; removed: goldfire.
shayan-najd edited the summary of this revision. (Show Details)Aug 2 2018, 4:59 AM
shayan-najd removed subscribers: goldfire, rwbarton, thomie and 2 others.
shayan-najd updated the Trac tickets for this revision.Aug 9 2018, 7:01 AM
shayan-najd edited the summary of this revision. (Show Details)Aug 14 2018, 4:47 PM
shayan-najd edited the test plan for this revision. (Show Details)

Seems like this patch would be a lot cleaner if a pattern synonym was defined for L. Having to use ViewPatterns everywhere is not very ergonomic.

Seems like this patch would be a lot cleaner if a pattern synonym was defined for L. Having to use ViewPatterns everywhere is not very ergonomic.

I would like to use a pattern synonym (as we did in the wiki), but then we will have issues with the incomplete pattern warnings, wouldn't we?
Can we use the COMPLETE pragma for such a polymorphic pattern instantiated at different types?

It's quite hard to review this patch because it's dominated by routine changes, so it's hard to find the actual payload.

Can you just identify the payload? You partly do that in the Description, but it'd be helpful to say which module these various things are defined in.

Why does HsPat change but not HsExpr say?

An unrelated thing: we should rename Pat to HsPat for consistency.

Other remaks by email.

compiler/deSugar/DsMeta.hs
1753

This is very strange. Why not just add a case to repP? We know this is GhcRn! Much more direct and (probably) more efficient.

bgamari requested changes to this revision.Oct 15 2018, 12:47 PM

What is the plan for this, @shayan-najd? I agree that we will really need to find a way to break this up a bit if we want meaningful review.

This revision now requires changes to proceed.Oct 15 2018, 12:47 PM

It's quite hard to review this patch because it's dominated by routine changes, so it's hard to find the actual payload.

Can you just identify the payload? You partly do that in the Description, but it'd be helpful to say which module these various things are defined in.

The design follows the Solution A described in the wiki page; it is the latest design choice that we decided to go for (e.g., via our latest email communication).
The payload, which is the definition of the HasSrcSpan is in the module basicTypes/SrcLoc.hs.
The rest are the routine changes, described in the description of this patch.

Why does HsPat change but not HsExpr say?

Following Ben and Alan's advice, to avoid making this patch too big, we do the changes one datatype (in HsSyn) at the time.
This patch lays the foundation, and removes the ping-pong style from HsPat only.
The next ones target the other datatypes.

An unrelated thing: we should rename Pat to HsPat for consistency.

I agree, hence ticket Trac #15752.

Other remaks by email.

We have discussed this via email and reconfirmed this design.

What is the plan for this, @shayan-najd? I agree that we will really need to find a way to break this up a bit if we want meaningful review.

Ben, how do you suggest us breaking this up even further?
Changes are routine, and I broken the patches to one datatype at a time.
One way I see this patch getting smaller is to support suppressing coverage checks for /polymorphic/ pattern synonyms by the COMPLETE pragma (see my reply earlier to Matthew).
If you are all on board, we just need to rebase this patch and commit.
The next step is to bring in patches for the next datatypes.

bgamari added inline comments.Wed, Oct 24, 4:16 PM
compiler/basicTypes/SrcLoc.hs
612

This really needs some documentation. The need or desired semantics of this class won't be at all obvious to someone just starting to work in the compiler. I think a Note is probably in order.

629

Can you add a Haddock comment for these saying that they are just abbreviated forms of (de)composeSrcSpan?

compiler/deSugar/DsArrows.hs
1206

I don't understand why we don't just write this as,

haskell​
collectl lpat bndrs
​ = go (unLoc pat)

Presumably this is due to the convenience of refactoring but this unnecessary use of ViewPatterns really does make the code much harder to read IMHO.

simonpj edited the summary of this revision. (Show Details)Thu, Oct 25, 11:39 AM

Many functions seem to treat NewPat in a predefined manner instead of recursing into the Pat inside it – I don't see how it could possibly work. Does this rely on the caller to call dL on the pattern? Very fragile, let's not do that.

compiler/deSugar/Check.hs
1082

This doesn't seem correct now that NewPat is used to store locations.

compiler/deSugar/DsArrows.hs
1232

This doesn't seem correct now that NewPat is used to store locations.

compiler/hsSyn/HsPat.hs
733

This doesn't seem correct now that NewPat is used to store locations.

780

This doesn't seem correct now that NewPat is used to store locations.

compiler/typecheck/TcPatSyn.hs
1000

This doesn't seem correct now that NewPat is used to store locations.

@shayan-najd, let's sort out the remaining review concerns, leaving the removal of the pattern synonyms for future work, and merge this.

We had another set of discussions through the email and decided to land this patch soon, followed up with a series of other patches that implement the same solution for the other data types and another series of patchers that make the code more idiomatic (e.g., removing the fooLPat by adding a clause to a fooPat).

More details on the general implementation plan can be found here.

The latest validated version is at wip/shnajd-TTG-SrcLocs in both https://github.com/shayan-najd/ghc and ​https://github.com/shayan-najd/haddock.

About Int-Index's comments, it is entirely safe to not to touch the XPat cases, or change the ping-ping (e.g., the pair of fooLPat and fooPat) program logic, as all handling of source locations (for at least Pat, for now) is done via dL and cL. You can test the validity by (a) keeping LPat p as Located (Pat p) and it validates as Located is an instance of HasSrcSpan, (b) introduce a random type data Box p = Box SrcSpan (Pat p) and type LPat p = Box p with HasSrcSpan (and instances of Outputable and Data) and it once again validates; the two (a) and (b) imply that
LPat is treated abstractly (regardless of the presentation) thanks to the class methods.
However, we will make the code more idiomatic in the upcoming patches, e.g., by merging fooLPat as a XPat clause of fooPat.
We decided to not to do such idiomatic changes in this patch as its size would grow too big to review.

alanz updated this revision to Diff 18647.Sat, Nov 10, 3:35 AM
alanz edited the summary of this revision. (Show Details)