Merge pull request #469 from yujinakayama/reassigned-but-unused-variable

Track usage of every assignment in UnusedLocalVariable

CHANGELOG.md

@@ -16,12 +16,16 @@
 
 * [#447](https://github.com/bbatsov/rubocop/issues/447) - `BlockAlignment` cop now allows `end` to be aligned with the start of the line containing `do`.
 * `SymbolName` now has an `AllowDots` config option to allow symbols like `:'whatever.submit_button'`.
+* [#469](https://github.com/bbatsov/rubocop/issues/469) - Extracted useless setter call tracking part of `UselessAssignment` cop to `UselessSetterCall`.
+* [#469](https://github.com/bbatsov/rubocop/issues/469) - Merged `UnusedLocalVariable` cop into `UselessAssignment`.
+* [#458](https://github.com/bbatsov/rubocop/issues/458) - The merged `UselessAssignment` cop now has advanced logic that tracks not only assignment at the end of the method but also every assignment in every scope.
 
 ### Bugs fixed
 
 * [#449](https://github.com/bbatsov/rubocop/issues/449) - Remove whitespaces between condition and `do` with `WhileUntilDo` auto-correction
 * Continue with file inspection after parser warnings. Give up only on syntax errors.
 * Don’t trigger the HashSyntax cop on digit-starting keys.
+* Fix crashes while inspecting class definition subclassing another class stored in a local variable in `UselessAssignment` (formerly of `UnusedLocalVariable`) and `ShadowingOuterLocalVariable` (like `clazz = Array; class SomeClass < clazz; end`).
 
 ## 0.12.0 (23/08/2013)
 

config/enabled.yml

@@ -463,10 +463,6 @@ UnreachableCode:
   Description: 'Unreachable code.'
   Enabled: true
 
-UnusedLocalVariable:
-  Description: 'Unused local variable.'
-  Enabled: true
-
 ShadowingOuterLocalVariable:
   Description: >
                  Do not use the same name as outer local variable
@@ -495,6 +491,10 @@ UselessAssignment:
   Description: 'Checks for useless assignment to a local variable.'
   Enabled: true
 
+UselessSetterCall:
+  Description: 'Checks for useless setter call to a local variable.'
+  Enabled: true
+
 UselessComparison:
   Description: 'Checks for comparison of something with itself.'
   Enabled: true

lib/rubocop.rb

@@ -8,13 +8,18 @@
 require 'powerpack'
 
 require 'rubocop/cop/util'
-require 'rubocop/cop/variable_inspector'
 require 'rubocop/cop/offence'
 require 'rubocop/cop/cop'
 require 'rubocop/cop/commissioner'
 require 'rubocop/cop/corrector'
 require 'rubocop/cop/team'
 
+require 'rubocop/cop/variable_inspector'
+require 'rubocop/cop/variable_inspector/variable'
+require 'rubocop/cop/variable_inspector/assignment'
+require 'rubocop/cop/variable_inspector/scope'
+require 'rubocop/cop/variable_inspector/variable_table'
+
 require 'rubocop/cop/lint/assignment_in_condition'
 require 'rubocop/cop/lint/block_alignment'
 require 'rubocop/cop/lint/empty_ensure'
@@ -29,9 +34,9 @@
 require 'rubocop/cop/lint/rescue_exception'
 require 'rubocop/cop/lint/shadowing_outer_local_variable'
 require 'rubocop/cop/lint/unreachable_code'
-require 'rubocop/cop/lint/unused_local_variable'
 require 'rubocop/cop/lint/useless_assignment'
 require 'rubocop/cop/lint/useless_comparison'
+require 'rubocop/cop/lint/useless_setter_call'
 require 'rubocop/cop/lint/void'
 
 require 'rubocop/cop/style/access_control'

lib/rubocop/cop/lint/shadowing_outer_local_variable.rb

@@ -16,17 +16,14 @@ def investigate(processed_source)
           inspect_variables(processed_source.ast)
         end
 
-        def before_declaring_variable(entry)
-          # Only block scope can reference outer local variables.
-          return unless variable_table.current_scope.node.type == :block
-          return unless ARGUMENT_DECLARATION_TYPES.include?(entry.node.type)
-          return if entry.name.to_s.start_with?('_')
+        def before_declaring_variable(variable)
+          return if variable.name.to_s.start_with?('_')
 
-          outer_local_variable = variable_table.find_variable_entry(entry.name)
+          outer_local_variable = variable_table.find_variable(variable.name)
           return unless outer_local_variable
 
-          message = sprintf(MSG, entry.name)
-          warning(entry.node, :expression, message)
+          message = sprintf(MSG, variable.name)
+          warning(variable.declaration_node, :expression, message)
         end
       end
     end

lib/rubocop/cop/lint/useless_assignment.rb

@@ -3,87 +3,46 @@
 module Rubocop
   module Cop
     module Lint
-      # This cop checks for useless assignment as the final expression
-      # of a function definition.
+      # This cop checks for every useless assignment to local variable in every
+      # scope.
+      # The basic idea for this cop was from the warning of `ruby -cw`:
       #
-      # @example
+      #   assigned but unused variable - foo
       #
-      #  def something
-      #    x = 5
-      #  end
-      #
-      #  def something
-      #    x = Something.new
-      #    x.attr = 5
-      #  end
+      # Currently this cop has advanced logic that detects unreferenced
+      # reassignments and properly handles varied cases such as branch, loop,
+      # rescue, ensure, etc.
       class UselessAssignment < Cop
-        MSG = 'Useless assignment to local variable %s.'
-
-        def on_def(node)
-          _name, args, body = *node
-
-          check_for_useless_assignment(body, args)
-        end
+        include VariableInspector
 
-        def on_defs(node)
-          _target, _name, args, body = *node
+        MSG = 'Useless assignment to variable - %s'
 
-          check_for_useless_assignment(body, args)
+        def investigate(processed_source)
+          inspect_variables(processed_source.ast)
         end
 
-        private
-
-        def check_for_useless_assignment(body, args)
-          return unless body
-
-          if body.type == :begin
-            expression = body.children
-          else
-            expression = body
-          end
-
-          last_expr = expression.is_a?(Array) ? expression.last : expression
-          return unless last_expr
-
-          case last_expr.type
-          when :lvasgn
-            var_name, = *last_expr
-            warning(last_expr, :name, MSG.format(var_name))
-          when :send
-            receiver, method, _args = *last_expr
-            return unless receiver
-            return unless receiver.type == :lvar
-            return unless method =~ /\w=$/
-
-            var_name, = *receiver
-            return if contains_object_passed_as_argument?(var_name, body, args)
-
-            warning(receiver,
-                    :name,
-                    MSG.format(receiver.loc.name.source))
+        def after_leaving_scope(scope)
+          scope.variables.each_value do |variable|
+            check_for_unused_assignments(variable)
+            check_for_unused_block_local_variable(variable)
           end
         end
 
-        def contains_object_passed_as_argument?(lvar_name, body, args)
-          variable_table = {}
+        def check_for_unused_assignments(variable)
+          return if variable.name.to_s.start_with?('_')
 
-          args.children.each do |arg_node|
-            arg_name, = *arg_node
-            variable_table[arg_name] = true
-          end
-
-          on_node([:lvasgn, :ivasgn, :cvasgn, :gvasgn], body) do |asgn_node|
-            lhs_var_name, rhs_node = *asgn_node
-
-            if [:lvar, :ivar, :cvar, :gvar].include?(rhs_node.type)
-              rhs_var_name, = *rhs_node
-              variable_table[lhs_var_name] = variable_table[rhs_var_name]
-            else
-              variable_table[lhs_var_name] = false
-            end
+          variable.assignments.each do |assignment|
+            next if assignment.used?
+            message = sprintf(MSG, variable.name)
+            warning(assignment.node, :expression, message)
           end
+        end
 
-          variable_table[lvar_name]
+        def check_for_unused_block_local_variable(variable)
+          return unless variable.block_local_variable?
+          return unless variable.assignments.empty?
+          message = sprintf(MSG, variable.name)
+          warning(variable.declaration_node, :expression, message)
         end
       end
     end

lib/rubocop/cop/lint/useless_setter_call.rb

@@ -0,0 +1,85 @@
+# encoding: utf-8
+
+module Rubocop
+  module Cop
+    module Lint
+      # This cop checks for setter call to local variable as the final
+      # expression of a function definition.
+      #
+      # @example
+      #
+      #  def something
+      #    x = Something.new
+      #    x.attr = 5
+      #  end
+      class UselessSetterCall < Cop
+        MSG = 'Useless setter call to local variable %s.'
+
+        def on_def(node)
+          _name, args, body = *node
+
+          check_for_useless_assignment(body, args)
+        end
+
+        def on_defs(node)
+          _target, _name, args, body = *node
+
+          check_for_useless_assignment(body, args)
+        end
+
+        private
+
+        def check_for_useless_assignment(body, args)
+          return unless body
+
+          if body.type == :begin
+            expression = body.children
+          else
+            expression = body
+          end
+
+          last_expr = expression.is_a?(Array) ? expression.last : expression
+
+          return unless setter_call_to_local_variable?(last_expr)
+
+          receiver, = *last_expr
+          var_name, = *receiver
+          return if contains_object_passed_as_argument?(var_name, body, args)
+
+          warning(receiver,
+                  :name,
+                  MSG.format(receiver.loc.name.source))
+        end
+
+        def setter_call_to_local_variable?(node)
+          return unless node && node.type == :send
+          receiver, method, _args = *node
+          return unless receiver && receiver.type == :lvar
+          method =~ /\w=$/
+        end
+
+        def contains_object_passed_as_argument?(lvar_name, body, args)
+          variable_table = {}
+
+          args.children.each do |arg_node|
+            arg_name, = *arg_node
+            variable_table[arg_name] = true
+          end
+
+          on_node([:lvasgn, :ivasgn, :cvasgn, :gvasgn], body) do |asgn_node|
+            lhs_var_name, rhs_node = *asgn_node
+
+            if [:lvar, :ivar, :cvar, :gvar].include?(rhs_node.type)
+              rhs_var_name, = *rhs_node
+              variable_table[lhs_var_name] = variable_table[rhs_var_name]
+            else
+              variable_table[lhs_var_name] = false
+            end
+          end
+
+          variable_table[lvar_name]
+        end
+      end
+    end
+  end
+end