Add Lint/Unused{Local,Instance,Class}VariableAccess (#552)

* ImplicitReturnVisitor keeps track of whether the current node is in a macro or not

Fixes for ImplicitReturnVisitor macro implicit returns

* Add `Lint/UnusedLocalVariableAccess`

* Add `Lint/UnusedInstanceVariableAccess`

* Add `Lint/UnusedClassVariableAccess`

* Update src/ameba/rule/lint/unused_local_variable_access.cr

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>

* Update src/ameba/rule/lint/unused_instance_variable_access.cr

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>

* Fix heredoc interpolations

* Update specs from review

* Apply suggestions from code review

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>

* Update specs, disable unused cvars in macros

* Update spec/ameba/rule/lint/unused_instance_variable_access_spec.cr

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>

* Reorder spec

---------

Co-authored-by: Sijawusz Pur Rahnama <[email protected]>

spec/ameba/rule/lint/unused_class_variable_access_spec.cr

@@ -0,0 +1,45 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  describe UnusedClassVariableAccess do
+    subject = UnusedClassVariableAccess.new
+
+    it "passes if class variables are used for assignment" do
+      expect_no_issues subject, <<-CRYSTAL
+        class MyClass
+          foo = @@ivar
+        end
+        CRYSTAL
+    end
+
+    it "passes if an class variable is used as a target in multi-assignment" do
+      expect_no_issues subject, <<-CRYSTAL
+        class MyClass
+          @@foo, @@bar = 1, 2
+        end
+        CRYSTAL
+    end
+
+    it "fails if class variables are unused in void context of class" do
+      expect_issue subject, <<-CRYSTAL
+        class Actor
+          @@name : String = "George"
+
+          @@name
+        # ^^^^^^ error: Value from class variable access is unused
+        end
+        CRYSTAL
+    end
+
+    it "fails if class variables are unused in void context of method" do
+      expect_issue subject, <<-'CRYSTAL'
+        def hello : String
+          @@name
+        # ^^^^^^ error: Value from class variable access is unused
+
+          "Hello, #{@@name}!"
+        end
+        CRYSTAL
+    end
+  end
+end

spec/ameba/rule/lint/unused_instance_variable_access_spec.cr

@@ -0,0 +1,64 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  describe UnusedInstanceVariableAccess do
+    subject = UnusedInstanceVariableAccess.new
+
+    it "passes if instance variables are used for assignment" do
+      expect_no_issues subject, <<-CRYSTAL
+        class MyClass
+          foo = @ivar
+        end
+        CRYSTAL
+    end
+
+    it "passes if an instance variable is used as a target in multi-assignment" do
+      expect_no_issues subject, <<-CRYSTAL
+        class MyClass
+          @foo, @bar = 1, 2
+        end
+        CRYSTAL
+    end
+
+    it "fails if instance variables are unused in void context of class" do
+      expect_issue subject, <<-CRYSTAL
+        class Actor
+          @name : String = "George"
+
+          @name
+        # ^^^^^ error: Value from instance variable access is unused
+        end
+        CRYSTAL
+    end
+
+    it "fails if instance variables are unused in void context of method" do
+      expect_issue subject, <<-'CRYSTAL'
+        def hello : String
+          @name
+        # ^^^^^ error: Value from instance variable access is unused
+
+          "Hello, #{@name}!"
+        end
+        CRYSTAL
+    end
+
+    it "passes if @type is unused within a macro expression" do
+      expect_no_issues subject, <<-CRYSTAL
+        def foo
+          {% @type %}
+          :bar
+        end
+        CRYSTAL
+    end
+
+    it "fails if instance variable is unused within a macro expression" do
+      expect_issue subject, <<-CRYSTAL
+        def foo
+          {% @bar %}
+           # ^^^^ error: Value from instance variable access is unused
+          :baz
+        end
+        CRYSTAL
+    end
+  end
+end

spec/ameba/rule/lint/unused_local_variable_access_spec.cr

@@ -0,0 +1,69 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  describe UnusedLocalVariableAccess do
+    subject = UnusedLocalVariableAccess.new
+
+    it "passes if local variables are used in assign" do
+      expect_no_issues subject, <<-CRYSTAL
+        foo = 1
+        foo += 1
+        foo, bar = 2, 3
+        CRYSTAL
+    end
+
+    it "passes if a local variable is a call argument" do
+      expect_no_issues subject, <<-CRYSTAL
+        foo = 1
+        puts foo
+        CRYSTAL
+    end
+
+    it "passes if local variable on left side of a comparison" do
+      expect_no_issues subject, <<-CRYSTAL
+        def hello
+          foo = 1
+          foo || (puts "foo is falsey")
+          foo
+        end
+        CRYSTAL
+    end
+
+    it "passes if skip_file is used in a macro" do
+      expect_no_issues subject, <<-CRYSTAL
+        {% skip_file %}
+        CRYSTAL
+    end
+
+    it "passes if debug is used in a macro" do
+      expect_no_issues subject, <<-CRYSTAL
+        {% debug %}
+        CRYSTAL
+    end
+
+    it "fails if a local variable is in a void context" do
+      expect_issue subject, <<-CRYSTAL
+        foo = 1
+
+        begin
+          foo
+        # ^^^ error: Value from local variable access is unused
+          puts foo
+        end
+        CRYSTAL
+    end
+
+    it "fails if a parameter is in a void context" do
+      expect_issue subject, <<-CRYSTAL
+        def foo(bar)
+          if bar > 0
+            bar
+          # ^^^ error: Value from local variable access is unused
+          end
+
+          nil
+        end
+        CRYSTAL
+    end
+  end
+end

src/ameba/ast/visitors/implicit_return_visitor.cr

@@ -11,6 +11,7 @@ module Ameba::AST
   class ImplicitReturnVisitor < BaseVisitor
     # When greater than zero, indicates the current node's return value is used
     @stack : Int32 = 0
+    @in_macro : Bool = false
 
     # The stack is swapped out here as `Crystal::Expressions` are isolated from
     # their parents scope. Only the last line in an expressions node can be
@@ -39,7 +40,9 @@ module Ameba::AST
     def visit(node : Crystal::BinaryOp) : Bool
       report_implicit_return(node)
 
-      if node.right.is_a?(Crystal::Call)
+      if node.right.is_a?(Crystal::Call) ||
+         node.right.is_a?(Crystal::Expressions) ||
+         node.right.is_a?(Crystal::ControlExpression)
         incr_stack { node.left.accept(self) }
       else
         node.left.accept(self)
@@ -91,7 +94,6 @@ module Ameba::AST
     def visit(node : Crystal::MultiAssign) : Bool
       report_implicit_return(node)
 
-      node.targets.each &.accept(self)
       incr_stack { node.values.each &.accept(self) }
 
       false
@@ -147,6 +149,9 @@ module Ameba::AST
         node.args.each &.accept(self)
         node.double_splat.try &.accept(self)
         node.block_arg.try &.accept(self)
+      end
+
+      swap_stack do
         node.body.accept(self)
       end
 
@@ -280,6 +285,14 @@ module Ameba::AST
       false
     end
 
+    def visit(node : Crystal::Block) : Bool
+      report_implicit_return(node)
+
+      node.body.accept(self)
+
+      false
+    end
+
     def visit(node : Crystal::ControlExpression) : Bool
       report_implicit_return(node)
 
@@ -329,10 +342,12 @@ module Ameba::AST
     def visit(node : Crystal::MacroExpression) : Bool
       report_implicit_return(node)
 
-      if node.output?
-        incr_stack { node.exp.accept(self) }
-      else
-        node.exp.accept(self)
+      in_macro do
+        if node.output?
+          incr_stack { node.exp.accept(self) }
+        else
+          swap_stack { node.exp.accept(self) }
+        end
       end
 
       false
@@ -341,17 +356,23 @@ module Ameba::AST
     def visit(node : Crystal::MacroIf) : Bool
       report_implicit_return(node)
 
-      incr_stack { node.cond.accept(self) }
-      node.then.accept(self)
-      node.else.accept(self)
+      in_macro do
+        swap_stack do
+          incr_stack { node.cond.accept(self) }
+          node.then.accept(self)
+          node.else.accept(self)
+        end
+      end
 
       false
     end
 
     def visit(node : Crystal::MacroFor) : Bool
       report_implicit_return(node)
 
-      node.body.accept(self)
+      in_macro do
+        swap_stack { node.body.accept(self) }
+      end
 
       false
     end
@@ -399,7 +420,7 @@ module Ameba::AST
     end
 
     private def report_implicit_return(node) : Nil
-      @rule.test(@source, node) unless @stack.positive?
+      @rule.test(@source, node, @in_macro) unless @stack.positive?
     end
 
     # Indicates that any nodes visited within the block are captured / used.
@@ -415,5 +436,12 @@ module Ameba::AST
       yield old_stack
       @stack = old_stack
     end
+
+    private def in_macro(&) : Nil
+      old_value = @in_macro
+      @in_macro = true
+      yield
+      @in_macro = old_value
+    end
   end
 end

src/ameba/rule/lint/unused_class_variable_access.cr

@@ -0,0 +1,61 @@
+module Ameba::Rule::Lint
+  # A rule that disallows unused class variable access.
+  #
+  # For example, this is considered invalid:
+  #
+  # ```
+  # class MyClass
+  #   @@my_var : String = "hello"
+  #
+  #   @@my_var
+  #
+  #   def hello : String
+  #     @@my_var
+  #
+  #     "hello, world!"
+  #   end
+  # end
+  # ```
+  #
+  # And these are considered valid:
+  #
+  # ```
+  # class MyClass
+  #   @@my_var : String = "hello"
+  #
+  #   @@my_other_var = @@my_var
+  #
+  #   def hello : String
+  #     return @@my_var if @@my_var == "hello"
+  #
+  #     "hello, world!"
+  #   end
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Lint/UnusedClassVariableAccess:
+  #   Enabled: true
+  # ```
+  class UnusedClassVariableAccess < Base
+    properties do
+      since_version "1.7.0"
+      description "Disallows unused access to class variables"
+    end
+
+    MSG = "Value from class variable access is unused"
+
+    def test(source : Source)
+      AST::ImplicitReturnVisitor.new(self, source)
+    end
+
+    def test(source, node : Crystal::ClassVar, in_macro : Bool)
+      # Class variables aren't supported in macros
+      return if in_macro
+
+      issue_for node, MSG
+    end
+  end
+end

src/ameba/rule/lint/unused_comparison.cr

@@ -48,7 +48,7 @@ module Ameba::Rule::Lint
       AST::ImplicitReturnVisitor.new(self, source)
     end
 
-    def test(source, node : Crystal::Call)
+    def test(source, node : Crystal::Call, in_macro : Bool)
       if node.name.in?(COMPARISON_OPERATORS) && node.args.size == 1
         issue_for node, MSG
       end

src/ameba/rule/lint/unused_generic_or_union.cr

@@ -47,11 +47,11 @@ module Ameba::Rule::Lint
       AST::ImplicitReturnVisitor.new(self, source)
     end
 
-    def test(source, node : Crystal::Call)
+    def test(source, node : Crystal::Call, in_macro : Bool)
       issue_for node, MSG_UNION if path_or_generic_union?(node)
     end
 
-    def test(source, node : Crystal::Generic)
+    def test(source, node : Crystal::Generic, in_macro : Bool)
       issue_for node, MSG_GENERIC
     end