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

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.

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.

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.

@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.

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.

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?

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.