Rubocop: Lint/RedundantSafeNavigation doesn't account for ActiveSupport core-ext

[natematykiewicz]

I just updated to RuboCop 0.93.0. Lint/RedundantSafeNavigation is suggesting an incorrect action.

mileage&.to_s(:delimited)
       ^^^^^^^^^^^^^^^^^^

RuboCop seems to assume that the & is unnecessary, because nil responds to to_s. The problem is, to_s(:delimited) is a core-ext on Numeric that ActiveSupport adds.
https://github.com/rails/rails/blob/7905bdfd8b2ae50319cd7a9a74ee1f8c865d648d/activesupport/lib/active_support/core_ext/numeric/conversions.rb#L122-L123

>> 123456.to_s(:delimited)   #=> "123,456"
>> nil.to_s(:delimited)
Traceback (most recent call last):
        2: from (irb):2
        1: from (irb):2:in `to_s'
ArgumentError (wrong number of arguments (given 1, expected 0))

---

Expected behavior

Don't flag these this line.

Actual behavior

It flagged this line.

Steps to reproduce the problem

This code shouldn't get flagged.

val = 123_456
val&.to_s(:delimited)

val = nil
val&.to_s(:delimited)

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler. Here's an example:

$ [bundle exec] rubocop -V
0.93.0 (using Parser 2.7.2.0, rubocop-ast 0.7.1, running on ruby 2.5.1 x86_64-linux)

[natematykiewicz]

I think a solution to this could be to see if nil responds to the method, with an arity of the same amount of arguments that are being handed in. Since I'm handing in 1 argument, but nil.to_s only allows for 0 arguments, it's definitely not a safe alternative.

Things can get tricky though, because what if nil DID respond with 1 argument, but :delimited wasn't an allowed option for NilClass#to_s (or maybe returned a different value)?

The other thing at play here is, what if I don't want an empty string? What if I either want a stringified number that contains a delimiter or nil? The & means that I don't call to_s on a nil at all -- whether nil responds to to_s is irrelevant, because I only want to to_s non-nils. It feels like RuboCop would be pushing me into val.to_s.presence, which is a pointless string allocation and present? regex check.

I'm not sure what to do about this this. I like the intention of this cop for sure. I've seen times in our code where people add an unnecessary & (on methods that don't return nil ever, for example). I just don't know how to implement it in a way that guarantees removing the & won't change the output.

[natematykiewicz]

The more I think about it, the more I don't understand the purpose of this cop. I don't understand why it's safe to assume that NilClass responding to a method you call, means you want _that_ method. Even if we were comparing that it's truly the same method (and not one that happens to have the same name), it seems like a bold leap to say that the & is unnecessary.

Does anyone have a real use-case for this cop that isn't foo and bar?

The comments https://github.com/rubocop-hq/rubocop/blob/db37beaf077ab68880b00f37949ebbfb8852bfed/lib/rubocop/cop/lint/redundant_safe_navigation.rb#L6-L23 and PR https://github.com/rubocop-hq/rubocop/issues/8668 don't really mention a case outside of respond_to?.

A few more examples where this cop would erroneously flag:

nil&.blank? # nil for nil, true/false for non-nil
nil&.present? # same as above

The cop I would like (that I don't know is possible) is detecting when & is used on a receiver that can never be nil.

[bbatsov]

Well, the purpose of the cop is simple - eliminate redundant use of .& for methods that normally operate safely on nil. All the examples for problems are coming from Rails, which I definitely didn't consider when reviewing the cop (e.g. I didn't even know how Rails extended to_s), so we're off the mark for sure. We'll do better with the next iteration. :-)

//cc @fatkodima

[bbatsov]

The more I think about it, the more I don't understand the purpose of this cop. I don't understand why it's safe to assume that NilClass responding to a method you call, means you want that method.

Well, I guess that depends on your perspective. For me it a method is nil safe tagging its call with .& just confuses the reader, because it implies it's not. I don't think that the example with blank? and present? are good, as lots of people have complained over the years about their API - in particular why isn't nil considered blank? That would be my expectation for such a method. I guess it's clear that I'm not a huge fan of ActiveSupport extensions, but I understand well enough we need to deal with them gracefully given the prominence of Rails.

[fatkodima]

I think a solution to this could be to see if nil responds to the method, with an arity of the same amount of arguments that are being handed in. Since I'm handing in 1 argument, but nil.to_s only allows for 0 arguments, it's definitely not a safe alternative.

This won't help, because rubocop does static analysis (without loading the actual application code), so we do not have info about what libraries extend which classes and how.

I just don't know how to implement it in a way that guarantees removing the & won't change the output.

We can't, so this is why this cop is marked as Safe: false.

A few more examples where this cop would erroneously flag:

nil&.blank? # nil for nil, true/false for non-nil
nil&.present? # same as above

This cop won't mark those lines as offenses.

[natematykiewicz]

I guess my point is, "safe" is a bit debatable. Even with the example given nil&.respond_to?(:to_s) is not the same as nil.respond_to?(:to_s). The first returns nil, the second returns true.

I hope I don't sound ungrateful here. I appreciate this gem. I try to avoid turning off any cops, as I genuinely find them helpful. I'm just trying to understand what some real world examples of this working are.

Basically the goal is anything from Object and BasicObject shouldn't have a & in front of it?

[natematykiewicz]

@bbatsov to be clear, nil is blank in Rails. My point was just that nil is not the same as false. In a truthy check it might be fine, but it's definitely not the same return value.

My hope is that this cop wouldn't change the code's meaning. I'm just not sure how to actually make the cop do it well. But I guess that's why I think the only valid version of this cop is one that checks if the receiver can't be nil. But that sounds like a Ruby 3 job, and not something RuboCop can figure out.

[natematykiewicz]

I just reread https://github.com/rubocop-hq/rubocop/issues/8668.

What's interesting as well, is next unless attrs && attrs.respond_to?(:[]) may have actually been a performance optimization. Calling respond_to? takes time. So, if the value is falsey, the original code would have skipped the respond_to? call all together.

require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('&&') { nil && nil.respond_to?(:[]) }
  x.report('&.') { nil&.respond_to?(:[]) }
  x.report('.') { nil.respond_to?(:[]) }

  x.compare!
end

Warming up --------------------------------------
                  &&     2.782M i/100ms
                  &.     2.915M i/100ms
                   .     1.361M i/100ms
Calculating -------------------------------------
                  &&     27.578M (± 7.0%) i/s -    139.100M in   5.070451s
                  &.     27.831M (± 5.6%) i/s -    139.903M in   5.043730s
                   .     12.737M (± 6.6%) i/s -     63.974M in   5.045247s

Comparison:
                  &.: 27831229.0 i/s
                  &&: 27578494.0 i/s - same-ish: difference falls within error
                   .: 12736655.0 i/s - 2.19x  (± 0.00) slower

The original code was twice as fast.

[natematykiewicz]

nil&.blank? # nil for nil, true/false for non-nil
nil&.present? # same as above

This cop won't mark those lines as offenses.

I was mistaken. Sorry about that.

[michaelglass]

just my 2c: I love blindly trusting autocorrect. This autocorrect would have caused a bunch of regressions in our code because nil and "" (empty string) are different enough.

p.s. I love rubocop and I think you are all the coolest.

[dvandersluis]

I think as is, this cop doesn't really satisfy what the rubocop documentation defines as a Lint IMO, given the concerns here and in #8868. I wonder if we should disable the cop by default, if not reconsider it completely, but maybe moving it to the Style namespace as well would be a good move?

[bbatsov]

I've been pondering this myself, but I've decided to keep the conversation going for a couple of days as I probably won't issue a new release before Sunday/Monday.

[marcandre]

I'm sorry I missed that cop in passing, I would have given my 2 cents: this cop makes no sense to me except in narrow condition: within a condition and with a predicate of Object

Besides the respond_to?, only case I can think of:

if anything&.is_a?(anything_else) # or instance_of? (and I don't use instance_of?)
# should be =>
if anything&.is_a?(anything_else)

This example requires the if (or unless/while/until)

I can't think of other circumstances where this cop could intervene in a meaningful fashion.

FWIW, the current bug report is not dependent on ActiveSupport. foo&.to_s is perfectly acceptable code and has no correct equivalent (since nil and "" are very different objects in Ruby).

My conclusion: let's delete this cop, or make an allow list (respond_to? + is_a? + instance_of?) + restrict to conditions and make a bug release ASAP.

[natematykiewicz]

@marcandre

I'm sorry I missed that cop in passing

I don't think you have anything to be sorry for. At first glance it looked like a good idea. When it first marked offenses, I thought it was right. But after giving it more thought, I think there's just too many pitfalls.

My conclusion: let's delete this cop, or make an allow list (respond_to? + is_a? + instance_of?) + restrict to conditions and make a bug release ASAP.

I agree. Though, as I mentioned above, it's 2.19x faster to call nil&.respond_to? than nil.respond_to?. I feel this cop might encourage people to write less performant code, which I know performance and readability can be tradeoffs at times, I just don't think the one less & is worth the performance hit. What you guys do is your call, but at this point, I'll personally just disable it if it doesn't get removed soon (I haven't actually accepted the 0.93.0 update PR on my app yet, still running the previous version).

Thanks for all you guys do.

[fatkodima]

Good points. Apologies guys, I will rework it today to be safe.