Implement DuplicateRecordFields
ClosedPublic

Authored by adamgundry on Mar 27 2015, 11:08 AM.

Details

Summary

This implements DuplicateRecordFields, the first part of the OverloadedRecordFields extension, as described at https://ghc.haskell.org/trac/ghc/wiki/Records/OverloadedRecordFields/DuplicateRecordFields

This includes fairly wide-ranging changes in order to allow multiple records within the same module to use the same field names. Note that it does *not* allow record selector functions to be used if they are ambiguous, and it does not have any form of type-based disambiguation for selectors (but it does for updates). Subsequent parts will make overloading selectors possible using orthogonal extensions, as described on the wiki pages. This part touches quite a lot of the codebase, and
requires changes to several GHC API datatypes in order to distinguish between field labels (which may be overloaded) and selector function names (which are always unique).

Haddock has been adapted to compile with the GHC API changes, but it will need further work to properly support modules that use the DuplicateRecordFields extension.

Test Plan

New tests added in testsuite/tests/overloadedrecflds; these will be extended once the other parts are implemented.

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.
adamgundry retitled this revision from to Implement part 1 of OverloadedRecordFields.Mar 27 2015, 11:08 AM
adamgundry updated this object.
adamgundry edited the test plan for this revision. (Show Details)
adamgundry added reviewers: austin, simonpj.
tibbe added a subscriber: tibbe.Mar 31 2015, 5:15 PM

This looks like a large improvement on its own. Can it be made into a separate language pragma from OverloadedRecordFields (i.e. the pragma that gives you type class-based overloading)?

@tibbe Yes, I agree it should be made available separately. My suggestion is that we make the functionality of this PR available as an extension called perhaps LiberalRecordFields, and add the proposed ImplicitValues extension separately (though I'm tempted to rename the latter to something like OverloadedLabels or OverloadedSymbols). Then OverloadedRecordFields would simply turn on both extensions.

Does that make sense? An alternative would be for the functionality of -XLiberalRecordFields alone to be ultimately available as -XOverloadedRecordFields -XNoImplicitValues, but I think that becomes less clear in order to save a single extension name.

adamgundry updated this revision to Diff 3356.Jun 29 2015, 3:35 AM
  • Merge remote-tracking branch 'origin/master' into wip/orf-reboot
  • Fix reporting of unused overloaded record fields
  • Merge remote-tracking branch 'origin/master' into wip/orf-reboot
  • Remove unnecessary Binary instance, since we don't serialize FieldLbl any more
  • Get rid of dead code
  • Distinguish AllowDuplicateRecordFields from OverloadedRecordFields
  • Remove unused test file
bgamari requested changes to this revision.Jul 8 2015, 9:45 AM
bgamari added a reviewer: bgamari.

@adamgundry, it seems there is a validation issue. Perhaps you could take care of this when you get a chance?

This revision now requires changes to proceed.Jul 8 2015, 9:45 AM
adamgundry updated this revision to Diff 3547.Jul 14 2015, 4:00 PM
  • Merge remote-tracking branch 'origin/master' into wip/orf-reboot
  • Get rid of extendRecordFieldEnv, do it in getLocalNonValBinders instead
  • Introduce FieldOcc for occurrences of fields in HsExpr
  • Use FieldOcc for ConDeclField names; hsLTyClDeclBinders et al become a bit nicer
  • Simplify hsLTyClDeclBinders still further
  • Use first datacon name instead of tycon name when mangling selector names
  • Get rid of dfid_rep_tycon and the whole assignInstDeclNames malarkey
  • Fix silly bug in ifaceConDeclFields for empty data types
  • Big refactoring to distinguish HsRecField from HsRecUpdField
  • Do not store (lexical) parents in hsRecUpdFieldSel
  • Extra tests for odd corner cases
  • Merge branch 'wip/orf-reboot' of git://git.haskell.org/ghc into wip/orf-reboot
  • Use a map when renaming ConDeclFields
  • Do a global lookup when renaming fields in updates
  • Return FieldOccs from lookupOccRn_overloaded
  • Store just selector Name in FieldOcc, not FieldLbl
  • Make parser report use of .. in record updates
  • Fix unused imports/declarations/constraints and missing deriving Typeable
  • Use Maybe Name instead of Parent where that's what we mean
  • Update haddock submodule
  • Get rid of OverloadedRecordFields for now; add ADRF to T4437
  • Tweak test output for T7145b
adamgundry retitled this revision from Implement part 1 of OverloadedRecordFields to Implement AllowDuplicateRecordFields.Jul 14 2015, 4:02 PM
adamgundry updated this object.
adamgundry edited the test plan for this revision. (Show Details)
tibbe added a comment.Jul 14 2015, 4:11 PM

If I'm allowed to bike shed a bit I would drop the "Allow" part. It's long and the default meaning of language pragmas is "enable X".

Hmm, I don't understand why the build is failing. Clearly addUsedSelector is used in RnEnv by lookupSubBndrOcc, which is itself exported. Have I done something silly in updating the diff?

In D761#28960, @tibbe wrote:

If I'm allowed to bike shed a bit I would drop the "Allow" part. It's long and the default meaning of language pragmas is "enable X".

I could live with that. Anyone else have objections to DuplicateRecordFields?

Hmm, I don't understand why the build is failing.

If you suspect Phabricator is only partially applying your changes: pull master, rebase your patches locally onto master, and run 'arc diff --update' again.

I could live with that. Anyone else have objections to DuplicateRecordFields?

Fine with me, and I agree it's better

S

adamgundry updated this revision to Diff 3562.Jul 16 2015, 2:51 AM

Squash changes into one commit on top of master

adamgundry retitled this revision from Implement AllowDuplicateRecordFields to Implement DuplicateRecordFields.Jul 16 2015, 3:55 AM
adamgundry updated this object.

I've made the change to DuplicateRecordFields.

@simonpj perhaps we could have another chat soon to review the most recent changes?

In D761#28972, @thomie wrote:

If you suspect Phabricator is only partially applying your changes: pull master, rebase your patches locally onto master, and run 'arc diff --update' again.

Thanks @thomie, that worked perfectly. (Well, actually I squash-merged rather than rebasing, but the same idea.)

adamgundry planned changes to this revision.Jul 23 2015, 6:02 AM
adamgundry updated this revision to Diff 4509.Oct 16 2015, 2:56 AM

Changes following Simon's review

adamgundry updated this object.Oct 16 2015, 3:06 AM

This builds fine on Travis but not on Phab, I suspect because the diff needs to be updated to apply cleanly to the latest HEAD. Is it okay for me to go ahead and merge this? I'll resolve whatever conflicts there are and do a final validate before merging.

adamgundry updated this revision to Diff 4513.Oct 16 2015, 8:07 AM

Bring up to date with master

mpickering added inline comments.Oct 16 2015, 10:11 AM
compiler/typecheck/TcRnTypes.hs
570

What happened to this field? You also deleted extendRecordFieldEnv, is this functionality recovered somewhere else? If so, I probably need to change there as well.

adamgundry added inline comments.Oct 16 2015, 10:20 AM
compiler/typecheck/TcRnTypes.hs
570

The only use we were making of the NameSet was to determine whether a GRE referred to a field (checkShadowedOccs.is_shadowed_gre in RnEnv). But with these changes, you can tell immediately whether a GRE is a field (using isRecFldGRE), so the set became unnecessary. I guess it shouldn't be hard to put it back if you need it for something else.

extendRecordFieldEnv is now incorporated into the tail end of getLocalNonValBinders in RnNames, so I guess your changes may need to move to there.

extendRecordFieldEnv is now incorporated into the tail end of getLocalNonValBinders in RnNames, so I guess your changes may need to move to there.

Ha ha. This exact point in my review of Matthew's code. This is a Good Change.

This revision was automatically updated to reflect the committed changes.
hvr added a subscriber: hvr.Dec 5 2015, 3:05 PM

FYI: turns out this caused a serious regression; see Trac #11167 for more details