Add Lint/DuplicateBranch rule
#582
Conversation
Sija
force-pushed
the
lint-duplicate-branch-rule
branch
3 times, most recently
from
19818c8 to
3337681
Compare
Sija
force-pushed
the
lint-duplicate-branch-rule
branch
from
3337681 to
bbeb92a
Compare
| @@ -27,7 +27,7 @@ module Ameba | |||
| def color : Colorize::Color | |||
| case self | |||
| in Error then Colorize::ColorANSI::Red | |||
| in Warning then Colorize::ColorANSI::Red | |||
| in Warning then Colorize::ColorANSI::Red # ameba:disable Lint/DuplicateBranch | |||
There was a problem hiding this comment.
can't this be reworked in a way when the disable directive is not needed?
There was a problem hiding this comment.
Maybe this indicates that duplicate paths in a case body should be accepted?
There was a problem hiding this comment.
@straight-shoota There's already IgnoreConstantBranches option for that.
There was a problem hiding this comment.
@veelenga Easiest way to do it is changing the color value.
There was a problem hiding this comment.
Then maybe that option should be enabled by default?
There was a problem hiding this comment.
@straight-shoota I'm open to see good use cases of this behavior. This case is IMO a great example where this rule uncovers dubious choices, since this branch shouldn't be repeated in the first place.
There was a problem hiding this comment.
@veelenga I'd suggest leaving it as-is for now and changing the returned value in a separate PR.
There was a problem hiding this comment.
This btw uncovers the fact that we only have a single rule - Lint/Syntax - with an Error severity.
There was a problem hiding this comment.
As mentioned before, especially case with then branches is often used for mappings. When the set of input values is bigger than the output set, there'll be multiple inputs pointing to the same output. And that's totally fine within the domain model.
However, I don't think there's a clear preference about how to express that in code. Sometimes it might be best to group input values which lead to the same value. Sometimes it's better to keep the input values separate and in their implicit order, for example.
IMO ameba shouldn't take preference here because this is a style matter specific to each individual use case.
Sure, having more strict rules may uncover dubious choices like the example here.
But there's also a high chance of too many false positives.
I think the default behaviour should be cautious to avoid noise. Nobody want's to deal with long reports of false violations.
There was a problem hiding this comment.
I agree. Too many rule violations highlighting too much of the document, especially for things that are IMO more stylistic choices, will turn people away from using ameba and it's editor integrations. If this rule should be added I think it should be disabled by default.
Sija
force-pushed
the
lint-duplicate-branch-rule
branch
from
bbeb92a to
17feb3b
Compare
| when Crystal::UninitializedVar then eql?(assign.var) | ||
| when Crystal::Assign, Crystal::OpAssign then eql?(assign.target) | ||
| when Crystal::MultiAssign | ||
| assign.targets.any? { |target| eql?(target) } |
There was a problem hiding this comment.
I'm not sure if this change (and similar above) is a good choice.
This aspect has not yet been mentioned in the discussion or anywhere in the documentation and specs: some conditions have an effect on flow typing and merging conditions affects typing.
Here the different branches have technically the same body. But combining them introduce a union type Crystal::Assign | Crystal::OpAssign which can lead to all kinds of potentially unintended consequences.
If we keep the branches separate, the type of assign is restricted to the respective type.
The compiler uses this pattern intentionally in a number of places (including ones dealing with switching over AST nodes). So I would assume it's also intentional here.
There was a problem hiding this comment.
I don't see how this affects dispatch, see here: https://carc.in/#/r/hqb4
| @@ -27,7 +27,7 @@ module Ameba | |||
| def color : Colorize::Color | |||
| case self | |||
| in Error then Colorize::ColorANSI::Red | |||
| in Warning then Colorize::ColorANSI::Red | |||
| in Warning then Colorize::ColorANSI::Red # ameba:disable Lint/DuplicateBranch | |||
There was a problem hiding this comment.
As mentioned before, especially case with then branches is often used for mappings. When the set of input values is bigger than the output set, there'll be multiple inputs pointing to the same output. And that's totally fine within the domain model.
However, I don't think there's a clear preference about how to express that in code. Sometimes it might be best to group input values which lead to the same value. Sometimes it's better to keep the input values separate and in their implicit order, for example.
IMO ameba shouldn't take preference here because this is a style matter specific to each individual use case.
Sure, having more strict rules may uncover dubious choices like the example here.
But there's also a high chance of too many false positives.
I think the default behaviour should be cautious to avoid noise. Nobody want's to deal with long reports of false violations.
Sija
force-pushed
the
lint-duplicate-branch-rule
branch
from
17feb3b to
28837ac
Compare
No description provided.