Add InferNonNilReceiver config to Lint/RedundantSafeNavigation
#14345
Conversation
fatkodima
force-pushed
the
new-SafeNavigationOnNonNil-cop
branch
from
1f0d4ed to
a547769
Compare
|
The removal of safe navigation needs to be done safely. If there is no guarantee that the logic remains safe, I think it shouldn't be added as a cop. Even if it is marked as unsafe, if removing a supposedly redundant safe navigation operator could break applications, users wouldn’t be able to use it with confidence. |
|
I don't have time or desire to convince people about anything. It is already marked as unsafe and I detected 200 valid offenses in my app and only 1 false positive, so I think this is a valuable addition. I see people use this operator without thinking. I have it as a custom cop in my app. |
|
Out of curiosity I tried it on bunch of my apps/gems and it actually found ~25 offenses in total, all of them were valid AFAICS. @fatkodima Could you share what was your false positive? |
|
It was an example like this one https://github.com/rubocop/rubocop/pull/14345/files#diff-9d8778d3350f73d4e62eb6bf104febb5b6ba700ec50ea92c44ebca8200cda41eR33 I haven't added more logic to detect cases where a receiver is redefined to not make the implementation more complicated. |
We're all busy here, but there's little point in jumping to hasty conclusions. I think the simplest path forward is to actually integrate your check in the existing cop and put it behind a flag there. |
fatkodima
changed the title
Lint/SafeNavigationOnNonNil copCheckReceiverPreviousOccurrences config to Lint/RedundantSafeNavigation
fatkodima
force-pushed
the
new-SafeNavigationOnNonNil-cop
branch
from
a547769 to
4a8d464
Compare
|
Updated existing cop instead of creating a new one. |
fatkodima
force-pushed
the
new-SafeNavigationOnNonNil-cop
branch
from
4a8d464 to
968b39c
Compare
| # In the example below, the safe navigation operator (`&.`) is unnecessary | ||
| # because `NilClass` has methods like `respond_to?` and `is_a?`. | ||
| # | ||
| # The `CheckReceiverPreviousOccurrences` option specifies whether to look into previous code |
There was a problem hiding this comment.
Perhaps something like InferNonNilReceiver, InferReceiverValue or perhaps AnalyzeReceiverValue would be a slightly better name here. Might be good to expand a bit on that the inferences is limited to the current scope with a couple of examples illustrating it.
| MSG = 'Redundant safe navigation detected, use `.` instead.' | ||
| MSG_LITERAL = 'Redundant safe navigation with default literal detected.' | ||
| MSG_NON_NIL = | ||
| 'Redundant safe navigation on non nil receiver (detected by previous code paths).' |
There was a problem hiding this comment.
non-nil
detected by analyzing previous code/method invocations
|
|
||
| NIL_METHODS = ( | ||
| nil.methods + | ||
| %i[present? blank? try try!] + # rails methods |
There was a problem hiding this comment.
Probably non-core methods should be in a configuration option. I'm guessing some people will want to add more methods here anyways for whatever reasons.
|
Overall the new version of the PR looks good to me. I left a few small remarks here and there. @rubocop/rubocop-core if someone has great naming suggestions for the config option - please share them! |
| # # bad | ||
| # if foo.condition? | ||
| # foo&.bar | ||
| # end |
There was a problem hiding this comment.
would it be possible to handle this special case?
# bad
if foo
foo&.bar
end(in one of our projects I found things like log(user&.name) if user which are currently false negatives)
| end | ||
|
|
||
| # rubocop:disable Metrics | ||
| def redundant?(node, receiver, checked_nodes) |
There was a problem hiding this comment.
should probably be renamed to be more specific (this entire cop is about redundancy checks, but this method is specifically for redundancy checks based on previous code paths)
this method might be more readable if packaged as a miniature service class (so you don't have to explicitly pass checked_nodes over and over and instead store it as an instance var)
| node.each_condition do |condition| | ||
| return true if redundant?(condition, receiver, checked_nodes) | ||
| end | ||
| when :lvasgn |
There was a problem hiding this comment.
:ivasgn, :cvasgn, :gvasgn, :casgn?
otherwise stuff like this is a false negative:
NAME = user.name
user&.name
fatkodima
force-pushed
the
new-SafeNavigationOnNonNil-cop
branch
from
968b39c to
be8da84
Compare
fatkodima
changed the title
CheckReceiverPreviousOccurrences config to Lint/RedundantSafeNavigationInferNonNilReceiver config to Lint/RedundantSafeNavigation
|
Thank you all for suggestions! Implemented all of them. |
5 participants
Closes #13835.
This cop detects safe navigation as unnecessary if there was some method call on the same receiver in the previous code paths.
cc @vlad-pisanov