New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lint/RedundantSafeNavigation doesn't account for ActiveSupport core-ext #8867
Comments
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 Things can get tricky though, because what if 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 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 |
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 Does anyone have a real use-case for this cop that isn't The comments https://github.com/rubocop-hq/rubocop/blob/db37beaf077ab68880b00f37949ebbfb8852bfed/lib/rubocop/cop/lint/redundant_safe_navigation.rb#L6-L23 and PR #8668 don't really mention a case outside of A few more examples where this cop would erroneously flag:
The cop I would like (that I don't know is possible) is detecting when |
Well, the purpose of the cop is simple - eliminate redundant use of //cc @fatkodima |
Well, I guess that depends on your perspective. For me it a method is |
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.
We can't, so this is why this cop is marked as
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 guess my point is, "safe" is a bit debatable. Even with the example given 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 |
@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. |
I just reread #8668. What's interesting as well, is 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
The original code was twice as fast. |
I was mistaken. Sorry about that. |
just my 2c: I love blindly trusting autocorrect. This autocorrect would have caused a bunch of regressions in our code because p.s. I love rubocop and I think you are all the coolest. |
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 |
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. |
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 Besides the 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 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. My conclusion: let's delete this cop, or make an allow list ( |
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.
I agree. Though, as I mentioned above, it's 2.19x faster to call Thanks for all you guys do. |
Good points. Apologies guys, I will rework it today to be safe. |
I just updated to RuboCop 0.93.0. Lint/RedundantSafeNavigation is suggesting an incorrect action.
RuboCop seems to assume that the
&
is unnecessary, becausenil
responds toto_s
. The problem is,to_s(:delimited)
is a core-ext onNumeric
that ActiveSupport adds.https://github.com/rails/rails/blob/7905bdfd8b2ae50319cd7a9a74ee1f8c865d648d/activesupport/lib/active_support/core_ext/numeric/conversions.rb#L122-L123
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.
RuboCop version
Include the output of
rubocop -V
orbundle exec rubocop -V
if using Bundler. Here's an example:The text was updated successfully, but these errors were encountered: