Open phadej opened 5 years ago
Oh, I noticed #683
I think that with
a module have both
DeriveAnyClass
and GeneralizedNewtypeDeriving `enabled
precondition that hint can be enabled by default.
ping @divarvel
Note that the new rule suggests deriving newtype
however very often (usually?) that suggestion is wrong. For (Show
, Read
) this modifies the behaviour (introduces bugs). Perhaps there should be a specific list of classes for which it always suggests deriving stock
?
I made the choice to suggest newtype
because it's always possible. When it comes to introducing bugs or not, it depends on the intent of the programmer (which sadly hlint can't guess).
Having a list of stock
-derivable instances could be a solution indeed. or maybe is there a way to have multiple suggestions? In any case, I didn't want to try and recompute the actual behaviour.
Regarding DeriveAnyClass
/ GND
, that'd be a good idea, but I don't know if I have access to this information in the hint code. If that's the case, I can emit a warning, else keep the ignore
. wdyt?
Personally, I want to be warned all the time, because I want to know what's happening just by looking at the definition, regardless of which extensions are enabled.
Maybe @ndmitchell has an opinion on this?
I made the choice to suggest
newtype
because it's always possible. When it comes to introducing bugs or not, it depends on the intent of the programmer (which sadly hlint can't guess).
I don't think it's always possible. Take this example:
data Foo = Foo Int Int
newtype Bar a = Bar Foo deriving stock Functor
This works with deriving stock
but not with deriving newtype
.
Having a list of
stock
-derivable instances could be a solution indeed. or maybe is there a way to have multiple suggestions? In any case, I didn't want to try and recompute the actual behaviour.
It's best to suggest the suggestion which doesn't modify the behaviour of the code!
Personally, I want to be warned all the time, because I want to know what's happening just by looking at the definition, regardless of which extensions are enabled.
Iiuc, currently the warning requires the user to manually enable it in their .yaml
configuration, and what @phadej is suggesting is that in addition it will be enabled more often, also where these extensions are active and the rule isn't explicitly enabled in the configuration.
It's best to suggest the suggestion which doesn't modify the behaviour of the code!
Indeed, that's a point I've mentioned in the original PR. I don't know if it's feasible to accurately know the current behaviour, though (if it is, it'd be indeed better to suggest something that preserves semantics).
I like @phadej's idea of enabling it by default based on enabled extensions. I'm not sure how to do it, sadly.
deriving stock
and deriving anyclass
produce well behaved instances, whereas deriving newtype
is the bug-prone option (Show
instances no longer match code etc)stock
derivable classes, we can't know which classes can be derived via either anyclass
or newtype
So is it possible to warn without suggesting (often wrong) code changes? @ndmitchell
The problem with DerivingStrategies is it's only available in a later version of GHC relative to the other extensions. As such, recommending it is probably not a good idea.
It would be great if the suggested hints were semantics preserving. If they can't be preserving you could omit the suggested output - better nothing than the wrong thing. Remember the stock derivable classes change with extensions like DeriveFunctor, and you can't tell what is in the users .cabal file, so it might just be impossible to do correctly.
I think the best solution would be to not suggest anything indeed. I was not aware of that possiblity (I thought it was only "put this instead" or "remove this"). Do you have an example of such a hint? I'll submit another patch to remove the controversial newtype suggestion.
You can write:
newTypeDerivingStrategiesHintDecl x = [rawIdeaN Ignore "Use DerivingStrategies" (srcInfoSpan $ ann x) (prettyPrint x) Nothing [] | Just new <- [newtypeDecl x]]
Of course, once you've done that, you probably want to turn newtypeDecl into a Bool
rather than returning a Just
that you don't use the contents of.
I'll do that. Since it's disabled by default, we can expect users who see it to know what deriving strategies are and how they can use it.
Thanks!
Le lun. 1 juil. 2019 à 18:03, Neil Mitchell notifications@github.com a écrit :
You can write:
newTypeDerivingStrategiesHintDecl x = [rawIdeaN Ignore "Use DerivingStrategies" (srcInfoSpan $ ann x) (prettyPrint x) Nothing [] | Just new <- [newtypeDecl x]]
Of course, once you've done that, you probably want to turn newtypeDecl into a Bool rather than returning a Just that you don't use the contents of.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ndmitchell/hlint/issues/690?email_source=notifications&email_token=AABKJ47FCS2X3HHLDU2OEY3P5ITFZA5CNFSM4H3UBRR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY6TE7I#issuecomment-507327101, or mute the thread https://github.com/notifications/unsubscribe-auth/AABKJ43YNWITFSDA2Q2G6PTP5ITFZANCNFSM4H3UBRRQ .
if
DeriveAnyClass
andGeneralizedNewtypeDeriving
enablednewtype
with non-specificderiving
clausethen
DerivingStrategies
if not already enabledderiving newtype
oranyclass
an example (in this case, probably irrelevant, in general: not so much).