Preserve empty variant constructor in data-deps. #7303
Conversation
This fixes #7207 and adds a regression test. In a separate PR I'll add a warning for variants with single argument of unit type and add a changelog entry. changelog_begin changelog_end
| @@ -479,6 +479,9 @@ generateSrcFromLf env = noLoc mod | |||
|
|
|||
| convConDetails :: LF.Type -> Gen (HsConDeclDetails GhcPs) | |||
| convConDetails = \case | |||
| -- empty variant constructor (see issue #7207) | |||
| LF.TUnit -> | |||
There was a problem hiding this comment.
This means that a variant where I explicilty specify a Unit argument will also be decompiled to a Prefix con right? I think that’s perfectly reasonable but probably worth documenting in a comment and perhaps warning on in the preprocessor.
There was a problem hiding this comment.
Yes, that's what we decided on in the issue and I'm preparing the warning in a separate PR.
| ] | ||
| writeFileUTF8 (tmpDir </> "type" </> "Foo.daml") $ unlines | ||
| [ "module Foo where" | ||
| , "data A = B | C Int" |
There was a problem hiding this comment.
Should we add a third case D () and check that it behaves like D when imported as data-dependency?
There was a problem hiding this comment.
I don't think so. D () is something we're deprecating, and the fact that it's treated the same as D in the current compiler is mostly a mistake.
There was a problem hiding this comment.
I generally agree but would feel more comfortable to have a test as long as users can still write it, even though they get warned about it.
There was a problem hiding this comment.
I added a test for D () in the single constructor case, which is something we had talked about adding as a special case (since we can't distinguish single-constructor variants from enums any other way, and there's no ambiguity in the single constructor case). (This actually also came up in one of the other tests, but better to add one here as well.)
| @@ -479,6 +479,9 @@ generateSrcFromLf env = noLoc mod | |||
|
|
|||
| convConDetails :: LF.Type -> Gen (HsConDeclDetails GhcPs) | |||
| convConDetails = \case | |||
| -- empty variant constructor (see issue #7207) | |||
There was a problem hiding this comment.
"Empty" is such an overloaded word:
| -- empty variant constructor (see issue #7207) | |
| -- variant constructor without argument (see issue #7207) |
Or "payload" instead of "argument".
There was a problem hiding this comment.
Right, but "variant constructor without argument" is such a clumsy phrase, and what else could "empty variant constructor" mean?
There was a problem hiding this comment.
I guess the right answer here is "nullary variant constructor".
There was a problem hiding this comment.
I like "nullary variant constructor". 👍
This fixes #7207 and adds a regression test. In a
separate PR I'll add a warning for variants with
single argument of unit type and add a changelog
entry.
changelog_begin
changelog_end