- User Since
- Mar 28 2016, 7:32 AM (151 w, 3 d)
Wed, Feb 13
Some long overdue responses to Ryan's comments:
Wed, Feb 6
Sorry, that was supposed to be *have*. Does anyone *have* any more feedback on this?
Does anyone there any more feedback on this? I'm not aware of any outstanding issues that need to be addressed.
Sat, Feb 2
I fixed a panic that could be triggered by a multi-field type with a CUSK (added a test for this as well). I've added a note about what's going on in tcConDecl. I rewrote the bottom part of the note for kcConDecl since I realized that kcConDecl would never actually be called on the example I gave. I believe I've now cross-referenced all the notes in appropriate places, but let me know if I've missed any of them.
- Improve outdated notes, cross reference more, quit using panic before validity check happens.
Fri, Feb 1
Marking a few old comments as done.
Tue, Jan 29
- use MCoercion in tcConDecl with unlifted newtypes
- Handle all three types of data constructors when dealing with kind-checking the field of an unlifted newtype
Mon, Jan 28
@simonpj The note with a high-level overview is Implementation of UnliftedNewtypes, found in compiler/typecheck/TcTyClsDecls.hs. I've updated the note to be more accurate.
- improve notes, update notes, rename decideNewtypeKind to unifyNewtypeKind
Sun, Jan 27
@goldfire I've implemented your suggestion. I picked the variant of it that didn't involve nesting solveEqualities. The error message we get for UnliftedNewtypesMismatchedKind is now excellent, especially because we can feed Just ... as the first argument to unifyKind. If @simonpj has a cleaner idea for how to accomplish this check in tcConDecl, I can switch to that. All tests are currently passing. I need to update some of the notes.
- Remove commented out garbage. Allow solving before checking coercion type in tcConDecl. Include original type in error message.
So, the problem was that I didn't actually do checks in tcConDecl to make sure that the kind of the type of the field in a newtype matched what was expected. I've added this check for GADT-style declarations. All it requires is checking that the kind of the field matches the kind of res_ty. I've written a note in the code requesting a little feedback.
- correctly check that the field in an unlifted newtype matches the expected kind
Sat, Jan 26
I've nearly got this fixed. I've broken the following:
- kind check the field in a newtype to ensure that it matches what the data type expects
The naive solution I attempted actually makes a bunch of tests fail. I'm looking into other options.
I've done the simplest thing to make this work: running kcLTyClDecl on all the cusk_decls. This makes Ryan's data type fail to typecheck, but the error message is the impenetrable:
- start performing kcConDecl checks on the data constructors of types with cusks
Oh, I see the problem. We only call kcLTyClDecl on no_cusk_decls. For cusk_decls, the path we follow never ends up calling unifyNewtypeKind. I think I know where to fix this.
I agree that T should be rejected. I am surprised that this wasn't caught by an existing test. I've added a test locally, and this patch does indeed currently accept T. I'm looking into this.
Fri, Jan 25
I've beefed up the UnliftedNewtypesDifficultUnification test with the examples that @goldfire provided. The errors appear to be as expected.
- beef up difficult unification test
That appears to do it. I've added the proposed additional check. It works, and all tests now pass. The error message on UnliftedNewtypesInfinite is better now as well.
- add another check for reflexive coercions
Jan 21 2019
I'm going to migrate this over to gitlab. Feel free to close this.
Certainly. This was just at attempt to write out some stuff I'd been musing about. I stuck it up here so that I couldn't accidentally delete it. If there's some kind of way to close a differential on phabricator so that you're able to see what's still being actively worked on, feel free to close this one.
Jan 19 2019
@goldfire I've done what you suggested in decideNewtypeKind, and I've reverted the changes I made that let unravelFamInstPats accept casts. However, there's a problem. In tcDataFamHeader, we have two options for what to do when the user doesn't use GADT-style syntax to provide a type signature for a newtype instance. We've discussed these briefly in the comments already. We can invent a fresh metavar, or we can reuse the LHS result kind. Currently, I've reverted to using the LHS result kind, since the problems it has are less bad.
- beef up the UnliftedNewtypesUnassociatedFamily test
- cross-referencing in more notes about UnliftedNewtypes
- improve kind unification for newtype instances a little bit, breaks different tests
Jan 11 2019
And I just missed that one as well. Sorry, my previous comment was in response to the one before your last one, not your last one.
Sorry, I just now saw your comment Richard. I had already made the appropriate change to kcConDecl, but that turned out to not be enough. To see why, consider the trace output I was looking at earlier:
I've made coerce and coerce# the same function. All comments about documentation of coerce# and about the rewrite rules stuff are now moot, so I have marked them complete. All tests now pass. I still need to do a few documentation things. Could @goldfire or @simonpj confirm that what I have done in unravelFamInstPats is sound?
- add test for VTA on coerce
- remove coerceLiftedKey
- Collapsed coerce# and coerce back into one function. Make sure all typechecker tests pass.
- make unravelFamInstPats accept a type with a cast. remove a trace statement that caused the compiler to hang.
Wait a second. What's this?!:
Hmmm... in tcDataFamHeader's doppelganger, tcTyFamInstEqnGuts, we find this telling comment:
I've confirmed that the fix I suggested does work. But I feel like the other solution (inventing a metavar) should work too. I analyzed it wrong above. In particular, UnliftedNewtypesDifficultUnification.D:R:Interpret is a coercion. Here's where I think the real problem may be:
Ah, that's good to know. I've now gotten to the bottom (or very close to the bottom) of it. Over in tcDataFamHeader, unravelFamInstPats was misbehaving. But, it only misbehaves because it's receiving a bad type:
Jan 10 2019
Ah, I had assumed the loop while tracing was something I had done, but you're right that it was already present. I found the bad trace statement and removed it. I figured out where this panic is happening, but I don't understand how to stop it. At some point during typechecking, we call checkUserTypeError on the data constructor. Here it is with some extra trace statements I've added:
Jan 9 2019
I've fixed the panic coming from the typeable machinery. But there's another panic that is more difficult to track down. If I use GADT syntax to provide a kind signature in UnliftedNewtypesDifficultUnification, I get the error I expect:
- improve some of the typechecking code. remove some unneeded imports. all a data for data families that have a type family in the result kind.
Nevermind, it looks like there's some additional filtering that happens in todoForTyCons. I'll keep looking.
The only check I see in TcTypeable.hs to figure out what to generate Typeable bindings for is needs_typeable_binds. But that cannot be the full picture. Totday, GHC already correctly rejects:
It gets worse! We can get the same exact problem without any data families. Behold:
I've clarified the effect of UnliftedNewtypes on CUSK determination in the user guide. I've cleaned up the typechecking code some, implementing some of Richard's suggestions. I've added a test UnliftedNewtypesDifficultUnification, which currently fails with a panic. We get a different (and likely more meaningful) panic when we remove the last line of this test. With the last line removed, we have:
- specify the impact of UnliftedNewtypes on CUSKs in the users manual
- improve some of the typechecking code. remove some unneeded imports. all a data for data families that have a type family in the result kind.
Jan 7 2019
Good catch. I've added T15883 and UnliftedNewtypesForall to confirm that trac ticket Trac #15883 is resolved.
- add two tests to confirm that 15883 is resolved
I've made several changes involving feeding in result kinds to kcConDecl and adding kind checks back to newtypes when UnliftedNewtypes is enabled. However, UnliftedNewtypesUnifySig is still failing.
- add UnliftedNewtypesUnifySig test
- use fresh metavar in tcDataFamHeader
- add back more kind checks when UnliftedNewtypes in enabled
- feed more result kinds into kcConDecl
Jan 5 2019
I've added some more tests and made some very minor improvements to docs. Currently failing test are:
- several documentation-related fixes, additional tests, remove MagicHash extension from modules that do not need it
Jan 4 2019
No worries! Since I plan on using this feature, I'd like for it to be correct 😄. Thanks for all the feedback.
Jan 3 2019
I've added a new test UnliftedNewtypesLevityBinder. I wasn't actually expecting it to pass, but it does. Good job Richard and Simon for being so thorough with the levity-polymorphism checks! I think this is ready to merge if there are no further concerns.
- add a test to ensure that a levity-polymorphic newtype does not let us subvert the levity-polymorphic binder check
Dec 28 2018
I have added more notes, one explaining coerce vs coerce# and one giving a high-level overview of the implementation of UnliftedNewtypes. Anyone please let me know if there are any comments I overlooked or if there is anything else that needs clarification or correction.
- Give a broad overview of the implementation of UnliftedNewtypes
I've been poking around in TcHsType trying to fix this, but I haven't made any headway. I'll just leave this in its moderately broken state for now. I think we're still supposed to open issues on trac up until the migration, but I'm not sure.
Dec 27 2018
I've added a test UnliftedNewtypesGnd the checks that generalized newtype deriving works. Surprisingly, it does. But, it only works when the runtime representation is fixed. For example, if instead of
- add some more tests
- provide example
I've addressed most of the issues that Simon brought up. There's still one place where I need to add a concrete example. I've implemented David's suggestion to add a little magic that effectively inlines the lifted coerce into coerce# @'LiftedRep whenever it sees it in the LHS of a rule. T2110 still passes, and we no longer get warnings when coerce is used in the LHS of a rule, so this appears to work correctly. I've moved coerce into GHC.Base, so I was able to revert all the places where I changed coerce to coerce#.
- allow coerce in the LHS of rewrite rules
Dec 24 2018
- add more documentation
- more changes
Dec 21 2018
Since coerce inlines to coerce# in every phase, we only need rules whose LHS mentions coerce#. That should be sufficient to cover both cases. It's an slight inconvenience to library maintainers though because it means that you need CPP to have the LHS say mapFoo coerce on older GHCs and mapFoo coerce# on newer ones.
Oh, and GHC.Base now no longer exports coerce. I'm not sure if this is a problem or if this module is even expected to provide a stable API.
Also, this most recent change infecting a lot more modules than I thought it would. I changed most occurrences of coerce in base to coerce# because I'm not sure if these other modules are allowed to depend on Data.Coerce, and I was looking for a safe option.
It turns out that Data.Coerce was really the only place it could go. One small wrinkle I noticed is that I now get a warning from containers;
- split up coerce# and coerce
I'm convinced by the symmetry argument. I'll rename the identifier in GHC.Prim to coerce# and then define the lifted coerce in GHC.Exts.
- update documentation and factor out mkTemplateKindVar
Responding to @dfeuer, I could go either way on the coerce vs coerce/coerce# thing. I'm not particularly worried about it being a breaking change, and I lean slightly in the direction of just having one function instead of two. That's the way it was discussed in https://ghc.haskell.org/trac/ghc/ticket/13595 and on the unlifted newtypes proposal. Since this is a question about a change to base's API, @ekmett or @RyanGlScott will need to make the call.
- clean up extraneous trace statements
Nevermind, I've figured out how to change the title and linked tickets. Let me know if anything else is missing or could be adjusted.
Thanks for the feedback. I've tried to address all of the concerns, and I've added a test to confirm that compilation fails with a good error message when the user does not enable the extension. I'm struggling to figure out how to change the title of the diff and the linked trac tickets though.
- add an entry to the users guide for UnliftedNewtypes
- add an entry to the changelog for base documenting the new type signature of coerce
- document UnliftedNewtypes in the release notes
- add test to ensure that unlifted newtypes are rejected when the extension is disabled
- add mkTemplateKiTyVar to avoid incomplete uni patterns
Dec 20 2018
Last major problem has been fixed. I'll do some documentation and cleanup some extraneous trace statements.
- another attempt at fixed unlifted newtypes dependent associated data family woes
- fix the problem with UnliftedNewtypes and data families
No one should bother looking at this. It is a work in progress, and I need to figure out a way to benchmark it. I don't know if there is already a benchmark out there that does a bunch of small stuff using the event manager and then measures allocations and time elapsed. There are also some places with boxed machine ints in callbacks that I want to change, but I have to finish UnliftedNewtypes to have a safe way to do this.
Dec 14 2018
This looks good to me other than the minor stylistic things I just added comments about.
Whoops. I see now that you had already added an entry to the user manual. I think the flag should always cause warnings regardless of whether DerivingStrategies is currently enabled.
Dec 10 2018
Added an example of bad behavior at GHCi.
Dec 7 2018
Added a comment with some additional context about the failing test that I cannot solve.
move data family unlifted newtype test to the right directory
Marking off some of Richard's comments as completed.
formatting clean up