Style/SafeNavigation throws an offense and can autocorrect into a style that violates another cop (Lint/SafeNavigationChain)

[brandonweiss]

Code that violates Style/SafeNavigation:

add_error(:ids, :parameter_validation, "Max limit for fetching conversations by ids is 50") if ids && ids.length > 50

Auto-corrects to code that violates Lint/SafeNavigationChain:

add_error(:ids, :parameter_validation, "Max limit for fetching conversations by ids is 50") if ids&.length > 50

This is especially problematic because it’s a style cop auto-correcting to code that violates a lint cop. That is, it's recommending (or auto-correcting) code that works fine to code that actually does not work.

---

$ rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.3.1 x86_64-darwin16)

[Drenmi]

Looks like Style/SafeNavigationChain suffers from the same issue that Style/SafeNavigation did. I.e. it considers operator methods part of the chain.

[chewi]

I think Style/SafeNavigationChain is fine, it's Style/SafeNavigation that needs to be smarter. If I start with this:

street.lines.first.to_s.strip.presence if street

Then it auto-corrects it to this, which is broken and violates Style/SafeNavigationChain:

street&.lines.first.to_s.strip.presence

To make it correct and satisfy both cops, I would need to change it to this:

street&.lines&.first&.to_s&.strip.presence

This is arguably slower and harder to read than the original.

[brandonweiss]

Yeah, essentially I don't think it’s correct for Style/SafeNavigation to throw an offense if the change it wants would throw an offense for Style/SafeNavigationChain. Style/SafeNavigation should include some of the logic from Style/SafeNavigationChain and check if there would be a chain or not.

[Tab10id]

It's maybe a little oftop but that:
street.lines.first.to_s.strip.presence if street
should not be modified to SafeNavigation. And more over it's shoud not be modified to SafeNavigationChain
when we have street.lines.first it's mean that lines is always exists and it's probably an array.
when we have street&.lines&.first we no that info about lines
It's horrible thing, it's like street.lines.first.to_s.strip rescue nil

[Tab10id]

I think that in normal cases we should not use more then one &. per line it's masks the real problem. It's says that we can get nil somewhere in chain but we have no idea where it will broken and It's not normal.

[Drenmi]

@Tab10id: I agree with you in principle, that the only function of the lonely operator is to make us feel better about bad code. However, it is the direction decided by the core team, so we'll have to work with it.

We might be able to add some more cops that can be used by teams who want to prevent writing nil leaky code.