Implement HasField constraint solving and modify OverloadedLabels
ClosedPublic

Authored by adamgundry on Nov 14 2016, 4:22 PM.

Details

Summary

This implements automatic constraint solving for the new HasField class and modifies the existing OverloadedLabels extension, as described in the GHC proposal (https://github.com/ghc-proposals/ghc-proposals/pull/6). Per the current form of the proposal, it does *not* currently introduce a separate OverloadedRecordFields extension.

This replaces D1687.

The users guide documentation still needs to be written, but I'll do that after the implementation is merged, in case there are further design changes.

Test Plan

new and modified tests in overloadedrecflds

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
wip/hasfield
Lint
Lint WarningsExcuse: Lexer.x is already full of overly-long lines
SeverityLocationCodeMessage
Warningcompiler/parser/Lexer.x:2234TXT3Line Too Long
Warningcompiler/parser/Lexer.x:2235TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 11917
Build 15159: [GHC] Linux/amd64: Patch building
Build 15158: arc lint + arc unit
adamgundry updated this revision to Diff 9408.Nov 14 2016, 4:22 PM
adamgundry retitled this revision from to Implement OverloadedRecordFields and modify OverloadedLabels.
adamgundry updated this object.
adamgundry edited the test plan for this revision. (Show Details)
adamgundry added reviewers: bgamari, simonpj.
adamgundry updated this revision to Diff 9421.Nov 15 2016, 2:32 AM
adamgundry edited edge metadata.
  • Remove unnecessary extensions, in particular NoImplicitPrelude
adamgundry updated this revision to Diff 9428.Nov 15 2016, 6:47 AM
adamgundry edited edge metadata.
  • Fix test output
dfeuer requested changes to this revision.Jan 9 2017, 6:49 PM
dfeuer added a reviewer: dfeuer.
dfeuer added a subscriber: dfeuer.
dfeuer added inline comments.
compiler/typecheck/TcHsSyn.hs
619–620

Why not zonkExpr _ e@HsOverLabel{} = return e? The same goes for several of these patterns.

compiler/typecheck/TcInteract.hs
2223

This do block is large and deeply nested, with lots of braces and semicolons to keep things on the left. Is there any way to break it up into meaningful functions?

2239

Can this list have multiple elements in this context? If not, we should document why, and produce an error if it does. If so, we should document why we are only concerned with the first element.

2250

That's a bit yucky. Is there some way to get the information from the original lookup and preserve it, rather than throwing it away and then asserting that we have to be able to find it again?

libraries/base/GHC/Records.hs
45

Is there a separate class somewhere for setters? HasField seems like a strange name for a class that only offers a getter.

This revision now requires changes to proceed.Jan 9 2017, 6:49 PM
adamgundry updated this revision to Diff 10779.Jan 28 2017, 3:31 PM
adamgundry edited edge metadata.
  • Add HasField class with magic constraint solving
  • Remove Proxy# from IsLabel and add function space instance using HasField
adamgundry marked 3 inline comments as done.Jan 28 2017, 3:40 PM

This is now up to date with the current state of the proposal, and somewhat simpler as a result. Thanks to @dfeuer's feedback I found and fixed a bug.

compiler/typecheck/TcInteract.hs
2223

I started to break this into smaller functions and ended up completely rewriting it (in part because I discovered a bug and had to change strategy). I think it's now clearer, albeit still a bit monolithic. Any suggestions for further improvement?

libraries/base/GHC/Records.hs
45

Not yet, but there hopefully will be in the future. Other names are possible, but I'm not repainting the bikeshed without a very good reason. ;-)

adamgundry retitled this revision from Implement OverloadedRecordFields and modify OverloadedLabels to Implement HasField constraint solving and modify OverloadedLabels.Jan 30 2017, 10:41 AM
adamgundry updated this object.
adamgundry edited edge metadata.
bgamari edited edge metadata.Feb 4 2017, 9:06 PM

This is looking quite good. Now we just need to sort out the lens matter. Comments inline.

compiler/basicTypes/RdrName.hs
812 ↗(On Diff #10779)

It would be nice if this comment offered an example of when this can happen.

compiler/hsSyn/HsExpr.hs
294

It would be nice if this mentioned what the Maybe id represents. Having to go all the way to the Note for this is unfortunate.

Also, I believe the RebindableSyntax overloads are typically represented with SyntaxExpr. However, I suppose if you don't need the flexibility that SyntaxExpr provides then so be it.

compiler/rename/RnPat.hs
634 ↗(On Diff #10779)

It's slightly surprising that we are using elem here. Do we really know that present_flds won't be large?

compiler/typecheck/TcExpr.hs
317

Given that our treatment of single-method classes may change in the future, perhaps we should start collecting all of the places where we make this assumption somewhere (a Trac ticket, perhaps?)

compiler/typecheck/TcInteract.hs
2220

Very clearly written. Thanks!

compiler/typecheck/TcSMonad.hs
2757

Be aware that I've also added this in my Typeable branch which may have landed by the time we get to land this.

2948

A comment on this would be appreciated.

I just thought I'd flag up that this patch is being discussed on github and reddit.

adamgundry updated this revision to Diff 11048.Feb 8 2017, 3:29 PM
  • Remove IsLabel instance for (->)
  • Comments only
  • Use OccSet instead of [FastString] in rnHsRecFields.rn_dotdot
adamgundry marked 4 inline comments as done.Feb 8 2017, 3:32 PM

I've removed the IsLabel instance for (->), following consensus on the proposal PR, and taken into account the review feedback so far. Thanks @bgamari!

compiler/rename/RnPat.hs
634 ↗(On Diff #10779)

I've changed it to use an OccSet instead. Seem reasonable?

bgamari accepted this revision.Feb 8 2017, 6:04 PM

Lovely, looks great. Thanks @adam!

compiler/rename/RnPat.hs
634 ↗(On Diff #10779)

Indeed it does. Thanks!

This revision was automatically updated to reflect the committed changes.