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

EmptyConditionalBody autocorrect can result in breaking logic changes #12669

Open
CvX opened this issue Feb 1, 2024 · 1 comment
Open

EmptyConditionalBody autocorrect can result in breaking logic changes #12669

CvX opened this issue Feb 1, 2024 · 1 comment
Labels

Comments

@CvX
Copy link
Contributor

CvX commented Feb 1, 2024

Looking at EmptyConditionalBody spec, some autocorrect scenarios (in expect_correction blocks) actually result in logic-breaking changes. For example:

it 'registers an offense for missing `elsif` body' do
expect_offense(<<~RUBY)
if condition
do_something1
elsif other_condition1
^^^^^^^^^^^^^^^^^^^^^^ Avoid `elsif` branches without a body.
elsif other_condition2
do_something2
end
RUBY
expect_correction(<<~RUBY)
if condition
do_something1
elsif other_condition2
do_something2
end
RUBY
end

To make it clearer here's the same case but with more real-world-y variable names:

# before
if show_version
  puts VERSION
elsif dry_run
elsif delete_files
  `rm -rf *`
end

# after
if show_version
  puts VERSION
elsif delete_files
  `rm -rf *`
end

After an autocorrect run, dry_run flag is no longer used which is a breaking change.

RuboCop version

1.60.2 (using Parser 3.3.0.5, rubocop-ast 1.30.0, running on ruby 3.2.1) [arm64-darwin22]
/the latest on the main branch

@vlad-pisanov
Copy link

The autocorrect is already marked as unsafe, but it could be improved to replace internal elsif branch bodies with nil, as opposed to removing them.

if show_version
  puts VERSION
elsif dry_run # internal branch, replace with `nil`
  nil
elsif delete_files
  `rm -rf *`
end

@koic koic added the bug label Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants