From 4ad151e5e024732187c11cdbe5b53ccbd7243699 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 14 Dec 2023 02:33:43 +0100 Subject: [PATCH 1/8] Do not automatically consider type definitions as referenced --- src/ameba/ast/variabling/assignment.cr | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/ameba/ast/variabling/assignment.cr b/src/ameba/ast/variabling/assignment.cr index 3ab1ba7f..7168f5cb 100644 --- a/src/ameba/ast/variabling/assignment.cr +++ b/src/ameba/ast/variabling/assignment.cr @@ -32,9 +32,7 @@ module Ameba::AST return unless scope = @variable.scope @branch = Branch.of(@node, scope) - @referenced = true if @variable.special? || - @variable.scope.type_definition? || - referenced_in_loop? + @referenced = true if @variable.special? || referenced_in_loop? end def referenced_in_loop? From 9745637cf93040f371ec0d05c80266b3239982e2 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 14 Dec 2023 02:35:59 +0100 Subject: [PATCH 2/8] Update `UselessAssign` rule to report unreferenced type declarations --- spec/ameba/rule/lint/useless_assign_spec.cr | 47 +++++++++++++++++++-- src/ameba/rule/lint/useless_assign.cr | 1 - 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index d764a7da..72afaefa 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -492,8 +492,8 @@ module Ameba::Rule::Lint subject.catch(s).should be_valid end - it "doesn't report if type declaration assigned inside class" do - s = Source.new %( + it "reports if type declaration assigned inside class" do + s = Source.new path: "source.cr", code: %( class A foo : String? = "foo" @@ -502,7 +502,11 @@ module Ameba::Rule::Lint end end ) - subject.catch(s).should be_valid + subject.catch(s).should_not be_valid + + issue = s.issues.first + issue.location.to_s.should eq "source.cr:2:3" + issue.message.should eq "Useless assignment to variable `foo`" end end @@ -1068,6 +1072,43 @@ module Ameba::Rule::Lint subject.catch(s).should be_valid end + context "type declaration" do + it "reports if it's not referenced at a top level" do + s = Source.new %( + a : String? + ) + subject.catch(s).should_not be_valid + end + + it "reports if it's not referenced in a method" do + s = Source.new %( + def foo + a : String? + end + ) + subject.catch(s).should_not be_valid + end + + it "reports if it's not referenced in a class" do + s = Source.new %( + class Foo + a : String? + end + ) + subject.catch(s).should_not be_valid + end + + it "doesn't report if it's referenced" do + s = Source.new %( + def foo + a : String? + a + end + ) + subject.catch(s).should be_valid + end + end + context "uninitialized" do it "reports if uninitialized assignment is not referenced at a top level" do s = Source.new %( diff --git a/src/ameba/rule/lint/useless_assign.cr b/src/ameba/rule/lint/useless_assign.cr index 85ac424f..7aec210d 100644 --- a/src/ameba/rule/lint/useless_assign.cr +++ b/src/ameba/rule/lint/useless_assign.cr @@ -39,7 +39,6 @@ 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? From 1b661d633d077eb317fc8f0cac1d3ed0e2a72ba6 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 27 Dec 2023 23:47:58 +0100 Subject: [PATCH 3/8] Refactor `ScopeVisitor` to ignore accessor macros --- src/ameba/ast/scope.cr | 10 ++++++++ src/ameba/ast/visitors/scope_visitor.cr | 33 +++++++++++++++---------- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 08b42931..38ef9944 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -186,6 +186,16 @@ module Ameba::AST node.is_a?(Crystal::Def) end + # Returns `true` if current scope is a class, `false` otherwise. + def class_def? + node.is_a?(Crystal::ClassDef) + end + + # Returns `true` if current scope is a module, `false` otherwise. + def module_def? + node.is_a?(Crystal::ModuleDef) + end + # Returns `true` if this scope is a top level scope, `false` otherwise. def top_level? outer_scope.nil? || type_definition? diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 73aa89bc..0b93cdb9 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -170,22 +170,29 @@ module Ameba::AST # :nodoc: def visit(node : Crystal::Call) - case - when @current_scope.def? - if node.name.in?(SPECIAL_NODE_NAMES) && node.args.empty? - @current_scope.arguments.each do |arg| - variable = arg.variable + scope = @current_scope - ref = variable.reference(variable.node, @current_scope) - ref.explicit = false - end + case + when scope.top_level? && record_macro?(node) then return false + when scope.type_definition? && record_macro?(node) then return false + when scope.type_definition? && accessor_macro?(node) then return false + when scope.def? && special_node?(node) + scope.arguments.each do |arg| + variable = arg.variable + + ref = variable.reference(variable.node, scope) + ref.explicit = false end - true - when @current_scope.top_level? && record_macro?(node) - false - else - true end + true + end + + private def special_node?(node) + node.name.in?(SPECIAL_NODE_NAMES) && node.args.empty? + end + + private def accessor_macro?(node) + node.name.matches? /^(class_)?(getter[?!]?|setter|property[?!]?)$/ end private def record_macro?(node) From 1dd531740c60190646ff10176448cdd833089ab8 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 27 Dec 2023 23:56:20 +0100 Subject: [PATCH 4/8] Fix reported ameba issue --- spec/ameba/ast/visitors/flow_expression_visitor_spec.cr | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/ameba/ast/visitors/flow_expression_visitor_spec.cr b/spec/ameba/ast/visitors/flow_expression_visitor_spec.cr index 5053ff67..3e69729b 100644 --- a/spec/ameba/ast/visitors/flow_expression_visitor_spec.cr +++ b/spec/ameba/ast/visitors/flow_expression_visitor_spec.cr @@ -1,8 +1,6 @@ require "../../../spec_helper" module Ameba::AST - source = Source.new "" - describe FlowExpressionVisitor do it "creates an expression for return" do rule = FlowExpressionRule.new From a49faa33a983c35057658bfc722520e487b25c6c Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 28 Dec 2023 00:37:03 +0100 Subject: [PATCH 5/8] Refactor `Lint/UselessAssign` spec --- spec/ameba/rule/lint/useless_assign_spec.cr | 556 ++++++++------------ 1 file changed, 218 insertions(+), 338 deletions(-) diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index 72afaefa..90f13c70 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -5,122 +5,104 @@ module Ameba::Rule::Lint subject = UselessAssign.new it "does not report used assignments" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 2 a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports a useless assignment in a method" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method a = 2 + # ^ error: Useless assignment to variable `a` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "reports a useless assignment in a proc" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL ->() { a = 2 + # ^ error: Useless assignment to variable `a` } - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "reports a useless assignment in a block" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method 3.times do a = 1 + # ^ error: Useless assignment to variable `a` end end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "reports a useless assignment in a proc inside def" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method ->() { a = 2 + # ^ error: Useless assignment to variable `a` } end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "does not report ignored assignments" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL payload, _header = decode puts payload - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports a useless assignment in a proc inside a block" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method 3.times do ->() { a = 2 + # ^ error: Useless assignment to variable `a` } end end - ) - subject.catch(s).should_not be_valid - end - - it "reports rule, position and a message" do - s = Source.new %( - def method - a = 2 - end - ), "source.cr" - subject.catch(s).should_not be_valid - - issue = s.issues.first - issue.rule.should_not be_nil - issue.location.to_s.should eq "source.cr:2:3" - issue.message.should eq "Useless assignment to variable `a`" + CRYSTAL end it "does not report useless assignment of instance var" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL class Cls def initialize(@name) end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if assignment used in the inner block scope" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method var = true 3.times { var = false } end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assigned is not referenced in the inner block scope" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method var = true + # ^^^ error: Useless assignment to variable `var` 3.times {} end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "doesn't report if assignment in referenced in inner block" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method two = true @@ -132,184 +114,172 @@ module Ameba::Rule::Lint two.should be_true end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if first assignment is useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method var = true + # ^^^ error: Useless assignment to variable `var` var = false var end - ) - subject.catch(s).should_not be_valid - s.issues.first.location.to_s.should eq ":2:3" + CRYSTAL end it "reports if variable reassigned and not used" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method var = true + # ^^^ error: Useless assignment to variable `var` var = false + # ^^^ error: Useless assignment to variable `var` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "does not report if variable used in a condition" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 1 if a nil end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports second assignment as useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method a = 1 a = a + 1 + # ^ error: Useless assignment to variable `a` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "does not report if variable is referenced in other assignment" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method if f = get_something @f = f end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if variable is referenced in a setter" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method foo = 2 table[foo] ||= "bar" end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if variable is reassigned but not referenced" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method foo = 1 puts foo foo = 2 + # ^^^ error: Useless assignment to variable `foo` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "does not report if variable is referenced in a call" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method if f = FORMATTER @formatter = f.new end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if a setter is invoked with operator assignment" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method obj = {} of Symbol => Int32 obj[:name] = 3 end - ) - subject.catch(s).should be_valid + CRYSTAL end context "block unpacking" do it "does not report if the first arg is transformed and not used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL collection.each do |(a, b)| puts b end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if the second arg is transformed and not used" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL collection.each do |(a, b)| puts a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if all transformed args are not used in a block" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL collection.each do |(foo, bar), (baz, _qux), index, object| end - ) - subject.catch(s).should be_valid + CRYSTAL end end it "does not report if assignment is referenced in a proc" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method called = false ->() { called = true } called end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if variable is shadowed in inner scope" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method i = 1 + # ^ error: Useless assignment to variable `i` 3.times do |i| i + 1 end end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "does not report if parameter is referenced after the branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(param) 3.times do param = 3 end param end - ) - subject.catch(s).should be_valid + CRYSTAL end context "op assigns" do it "does not report if variable is referenced below the op assign" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 1 a += 1 a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if variable is referenced in op assign few times" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 1 a += 1 @@ -317,103 +287,76 @@ module Ameba::Rule::Lint a = a + 1 a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if variable is not referenced below the op assign" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method a = 1 a += 1 + # ^ error: Useless assignment to variable `a` end - ) - subject.catch(s).should_not be_valid - end - - it "reports rule, location and a message" do - s = Source.new %( - def method - b = 2 - a = 3 - a += 1 - end - ), "source.cr" - subject.catch(s).should_not be_valid - - issue = s.issues.last - issue.rule.should_not be_nil - issue.location.to_s.should eq "source.cr:4:3" - issue.message.should eq "Useless assignment to variable `a`" + CRYSTAL end end context "multi assigns" do it "does not report if all assigns are referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a, b = {1, 2} a + b end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if one assign is not referenced" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method a, b = {1, 2} + # ^ error: Useless assignment to variable `b` a end - ) - subject.catch(s).should_not be_valid - issue = s.issues.first - issue.location.to_s.should eq ":2:6" - issue.message.should eq "Useless assignment to variable `b`" + CRYSTAL end it "reports if both assigns are reassigned and useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method a, b = {1, 2} + # ^ error: Useless assignment to variable `a` + # ^ error: Useless assignment to variable `b` a, b = {3, 4} + # ^ error: Useless assignment to variable `a` + # ^ error: Useless assignment to variable `b` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "reports if both assigns are not referenced" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method a, b = {1, 2} + # ^ error: Useless assignment to variable `a` + # ^ error: Useless assignment to variable `b` end - ) - subject.catch(s).should_not be_valid - - issue = s.issues.first - issue.location.to_s.should eq ":2:3" - issue.message.should eq "Useless assignment to variable `a`" - - issue = s.issues.last - issue.location.to_s.should eq ":2:6" - issue.message.should eq "Useless assignment to variable `b`" + CRYSTAL end end context "top level" do it "reports if assignment is not referenced" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL a = 1 + # ^{} error: Useless assignment to variable `a` a = 2 - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":1:1" - s.issues.last.location.to_s.should eq ":2:1" + # ^{} error: Useless assignment to variable `a` + CRYSTAL end it "doesn't report if assignments are referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL a = 1 a += 1 a @@ -421,63 +364,53 @@ module Ameba::Rule::Lint b, c = {1, 2} b c - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is captured by block" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL a = 1 3.times do a = 2 end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment initialized and captured by block" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL a : String? = nil 1.times do a = "Fotis" end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if this is a record declaration" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL record Foo, foo = "foo" - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if assignment is referenced after the record declaration" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL foo = 2 record Bar, foo = 3 # foo = 3 is not parsed as assignment puts foo - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assignment is not referenced after the record declaration" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL foo = 2 + # ^ error: Useless assignment to variable `foo` record Bar, foo = 3 - ), "source.cr" - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - - issue = s.issues.first - issue.location.to_s.should eq "source.cr:1:1" - issue.message.should eq "Useless assignment to variable `foo`" + CRYSTAL end it "doesn't report if type declaration assigned inside module and referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL module A foo : String? = "foo" @@ -487,33 +420,28 @@ module Ameba::Rule::Lint p foo end - - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if type declaration assigned inside class" do - s = Source.new path: "source.cr", code: %( + expect_issue subject, <<-CRYSTAL class A foo : String? = "foo" + # ^^^^^^^^^^^^^^^^^^^^^ error: Useless assignment to variable `foo` def method foo = "bar" + # ^^^ error: Useless assignment to variable `foo` end end - ) - subject.catch(s).should_not be_valid - - issue = s.issues.first - issue.location.to_s.should eq "source.cr:2:3" - issue.message.should eq "Useless assignment to variable `foo`" + CRYSTAL end end context "branching" do context "if-then-else" do it "doesn't report if assignment is consumed by branches" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 0 if something @@ -523,12 +451,11 @@ module Ameba::Rule::Lint end a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is in one branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 0 if something @@ -538,65 +465,59 @@ module Ameba::Rule::Lint end a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is in one line branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 0 a = 1 if something a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assignment is useless in the branch" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method(a) if a a = 2 + # ^ error: Useless assignment to variable `a` end end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "reports if only last assignment is referenced in a branch" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method(a) a = 1 if a a = 2 + # ^ error: Useless assignment to variable `a` a = 3 end a end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":4:5" + CRYSTAL end it "does not report of assignments are referenced in all branches" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method if matches matches = owner.lookup_matches signature else matches = owner.lookup_matches signature end - matches end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report referenced assignments in inner branches" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method has_newline = false @@ -610,14 +531,13 @@ module Ameba::Rule::Lint has_newline end - ) - subject.catch(s).should be_valid + CRYSTAL end end context "unless-then-else" do it "doesn't report if assignment is consumed by branches" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 0 unless something @@ -627,32 +547,29 @@ module Ameba::Rule::Lint end a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if there is a useless assignment in a branch" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method a = 0 unless something a = 1 + # ^ error: Useless assignment to variable `a` a = 2 else a = 2 end a end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":4:5" + CRYSTAL end end context "case" do it "does not report if assignment is referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) case a when /foo/ @@ -662,92 +579,82 @@ module Ameba::Rule::Lint end puts a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assignment is useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method(a) case a when /foo/ a = 1 + # ^ error: Useless assignment to variable `a` when /bar/ a = 2 + # ^ error: Useless assignment to variable `a` end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":4:5" - s.issues.last.location.to_s.should eq ":6:5" + CRYSTAL end it "doesn't report if assignment is referenced in cond" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 2 case a when /foo/ end end - ) - subject.catch(s).should be_valid + CRYSTAL end end context "binary operator" do it "does not report if assignment is referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) (a = 1) && (b = 1) a + b end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assignment is useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method(a) (a = 1) || (b = 1) + # ^ error: Useless assignment to variable `b` a end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":2:15" + CRYSTAL end end context "while" do it "does not report if assignment is referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) while a < 10 a = a + 1 end a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assignment is useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method(a) while a < 10 b = a + # ^ error: Useless assignment to variable `b` end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":3:5" + CRYSTAL end it "does not report if assignment is referenced in a loop" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 3 result = 0 @@ -758,12 +665,11 @@ module Ameba::Rule::Lint end result end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if assignment is referenced as param in a loop" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) result = 0 @@ -773,12 +679,11 @@ module Ameba::Rule::Lint end result end - ) - subject.catch(s).should be_valid + CRYSTAL end it "does not report if assignment is referenced in loop and inner branch" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) result = 0 @@ -792,12 +697,11 @@ module Ameba::Rule::Lint end result end - ) - subject.catch(s).should be_valid + CRYSTAL end it "works properly if there is branch with blank node" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def visit count = 0 while true @@ -810,108 +714,94 @@ module Ameba::Rule::Lint count += 1 end end - ) - subject.catch(s).should be_valid + CRYSTAL end end context "until" do it "does not report if assignment is referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) until a > 10 a = a + 1 end a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assignment is useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method(a) until a > 10 b = a + 1 + # ^ error: Useless assignment to variable `b` end end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":3:5" + CRYSTAL end end context "exception handler" do it "does not report if assignment is referenced in body" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) a = 2 rescue a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in ensure" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) a = 2 ensure a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in else" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method(a) a = 2 rescue else a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "reports if assignment is useless" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def method(a) rescue a = 2 + # ^ error: Useless assignment to variable `a` end - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 1 - s.issues.first.location.to_s.should eq ":3:3" + CRYSTAL end end end context "typeof" do it "reports useless assignments in typeof" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL typeof(begin foo = 1 + # ^^^ error: Useless assignment to variable `foo` bar = 2 + # ^^^ error: Useless assignment to variable `bar` end) - ) - subject.catch(s).should_not be_valid - s.issues.size.should eq 2 - s.issues.first.location.to_s.should eq ":2:3" - s.issues.first.message.should eq "Useless assignment to variable `foo`" - - s.issues.last.location.to_s.should eq ":3:3" - s.issues.last.message.should eq "Useless assignment to variable `bar`" + CRYSTAL end end context "macro" do it "doesn't report if assignment is referenced in macro" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 2 {% if flag?(:bits64) %} @@ -920,12 +810,11 @@ module Ameba::Rule::Lint a {% end %} end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report referenced assignments in macro literal" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def method a = 2 {% if flag?(:bits64) %} @@ -935,12 +824,11 @@ module Ameba::Rule::Lint {% end %} puts a end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in macro def" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL macro macro_call puts x end @@ -949,12 +837,11 @@ module Ameba::Rule::Lint x = 1 macro_call end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in a macro below" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL class Foo def foo a = 1 @@ -965,73 +852,69 @@ module Ameba::Rule::Lint puts a end end - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in a macro expression as string" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL foo = 1 puts {{ "foo".id }} - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in for macro in exp" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL foo = 22 {% for x in %w[foo] %} add({{ x.id }}) {% end %} - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in for macro in body" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL foo = 22 {% for x in %w[bar] %} puts {{ "foo".id }} {% end %} - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in if macro in cond" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL foo = 22 + {% if "foo".id %} {% end %} - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in if macro in then" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL foo = 22 + {% if true %} puts {{ "foo".id }} {% end %} - ) - subject.catch(s).should be_valid + CRYSTAL end it "doesn't report if assignment is referenced in if macro in else" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL foo = 22 + {% if true %} {% else %} puts {{ "foo".id }} {% end %} - ) - subject.catch(s).should be_valid + CRYSTAL end end it "does not report if variable is referenced and there is a deep level scope" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL response = JSON.build do |json| json.object do json.object do @@ -1068,72 +951,69 @@ module Ameba::Rule::Lint response = JSON.parse(response) response - ) - subject.catch(s).should be_valid + CRYSTAL end context "type declaration" do it "reports if it's not referenced at a top level" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL a : String? - ) - subject.catch(s).should_not be_valid + # ^^^^^^^^^ error: Useless assignment to variable `a` + CRYSTAL end it "reports if it's not referenced in a method" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def foo a : String? + # ^^^^^^^^^^^ error: Useless assignment to variable `a` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "reports if it's not referenced in a class" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL class Foo a : String? + # ^^^^^^^^^^^ error: Useless assignment to variable `a` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "doesn't report if it's referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def foo a : String? a end - ) - subject.catch(s).should be_valid + CRYSTAL end end context "uninitialized" do it "reports if uninitialized assignment is not referenced at a top level" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL a = uninitialized U - ) - subject.catch(s).should_not be_valid + # ^{} error: Useless assignment to variable `a` + CRYSTAL end it "reports if uninitialized assignment is not referenced in a method" do - s = Source.new %( + expect_issue subject, <<-CRYSTAL def foo a = uninitialized U + # ^ error: Useless assignment to variable `a` end - ) - subject.catch(s).should_not be_valid + CRYSTAL end it "doesn't report if uninitialized assignment is referenced" do - s = Source.new %( + expect_no_issues subject, <<-CRYSTAL def foo a = uninitialized U a end - ) - subject.catch(s).should be_valid + CRYSTAL end end end From 4567293add134a591e79e1177c5d110a3c89e6ac Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 28 Dec 2023 00:50:33 +0100 Subject: [PATCH 6/8] Drop `type_definition?` check from `Scope#top_level?` --- src/ameba/ast/scope.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 38ef9944..97b71483 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -198,7 +198,7 @@ module Ameba::AST # Returns `true` if this scope is a top level scope, `false` otherwise. def top_level? - outer_scope.nil? || type_definition? + outer_scope.nil? end # Returns `true` if var is an argument in current scope, `false` otherwise. From aeffa6ad002f354319cff5ec579858778dcf20e1 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 28 Dec 2023 03:29:09 +0100 Subject: [PATCH 7/8] Add test spec covering accessor macros --- spec/ameba/rule/lint/useless_assign_spec.cr | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index 90f13c70..726acfbb 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -393,6 +393,23 @@ module Ameba::Rule::Lint CRYSTAL end + it "doesn't report if this is an accessor declaration" do + accessor_macros = %w[setter class_setter] + %w[getter class_getter property class_property].each do |name| + accessor_macros << name + accessor_macros << "#{name}?" + accessor_macros << "#{name}!" + end + accessor_macros.each do |accessor| + expect_no_issues subject, <<-CRYSTAL + class Foo + #{accessor} foo : String? + #{accessor} bar = "bar" + end + CRYSTAL + end + end + it "does not report if assignment is referenced after the record declaration" do expect_no_issues subject, <<-CRYSTAL foo = 2 From 5a24f1eba58bfb58221ebe43b2ced77db82d7a3b Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 29 Dec 2023 01:50:19 +0100 Subject: [PATCH 8/8] Add `UselessAssign#exclude_type_declarations` --- spec/ameba/rule/lint/useless_assign_spec.cr | 22 +++++++++++++++++++++ src/ameba/rule/lint/useless_assign.cr | 3 +++ 2 files changed, 25 insertions(+) diff --git a/spec/ameba/rule/lint/useless_assign_spec.cr b/spec/ameba/rule/lint/useless_assign_spec.cr index 726acfbb..7c7310de 100644 --- a/spec/ameba/rule/lint/useless_assign_spec.cr +++ b/spec/ameba/rule/lint/useless_assign_spec.cr @@ -3,6 +3,7 @@ require "../../../spec_helper" module Ameba::Rule::Lint describe UselessAssign do subject = UselessAssign.new + .tap(&.exclude_type_declarations = false) it "does not report used assignments" do expect_no_issues subject, <<-CRYSTAL @@ -1007,6 +1008,27 @@ module Ameba::Rule::Lint end end + context "#properties" do + context "#exclude_type_declarations" do + it "doesn't report type declarations if enabled" do + rule = UselessAssign.new + rule.exclude_type_declarations = true + + expect_no_issues rule, <<-CRYSTAL + a : String? + + class Foo + b : String? + end + + def foo + c : String? + end + CRYSTAL + end + end + end + context "uninitialized" do it "reports if uninitialized assignment is not referenced at a top level" do expect_issue subject, <<-CRYSTAL diff --git a/src/ameba/rule/lint/useless_assign.cr b/src/ameba/rule/lint/useless_assign.cr index 7aec210d..ed7d536d 100644 --- a/src/ameba/rule/lint/useless_assign.cr +++ b/src/ameba/rule/lint/useless_assign.cr @@ -24,10 +24,12 @@ module Ameba::Rule::Lint # ``` # Lint/UselessAssign: # Enabled: true + # ExcludeTypeDeclarations: false # ``` class UselessAssign < Base properties do description "Disallows useless variable assignments" + exclude_type_declarations false end MSG = "Useless assignment to variable `%s`" @@ -39,6 +41,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 exclude_type_declarations? && scope.assigns_type_dec?(var.name) var.assignments.each do |assign| next if assign.referenced?