From ddbcf5cb3fbf4a5ee41aa2ec6acc93aaa6380e2c Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Sat, 4 Feb 2023 16:56:37 +0200 Subject: [PATCH 1/6] fix(lint): useless assignment for type definition closes #342 --- spec/ameba/rule/lint/useless_assign_spec.cr | 26 +++++++++++++++++++++ src/ameba/ast/scope.cr | 2 +- src/ameba/ast/visitors/scope_visitor.cr | 12 ++++++++-- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index 2f67281c..d3b3f238 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -475,6 +475,32 @@ 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 top level variable defined inside class is referenced" do + s = Source.new %( + class A + foo : String? = "foo" + end + + puts foo + ) + subject.catch(s).should be_valid + end + + it "doesn't report if top level variable assigned inside class and referenced" do + s = Source.new %( + class A + foo : String? = "foo" + + bar do + foo = "bar" + end + end + + puts foo + ) + subject.catch(s).should be_valid + end end context "branching" do diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index ba0459c3..ff488ca4 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -161,7 +161,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. diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 1dec9af8..16868da6 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -102,10 +102,18 @@ 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_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(node.var, node) + @current_assign = nil + on_scope_end(node) if @current_scope.eql?(node) end # :nodoc: From 14a9ec3a754229bea41a07b7f7b8f7ec97324545 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Sat, 4 Feb 2023 20:40:59 +0200 Subject: [PATCH 2/6] Incorporate changes for shadowing outer local var --- .../lint/shadowing_outer_local_var_spec.cr | 13 +++++++++ src/ameba/ast/scope.cr | 18 ++++++++++++ src/ameba/ast/variabling/type_def_variable.cr | 29 +++++++++++++++++++ src/ameba/ast/visitors/scope_visitor.cr | 3 +- .../rule/lint/shadowing_outer_local_var.cr | 1 + src/ameba/rule/lint/useless_assign.cr | 1 + 6 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 src/ameba/ast/variabling/type_def_variable.cr diff --git a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr index ba44bc11..22027c0d 100644 --- a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr +++ b/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 definition" 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"}] diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index ff488ca4..59c7f3de 100644 --- a/src/ameba/ast/scope.cr +++ b/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. # # ``` @@ -126,6 +139,11 @@ module Ameba::AST ivariables.find(&.name.== "@#{name}") end + # Returns `true` if type declaration variable is assigned in this scope. + def assigns_type_dec?(name) + type_dec_variables.find(&.name.== name) || outer_scope.try(&.assigns_type_dec?(name)) + end + # Returns `true` if and only if current scope represents some # type definition, for example a class. def type_definition? diff --git a/src/ameba/ast/variabling/type_def_variable.cr b/src/ameba/ast/variabling/type_def_variable.cr new file mode 100644 index 00000000..9509eec4 --- /dev/null +++ b/src/ameba/ast/variabling/type_def_variable.cr @@ -0,0 +1,29 @@ +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 + var = @node.var + + case var + when Crystal::Var + var.name + when Crystal::InstanceVar + var.name + when Crystal::ClassVar + var.name + when Crystal::Global + var.name + else + raise "unsupported type declaration var node" + end + end + end +end diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 16868da6..a5c578ae 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -105,13 +105,14 @@ module Ameba::AST 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(node.var, node) + on_assign_end(var, node) @current_assign = nil on_scope_end(node) if @current_scope.eql?(node) end diff --git a/src/ameba/rule/lint/shadowing_outer_local_var.cr b/src/ameba/rule/lint/shadowing_outer_local_var.cr index d61221ec..9a1f8c84 100644 --- a/src/ameba/rule/lint/shadowing_outer_local_var.cr +++ b/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 diff --git a/src/ameba/rule/lint/useless_assign.cr b/src/ameba/rule/lint/useless_assign.cr index e09df45e..f0319329 100644 --- a/src/ameba/rule/lint/useless_assign.cr +++ b/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? From 6b2ddcb1d9ef5b0c71412f65d4a648e9d812d479 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Tue, 7 Feb 2023 17:19:04 +0200 Subject: [PATCH 3/6] Address feedback, add tests --- .../ast/variabling/type_def_variable_spec.cr | 30 +++++++++++++++++++ src/ameba/ast/scope.cr | 6 ++-- src/ameba/ast/variabling/type_def_variable.cr | 10 ++----- 3 files changed, 35 insertions(+), 11 deletions(-) create mode 100644 spec/ameba/ast/variabling/type_def_variable_spec.cr diff --git a/spec/ameba/ast/variabling/type_def_variable_spec.cr b/spec/ameba/ast/variabling/type_def_variable_spec.cr new file mode 100644 index 00000000..7d273b1f --- /dev/null +++ b/spec/ameba/ast/variabling/type_def_variable_spec.cr @@ -0,0 +1,30 @@ +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 diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 59c7f3de..8f930642 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -135,13 +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.find(&.name.== name) || outer_scope.try(&.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 diff --git a/src/ameba/ast/variabling/type_def_variable.cr b/src/ameba/ast/variabling/type_def_variable.cr index 9509eec4..e8ad83ae 100644 --- a/src/ameba/ast/variabling/type_def_variable.cr +++ b/src/ameba/ast/variabling/type_def_variable.cr @@ -13,16 +13,10 @@ module Ameba::AST var = @node.var case var - when Crystal::Var - var.name - when Crystal::InstanceVar - var.name - when Crystal::ClassVar - var.name - when Crystal::Global + when Crystal::Var, Crystal::InstanceVar, Crystal::ClassVar, Crystal::Global var.name else - raise "unsupported type declaration var node" + raise "unsupported var node type: #{var.class}" end end end From d20cc212b9241b242f1c32dc3210bfb291f6d431 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Tue, 7 Feb 2023 20:02:13 +0200 Subject: [PATCH 4/6] Styling changes --- spec/ameba/ast/variabling/type_def_variable_spec.cr | 3 ++- src/ameba/ast/scope.cr | 2 +- src/ameba/ast/variabling/type_def_variable.cr | 6 ++---- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/spec/ameba/ast/variabling/type_def_variable_spec.cr b/spec/ameba/ast/variabling/type_def_variable_spec.cr index 7d273b1f..217e8ada 100644 --- a/spec/ameba/ast/variabling/type_def_variable_spec.cr +++ b/spec/ameba/ast/variabling/type_def_variable_spec.cr @@ -21,7 +21,8 @@ module Ameba::AST 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 + + expect_raises(Exception, "Unsupported var node type: Crystal::Path") do TypeDecVariable.new(type_dec).name end end diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 8f930642..b74f8b62 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -141,7 +141,7 @@ module Ameba::AST # 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)) + type_dec_variables.any?(&.name.== name) || !!outer_scope.try(&.assigns_type_dec?(name)) end # Returns `true` if and only if current scope represents some diff --git a/src/ameba/ast/variabling/type_def_variable.cr b/src/ameba/ast/variabling/type_def_variable.cr index e8ad83ae..3376b37f 100644 --- a/src/ameba/ast/variabling/type_def_variable.cr +++ b/src/ameba/ast/variabling/type_def_variable.cr @@ -10,13 +10,11 @@ module Ameba::AST end def name - var = @node.var - - case var + case var = @node.var when Crystal::Var, Crystal::InstanceVar, Crystal::ClassVar, Crystal::Global var.name else - raise "unsupported var node type: #{var.class}" + raise "Unsupported var node type: #{var.class}" end end end From 61a45d4321f49be728c03ecd218549947a5db77b Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Tue, 7 Feb 2023 22:20:10 +0200 Subject: [PATCH 5/6] Rework tests --- spec/ameba/rule/lint/useless_assign_spec.cr | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index d3b3f238..aeddcb71 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -476,13 +476,18 @@ module Ameba::Rule::Lint issue.message.should eq "Useless assignment to variable `foo`" end - it "doesn't report if top level variable defined inside class is referenced" do + it "doesn't report if top level variable defined inside module and referenced" do s = Source.new %( - class A + module A foo : String? = "foo" + + bar do + foo = "bar" + end + + p foo end - puts foo ) subject.catch(s).should be_valid end @@ -492,12 +497,10 @@ module Ameba::Rule::Lint class A foo : String? = "foo" - bar do + def method foo = "bar" end end - - puts foo ) subject.catch(s).should be_valid end From e7f4bb6ae3b0bc7ef2da400d3747db6dc6a3affb Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Tue, 7 Feb 2023 22:22:49 +0200 Subject: [PATCH 6/6] Fix wording --- spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr | 2 +- spec/ameba/rule/lint/useless_assign_spec.cr | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr index 22027c0d..a321f08a 100644 --- a/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr +++ b/spec/ameba/rule/lint/shadowing_outer_local_var_spec.cr @@ -136,7 +136,7 @@ module Ameba::Rule::Lint CRYSTAL end - it "doesn't report if it shadows type definition" do + it "doesn't report if it shadows type declaration" do expect_no_issues subject, <<-CRYSTAL class FooBar getter index : String diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index aeddcb71..a556d680 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -476,7 +476,7 @@ module Ameba::Rule::Lint issue.message.should eq "Useless assignment to variable `foo`" end - it "doesn't report if top level variable defined inside module and referenced" do + it "doesn't report if type declaration assigned inside module and referenced" do s = Source.new %( module A foo : String? = "foo" @@ -492,7 +492,7 @@ module Ameba::Rule::Lint subject.catch(s).should be_valid end - it "doesn't report if top level variable assigned inside class and referenced" do + it "doesn't report if type declaration assigned inside class" do s = Source.new %( class A foo : String? = "foo"