From 22c46efbf8a3b8ba1914e62f93bac6419f4c0ae8 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Wed, 12 Feb 2025 13:09:42 -0500 Subject: [PATCH 01/12] ImplicitReturnVisitor keeps track of whether the current node is in a macro or not Fixes for ImplicitReturnVisitor macro implicit returns --- .../ast/visitors/implicit_return_visitor.cr | 50 +++++++++++++++---- src/ameba/rule/lint/unused_comparison.cr | 2 +- .../rule/lint/unused_generic_or_union.cr | 4 +- src/ameba/rule/lint/unused_literal.cr | 3 +- 4 files changed, 44 insertions(+), 15 deletions(-) diff --git a/src/ameba/ast/visitors/implicit_return_visitor.cr b/src/ameba/ast/visitors/implicit_return_visitor.cr index bb8d8a327..6de191c33 100644 --- a/src/ameba/ast/visitors/implicit_return_visitor.cr +++ b/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,9 +356,13 @@ 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 @@ -351,7 +370,9 @@ module Ameba::AST 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 diff --git a/src/ameba/rule/lint/unused_comparison.cr b/src/ameba/rule/lint/unused_comparison.cr index afbd93c02..2547fc5b9 100644 --- a/src/ameba/rule/lint/unused_comparison.cr +++ b/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 diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 1f21d4672..bd2866bc0 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/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 diff --git a/src/ameba/rule/lint/unused_literal.cr b/src/ameba/rule/lint/unused_literal.cr index a6aa58f49..480417499 100644 --- a/src/ameba/rule/lint/unused_literal.cr +++ b/src/ameba/rule/lint/unused_literal.cr @@ -55,7 +55,7 @@ module Ameba::Rule::Lint AST::ImplicitReturnVisitor.new(self, source) end - def test(source, node : Crystal::RegexLiteral) + def test(source, node : Crystal::RegexLiteral, in_macro : Bool) # Locations for Regex literals were added in Crystal v1.15.0 {% if compare_versions(Crystal::VERSION, "1.15.0") >= 0 %} issue_for node, MSG @@ -69,6 +69,7 @@ module Ameba::Rule::Lint Crystal::TupleLiteral | Crystal::NumberLiteral | Crystal::StringLiteral | Crystal::SymbolLiteral | Crystal::NamedTupleLiteral | Crystal::StringInterpolation, + in_macro : Bool, ) issue_for node, MSG end From cc7e293eba9ca542cb3e4c46b72dec4812b937fb Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Wed, 12 Feb 2025 13:19:38 -0500 Subject: [PATCH 02/12] Add `Lint/UnusedLocalVariableAccess` --- .../lint/unused_local_variable_access_spec.cr | 69 ++++++++++++++++ .../rule/lint/unused_local_variable_access.cr | 78 +++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 spec/ameba/rule/lint/unused_local_variable_access_spec.cr create mode 100644 src/ameba/rule/lint/unused_local_variable_access.cr diff --git a/spec/ameba/rule/lint/unused_local_variable_access_spec.cr b/spec/ameba/rule/lint/unused_local_variable_access_spec.cr new file mode 100644 index 000000000..31e267a88 --- /dev/null +++ b/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 + a = 1 + a += 1 + a, b = 2, 3 + CRYSTAL + end + + it "passes if a local variable is a call argument" do + expect_no_issues subject, <<-CRYSTAL + a = 1 + puts a + CRYSTAL + end + + it "passes if local variable on left side of a comparison" do + expect_no_issues subject, <<-CRYSTAL + def hello + a = 1 + a || (puts "a is falsey") + a + 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 + a = 1 + + begin + a + # ^ error: Value from local variable access is unused + puts a + 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 diff --git a/src/ameba/rule/lint/unused_local_variable_access.cr b/src/ameba/rule/lint/unused_local_variable_access.cr new file mode 100644 index 000000000..09d916288 --- /dev/null +++ b/src/ameba/rule/lint/unused_local_variable_access.cr @@ -0,0 +1,78 @@ +module Ameba::Rule::Lint + # A rule that disallows unused local variable access. + # + # For example, this is considered invalid: + # + # ``` + # a = 1 + # b = "2" + # c = :3 + # + # case method_call + # when Int32 + # a + # when String + # b + # else + # c + # end + # + # def hello(name) + # if name.size < 10 + # name + # end + # + # name[...10] + # end + # ``` + # + # And these are considered valid: + # + # ``` + # a = 1 + # b = "2" + # c = :3 + # + # d = case method_call + # when Int32 + # a + # when String + # b + # else + # c + # end + # + # def hello(name) + # if name.size < 10 + # return name + # end + # + # name[...10] + # end + # ``` + # + # YAML configuration example: + # + # ``` + # Lint/UnusedLocalVariableAccess: + # Enabled: true + # ``` + class UnusedLocalVariableAccess < Base + properties do + since_version "1.7.0" + description "Disallows unused access to local variables" + end + + MSG = "Value from local variable access is unused" + + def test(source : Source) + AST::ImplicitReturnVisitor.new(self, source) + end + + def test(source, node : Crystal::Var, in_macro : Bool) + return if node.name.in?("self") || (in_macro && node.name.in?("skip_file", "debug")) + + issue_for node, MSG + end + end +end From d5242be7be721791e826edd69f77ee2c6370c921 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Wed, 12 Feb 2025 13:20:05 -0500 Subject: [PATCH 03/12] Add `Lint/UnusedInstanceVariableAccess` --- .../unused_instance_variable_access_spec.cr | 55 +++++++++++++++++ .../lint/unused_instance_variable_access.cr | 60 +++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 spec/ameba/rule/lint/unused_instance_variable_access_spec.cr create mode 100644 src/ameba/rule/lint/unused_instance_variable_access.cr diff --git a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr new file mode 100644 index 000000000..789a3e2ab --- /dev/null +++ b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr @@ -0,0 +1,55 @@ +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 + a = @ivar + B = @ivar + 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 "passes if an instance variable is used as a target in multi-assignment" do + expect_no_issues subject, <<-CRYSTAL + class MyClass + @a, @b = 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 + end +end diff --git a/src/ameba/rule/lint/unused_instance_variable_access.cr b/src/ameba/rule/lint/unused_instance_variable_access.cr new file mode 100644 index 000000000..f11b88ee3 --- /dev/null +++ b/src/ameba/rule/lint/unused_instance_variable_access.cr @@ -0,0 +1,60 @@ +module Ameba::Rule::Lint + # A rule that disallows unused instance 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/UnusedInstanceVariableAccess: + # Enabled: true + # ``` + class UnusedInstanceVariableAccess < Base + properties do + since_version "1.7.0" + description "Disallows unused access to instance variables" + end + + MSG = "Value from instance variable access is unused" + + def test(source : Source) + AST::ImplicitReturnVisitor.new(self, source) + end + + def test(source, node : Crystal::InstanceVar, in_macro : Bool) + return if in_macro && node.name.in?("@type") + + issue_for node, MSG + end + end +end From 258a812feaffe886f50857b5306a145f853b39fc Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Wed, 12 Feb 2025 13:20:12 -0500 Subject: [PATCH 04/12] Add `Lint/UnusedClassVariableAccess` --- .../lint/unused_class_variable_access_spec.cr | 46 +++++++++++++++ .../rule/lint/unused_class_variable_access.cr | 58 +++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 spec/ameba/rule/lint/unused_class_variable_access_spec.cr create mode 100644 src/ameba/rule/lint/unused_class_variable_access.cr diff --git a/spec/ameba/rule/lint/unused_class_variable_access_spec.cr b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr new file mode 100644 index 000000000..f15458d8d --- /dev/null +++ b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr @@ -0,0 +1,46 @@ +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 + a = @@ivar + B = @@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 + @@a, @@b = 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 diff --git a/src/ameba/rule/lint/unused_class_variable_access.cr b/src/ameba/rule/lint/unused_class_variable_access.cr new file mode 100644 index 000000000..49118c05a --- /dev/null +++ b/src/ameba/rule/lint/unused_class_variable_access.cr @@ -0,0 +1,58 @@ +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) + issue_for node, MSG + end + end +end From ad763763e44bbfb3c3d6f2bebad7178762b3fbae Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 12:14:34 -0500 Subject: [PATCH 05/12] Update src/ameba/rule/lint/unused_local_variable_access.cr Co-authored-by: Sijawusz Pur Rahnama --- src/ameba/rule/lint/unused_local_variable_access.cr | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ameba/rule/lint/unused_local_variable_access.cr b/src/ameba/rule/lint/unused_local_variable_access.cr index 09d916288..a0c3b6511 100644 --- a/src/ameba/rule/lint/unused_local_variable_access.cr +++ b/src/ameba/rule/lint/unused_local_variable_access.cr @@ -70,7 +70,8 @@ module Ameba::Rule::Lint end def test(source, node : Crystal::Var, in_macro : Bool) - return if node.name.in?("self") || (in_macro && node.name.in?("skip_file", "debug")) + return if node.name == "self" # This case will be reported by `Lint/UnusedSelf` rule + return if in_macro && node.name.in?("debug", "skip_file") issue_for node, MSG end From 7441e0f6c748f04b90462a198291677854f02a7d Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 12:45:55 -0500 Subject: [PATCH 06/12] Update src/ameba/rule/lint/unused_instance_variable_access.cr Co-authored-by: Sijawusz Pur Rahnama --- src/ameba/rule/lint/unused_instance_variable_access.cr | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ameba/rule/lint/unused_instance_variable_access.cr b/src/ameba/rule/lint/unused_instance_variable_access.cr index f11b88ee3..5fa74dc23 100644 --- a/src/ameba/rule/lint/unused_instance_variable_access.cr +++ b/src/ameba/rule/lint/unused_instance_variable_access.cr @@ -52,7 +52,8 @@ module Ameba::Rule::Lint end def test(source, node : Crystal::InstanceVar, in_macro : Bool) - return if in_macro && node.name.in?("@type") + # Handle special case when using `@type` within a method body has side-effects + return if in_macro && node.name == "@type" issue_for node, MSG end From e5da8ccf06662082a3f1d0311eb21217ba8341fa Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 15:37:22 -0500 Subject: [PATCH 07/12] Fix heredoc interpolations --- spec/ameba/rule/lint/unused_class_variable_access_spec.cr | 4 ++-- spec/ameba/rule/lint/unused_instance_variable_access_spec.cr | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/ameba/rule/lint/unused_class_variable_access_spec.cr b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr index f15458d8d..43647f2c0 100644 --- a/spec/ameba/rule/lint/unused_class_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr @@ -33,12 +33,12 @@ module Ameba::Rule::Lint end it "fails if class variables are unused in void context of method" do - expect_issue subject, <<-CRYSTAL + expect_issue subject, <<-'CRYSTAL' def hello : String @@name # ^^^^^^ error: Value from class variable access is unused - "Hello, \#{@@name}!" + "Hello, #{@@name}!" end CRYSTAL end diff --git a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr index 789a3e2ab..0ec7050ec 100644 --- a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr @@ -42,12 +42,12 @@ module Ameba::Rule::Lint end it "fails if instance variables are unused in void context of method" do - expect_issue subject, <<-CRYSTAL + expect_issue subject, <<-'CRYSTAL' def hello : String @name # ^^^^^ error: Value from instance variable access is unused - "Hello, \#{@name}!" + "Hello, #{@name}!" end CRYSTAL end From fa7244e774a17421ad34f153a54878c3e306f876 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 15:39:21 -0500 Subject: [PATCH 08/12] Update specs from review --- .../lint/unused_class_variable_access_spec.cr | 6 ++--- .../unused_instance_variable_access_spec.cr | 6 ++--- .../lint/unused_local_variable_access_spec.cr | 22 +++++++++---------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/spec/ameba/rule/lint/unused_class_variable_access_spec.cr b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr index 43647f2c0..5a9f4b110 100644 --- a/spec/ameba/rule/lint/unused_class_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr @@ -7,8 +7,8 @@ module Ameba::Rule::Lint it "passes if class variables are used for assignment" do expect_no_issues subject, <<-CRYSTAL class MyClass - a = @@ivar - B = @@ivar + foo = @@ivar + Bar = @@ivar end CRYSTAL end @@ -16,7 +16,7 @@ module Ameba::Rule::Lint it "passes if an class variable is used as a target in multi-assignment" do expect_no_issues subject, <<-CRYSTAL class MyClass - @@a, @@b = 1, 2 + @@foo, @@bar = 1, 2 end CRYSTAL end diff --git a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr index 0ec7050ec..19e42e070 100644 --- a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr @@ -7,8 +7,8 @@ module Ameba::Rule::Lint it "passes if instance variables are used for assignment" do expect_no_issues subject, <<-CRYSTAL class MyClass - a = @ivar - B = @ivar + foo = @ivar + Bar = @ivar end CRYSTAL end @@ -25,7 +25,7 @@ module Ameba::Rule::Lint it "passes if an instance variable is used as a target in multi-assignment" do expect_no_issues subject, <<-CRYSTAL class MyClass - @a, @b = 1, 2 + @foo, @bar = 1, 2 end CRYSTAL end diff --git a/spec/ameba/rule/lint/unused_local_variable_access_spec.cr b/spec/ameba/rule/lint/unused_local_variable_access_spec.cr index 31e267a88..319117d87 100644 --- a/spec/ameba/rule/lint/unused_local_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_local_variable_access_spec.cr @@ -6,25 +6,25 @@ module Ameba::Rule::Lint it "passes if local variables are used in assign" do expect_no_issues subject, <<-CRYSTAL - a = 1 - a += 1 - a, b = 2, 3 + 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 - a = 1 - puts a + 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 - a = 1 - a || (puts "a is falsey") - a + foo = 1 + foo || (puts "foo is falsey") + foo end CRYSTAL end @@ -43,12 +43,12 @@ module Ameba::Rule::Lint it "fails if a local variable is in a void context" do expect_issue subject, <<-CRYSTAL - a = 1 + foo = 1 begin - a + foo # ^ error: Value from local variable access is unused - puts a + puts foo end CRYSTAL end From 37cd40ef21283f4a5216c4816d18ba4aac6e33ba Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 16:13:55 -0500 Subject: [PATCH 09/12] Apply suggestions from code review Co-authored-by: Sijawusz Pur Rahnama --- spec/ameba/rule/lint/unused_class_variable_access_spec.cr | 1 - spec/ameba/rule/lint/unused_instance_variable_access_spec.cr | 1 - spec/ameba/rule/lint/unused_local_variable_access_spec.cr | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/ameba/rule/lint/unused_class_variable_access_spec.cr b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr index 5a9f4b110..6c2d38171 100644 --- a/spec/ameba/rule/lint/unused_class_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_class_variable_access_spec.cr @@ -8,7 +8,6 @@ module Ameba::Rule::Lint expect_no_issues subject, <<-CRYSTAL class MyClass foo = @@ivar - Bar = @@ivar end CRYSTAL end diff --git a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr index 19e42e070..ffe287638 100644 --- a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr @@ -8,7 +8,6 @@ module Ameba::Rule::Lint expect_no_issues subject, <<-CRYSTAL class MyClass foo = @ivar - Bar = @ivar end CRYSTAL end diff --git a/spec/ameba/rule/lint/unused_local_variable_access_spec.cr b/spec/ameba/rule/lint/unused_local_variable_access_spec.cr index 319117d87..eda28bcbd 100644 --- a/spec/ameba/rule/lint/unused_local_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_local_variable_access_spec.cr @@ -47,7 +47,7 @@ module Ameba::Rule::Lint begin foo - # ^ error: Value from local variable access is unused + # ^^^ error: Value from local variable access is unused puts foo end CRYSTAL From 1f4e88565930b2ddc904b0f3d0983d70535da079 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 16:17:35 -0500 Subject: [PATCH 10/12] Update specs, disable unused cvars in macros --- .../rule/lint/unused_instance_variable_access_spec.cr | 10 ++++++++++ src/ameba/rule/lint/unused_class_variable_access.cr | 3 +++ 2 files changed, 13 insertions(+) diff --git a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr index ffe287638..f1dafbf37 100644 --- a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr @@ -50,5 +50,15 @@ module Ameba::Rule::Lint end CRYSTAL end + + it "fails if @def is unused within a macro expression" do + expect_issue subject, <<-CRYSTAL + def foo + {% @def %} + # ^^^^ error: Value from instance variable access is unused + :bar + end + CRYSTAL + end end end diff --git a/src/ameba/rule/lint/unused_class_variable_access.cr b/src/ameba/rule/lint/unused_class_variable_access.cr index 49118c05a..ba5ad388d 100644 --- a/src/ameba/rule/lint/unused_class_variable_access.cr +++ b/src/ameba/rule/lint/unused_class_variable_access.cr @@ -52,6 +52,9 @@ module Ameba::Rule::Lint 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 From 586da6ae3bb0b3ff0f1741b5a6de4a5acc94af25 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 16:22:34 -0500 Subject: [PATCH 11/12] Update spec/ameba/rule/lint/unused_instance_variable_access_spec.cr Co-authored-by: Sijawusz Pur Rahnama --- .../ameba/rule/lint/unused_instance_variable_access_spec.cr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr index f1dafbf37..61330d796 100644 --- a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr @@ -51,12 +51,12 @@ module Ameba::Rule::Lint CRYSTAL end - it "fails if @def is unused within a macro expression" do + it "fails if instance variable is unused within a macro expression" do expect_issue subject, <<-CRYSTAL def foo - {% @def %} + {% @bar %} # ^^^^ error: Value from instance variable access is unused - :bar + :baz end CRYSTAL end From 50a4ce4d1416c56ce9755d6958666a674ff35b4b Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 14 Feb 2025 16:29:13 -0500 Subject: [PATCH 12/12] Reorder spec --- .../unused_instance_variable_access_spec.cr | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr index 61330d796..7408df2f9 100644 --- a/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr +++ b/spec/ameba/rule/lint/unused_instance_variable_access_spec.cr @@ -12,15 +12,6 @@ module Ameba::Rule::Lint 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 "passes if an instance variable is used as a target in multi-assignment" do expect_no_issues subject, <<-CRYSTAL class MyClass @@ -51,6 +42,15 @@ module Ameba::Rule::Lint 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