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 #8006] Fix infinite loop for Style/IfUnlessModifier #8026

Closed
wants to merge 1 commit into from

Conversation

tejasbubane
Copy link
Contributor

@tejasbubane tejasbubane commented May 24, 2020

Closes #8006
Closes #8025
Closes #7969


Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

CHANGELOG.md Show resolved Hide resolved
spec/rubocop/cop/style/if_unless_modifier_spec.rb Outdated Show resolved Hide resolved
@tejasbubane tejasbubane force-pushed the fix-8006 branch 2 times, most recently from 2ddb69c to 7f6ebcf Compare May 25, 2020 10:56
@joe-p
Copy link

joe-p commented May 25, 2020

The following line will still produce an infinite loop with this commit pulled into master:

puts "                                                                                                    #{' ' if true}"
$ rubocop -ad test.rb
For /home/joe/rubo_test/test: Default configuration from /home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/config/default.yml
Inspecting 1 file
Scanning /home/joe/rubo_test/test/test.rb
C

Offenses:

test.rb:3:109: C: [Corrected] Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
puts "                                                                                                    #{if true
                                                                                                            ^^
test.rb:3:113: C: [Corrected] Style/IfUnlessModifier: Modifier form of if makes the line too long.
puts "                                                                                                    #{' ' if true}"
                                                                                                                ^^

0 files inspected, 2 offenses detected, 2 offenses corrected
Infinite loop detected in /home/joe/rubo_test/test/test.rb.
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:290:in `check_for_infinite_loop'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:273:in `block in iterate_until_no_changes'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:272:in `loop'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:272:in `iterate_until_no_changes'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:243:in `do_inspection_loop'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:122:in `block in file_offenses'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:146:in `file_offense_cache'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:120:in `file_offenses'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:111:in `process_file'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:90:in `block in each_inspected_file'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:89:in `each'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:89:in `reduce'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:89:in `each_inspected_file'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:78:in `inspect_files'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/runner.rb:39:in `run'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/cli/command/execute_runner.rb:21:in `execute_runner'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/cli/command/execute_runner.rb:13:in `run'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/cli/command.rb:10:in `run'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/cli/environment.rb:17:in `run'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/cli.rb:65:in `run_command'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/cli.rb:72:in `execute_runners'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/lib/rubocop/cli.rb:41:in `run'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/exe/rubocop:13:in `block in <top (required)>'
/home/joe/.rvm/rubies/ruby-2.7.1/lib/ruby/2.7.0/benchmark.rb:308:in `realtime'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/gems/rubocop-0.84.0/exe/rubocop:12:in `<top (required)>'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/bin/rubocop:23:in `load'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/bin/rubocop:23:in `<main>'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/bin/ruby_executable_hooks:24:in `eval'
/home/joe/.rvm/gems/ruby-2.7.1@kisscraft/bin/ruby_executable_hooks:24:in `<main>'
Finished in 0.10348649999991721 seconds

@tejasbubane
Copy link
Contributor Author

@joe-p Now it should not.

@tejasbubane tejasbubane changed the title Fix infinite loop for Style/IfUnlessModifier [Fix #8006] Fix infinite loop for Style/IfUnlessModifier May 31, 2020
RUBY
end

it 'does not result in infinite loop with any other statement' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An infinite loop error is caused by a combination of cops. Con you add a reproduction test for infinite loop error to spec/rubocop/cli/cli_autocorrect_spec.rb? E.g.: https://github.com/rubocop-hq/rubocop/pull/7901/files#diff-0e7dbdd6e99e0defc2bab2cb544b42da
And I think the description of this test better to be updated to not register an offense.

@iGEL
Copy link
Contributor

iGEL commented Jul 9, 2020

I'd love to see this merged. Is there anything I could help with?

@tejasbubane
Copy link
Contributor Author

@iGEL I missed @koic's comment. Working on it now.

@marcandre
Copy link
Contributor

I also opened #8283 (but didn't know about this)

As I wrote there, I feel it is imperative that the code that builds the replacement string should be used for the calculation length.

The same bugs exist for WhileUntilModifier too.

The refactoring I made yesterday should help a bit.

@tejasbubane
Copy link
Contributor Author

I feel it is imperative that the code that builds the replacement string should be used for the calculation length.

I agree with @marcandre's approach. Closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants