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

[Fix #9980] Fix a false positive for Style/IdenticalConditionalBranches #9982

Conversation

koic
Copy link
Member

@koic koic commented Aug 5, 2021

Fixes #9980.

This PR fixes a false positive for Style/IdenticalConditionalBranches when assigning to a variable used in a condition.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

…alBranches`

Fixes rubocop#9980.

This PR fixes a false positive for `Style/IdenticalConditionalBranches`
when assigning to a variable used in a condition.
@Drowze
Copy link
Contributor

Drowze commented Aug 5, 2021

One case that I had issues with autocorrection for this particular cop on our codebase is specifically as follows:

if shipment.assignments.find_by(employee_id: employee_id)
  shipment.assignments.destroy_all
else
  shipment.assignments.destroy_all

  # do other stuff
end

When running this cop's autocorrection, that excerpt was refactored to:

shipment.assignments.destroy_all

if shipment.assignments.find_by(employee_id: employee_id)  
else
  # do other stuff
end

After rubocop's autocorrection, the code always falls to the else block. I think this PR wouldn't solve that particular case, right? I'm also not sure if there's a way for this cop to be safe on such cases 🤔

@vlad-pisanov
Copy link

vlad-pisanov commented Aug 6, 2021

I believe this autocorrect is inherently unsafe. Consider:

if method_that_modifies_global_state
  method_that_relies_on_global_state
  foo
else
  method_that_relies_on_global_state
  goo
end

There's no way to safely factor out method_that_relies_on_global_state before method_that_modifies_global_state is executed. method_that_modifies_global_state could modify all sorts of things like global vars, mocks, DB records, etc. that method_that_relies_on_global_state needs to function properly.

@koic
Copy link
Member Author

koic commented Aug 6, 2021

@vlad-pisanov Ah... you're right! This cop can try to reduce false positives, but I understand that it cannot be safe auto-correction because a side effect of method invocation.

@koic
Copy link
Member Author

koic commented Aug 6, 2021

I opened a separated PR #9985 because I think this should be marked as unsafe for auto-correction.
https://docs.rubocop.org/rubocop/1.18/usage/auto_correct.html#safe-auto-correct

koic added a commit to koic/rubocop that referenced this pull request Aug 6, 2021
Follow up to rubocop#9982 (comment)

This PR marks `Style/IdenticalConditionalBranches` as unsafe auto-correction.

`Style/IdenticalConditionalBranches` cop is marked unsafe auto-correction
as the order of method calls must be guaranteed in the following case:

```ruby
if method_that_modifies_global_state # 1
  method_that_relies_on_global_state # 2
  foo                                # 3
else
  method_that_relies_on_global_state # 2
  bar                                # 3
end
```

In such a case, auto-correction may change the invocation order.
@bbatsov bbatsov merged commit cda3f43 into rubocop:master Aug 12, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 12, 2021

Thanks for tackling this!

@koic koic deleted the fix_false_positive_for_style_identical_conditional_branches branch August 12, 2021 07:33
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 this pull request may close these issues.

Style/IdenticalConditionalBranches should be marked as unsafe
4 participants