Skip to content
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

Style/RedundantCondition incorrectly reports error with range in ternary #7709

Closed
mehagar opened this issue Feb 12, 2020 · 1 comment · Fixed by #7738
Closed

Style/RedundantCondition incorrectly reports error with range in ternary #7709

mehagar opened this issue Feb 12, 2020 · 1 comment · Fixed by #7738

Comments

@mehagar
Copy link
Contributor

mehagar commented Feb 12, 2020

The Style/RedundantCondition check can product a false positive when the second part of the ternary condition contains the .. operator. This is because of the order of precedence - in order, it is:

  1. ||
  2. ..
  3. ?:

Since the check replaces ?: with ||, the expression may not evaluate to the same thing as before.


Expected behavior

No error reported for

time_period = updated_during ? updated_during : 2.days.ago..Time.now

Actual behavior

It reports this error:

`Style/RedundantCondition: Use double pipes || instead.
    time_period = updated_during ? updated_during : 2.days.ago..Time.now
                                 ^^^^^^^^^^^^^^^^^^

Autofix will change the expression to

time_period = updated_during || 2.days.ago..Time.now

and when updated_during is not nil, the expression will always throw a bad range exception.

Steps to reproduce the problem

Enter the code above and run the rubocop on the file.

RuboCop version

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

$ exe/rubocop -V
0.79.0 (using Parser 2.7.0.2, running on ruby 2.7.0 x86_64-darwin19)
@rrosenblum
Copy link
Contributor

rrosenblum commented Feb 14, 2020

This is very interesting. It seems like the order of operations for || and .. isn't what you would expect it to be

[5] pry(#<RuboCop::Cop::Style::RedundantCondition>)> node.source
=> "updated_during || 2.days.ago"
[6] pry(#<RuboCop::Cop::Style::RedundantCondition>)> node
=> s(:or,
  s(:send, nil, :updated_during),
  s(:send,
    s(:send,
      s(:int, 2), :days), :ago))
[7] pry(#<RuboCop::Cop::Style::RedundantCondition>)> node.parent.source
=> "updated_during || 2.days.ago..Time.now"
[8] pry(#<RuboCop::Cop::Style::RedundantCondition>)> node.parent
=> s(:irange,
  s(:or,
    s(:send, nil, :updated_during),
    s(:send,
      s(:send,
        s(:int, 2), :days), :ago)),
  s(:send,
    s(:const, nil, :Time), :now))
[9] pry(#<RuboCop::Cop::Style::RedundantCondition>)> node.parent.parent.source
=> "time_period = updated_during || 2.days.ago..Time.now"
[10] pry(#<RuboCop::Cop::Style::RedundantCondition>)> node.parent.parent
=> s(:lvasgn, :time_period,
  s(:irange,
    s(:or,
      s(:send, nil, :updated_during),
      s(:send,
        s(:send,
          s(:int, 2), :days), :ago)),
    s(:send,
      s(:const, nil, :Time), :now)))

Essentially Ruby is treating this as time_period = (updated_during || 2.days.ago)..Time.now when we want, and think, it to be treating this as time_period = updated_during || (2.days.ago..Time.now).

The best solution to this would seem to be that we should check if the "else branch" is a range and wrap it in parenthesis if it is. I feel like there could be some other similar edge cases, but I can't think of any right now.

rrosenblum added a commit to rrosenblum/rubocop that referenced this issue Feb 20, 2020
rrosenblum added a commit to rrosenblum/rubocop that referenced this issue Feb 20, 2020
rrosenblum added a commit to rrosenblum/rubocop that referenced this issue Feb 20, 2020
rrosenblum added a commit to rrosenblum/rubocop that referenced this issue Feb 21, 2020
ty-porter pushed a commit to ty-porter/rubocop that referenced this issue Mar 21, 2020
Neodelf pushed a commit to Neodelf/rubocop that referenced this issue Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants