fix DeriveGeneric generated types and instances to make it working with types with same names -- fixes #10487
ClosedPublic

Authored by osa1 on Jul 17 2015, 7:57 PM.

Details

Summary

DeriveGeneric generates some data types(for data type constructors and for
selectors of those constructors) and instances for those types. This patch
changes name generation for these new types to make it working with data types
with same names imported from different modules and with data types with same
names imported from same modules(using module imports).

Bonus content:

  • Some refactoring in TcGenGenerics.metaTyConsToDerivStuff to remove some redundant partial function applications and to remove a duplicated function.
  • Remove some unused names from OccName. (those were used for an old implementation of DeriveGeneric)

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
t10487
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5212
Build 5460: GHC Patch Validation (amd64/Linux)
Build 5459: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

SPJ: you go to the bottom of the page. In the dropdown menu you select "Add Reviewers". I just added kosmikus already though.

bgamari updated this object.Jul 20 2015, 8:27 AM
bgamari edited edge metadata.
bgamari edited edge metadata.Jul 20 2015, 8:30 AM
bgamari added a subscriber: ezyang.

@ezyang, perhaps the package key or some similar identifier also needs to be included here?

dreixel edited edge metadata.Jul 20 2015, 10:03 AM

I'm happy to review this next week, once I'm back from holidays.

I wrote some comments for specific lines, but actually I think I have a problem with the general philosophy of this patch. For one, adding the original module name is insufficient, because with package qualified imports I can get two identifiers in scope with the same module name but different package keys.

But more generally, it seems like the prefix should be based on the QUALIFIED name of the import (if it exists), or no prefix if it doesn't; this will be guaranteed to uniquely identify it, and is a lot more meaningful if this output shows up user-side.

compiler/basicTypes/OccName.hs
637–652

Much rather prefer this to be 'Module -> OccName -> OccName', assuming there aren't any import circularity problems. This also addresses the package key concern.

compiler/typecheck/TcDeriv.hs
490–492

In the future, add the type signature in a different commit; makes it easier to look for the diff here (which is that genGenericMetaTyCons just had something deleted.

compiler/typecheck/TcGenGenerics.hs
76

Are we sure getModule is always the same as nameModule from the DerivSpec?

ezyang requested changes to this revision.Jul 20 2015, 12:21 PM
ezyang added a reviewer: ezyang.
This revision now requires changes to proceed.Jul 20 2015, 12:21 PM
osa1 updated this object.Jul 20 2015, 12:45 PM
osa1 edited edge metadata.
osa1 added a comment.Jul 20 2015, 12:48 PM

@ezyang, Using qualified names sounds good, I'll try to implement that.

compiler/basicTypes/OccName.hs
637–652

Yeah, the reason I used String is to prevent a circular import.

compiler/typecheck/TcDeriv.hs
490–492

I didn't bother because all commits are squashed before the landing, I forgot about reviewing. I'll be more careful about that in the future.

compiler/typecheck/TcGenGenerics.hs
76

I don't know about DerivSpec, but I made this change because I thought the module used here should always be the module we're currently generating code for. Is this assumption wrong?

How is this going, @osa1? Can I be of any help?

osa1 added a comment.Aug 5 2015, 9:10 AM

I'm waiting for comments from @kosmikus and/or @dreixel for the current approach and qualified names idea.

I was waiting for @ezyang to reply back on the changes after his comment.

ezyang added inline comments.Aug 5 2015, 1:15 PM
compiler/basicTypes/OccName.hs
637–652

In that case, I'd advise either: (1) SOURCE importing Module, or (2) moving these definitions to TcGenGenerics.

compiler/typecheck/TcGenGenerics.hs
76

Well, it's your responsibility to check! I looked at DerivSpec creation and it looks like the Names are always local, so this should be fine.

osa1 added a comment.Aug 6 2015, 11:52 AM

OK, I wasted quite a lot of time trying to figure out how can I get qualified names of types from DerivSpecs. My plan was to get it in TcDeriv.tcDeriving and propagate it further to the code that generates new data types for ConstructorName and DataType instances(TcGenGenerics.genGenericMetaTyCons). I feel like this is possible, because tcDeriving generated instances already use these qualified names(for example, it generates instance Show Blah.Name where ... if I have import M as Blah). Any help would be appreciated.

Hey @osa1, if using the qualified names is a lot of work, maybe we can just continue to use the full names (the error messages will just be worse.)

osa1 added a comment.Aug 6 2015, 1:39 PM

I don't know if it's a lot of work, it's just that I'm lost in the API and I feel like an experienced person could tell us how to do that(or that it's not possible to do) in 5 seconds.

By full names do I presume you mean the current approach?

osa1 updated this revision to Diff 3789.Aug 6 2015, 3:19 PM
osa1 edited edge metadata.
  • bump stderr files
  • minor refactoring in Generic class key test
osa1 added a comment.Aug 6 2015, 5:23 PM

We discussed this on IRC and I want to keep this thread up-to-date.

The problem is, given a TyCon we need to generate a name unique to the module that the TyCon is imported from. (this should take package imports into account)

A TyCon has a Unique, Name, Kind and Arity and none of these help.

I tried using PrintUnqualified data, but the qualified names it gives us are module names. E.g. if I have "import A.B as C" it gives me "A.B" and not "C". So it doesn't solve the problem with package imports.

RdrName has all we need, but they're eliminated in the process of renaming. So if we want to use RdrNames we need to somehow propagate it from renamer to type checker, which is a lot of work. Alternatively, we can add one more field to Name for RdrNames. That's also a lot of work because Names are used everywhere, and bgamari and ezyang didn't like this idea.

That's where I got stuck.

I am "an experienced person" but I'm totally lost in all this generics stuff. If you'd like me to help I'll need more context.

Can you give a minimal, concrete example, giving

  • The source code
  • The code that is currently generated from it
  • What the problem with the generated code is
  • How you would like to fix it if you could only get hold of the relevant info

Please do hat on the ticket, not buried her in Phab.

Then I'll look when I get back from holiday.

Maybe Andres will too.

Thanks

Simon

osa1 updated this revision to Diff 3805.Aug 8 2015, 12:40 PM
osa1 edited edge metadata.
  • bump stderr files
  • minor refactoring in Generic class key test
  • refactor metaTyConsToDerivStuff: remove a duplicated function
  • refactor metaTyConsToDerivStuff: remove some redundant calls to partial myZip functions
osa1 updated this revision to Diff 3806.Aug 8 2015, 3:00 PM
osa1 edited edge metadata.
  • remove unused definitions
  • finally implement unique name generation for DeriveGeneric generated data type, constructor and selector types
osa1 retitled this revision from add module name as prefix to data types created for Generics -- fixes #10487 to fix DeriveGeneric generated types and instances to make it working with types with same names -- fixes #10487.Aug 8 2015, 3:07 PM
osa1 updated this object.
osa1 edited edge metadata.
osa1 updated this revision to Diff 3807.Aug 8 2015, 3:12 PM
  • finally implement unique name generation for DeriveGeneric generated data type, constructor and selector types
osa1 updated this revision to Diff 3808.Aug 8 2015, 3:42 PM
osa1 edited edge metadata.
  • modify generated names to make them look like old names. bump stdout files.
osa1 updated this object.Aug 9 2015, 5:04 PM
osa1 edited edge metadata.
osa1 updated this revision to Diff 3814.Aug 9 2015, 5:13 PM
  • finally implement unique name generation for DeriveGeneric generated data type, constructor and selector types
  • Revert "finally implement unique name generation for DeriveGeneric generated data type, constructor and selector types"
  • another try at fixing name collisions: this time add package id as prefix
  • remove left-over comment
  • bump stderr files hopefully last time

@osa1, is this ready for a review?

bgamari requested changes to this revision.Aug 29 2015, 4:25 AM
bgamari edited edge metadata.

On the whole this looks quite good.

I've requested changes due to the first inline comment. I would really like to avoid passing Strings around if at all possible.

The other two comments are things that would nice to address in a future commit.

compiler/basicTypes/OccName.hs
637–652

At very least these need comments on these describing what they are used for. These should also describe what the String actually means (that it is a prefix derived from the package and module names).

However, I would prefer it if the String were a Module, as mentioned by @ezyang. Is moving these to TcGenGenerics not possible?

compiler/typecheck/TcDeriv.hs
486

It might be nice to fix this in a future commit if you are up for it. If I'm not mistaken you could probably just use a NameEnv keyed on tyConName. It's not clear that this will be much of a performance improvement since the accumulator in extendCommAux likely won't get terribly long, but it's nevertheless worth fixing.

compiler/typecheck/TcEnv.hs
739

Moving the comments below into a Haddock comment for this binding would be helpful. Currently you need to look rather hard to find what distinguishes new_dfun_name from newDFunName. Arguably you could also move the comment above newDFunName into a haddock comment to better associate it with its binding.

This revision now requires changes to proceed.Aug 29 2015, 4:25 AM
osa1 added a comment.Sep 2 2015, 5:27 PM

Sorry for the delay, I'll address the comments once I return from ICFP.

osa1 updated this revision to Diff 4129.Sep 9 2015, 8:14 AM
osa1 edited edge metadata.
  • convert docs for newDFunName to haddock. add haddock for new_dfun_name.
  • s/new_dfun_name/newDFunName' for consistency
  • refactor Generics name generation functions to pass a Module instead of String for prefix
osa1 marked 5 inline comments as done.Sep 9 2015, 8:15 AM
osa1 added inline comments.
compiler/typecheck/TcDeriv.hs
486

Hm, I think I get the idea but let's leave this to another patch.

osa1 added a comment.Sep 9 2015, 8:19 AM

@bgamari, how does it look?
@bgamari, do you know any ways to add a test with multiple packages? I was hoping to add a regression test.

@osa1, sorry for the delay! I was pulled away from GHC things for the last week or so.

This now looks quite good. @ezyang, any final thoughts?

compiler/basicTypes/OccName.hs
637–652

Looks much better.

compiler/typecheck/TcDeriv.hs
486

Alright, fair enough.

osa1 updated this object.Sep 20 2015, 11:59 AM
osa1 edited edge metadata.
bgamari accepted this revision.Sep 21 2015, 6:17 AM
bgamari edited edge metadata.

Barring any last minute objections I think this is ready to merge!

Thanks @osa1!

austin requested changes to this revision.Sep 21 2015, 5:40 PM
austin edited edge metadata.

Alright, this LGTM, but in light of D1265 (the refactor @bgamari mentioned) you'll need to rebase this. With that and the mentioned comment, should be good to go!

compiler/typecheck/TcGenGenerics.hs
76

Let's at least make sure we add an inline note about this as a comment.

This revision now requires changes to proceed.Sep 21 2015, 5:40 PM
osa1 updated this revision to Diff 4254.Sep 21 2015, 7:36 PM
osa1 edited edge metadata.
  • adding some type signatures, minor refactoring
  • fixing Trac #10487
  • add test case
  • bump stderr files
  • minor refactoring in Generic class key test
  • refactor metaTyConsToDerivStuff: remove a duplicated function
  • refactor metaTyConsToDerivStuff: remove some redundant calls to partial myZip functions
  • remove unused definitions
  • finally implement unique name generation for DeriveGeneric generated data type, constructor and selector types
  • Revert "finally implement unique name generation for DeriveGeneric generated data type, constructor and selector types"
  • another try at fixing name collisions: this time add package id as prefix
  • remove left-over comment
  • bump stderr files hopefully last time
  • convert docs for newDFunName to haddock. add haddock for new_dfun_name.
  • s/new_dfun_name/newDFunName' for consistency
  • refactor Generics name generation functions to pass a Module instead of String for prefix
  • move some variables closer to the use site, add a piece of comment
osa1 marked an inline comment as done.Sep 21 2015, 7:37 PM

@austin, All done.

austin accepted this revision.Sep 21 2015, 7:40 PM
austin edited edge metadata.

noice

osa1 added a comment.Sep 23 2015, 8:22 AM

Should this be moved to "landing" stage? (I don't want to forget this and then have to rebase)

This revision was automatically updated to reflect the committed changes.