Re-export the data family when exporting a data instance without an export list
ClosedPublic

Authored by KaneTW on Dec 5 2015, 12:06 AM.

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.
KaneTW updated this revision to Diff 5470.Dec 5 2015, 12:06 AM
KaneTW retitled this revision from to Re-export the data family when exporting a data instance without an export list.
KaneTW updated this object.
KaneTW edited the test plan for this revision. (Show Details)
KaneTW edited edge metadata.Dec 5 2015, 12:07 AM
KaneTW updated the Trac tickets for this revision.
KaneTW updated this revision to Diff 5480.Dec 5 2015, 5:08 AM

Error message wibbles (see trac)

bgamari accepted this revision.Dec 5 2015, 5:16 AM
bgamari edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Dec 5 2015, 5:16 AM
goldfire added inline comments.
compiler/rename/RnNames.hs
1218

I would think that not (elem n ns) would imply that n is a data family. Is there a scenario where this is not the case? I'm a little disturbed by calling tcLookupTyCon in the RnM monad. Yes yes, I know RnM and TcM are synonyms, but we should take advantage of that rarely. (Interleaving renaming and typechecking when doing Template Haskell is a good reason, for example. I'm less convinced here.)

KaneTW added a comment.Dec 5 2015, 9:41 AM

I was thinking that, but I wasn't entirely sure that's always the case. Can't think of a counterexample though.

goldfire requested changes to this revision.Dec 5 2015, 9:53 AM
goldfire added a reviewer: goldfire.

More broadly, if n is not in ns, and we've omitted an export list, I think we really should be re-exporting n. The scenario means that a module is defining some sub-parts of a declaration, but not the declaration itself. If we're exporting the sub-parts, let's also (re-)export the whole thing.

I still think this case applies only for data families, and the user manual needs document it as such, but the implementation can be broader.

Also, I think this deserves a release note, as it's a user-facing change.

Thanks!

This revision now requires changes to proceed.Dec 5 2015, 9:53 AM
KaneTW updated this revision to Diff 5483.Dec 5 2015, 10:17 AM
KaneTW edited edge metadata.

Added a release note entry and generalized fix_faminst

goldfire accepted this revision.Dec 5 2015, 10:20 AM
goldfire edited edge metadata.

Thanks, @KaneTW.

compiler/rename/RnNames.hs
1216

Nitpick: is isTyConName n ever false? I doubt it.

This revision is now accepted and ready to land.Dec 5 2015, 10:20 AM

Yeah, it's always a type or class (otherwise it'd be in Avail, not AvailTC). I'll remove it; didn't occur to me that it's a tautology.

KaneTW updated this revision to Diff 5485.Dec 5 2015, 10:34 AM
KaneTW edited edge metadata.

Remove redundant isTyConName check.

This revision was automatically updated to reflect the committed changes.
simonpj added a subscriber: simonpj.Dec 8 2015, 9:09 AM

Great. Let's land this along with D1587