diff --git a/changelog/change_make_lint_useless_assignment_autocorrection_safe.md b/changelog/change_make_lint_useless_assignment_autocorrection_safe.md new file mode 100644 index 000000000000..1f8d74b654d5 --- /dev/null +++ b/changelog/change_make_lint_useless_assignment_autocorrection_safe.md @@ -0,0 +1 @@ +* [#13103](https://github.com/rubocop/rubocop/issues/13103): Make `Lint/UselessAssignment` autocorrection safe. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 4646ba29dc6a..bc52d408ecf7 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2543,8 +2543,7 @@ Lint/UselessAssignment: Enabled: true AutoCorrect: contextual VersionAdded: '0.11' - VersionChanged: '1.61' - SafeAutoCorrect: false + VersionChanged: '<>' Lint/UselessElseWithoutRescue: Description: 'Checks for useless `else` in `begin..end` without `rescue`.' diff --git a/lib/rubocop/cop/lint/useless_assignment.rb b/lib/rubocop/cop/lint/useless_assignment.rb index e8f2475b1752..f06f4ebbb813 100644 --- a/lib/rubocop/cop/lint/useless_assignment.rb +++ b/lib/rubocop/cop/lint/useless_assignment.rb @@ -16,15 +16,14 @@ module Lint # reassignments and properly handles varied cases such as branch, loop, # rescue, ensure, etc. # + # This cop's autocorrection avoids cases like `a ||= 1` because removing assignment from + # operator assignment can cause NameError if this assignment has been used to declare + # a local variable. For example, replacing `a ||= 1` with `a || 1` may cause + # "undefined local variable or method `a' for main:Object (NameError)". + # # NOTE: Given the assignment `foo = 1, bar = 2`, removing unused variables # can lead to a syntax error, so this case is not autocorrected. # - # @safety - # This cop's autocorrection is unsafe because removing assignment from - # operator assignment can cause NameError if this assignment has been used to declare - # local variable. For example, replacing `a ||= 1` to `a || 1` may cause - # "undefined local variable or method `a' for main:Object (NameError)". - # # @example # # # bad @@ -53,24 +52,32 @@ def after_leaving_scope(scope, _variable_table) scope.variables.each_value { |variable| check_for_unused_assignments(variable) } end - # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity def check_for_unused_assignments(variable) return if variable.should_be_unused? variable.assignments.reverse_each do |assignment| - next if assignment.used? || part_of_ignored_node?(assignment.node) + assignment_node = assignment.node + next if assignment.used? || part_of_ignored_node?(assignment_node) message = message_for_useless_assignment(assignment) range = offense_range(assignment) add_offense(range, message: message) do |corrector| - autocorrect(corrector, assignment) unless sequential_assignment?(assignment.node) + # In cases like `x = 1, y = 2`, where removing a variable would cause a syntax error, + # and where changing `x ||= 1` to `x = 1` would cause `NameError`, + # the autocorrect will be skipped, even if the variable is unused. + if sequential_assignment?(assignment_node) || assignment_node.parent&.or_asgn_type? + next + end + + autocorrect(corrector, assignment) end - ignore_node(assignment.node) if chained_assignment?(assignment.node) + ignore_node(assignment_node) if chained_assignment?(assignment_node) end end - # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity def message_for_useless_assignment(assignment) variable = assignment.variable diff --git a/spec/rubocop/cli/options_spec.rb b/spec/rubocop/cli/options_spec.rb index eb20a8ac8437..25feddbcf8a8 100644 --- a/spec/rubocop/cli/options_spec.rb +++ b/spec/rubocop/cli/options_spec.rb @@ -979,7 +979,7 @@ def on_send(node) 1 Layout/SpaceAroundOperators [Safe Correctable] 1 Layout/TrailingWhitespace [Safe Correctable] 1 Lint/MissingCopEnableDirective - 1 Lint/UselessAssignment [Unsafe Correctable] + 1 Lint/UselessAssignment [Safe Correctable] 1 Migration/DepartmentName [Safe Correctable] 1 Style/FrozenStringLiteralComment [Unsafe Correctable] 1 Style/NumericPredicate [Unsafe Correctable] @@ -2016,11 +2016,10 @@ def f a = "Hello" RUBY - expect(cli.run(['--autocorrect', '--format', 'simple', target_file])).to eq(1) + expect(cli.run(['--autocorrect', '--format', 'simple', target_file])).to eq(0) expect($stdout.string.lines.to_a.last) - .to eq('1 file inspected, 2 offenses detected, 1 offense corrected, 1 more offense can ' \ - "be corrected with `rubocop -A`\n") + .to eq("1 file inspected, 3 offenses detected, 3 offenses corrected\n") end end @@ -2073,8 +2072,7 @@ def f expect(cli.run(['--autocorrect', '--format', 'simple', target_file])).to eq(1) expect($stdout.string.lines.to_a.last).to eq( - '1 file inspected, 2 offenses detected, 1 more offense can be corrected with ' \ - "`rubocop -A`\n" + "1 file inspected, 2 offenses detected, 1 offense corrected\n" ) end end diff --git a/spec/rubocop/cop/lint/useless_assignment_spec.rb b/spec/rubocop/cop/lint/useless_assignment_spec.rb index 591b6832400a..0d6c72d93263 100644 --- a/spec/rubocop/cop/lint/useless_assignment_spec.rb +++ b/spec/rubocop/cop/lint/useless_assignment_spec.rb @@ -321,9 +321,7 @@ module x::Foo ^^^ Useless assignment to variable - `foo`. Use `||` instead of `||=`. RUBY - expect_correction(<<~RUBY) - foo || 1 - RUBY + expect_no_corrections end end @@ -1149,12 +1147,7 @@ def some_method end RUBY - expect_correction(<<~RUBY) - def some_method - foo = do_something_returns_object_or_nil - foo || 1 - end - RUBY + expect_no_corrections end end @@ -1169,13 +1162,7 @@ def some_method end RUBY - expect_correction(<<~RUBY) - def some_method - foo = do_something_returns_object_or_nil - foo || 1 - some_return_value - end - RUBY + expect_no_corrections end end