Style/SafeNavigation does't understand order of operations

[ptarjan]

---

Expected behavior

This should't be flagged as an error

Actual behavior

-      while ref && ref.children.length > 0
+      while ref&.children&.length > 0

Breaks the code with:

undefined method `>' for nil:NilClass

Steps to reproduce the problem

RuboCop version

Include the output of rubocop -V. Here's an example:

$ rubocop -V
0.52.1 (using Parser 2.4.0.2, running on ruby 2.4.1 x86_64-darwin16)

[bunnymatic]

I ran into a similar problem with the following construct

if user && user.thing.blank?
   ... do stuff
end

This cop auto-corrects this to

if user&.thing.blank?
  ... do stuff
end

Which is not the same.

The first construct does not run the do stuff piece when user is nil.
The second actually does do stuff because the user&.thing returns nil and nil.blank? returns true.

Granted, the statement would probably be better written with a guard clause (which I'll do to avoid rubocop complaining and or fixing it incorrectly). But it seems like this cop might need to be a bit smarter about the order of operations.

[rrosenblum]

@ptarjan & @bunnymatic this was addressed in #5419. Please test against master and let me know if there is still any issues.

We will no longer register an offense for ref && ref.children.length > 0 since > cannot be elegantly converted to use safe navigation. Technically, we could correct it to ref&.children&.length&.> 0 or ref&.children&.length &.> 0, but I am not sure that we want to advocate for safe navigation with non dot methods.

Style/SafeNavigation will also now handle converting all methods in a chain to use safe navigation so the correction of user && user.thing.blank? will now be user&.thing&.blank?.

[russell-stripe]

Style/SafeNavigation will also now handle converting all methods in a chain to use safe navigation so the correction of user && user.thing.blank? will now be user&.thing&.blank?.

@rrosenblum that's not a safe transformation either. If user is non-nil and thing is nil, that transformation changes the behavior from a NoMethodError to silently returning nil.

[rrosenblum]

@russell-stripe the example that you provided will not produce a NoMethodError because blank? is implemented on nil, however I do see your point. Lint/SafeNavigationChain was already handling converting methods in a chain to use safe navigation. The change to Style/SafeNavigation was to follow the same kind of changes.

This is an odd issue because it borders on the edge of how is the code explicitly written vs the intention of the code. There was something similar mentioned in another issue that user && user.thing.blank? would imply that we know user can be nil, but if user is not nil then thing is guaranteed to be there.

I think you raise a good point, however I think there is a much larger conversation that is needed around this. Should we stop flagging method chains to use safe navigation all together? What would this mean for SafeNavigationChain? By eliminating a potential NoMethodError are we helping the code more than leaving code that code produce a NoMethodError. Should there be a cop that flags user && user.thing.other_thing as code that could produce a NoMethodError?

@bbatsov @jonas054 @pocke @Drenmi care to weigh in on this?

[bunnymatic]

From my perspective, the fix is fine. At some point, if you need to safe navigate that far down into an object, maybe that in itself is a smell and you should revamp the code. I appreciate your looking into this. If nothing else, it forced me to rewrite my code to not be so smelly. So the cops are keeping me on the straight and narrow. Job well done.

[jonas054]

Should there be a cop that flags user && user.thing.other_thing as code that could produce a NoMethodError?

I foresee a lot of false positives on that one. I think the normal case is that the author knows or have ensured that the thing object responds to other_thing if user is non-nil. It would have to be disabled by default if we add such a cop.

About user && user.thing.blank?, I think Style/SafeNavigation should borrow some functionality from Lint/SafeNavigationChain and allow such constructs when the method call is something that nil responds to (such as nil?), or one that's in a white list (such as blank?).

[ptarjan]

Thanks for the discussion. I’d like to note that any false positives for this autocorrect make it impossible for us to apply it to our whole codebase. Is it possible to have two modes? A sound and simple one and a more aggressive one with the false positives?

…

On Thu, Feb 22, 2018 at 1:32 AM Jonas Arvidsson ***@***.***> wrote: Should there be a cop that flags user && user.thing.other_thing as code that could produce a NoMethodError? I foresee a lot of false positives on that one. I think the normal case is that the author knows or have ensured that the thing object responds to other_thing if user is non-nil. It would have to be disabled by default if we add such a cop. About user && user.thing.blank?, I think Style/SafeNavigation should borrow some functionality from Lint/SafeNavigationChain and allow such constructs when the method call is something that nil responds to (such as nil?), or one that's in a white list (such as blank?). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5527 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACczwaMqRZVrfRLYBmBSXU0sNsJaGgEks5tXTQxgaJpZM4RxU-b> .

[jonas054]

@ptarjan I guess we could have two modes, but I don't think it's necessary. Could you take a look at my PR, which changes Style/SafeNavigation to be more cautious?