Use explicit deriving strategies where it's ambiguous #3022
Conversation
| @@ -180,3 +158,27 @@ executables: | |||
| - unison-util | |||
| - unison-util-relation | |||
| - unison-pretty-printer | |||
|
|
|||
There was a problem hiding this comment.
Just moved this to the bottom because it's the most boring clause, I think. Also, I may have added an extension or two, for consistency with other packages.
There was a problem hiding this comment.
We should consider a defaults.yaml for the monorepo for this in the future I think, you can then just import it into each package.yaml, we did it at Wire and it worked out well 馃槃
| @@ -477,9 +477,13 @@ data ANormalF v e | |||
| -- Types representing components that will go into the runtime tag of | |||
| -- a data type value. RTags correspond to references, while CTags | |||
| -- correspond to constructors. | |||
| newtype RTag = RTag Word64 deriving (Eq, Ord, Show, Read, EC.EnumKey) | |||
| newtype RTag = RTag Word64 | |||
| deriving stock (Eq, Ord, Show, Read) | |||
There was a problem hiding this comment.
Hrmm, are you sure about the Read instance? The stock one would expect RTag 42 whereas the newtype instance would be 42; Same for CTag below;
I guess it depends on where and how this is used and whether it expects to read numbers or just requires a correct roundtrip through String.
Even if it was currently behaving as a stock instance that doesn't necessarily mean that's what Dan intended 馃
There was a problem hiding this comment.
Yeah, Read was stock-derived before and after this PR, so that'd be "RTag 42". Seems right to me, especially because of the (informal?) rule that Read should agree with Show.
There was a problem hiding this comment.
I double-checked and these Read instances are unused (everything still compiles after removing them), I'm guessing someone just went a little derive-happy so I'd personally advocate for removing it haha.
I know the "expected" behaviour is for it to agree with Show, but in my experience using Read for anything other than primitive types is an anti-pattern, better to write a proper parser, or a String -> Either String a function IMO
There was a problem hiding this comment.
Cool. Yeah, I don't use Read myself, but I didn't author this code originally. Let's maybe suggest removing it in another PR?
|
What do you mean check usage of the Read instances? Edit: oh -- I see now, your other comment |
Overview
This PR adds a few explicit
stock/newtypederiving strategies where they were ambiguous before.These clauses were previously causing warnings when loading more than one component into ghci (i.e.
stack ghci), because:DeriveAnyClassandGeneralizedNewtypeDerivingenabled, GHC emits a warning for some deriving clauses indicating that it's defaulting toDeriveAnyClassfor some type classes, but you should be explicit about which strategy to use.unison-parser-typecheckeronly hasGeneralizedNewtypeDerivingenabled, but some other package(s) haveDeriveAnyClassstack ghci, it just combines all of the flags/extensions together.With this PR, you should be able to
stack ghciwithout warning. (Better would be to have the same set of extensions enabled in every package 馃槃)