Merge pull request #351 from crystal-ameba/fix/useless-assign-type-def

fix(lint): useless assignment for type definition

spec/ameba/ast/variabling/type_def_variable_spec.cr

@@ -0,0 +1,31 @@
+require "../../../spec_helper"
+
+module Ameba::AST
+  describe TypeDecVariable do
+    var = Crystal::Var.new("foo")
+    declared_type = Crystal::Path.new("String")
+    type_dec = Crystal::TypeDeclaration.new(var, declared_type)
+
+    describe "#initialize" do
+      it "creates a new type dec variable" do
+        variable = TypeDecVariable.new(type_dec)
+        variable.node.should_not be_nil
+      end
+    end
+
+    describe "#name" do
+      it "returns var name" do
+        variable = TypeDecVariable.new(type_dec)
+        variable.name.should eq var.name
+      end
+
+      it "raises if type declaration is incorrect" do
+        type_dec = Crystal::TypeDeclaration.new(declared_type, declared_type)
+
+        expect_raises(Exception, "Unsupported var node type: Crystal::Path") do
+          TypeDecVariable.new(type_dec).name
+        end
+      end
+    end
+  end
+end

spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr

@@ -136,6 +136,19 @@ module Ameba::Rule::Lint
         CRYSTAL
     end
 
+    it "doesn't report if it shadows type declaration" do
+      expect_no_issues subject, <<-CRYSTAL
+        class FooBar
+          getter index : String
+
+          def bar
+            3.times do |index|
+            end
+          end
+        end
+        CRYSTAL
+    end
+
     it "doesn't report if it shadows throwaway arguments" do
       expect_no_issues subject, <<-CRYSTAL
         data = [{1, "a"}, {2, "b"}, {3, "c"}]

spec/ameba/rule/lint/useless_assign_spec.cr

@@ -475,6 +475,35 @@ module Ameba::Rule::Lint
         issue.location.to_s.should eq "source.cr:1:1"
         issue.message.should eq "Useless assignment to variable `foo`"
       end
+
+      it "doesn't report if type declaration assigned inside module and referenced" do
+        s = Source.new %(
+          module A
+            foo : String? = "foo"
+
+            bar do
+              foo = "bar"
+            end
+
+            p foo
+          end
+
+        )
+        subject.catch(s).should be_valid
+      end
+
+      it "doesn't report if type declaration assigned inside class" do
+        s = Source.new %(
+          class A
+            foo : String? = "foo"
+
+            def method
+              foo = "bar"
+            end
+          end
+        )
+        subject.catch(s).should be_valid
+      end
     end
 
     context "branching" do

src/ameba/ast/scope.cr

@@ -19,6 +19,9 @@ module Ameba::AST
     # Link to the instance variables used in current scope
     getter ivariables = [] of InstanceVariable
 
+    # Link to the type declaration variables used in current scope
+    getter type_dec_variables = [] of TypeDecVariable
+
     # Link to the outer scope
     getter outer_scope : Scope?
 
@@ -74,6 +77,16 @@ module Ameba::AST
       ivariables << InstanceVariable.new(node)
     end
 
+    # Adds a new type declaration variable to the current scope.
+    #
+    # ```
+    # scope = Scope.new(class_node, nil)
+    # scope.add_type_dec_variable(node)
+    # ```
+    def add_type_dec_variable(node)
+      type_dec_variables << TypeDecVariable.new(node)
+    end
+
     # Returns variable by its name or `nil` if it does not exist.
     #
     # ```
@@ -122,8 +135,13 @@ module Ameba::AST
 
     # Returns `true` if instance variable is assigned in this scope.
     def assigns_ivar?(name)
-      arguments.find(&.name.== name) &&
-        ivariables.find(&.name.== "@#{name}")
+      arguments.any?(&.name.== name) &&
+        ivariables.any?(&.name.== "@#{name}")
+    end
+
+    # Returns `true` if type declaration variable is assigned in this scope.
+    def assigns_type_dec?(name)
+      type_dec_variables.any?(&.name.== name) || !!outer_scope.try(&.assigns_type_dec?(name))
     end
 
     # Returns `true` if and only if current scope represents some
@@ -161,7 +179,7 @@ module Ameba::AST
 
     # Returns `true` if this scope is a top level scope, `false` otherwise.
     def top_level?
-      outer_scope.nil?
+      outer_scope.nil? || type_definition?
     end
 
     # Returns `true` if var is an argument in current scope, `false` otherwise.

src/ameba/ast/variabling/type_def_variable.cr

@@ -0,0 +1,21 @@
+module Ameba::AST
+  class TypeDecVariable
+    getter node : Crystal::TypeDeclaration
+
+    delegate location, to: @node
+    delegate end_location, to: @node
+    delegate to_s, to: @node
+
+    def initialize(@node)
+    end
+
+    def name
+      case var = @node.var
+      when Crystal::Var, Crystal::InstanceVar, Crystal::ClassVar, Crystal::Global
+        var.name
+      else
+        raise "Unsupported var node type: #{var.class}"
+      end
+    end
+  end
+end

src/ameba/ast/visitors/scope_visitor.cr

@@ -102,10 +102,19 @@ module Ameba::AST
 
     # :nodoc:
     def visit(node : Crystal::TypeDeclaration)
-      return if @current_scope.type_definition?
-      return if !(var = node.var).is_a?(Crystal::Var)
+      return unless (var = node.var).is_a?(Crystal::Var)
 
       @current_scope.add_variable(var)
+      @current_scope.add_type_dec_variable(node)
+      @current_assign = node.value unless node.value.nil?
+    end
+
+    def end_visit(node : Crystal::TypeDeclaration)
+      return unless (var = node.var).is_a?(Crystal::Var)
+
+      on_assign_end(var, node)
+      @current_assign = nil
+      on_scope_end(node) if @current_scope.eql?(node)
     end
 
     # :nodoc:

src/ameba/rule/lint/shadowing_outer_local_var.cr

@@ -57,6 +57,7 @@ module Ameba::Rule::Lint
 
         next if variable.nil? || !variable.declared_before?(arg)
         next if outer_scope.assigns_ivar?(arg.name)
+        next if outer_scope.assigns_type_dec?(arg.name)
 
         issue_for arg.node, MSG % arg.name
       end

src/ameba/rule/lint/useless_assign.cr

@@ -39,6 +39,7 @@ module Ameba::Rule::Lint
     def test(source, node, scope : AST::Scope)
       scope.variables.each do |var|
         next if var.ignored? || var.used_in_macro? || var.captured_by_block?
+        next if scope.assigns_type_dec?(var.name)
 
         var.assignments.each do |assign|
           next if assign.referenced? || assign.transformed?