From 35d3abc4be49a825c83fa6337c69853744ff229e Mon Sep 17 00:00:00 2001 From: Igor Tsykalo Date: Fri, 11 Sep 2020 20:33:00 +0300 Subject: [PATCH] [Fix #8621] Helpful Infinite Loop message Closes #8621 Adds the information about which Cops exactly caused an infinite loop --- CHANGELOG.md | 2 + lib/rubocop/runner.rb | 38 +++++--- spec/rubocop/runner_spec.rb | 148 ++++++++++++++++++++++++++------ spec/support/cops/a_to_b_cop.rb | 27 ++++++ spec/support/cops/b_to_a_cop.rb | 27 ++++++ spec/support/cops/b_to_c_cop.rb | 27 ++++++ spec/support/cops/c_to_a_cop.rb | 27 ++++++ spec/support/file_helper.rb | 2 + 8 files changed, 259 insertions(+), 39 deletions(-) create mode 100644 spec/support/cops/a_to_b_cop.rb create mode 100644 spec/support/cops/b_to_a_cop.rb create mode 100644 spec/support/cops/b_to_c_cop.rb create mode 100644 spec/support/cops/c_to_a_cop.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index fbf7a49a484..8871df4ff79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ * [#8617](https://github.com/rubocop-hq/rubocop/issues/8617): Fix `Style/HashAsLastArrayItem` to not register an offense when all items in an array are hashes. ([@dvandersluis][]) * [#8500](https://github.com/rubocop-hq/rubocop/issues/8500): Add `in?` to AllowedMethods for `Lint/SafeNavigationChain` cop. ([@tejasbubane][]) * [#8629](https://github.com/rubocop-hq/rubocop/pull/8629): Fix the cache being reusable in CI by using crc32 to calculate file hashes rather than `mtime`, which changes each CI build. ([@dvandersluis][]) +* [#8621](https://github.com/rubocop-hq/rubocop/issues/8621): Add helpful Infinite Loop error message. ([@iSarCasm][]) ## 0.90.0 (2020-09-01) @@ -4862,3 +4863,4 @@ [@jaimerave]: https://github.com/jaimerave [@Skipants]: https://github.com/Skipants [@sascha-wolf]: https://github.com/sascha-wolf +[@iSarCasm]: https://github.com/iSarCasm diff --git a/lib/rubocop/runner.rb b/lib/rubocop/runner.rb index 44b23ee246b..f639f667e4f 100644 --- a/lib/rubocop/runner.rb +++ b/lib/rubocop/runner.rb @@ -11,9 +11,12 @@ class Runner # rubocop:disable Metrics/ClassLength class InfiniteCorrectionLoop < RuntimeError attr_reader :offenses - def initialize(path, offenses) - super "Infinite loop detected in #{path}." - @offenses = offenses + def initialize(path, offenses_by_iteration, loop_start: -1) + @offenses = offenses_by_iteration.flatten.uniq + root_cause = offenses_by_iteration[loop_start..-1] + .map { |x| x.map(&:cop_name).uniq.join(', ') } + .join(' -> ') + super "Infinite loop detected in #{path} and caused by #{root_cause}" end end @@ -236,18 +239,21 @@ def save_in_cache(cache, offenses) def do_inspection_loop(file) processed_source = get_processed_source(file) - offenses = [] + # This variable is 2d array used to track corrected offenses after each + # inspection iteration. This is used to output meaningful infinite loop + # error message. + offenses_by_iteration = [] # When running with --auto-correct, we need to inspect the file (which # includes writing a corrected version of it) until no more corrections # are made. This is because automatic corrections can introduce new # offenses. In the normal case the loop is only executed once. - iterate_until_no_changes(processed_source, offenses) do + iterate_until_no_changes(processed_source, offenses_by_iteration) do # The offenses that couldn't be corrected will be found again so we # only keep the corrected ones in order to avoid duplicate reporting. - offenses.select!(&:corrected?) + !offenses_by_iteration.empty? && offenses_by_iteration.last.select!(&:corrected?) new_offenses, updated_source_file = inspect_file(processed_source) - offenses.concat(new_offenses).uniq! + offenses_by_iteration.push(new_offenses) # We have to reprocess the source to pickup the changes. Since the # change could (theoretically) introduce parsing errors, we break the @@ -257,10 +263,12 @@ def do_inspection_loop(file) processed_source = get_processed_source(file) end + # Return summary of corrected offenses after all iterations + offenses = offenses_by_iteration.flatten.uniq [processed_source, offenses] end - def iterate_until_no_changes(source, offenses) + def iterate_until_no_changes(source, offenses_by_iteration) # Keep track of the state of the source. If a cop modifies the source # and another cop undoes it producing identical source we have an # infinite loop. @@ -272,10 +280,10 @@ def iterate_until_no_changes(source, offenses) iterations = 0 loop do - check_for_infinite_loop(source, offenses) + check_for_infinite_loop(source, offenses_by_iteration) if (iterations += 1) > MAX_ITERATIONS - raise InfiniteCorrectionLoop.new(source.path, offenses) + raise InfiniteCorrectionLoop.new(source.path, offenses_by_iteration) end source = yield @@ -285,11 +293,15 @@ def iterate_until_no_changes(source, offenses) # Check whether a run created source identical to a previous run, which # means that we definitely have an infinite loop. - def check_for_infinite_loop(processed_source, offenses) + def check_for_infinite_loop(processed_source, offenses_by_iteration) checksum = processed_source.checksum - if @processed_sources.include?(checksum) - raise InfiniteCorrectionLoop.new(processed_source.path, offenses) + if (loop_start_index = @processed_sources.index(checksum)) + raise InfiniteCorrectionLoop.new( + processed_source.path, + offenses_by_iteration, + loop_start: loop_start_index + ) end @processed_sources << checksum diff --git a/spec/rubocop/runner_spec.rb b/spec/rubocop/runner_spec.rb index ef80c03f3e7..10cb554a02e 100644 --- a/spec/rubocop/runner_spec.rb +++ b/spec/rubocop/runner_spec.rb @@ -189,23 +189,7 @@ def INVALID_CODE; end end describe '#run with cops autocorrecting each-other' do - subject(:runner) do - runner_class = Class.new(RuboCop::Runner) do - def mobilized_cop_classes(_config) - RuboCop::Cop::Registry.new( - [ - RuboCop::Cop::Test::ClassMustBeAModuleCop, - RuboCop::Cop::Test::ModuleMustBeAClassCop - ] - ) - end - end - runner_class.new(options, RuboCop::ConfigStore.new) - end - - before do - create_file('example.rb', source) - end + let(:source_file_path) { create_file('example.rb', source) } let(:options) do { @@ -214,17 +198,129 @@ def mobilized_cop_classes(_config) } end - context 'if there is an offense in an inspected file' do - let(:source) { <<~RUBY } - # frozen_string_literal: true - class Klass + context 'with two conflicting cops' do + subject(:runner) do + runner_class = Class.new(RuboCop::Runner) do + def mobilized_cop_classes(_config) + RuboCop::Cop::Registry.new( + [ + RuboCop::Cop::Test::ClassMustBeAModuleCop, + RuboCop::Cop::Test::ModuleMustBeAClassCop + ] + ) + end end - RUBY + runner_class.new(options, RuboCop::ConfigStore.new) + end - it 'aborts because of an infinite loop' do - expect do - runner.run([]) - end.to raise_error RuboCop::Runner::InfiniteCorrectionLoop + context 'if there is an offense in an inspected file' do + let(:source) { <<~RUBY } + # frozen_string_literal: true + class Klass + end + RUBY + + it 'aborts because of an infinite loop' do + expect do + runner.run([]) + end.to raise_error( + RuboCop::Runner::InfiniteCorrectionLoop, + "Infinite loop detected in #{source_file_path} and caused by " \ + 'Test/ClassMustBeAModuleCop -> Test/ModuleMustBeAClassCop' + ) + end + end + + context 'if there are multiple offenses in an inspected file' do + let(:source) { <<~RUBY } + # frozen_string_literal: true + class Klass + end + class AnotherKlass + end + RUBY + + it 'aborts because of an infinite loop' do + expect do + runner.run([]) + end.to raise_error( + RuboCop::Runner::InfiniteCorrectionLoop, + "Infinite loop detected in #{source_file_path} and caused by " \ + 'Test/ClassMustBeAModuleCop -> Test/ModuleMustBeAClassCop' + ) + end + end + end + + context 'with two pairs of conflicting cops' do + subject(:runner) do + runner_class = Class.new(RuboCop::Runner) do + def mobilized_cop_classes(_config) + RuboCop::Cop::Registry.new( + [ + RuboCop::Cop::Test::ClassMustBeAModuleCop, + RuboCop::Cop::Test::ModuleMustBeAClassCop, + RuboCop::Cop::Test::AtoB, + RuboCop::Cop::Test::BtoA + ] + ) + end + end + runner_class.new(options, RuboCop::ConfigStore.new) + end + + context 'if there is an offense in an inspected file' do + let(:source) { <<~RUBY } + # frozen_string_literal: true + class A_A + end + RUBY + + it 'aborts because of an infinite loop' do + expect do + runner.run([]) + end.to raise_error( + RuboCop::Runner::InfiniteCorrectionLoop, + "Infinite loop detected in #{source_file_path} and caused by " \ + 'Test/ClassMustBeAModuleCop, Test/AtoB ' \ + '-> Test/ModuleMustBeAClassCop, Test/BtoA' + ) + end + end + + context 'with three cop cycle' do + subject(:runner) do + runner_class = Class.new(RuboCop::Runner) do + def mobilized_cop_classes(_config) + RuboCop::Cop::Registry.new( + [ + RuboCop::Cop::Test::AtoB, + RuboCop::Cop::Test::BtoC, + RuboCop::Cop::Test::CtoA + ] + ) + end + end + runner_class.new(options, RuboCop::ConfigStore.new) + end + + context 'if there is an offense in an inspected file' do + let(:source) { <<~RUBY } + # frozen_string_literal: true + class A + end + RUBY + + it 'aborts because of an infinite loop' do + expect do + runner.run([]) + end.to raise_error( + RuboCop::Runner::InfiniteCorrectionLoop, + "Infinite loop detected in #{source_file_path} and caused by " \ + 'Test/AtoB -> Test/BtoC -> Test/CtoA' + ) + end + end end end end diff --git a/spec/support/cops/a_to_b_cop.rb b/spec/support/cops/a_to_b_cop.rb new file mode 100644 index 00000000000..7d17f8101bc --- /dev/null +++ b/spec/support/cops/a_to_b_cop.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Test + # This cop is only used to test infinite loop detection + class AtoB < RuboCop::Cop::Base + extend AutoCorrector + + def on_class(node) + return unless /A/.match?(node.loc.name.source) + + add_offense(node.loc.name, message: 'A to B') do |corrector| + autocorrect(corrector, node) + end + end + alias on_module on_class + + private + + def autocorrect(corrector, node) + corrector.replace(node.loc.name, node.loc.name.source.tr('A', 'B')) + end + end + end + end +end diff --git a/spec/support/cops/b_to_a_cop.rb b/spec/support/cops/b_to_a_cop.rb new file mode 100644 index 00000000000..00308f5e31b --- /dev/null +++ b/spec/support/cops/b_to_a_cop.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Test + # This cop is only used to test infinite loop detection + class BtoA < RuboCop::Cop::Base + extend AutoCorrector + + def on_class(node) + return unless /B/.match?(node.loc.name.source) + + add_offense(node.loc.name, message: 'B to A') do |corrector| + autocorrect(corrector, node) + end + end + alias on_module on_class + + private + + def autocorrect(corrector, node) + corrector.replace(node.loc.name, node.loc.name.source.tr('B', 'A')) + end + end + end + end +end diff --git a/spec/support/cops/b_to_c_cop.rb b/spec/support/cops/b_to_c_cop.rb new file mode 100644 index 00000000000..781851b033c --- /dev/null +++ b/spec/support/cops/b_to_c_cop.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Test + # This cop is only used to test infinite loop detection + class BtoC < RuboCop::Cop::Base + extend AutoCorrector + + def on_class(node) + return unless /B/.match?(node.loc.name.source) + + add_offense(node.loc.name, message: 'B to C') do |corrector| + autocorrect(corrector, node) + end + end + alias on_module on_class + + private + + def autocorrect(corrector, node) + corrector.replace(node.loc.name, node.loc.name.source.tr('B', 'C')) + end + end + end + end +end diff --git a/spec/support/cops/c_to_a_cop.rb b/spec/support/cops/c_to_a_cop.rb new file mode 100644 index 00000000000..69e921e29a3 --- /dev/null +++ b/spec/support/cops/c_to_a_cop.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Test + # This cop is only used to test infinite loop detection + class CtoA < RuboCop::Cop::Base + extend AutoCorrector + + def on_class(node) + return unless /C/.match?(node.loc.name.source) + + add_offense(node.loc.name, message: 'C to A') do |corrector| + autocorrect(corrector, node) + end + end + alias on_module on_class + + private + + def autocorrect(corrector, node) + corrector.replace(node.loc.name, node.loc.name.source.tr('C', 'A')) + end + end + end + end +end diff --git a/spec/support/file_helper.rb b/spec/support/file_helper.rb index 1e859eb0fd4..b5418cfb737 100644 --- a/spec/support/file_helper.rb +++ b/spec/support/file_helper.rb @@ -17,6 +17,8 @@ def create_file(file_path, content) file.puts content.join("\n") end end + + file_path end def create_empty_file(file_path)