From e6d0abeb927939dccab390176529cdd6b62194b7 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Tue, 14 Jan 2025 19:49:24 -0500 Subject: [PATCH 01/21] Add `Lint/UnusedPathGenericOrUnion` --- .../lint/unused_path_generic_or_union_spec.cr | 80 +++++++++++++++++ .../ast/visitors/implicit_return_visitor.cr | 74 ++++++++++++++-- .../rule/lint/unused_path_generic_or_union.cr | 85 +++++++++++++++++++ 3 files changed, 232 insertions(+), 7 deletions(-) create mode 100644 spec/ameba/rule/lint/unused_path_generic_or_union_spec.cr create mode 100644 src/ameba/rule/lint/unused_path_generic_or_union.cr diff --git a/spec/ameba/rule/lint/unused_path_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_path_generic_or_union_spec.cr new file mode 100644 index 000000000..8201da69d --- /dev/null +++ b/spec/ameba/rule/lint/unused_path_generic_or_union_spec.cr @@ -0,0 +1,80 @@ +require "../../../spec_helper" + +module Ameba::Rule::Lint + subject = UnusedPathGenericOrUnion.new + + describe UnusedPathGenericOrUnion do + it "passes if paths and generic types are used" do + expect_no_issues subject, <<-CRYSTAL + MyConst = 1 + + my_var : String? = EMPTY_STRING + + my_var.as(String) + + puts StaticArray(Int32, 10) + + class MyClass < MySuperClass + include MyModule + extend MyModule + end + + a : Int32 = 10 + + klass = String? + + alias MyType = Float64 | StaticArray(Float64, 10) + + def size : Float64 + 0.1 + end + + lib MyLib + type MyType = Void* + + struct MyStruct + field1, field2 : Float64 + end + end + + fun fun_name : Int32 + 1234 + end + + Int32 | "Float64" + + MyClass.run + CRYSTAL + end + + it "fails if paths and generic types are top-level" do + expect_issue subject, <<-CRYSTAL + Int32 + # ^^^ error: Path or generic type is not used + String? + # ^^^^^ error: Path or generic type is not used + Int32 | Float64 | Nil + # ^^^^^^^^^^^^^^^^^^^ error: Path or generic type is not used + StaticArray(Int32, 10) + # ^^^^^^^^^^^^^^^^^^^^ error: Path or generic type is not used + + def hello + Float64 + # ^^^^^^^ error: Path or generic type is not used + 0.1 + end + + fun fun_name : Int32 + Int32 + # ^^^^^ error: Path or generic type is not used + 1234 + end + + class MyClass + MyModule + # ^^^^^^^^ error: Path or generic type is not used + end + CRYSTAL + end + end +end diff --git a/src/ameba/ast/visitors/implicit_return_visitor.cr b/src/ameba/ast/visitors/implicit_return_visitor.cr index 27828aa12..7db58b1f3 100644 --- a/src/ameba/ast/visitors/implicit_return_visitor.cr +++ b/src/ameba/ast/visitors/implicit_return_visitor.cr @@ -41,13 +41,8 @@ module Ameba::AST def visit(node : Crystal::Call) : Bool @rule.test(@source, node, @stack > 0) - if node.block || !node.args.empty? - incr_stack { node.obj.try &.accept(self) } - else - node.obj.try &.accept(self) - end - incr_stack do + node.obj.try &.accept(self) node.args.each &.accept(self) node.named_args.try &.each &.accept(self) node.block_arg.try &.accept(self) @@ -133,6 +128,48 @@ module Ameba::AST false end + def visit(node : Crystal::ClassDef) : Bool + incr_stack do + node.name.accept(self) + node.superclass.try &.accept(self) + end + + node.body.accept(self) + + false + end + + def visit(node : Crystal::FunDef) : Bool + incr_stack do + node.args.each &.accept(self) + node.return_type.try &.accept(self) + node.body.try &.accept(self) + end + + false + end + + def visit(node : Crystal::ExternalVar) : Bool + incr_stack { node.type_spec.accept(self) } + + false + end + + def visit(node : Crystal::Include | Crystal::Extend) : Bool + incr_stack do + node.name.accept(self) + end + + false + end + + def visit(node : Crystal::Cast | Crystal::NilableCast) : Bool + node.obj.accept(self) + incr_stack { node.to.accept(self) } + + false + end + def visit(node : Crystal::Annotation) : Bool @rule.test(@source, node, @stack > 0) @@ -282,7 +319,30 @@ module Ameba::AST false end - def visit(node : Crystal::Generic) : Bool + def visit(node : Crystal::Generic | Crystal::Path | Crystal::Union) : Bool + @rule.test(@source, node, @stack > 0) + + false + end + + def visit(node : Crystal::Alias) + @rule.test(@source, node, @stack > 0) + + incr_stack do + node.name.accept(self) + node.value.accept(self) + end + + false + end + + def visit(node : Crystal::TypeDef) + @rule.test(@source, node, @stack > 0) + + incr_stack do + node.type_spec.accept(self) + end + false end diff --git a/src/ameba/rule/lint/unused_path_generic_or_union.cr b/src/ameba/rule/lint/unused_path_generic_or_union.cr new file mode 100644 index 000000000..bc61a4a17 --- /dev/null +++ b/src/ameba/rule/lint/unused_path_generic_or_union.cr @@ -0,0 +1,85 @@ +module Ameba::Rule::Lint + # A rule that disallows unused Paths, Generics, or Unions (Int32, String?, StaticArray(Int32, 10), etc). + # + # For example, these are considered invalid: + # + # ``` + # Int32 + # + # String? + # + # Float64 | StaticArray(Float64, 10) + # + # def size + # Float64 + # 0.1 + # end + # ``` + # + # And these are considered valid: + # + # ``` + # a : Int32 = 10 + # + # klass = String? + # + # alias MyType = Float64 | StaticArray(Float64, 10) + # + # def size : Float64 + # 0.1 + # end + # ``` + # + # YAML configuration example: + # + # ``` + # Lint/UnusedPathGenericOrUnion: + # Enabled: true + # ``` + class UnusedPathGenericOrUnion < Base + properties do + since_version "1.7.0" + description "Disallows unused literal values" + end + + MSG = "Path or generic type is not used" + + def test(source : Source) + AST::ImplicitReturnVisitor.new(self, source) + end + + def test(source, node : Crystal::Call, last_is_used : Bool) + return if last_is_used || !path_or_generic_union?(node) + + issue_for node, MSG + end + + def test(source, node : Crystal::Path | Crystal::Generic | Crystal::Union, last_is_used : Bool) + issue_for node, MSG unless last_is_used + end + + def path_or_generic_union?(node : Crystal::Call) : Bool + return false unless node.name == "|" && node.args.size == 1 + + case lhs = node.obj + when Crystal::Path, Crystal::Generic + # Okay + when Crystal::Call + return false unless path_or_generic_union?(lhs) + else + return false + end + + case rhs = node.args.first? + when Crystal::Path, Crystal::Generic + # Okay + when Crystal::Call + return false unless path_or_generic_union?(rhs) + else + return false + end + + true + end + end +end From 584ac3ad4bfaaba3dab00e593ca48a73872de8fa Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Fri, 17 Jan 2025 20:38:29 -0500 Subject: [PATCH 02/21] UnusedPathGenericOrUnion -> UnusedTypeOrConstant --- ...ric_or_union_spec.cr => unused_type_or_constant_spec.cr} | 4 ++-- ..._path_generic_or_union.cr => unused_type_or_constant.cr} | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) rename spec/ameba/rule/lint/{unused_path_generic_or_union_spec.cr => unused_type_or_constant_spec.cr} (95%) rename src/ameba/rule/lint/{unused_path_generic_or_union.cr => unused_type_or_constant.cr} (89%) diff --git a/spec/ameba/rule/lint/unused_path_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_type_or_constant_spec.cr similarity index 95% rename from spec/ameba/rule/lint/unused_path_generic_or_union_spec.cr rename to spec/ameba/rule/lint/unused_type_or_constant_spec.cr index 8201da69d..4c5a3d3ea 100644 --- a/spec/ameba/rule/lint/unused_path_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_type_or_constant_spec.cr @@ -1,9 +1,9 @@ require "../../../spec_helper" module Ameba::Rule::Lint - subject = UnusedPathGenericOrUnion.new + subject = UnusedTypeOrConstant.new - describe UnusedPathGenericOrUnion do + describe UnusedTypeOrConstant do it "passes if paths and generic types are used" do expect_no_issues subject, <<-CRYSTAL MyConst = 1 diff --git a/src/ameba/rule/lint/unused_path_generic_or_union.cr b/src/ameba/rule/lint/unused_type_or_constant.cr similarity index 89% rename from src/ameba/rule/lint/unused_path_generic_or_union.cr rename to src/ameba/rule/lint/unused_type_or_constant.cr index bc61a4a17..14e3f25e7 100644 --- a/src/ameba/rule/lint/unused_path_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_type_or_constant.cr @@ -1,5 +1,5 @@ module Ameba::Rule::Lint - # A rule that disallows unused Paths, Generics, or Unions (Int32, String?, StaticArray(Int32, 10), etc). + # A rule that disallows unused constants, generics, or unions (`Int32`, `String?`, `StaticArray(Int32, 10)`, etc). # # For example, these are considered invalid: # @@ -33,10 +33,10 @@ module Ameba::Rule::Lint # YAML configuration example: # # ``` - # Lint/UnusedPathGenericOrUnion: + # Lint/UnusedTypeOrConstant: # Enabled: true # ``` - class UnusedPathGenericOrUnion < Base + class UnusedTypeOrConstant < Base properties do since_version "1.7.0" description "Disallows unused literal values" From 164e8c2729c856414eb437cc0240d3e82f057ad0 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 19 Jan 2025 10:17:57 -0500 Subject: [PATCH 03/21] Split up specs for clarity --- .../rule/lint/unused_type_or_constant_spec.cr | 86 +++++++++++++------ .../rule/lint/unused_type_or_constant.cr | 2 +- 2 files changed, 63 insertions(+), 25 deletions(-) diff --git a/spec/ameba/rule/lint/unused_type_or_constant_spec.cr b/spec/ameba/rule/lint/unused_type_or_constant_spec.cr index 4c5a3d3ea..e3ebc4361 100644 --- a/spec/ameba/rule/lint/unused_type_or_constant_spec.cr +++ b/spec/ameba/rule/lint/unused_type_or_constant_spec.cr @@ -4,75 +4,113 @@ module Ameba::Rule::Lint subject = UnusedTypeOrConstant.new describe UnusedTypeOrConstant do - it "passes if paths and generic types are used" do + it "passes if paths and generic types are used for assign and method calls" do expect_no_issues subject, <<-CRYSTAL MyConst = 1 my_var : String? = EMPTY_STRING + a : Int32 = 10 + + klass = String? + my_var.as(String) puts StaticArray(Int32, 10) - class MyClass < MySuperClass - include MyModule - extend MyModule + Int32 | "Float64" + + MyClass.run + CRYSTAL + end + + it "passes if paths are used for methods" do + expect_no_issues subject, <<-CRYSTAL + def size(a : Float64) : Float64 + 0.1 + a end - a : Int32 = 10 + fun fun_name = FunName(a : Int32) : Int32 + 1234 + a + end + CRYSTAL + end - klass = String? + it "passes if paths are used for classes, modules, aliases, and annotations" do + expect_no_issues subject, <<-CRYSTAL + module MyModule(T) + struct MyStruct < MySuperStruct + end + end - alias MyType = Float64 | StaticArray(Float64, 10) + @[MyAnnotation] + class MyClass < MySuperClass + include MyModule(Int23) + extend MyModule(String) - def size : Float64 - 0.1 + alias MyType = Float64 | StaticArray(Float64, 10) end + annotation MyAnnotation + end + CRYSTAL + end + + it "passes if paths are used for lib objects" do + expect_no_issues subject, <<-CRYSTAL lib MyLib + $external_var = ExternalVarName : Int32 + type MyType = Void* struct MyStruct field1, field2 : Float64 end - end - fun fun_name : Int32 - 1234 + fun fun_name = FunName(Void*) : Void end - - Int32 | "Float64" - - MyClass.run CRYSTAL end - it "fails if paths and generic types are top-level" do + it "fails if paths and generic types are unused top-level" do expect_issue subject, <<-CRYSTAL Int32 - # ^^^ error: Path or generic type is not used + # ^^^ error: Type or constant is not used String? - # ^^^^^ error: Path or generic type is not used + # ^^^^^ error: Type or constant is not used Int32 | Float64 | Nil - # ^^^^^^^^^^^^^^^^^^^ error: Path or generic type is not used + # ^^^^^^^^^^^^^^^^^^^ error: Type or constant is not used StaticArray(Int32, 10) - # ^^^^^^^^^^^^^^^^^^^^ error: Path or generic type is not used + # ^^^^^^^^^^^^^^^^^^^^ error: Type or constant is not used + CRYSTAL + end + it "fails if types and constants are unused inside methods" do + expect_issue subject, <<-CRYSTAL def hello Float64 - # ^^^^^^^ error: Path or generic type is not used + # ^^^^^^^ error: Type or constant is not used 0.1 end fun fun_name : Int32 Int32 - # ^^^^^ error: Path or generic type is not used + # ^^^^^ error: Type or constant is not used 1234 end + CRYSTAL + end + it "fails if types and constants are unused inside classes and modules" do + expect_issue subject, <<-CRYSTAL class MyClass MyModule - # ^^^^^^^^ error: Path or generic type is not used + # ^^^^^^^^ error: Type or constant is not used + end + + module MyModule + Int32 + # ^^^^^ error: Type or constant is not used end CRYSTAL end diff --git a/src/ameba/rule/lint/unused_type_or_constant.cr b/src/ameba/rule/lint/unused_type_or_constant.cr index 14e3f25e7..48527462e 100644 --- a/src/ameba/rule/lint/unused_type_or_constant.cr +++ b/src/ameba/rule/lint/unused_type_or_constant.cr @@ -42,7 +42,7 @@ module Ameba::Rule::Lint description "Disallows unused literal values" end - MSG = "Path or generic type is not used" + MSG = "Type or constant is not used" def test(source : Source) AST::ImplicitReturnVisitor.new(self, source) From fc4f29e9124277995dd6ffb0731fb0ef09f4267c Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 19 Jan 2025 10:52:41 -0500 Subject: [PATCH 04/21] Rename `last_is_used` param for clarity, add documentation on ImplicitReturnVisitor --- src/ameba/ast/visitors/implicit_return_visitor.cr | 13 +++++++++++++ src/ameba/rule/lint/unused_comparison.cr | 4 ++-- src/ameba/rule/lint/unused_literal.cr | 8 ++++---- src/ameba/rule/lint/unused_type_or_constant.cr | 8 ++++---- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/ameba/ast/visitors/implicit_return_visitor.cr b/src/ameba/ast/visitors/implicit_return_visitor.cr index 7db58b1f3..ddc9534d8 100644 --- a/src/ameba/ast/visitors/implicit_return_visitor.cr +++ b/src/ameba/ast/visitors/implicit_return_visitor.cr @@ -1,8 +1,20 @@ module Ameba::AST + # AST visitor that finds nodes that are not used by their surrounding scope. + # + # A stack is used to keep track of when a node is used, incrementing every time + # something will capture its implicit (or explicit) return value, such as the + # path in a class name or the value in an assign. + # + # This also allows for passing through whether a node is captured from a nodes + # parent to its children, such as from an `if` statements parent to it's body, + # as the body is not used by the `if` itself, but by its parent scope. class ImplicitReturnVisitor < BaseVisitor # When greater than zero, indicates the current node's return value is used @stack : Int32 = 0 + # 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 + # captured by their parent node. def visit(node : Crystal::Expressions) : Bool @rule.test(@source, node, @stack > 0) @@ -346,6 +358,7 @@ module Ameba::AST false end + # Indicates that any nodes visited within the block are captured / used. private def incr_stack(&) : Nil @stack += 1 yield diff --git a/src/ameba/rule/lint/unused_comparison.cr b/src/ameba/rule/lint/unused_comparison.cr index f84b716be..152948bbd 100644 --- a/src/ameba/rule/lint/unused_comparison.cr +++ b/src/ameba/rule/lint/unused_comparison.cr @@ -48,8 +48,8 @@ module Ameba::Rule::Lint AST::ImplicitReturnVisitor.new(self, source) end - def test(source, node : Crystal::Call, last_is_used : Bool) - if !last_is_used && node.name.in?(COMPARISON_OPERATORS) && node.args.size == 1 + def test(source, node : Crystal::Call, node_is_used : Bool) + if !node_is_used && node.name.in?(COMPARISON_OPERATORS) && node.args.size == 1 issue_for node, MSG end end diff --git a/src/ameba/rule/lint/unused_literal.cr b/src/ameba/rule/lint/unused_literal.cr index 2a37bc917..33e504293 100644 --- a/src/ameba/rule/lint/unused_literal.cr +++ b/src/ameba/rule/lint/unused_literal.cr @@ -57,10 +57,10 @@ module Ameba::Rule::Lint AST::ImplicitReturnVisitor.new(self, source) end - def test(source, node : Crystal::RegexLiteral, last_is_used : Bool) + def test(source, node : Crystal::RegexLiteral, node_is_used : 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 unless last_is_used + issue_for node, MSG unless node_is_used {% end %} end @@ -71,9 +71,9 @@ module Ameba::Rule::Lint Crystal::TupleLiteral | Crystal::NumberLiteral | Crystal::StringLiteral | Crystal::SymbolLiteral | Crystal::NamedTupleLiteral | Crystal::StringInterpolation, - last_is_used : Bool, + node_is_used : Bool, ) - issue_for node, MSG unless last_is_used + issue_for node, MSG unless node_is_used end end end diff --git a/src/ameba/rule/lint/unused_type_or_constant.cr b/src/ameba/rule/lint/unused_type_or_constant.cr index 48527462e..8ac452191 100644 --- a/src/ameba/rule/lint/unused_type_or_constant.cr +++ b/src/ameba/rule/lint/unused_type_or_constant.cr @@ -48,14 +48,14 @@ module Ameba::Rule::Lint AST::ImplicitReturnVisitor.new(self, source) end - def test(source, node : Crystal::Call, last_is_used : Bool) - return if last_is_used || !path_or_generic_union?(node) + def test(source, node : Crystal::Call, node_is_used : Bool) + return if node_is_used || !path_or_generic_union?(node) issue_for node, MSG end - def test(source, node : Crystal::Path | Crystal::Generic | Crystal::Union, last_is_used : Bool) - issue_for node, MSG unless last_is_used + def test(source, node : Crystal::Path | Crystal::Generic | Crystal::Union, node_is_used : Bool) + issue_for node, MSG unless node_is_used end def path_or_generic_union?(node : Crystal::Call) : Bool From c068d8633c044f130fa68f68ea5491748917f33f Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 20 Jan 2025 13:01:22 -0500 Subject: [PATCH 05/21] Remove unneeded visits from ImplicitReturnVisitor --- .../ast/visitors/implicit_return_visitor.cr | 75 ++++++++----------- 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/src/ameba/ast/visitors/implicit_return_visitor.cr b/src/ameba/ast/visitors/implicit_return_visitor.cr index ddc9534d8..80181e620 100644 --- a/src/ameba/ast/visitors/implicit_return_visitor.cr +++ b/src/ameba/ast/visitors/implicit_return_visitor.cr @@ -140,11 +140,8 @@ module Ameba::AST false end - def visit(node : Crystal::ClassDef) : Bool - incr_stack do - node.name.accept(self) - node.superclass.try &.accept(self) - end + def visit(node : Crystal::ClassDef | Crystal::ModuleDef) : Bool + @rule.test(@source, node, @stack > 0) node.body.accept(self) @@ -152,32 +149,20 @@ module Ameba::AST end def visit(node : Crystal::FunDef) : Bool + @rule.test(@source, node, @stack > 0) + incr_stack do node.args.each &.accept(self) - node.return_type.try &.accept(self) node.body.try &.accept(self) end false end - def visit(node : Crystal::ExternalVar) : Bool - incr_stack { node.type_spec.accept(self) } - - false - end - - def visit(node : Crystal::Include | Crystal::Extend) : Bool - incr_stack do - node.name.accept(self) - end - - false - end - def visit(node : Crystal::Cast | Crystal::NilableCast) : Bool + @rule.test(@source, node, @stack > 0) + node.obj.accept(self) - incr_stack { node.to.accept(self) } false end @@ -201,14 +186,6 @@ module Ameba::AST false end - def visit(node : Crystal::Macro | Crystal::MacroIf | Crystal::MacroFor) : Bool - false - end - - def visit(node : Crystal::UninitializedVar) : Bool - false - end - def visit(node : Crystal::ArrayLiteral | Crystal::TupleLiteral) : Bool @rule.test(@source, node, @stack > 0) @@ -293,11 +270,8 @@ module Ameba::AST def visit(node : Crystal::RangeLiteral) : Bool @rule.test(@source, node, @stack > 0) - unless node.from.is_a?(Crystal::NumberLiteral) + incr_stack do node.from.accept(self) - end - - unless node.to.is_a?(Crystal::NumberLiteral) node.to.accept(self) end @@ -337,27 +311,44 @@ module Ameba::AST false end - def visit(node : Crystal::Alias) + def visit(node : Crystal::Macro | Crystal::MacroIf | Crystal::MacroFor) : Bool @rule.test(@source, node, @stack > 0) - incr_stack do - node.name.accept(self) - node.value.accept(self) - end + false + end + + def visit(node : Crystal::UninitializedVar) : Bool + @rule.test(@source, node, @stack > 0) false end - def visit(node : Crystal::TypeDef) + def visit(node : Crystal::LibDef) : Bool @rule.test(@source, node, @stack > 0) - incr_stack do - node.type_spec.accept(self) - end + false + end + def visit(node : Crystal::Include | Crystal::Extend) : Bool + @rule.test(@source, node, @stack > 0) + + false + end + + def visit(node : Crystal::Alias) + false + end + + def visit(node : Crystal::TypeDef) false end + def visit(node) + @rule.test(@source, node, @stack > 0) + + true + end + # Indicates that any nodes visited within the block are captured / used. private def incr_stack(&) : Nil @stack += 1 From a46492bed52219b345d43be58e9661c296219f2e Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 20 Jan 2025 13:04:07 -0500 Subject: [PATCH 06/21] UnusedTypeOrConstant -> UnusedGenericOrUnion --- ...nt_spec.cr => unused_generic_or_union_spec.cr} | 15 +++++++-------- ..._or_constant.cr => unused_generic_or_union.cr} | 14 ++++++-------- 2 files changed, 13 insertions(+), 16 deletions(-) rename spec/ameba/rule/lint/{unused_type_or_constant_spec.cr => unused_generic_or_union_spec.cr} (90%) rename src/ameba/rule/lint/{unused_type_or_constant.cr => unused_generic_or_union.cr} (80%) diff --git a/spec/ameba/rule/lint/unused_type_or_constant_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr similarity index 90% rename from spec/ameba/rule/lint/unused_type_or_constant_spec.cr rename to spec/ameba/rule/lint/unused_generic_or_union_spec.cr index e3ebc4361..5956d4a26 100644 --- a/spec/ameba/rule/lint/unused_type_or_constant_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -1,26 +1,25 @@ require "../../../spec_helper" module Ameba::Rule::Lint - subject = UnusedTypeOrConstant.new + subject = UnusedGenericOrUnion.new - describe UnusedTypeOrConstant do - it "passes if paths and generic types are used for assign and method calls" do + describe UnusedGenericOrUnion do + it "passes if generics and unions are used for assign and method calls" do expect_no_issues subject, <<-CRYSTAL - MyConst = 1 - my_var : String? = EMPTY_STRING - a : Int32 = 10 + a : Int32? = 10 klass = String? - my_var.as(String) + my_var.as(Array(Char)) puts StaticArray(Int32, 10) + # Not a union Int32 | "Float64" - MyClass.run + MyClass(String).new.run CRYSTAL end diff --git a/src/ameba/rule/lint/unused_type_or_constant.cr b/src/ameba/rule/lint/unused_generic_or_union.cr similarity index 80% rename from src/ameba/rule/lint/unused_type_or_constant.cr rename to src/ameba/rule/lint/unused_generic_or_union.cr index 8ac452191..51d2d1de1 100644 --- a/src/ameba/rule/lint/unused_type_or_constant.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -1,17 +1,15 @@ module Ameba::Rule::Lint - # A rule that disallows unused constants, generics, or unions (`Int32`, `String?`, `StaticArray(Int32, 10)`, etc). + # A rule that disallows unused generics or unions (`String?`, `StaticArray(Int32, 10)`, etc). # # For example, these are considered invalid: # # ``` - # Int32 - # # String? # # Float64 | StaticArray(Float64, 10) # # def size - # Float64 + # Float64 | Int32 # 0.1 # end # ``` @@ -33,16 +31,16 @@ module Ameba::Rule::Lint # YAML configuration example: # # ``` - # Lint/UnusedTypeOrConstant: + # Lint/UnusedGenericOrUnion: # Enabled: true # ``` - class UnusedTypeOrConstant < Base + class UnusedGenericOrUnion < Base properties do since_version "1.7.0" description "Disallows unused literal values" end - MSG = "Type or constant is not used" + MSG = "Generic or union is not used" def test(source : Source) AST::ImplicitReturnVisitor.new(self, source) @@ -54,7 +52,7 @@ module Ameba::Rule::Lint issue_for node, MSG end - def test(source, node : Crystal::Path | Crystal::Generic | Crystal::Union, node_is_used : Bool) + def test(source, node : Crystal::Generic | Crystal::Union, node_is_used : Bool) issue_for node, MSG unless node_is_used end From 66a8cc1a3605a53ee3c65fddb9257eb25f905336 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 20 Jan 2025 13:04:40 -0500 Subject: [PATCH 07/21] Update specs --- .../rule/lint/unused_generic_or_union_spec.cr | 76 +++++-------------- 1 file changed, 19 insertions(+), 57 deletions(-) diff --git a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr index 5956d4a26..cfe93a605 100644 --- a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -23,93 +23,55 @@ module Ameba::Rule::Lint CRYSTAL end - it "passes if paths are used for methods" do + it "passes if generics and unions are used for methods" do expect_no_issues subject, <<-CRYSTAL - def size(a : Float64) : Float64 - 0.1 + a + def size(a : Float64?) : Float64? + 0.1.try(&.+(a)) end - fun fun_name = FunName(a : Int32) : Int32 - 1234 + a + def append(a : Array(String)) : Array(String) + a << "hello" end CRYSTAL end - it "passes if paths are used for classes, modules, aliases, and annotations" do - expect_no_issues subject, <<-CRYSTAL - module MyModule(T) - struct MyStruct < MySuperStruct - end - end - - @[MyAnnotation] - class MyClass < MySuperClass - include MyModule(Int23) - extend MyModule(String) - - alias MyType = Float64 | StaticArray(Float64, 10) - end - - annotation MyAnnotation - end - CRYSTAL - end - - it "passes if paths are used for lib objects" do - expect_no_issues subject, <<-CRYSTAL - lib MyLib - $external_var = ExternalVarName : Int32 - - type MyType = Void* - - struct MyStruct - field1, field2 : Float64 - end - - fun fun_name = FunName(Void*) : Void - end - CRYSTAL - end - - it "fails if paths and generic types are unused top-level" do + it "fails if generics or unions are unused top-level" do expect_issue subject, <<-CRYSTAL - Int32 - # ^^^ error: Type or constant is not used String? - # ^^^^^ error: Type or constant is not used + # ^^^^^ error: Generic or union is not used Int32 | Float64 | Nil - # ^^^^^^^^^^^^^^^^^^^ error: Type or constant is not used + # ^^^^^^^^^^^^^^^^^^^ error: Generic or union is not used StaticArray(Int32, 10) - # ^^^^^^^^^^^^^^^^^^^^ error: Type or constant is not used + # ^^^^^^^^^^^^^^^^^^^^ error: Generic or union is not used CRYSTAL end - it "fails if types and constants are unused inside methods" do + it "fails if generics or unions are unused inside methods" do expect_issue subject, <<-CRYSTAL def hello - Float64 - # ^^^^^^^ error: Type or constant is not used + Float64? + # ^^^^^^^^ error: Generic or union is not used 0.1 end fun fun_name : Int32 - Int32 - # ^^^^^ error: Type or constant is not used + Array(String) + # ^^^^^^^^^^^^^ error: Generic or union is not used 1234 end CRYSTAL end - it "fails if types and constants are unused inside classes and modules" do + it "fails if generics or unions are unused inside classes and modules" do expect_issue subject, <<-CRYSTAL class MyClass - MyModule - # ^^^^^^^^ error: Type or constant is not used + String? + # ^^^^^^^ error: Generic or union is not used end module MyModule - Int32 - # ^^^^^ error: Type or constant is not used + Array(Int32) + # ^^^^^^^^^^^^ error: Generic or union is not used end CRYSTAL end From 941153401d746212b486224a113f949a7493a3df Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 20 Jan 2025 13:19:45 -0500 Subject: [PATCH 08/21] Expand union check to include `self` --- spec/ameba/rule/lint/unused_generic_or_union_spec.cr | 10 ++++++++++ src/ameba/rule/lint/unused_generic_or_union.cr | 12 ++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr index cfe93a605..6c85703ad 100644 --- a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -67,6 +67,16 @@ module Ameba::Rule::Lint class MyClass String? # ^^^^^^^ error: Generic or union is not used + Array(self) + # ^^^^^^^^^^^ error: Generic or union is not used + Array(typeof(1)) + # ^^^^^^^^^^^^^^^^ error: Generic or union is not used + + def hello + self | Nil + # ^^^^^^^^^^ error: Generic or union is not used + "Hello, Gordon!" + end end module MyModule diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 51d2d1de1..3a7926934 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -60,19 +60,23 @@ module Ameba::Rule::Lint return false unless node.name == "|" && node.args.size == 1 case lhs = node.obj - when Crystal::Path, Crystal::Generic + when Crystal::Path, Crystal::Generic, Crystal::Self # Okay + when Crystal::Var + return false unless lhs.name == "self" when Crystal::Call - return false unless path_or_generic_union?(lhs) + return false unless (lhs.name == "self") || path_or_generic_union?(lhs) else return false end case rhs = node.args.first? - when Crystal::Path, Crystal::Generic + when Crystal::Path, Crystal::Generic, Crystal::Self # Okay + when Crystal::Var + return false unless rhs.name == "self" when Crystal::Call - return false unless path_or_generic_union?(rhs) + return false unless (rhs.name == "self") || path_or_generic_union?(rhs) else return false end From 0b4403a25f908c4cdcf16f773b8d6a1fe68be22d Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 20 Jan 2025 13:22:39 -0500 Subject: [PATCH 09/21] Update description --- src/ameba/rule/lint/unused_generic_or_union.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 3a7926934..56a7d4cd9 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -37,7 +37,7 @@ module Ameba::Rule::Lint class UnusedGenericOrUnion < Base properties do since_version "1.7.0" - description "Disallows unused literal values" + description "Disallows unused generics or unions" end MSG = "Generic or union is not used" From f32b3c2f531f06ffb9fcb671b5e385a797037f1a Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 20 Jan 2025 13:52:08 -0500 Subject: [PATCH 10/21] Check unions including typeof and underscores These are the only other nodes allowed in type unions (excluding "self?", as it can point to a method) --- spec/ameba/rule/lint/unused_generic_or_union_spec.cr | 12 ++++++++++++ src/ameba/rule/lint/unused_generic_or_union.cr | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr index 6c85703ad..2f3c16201 100644 --- a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -23,6 +23,16 @@ module Ameba::Rule::Lint CRYSTAL end + it "passes plain pseudo methods, self, and paths" do + expect_no_issues subject, <<-CRYSTAL + _ + self + self? + typeof(1) + Int32 + CRYSTAL + end + it "passes if generics and unions are used for methods" do expect_no_issues subject, <<-CRYSTAL def size(a : Float64?) : Float64? @@ -75,6 +85,8 @@ module Ameba::Rule::Lint def hello self | Nil # ^^^^^^^^^^ error: Generic or union is not used + typeof(1) | Nil | _ + # ^^^^^^^^^^^^^^^^^^^ error: Generic or union is not used "Hello, Gordon!" end end diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 56a7d4cd9..6482c1e11 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -60,7 +60,7 @@ module Ameba::Rule::Lint return false unless node.name == "|" && node.args.size == 1 case lhs = node.obj - when Crystal::Path, Crystal::Generic, Crystal::Self + when Crystal::Path, Crystal::Generic, Crystal::Self, Crystal::TypeOf, Crystal::Underscore # Okay when Crystal::Var return false unless lhs.name == "self" @@ -71,7 +71,7 @@ module Ameba::Rule::Lint end case rhs = node.args.first? - when Crystal::Path, Crystal::Generic, Crystal::Self + when Crystal::Path, Crystal::Generic, Crystal::Self, Crystal::TypeOf, Crystal::Underscore # Okay when Crystal::Var return false unless rhs.name == "self" From 01e367fcee247e65fd73cb55aa916acb3ef46393 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sat, 25 Jan 2025 06:30:10 -0500 Subject: [PATCH 11/21] De-duplicate method in UnusedGenericOrUnion --- .../rule/lint/unused_generic_or_union.cr | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 6482c1e11..c501e625e 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -56,32 +56,24 @@ module Ameba::Rule::Lint issue_for node, MSG unless node_is_used end - def path_or_generic_union?(node : Crystal::Call) : Bool - return false unless node.name == "|" && node.args.size == 1 + private def path_or_generic_union?(node : Crystal::Call) : Bool + return false unless node.name == "|" && node.args.size == 1 && + valid_type_node?(node.obj) && valid_type_node?(node.args.first?) - case lhs = node.obj - when Crystal::Path, Crystal::Generic, Crystal::Self, Crystal::TypeOf, Crystal::Underscore - # Okay - when Crystal::Var - return false unless lhs.name == "self" - when Crystal::Call - return false unless (lhs.name == "self") || path_or_generic_union?(lhs) - else - return false - end + true + end - case rhs = node.args.first? + private def valid_type_node?(node : Crystal::ASTNode?) : Bool + case node when Crystal::Path, Crystal::Generic, Crystal::Self, Crystal::TypeOf, Crystal::Underscore - # Okay + true when Crystal::Var - return false unless rhs.name == "self" + node.name == "self" when Crystal::Call - return false unless (rhs.name == "self") || path_or_generic_union?(rhs) + (node.name == "self") || path_or_generic_union?(node) else - return false + false end - - true end end end From a5a8b8a8e3b5691969f1be585e0d1534ec1d0de1 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sat, 25 Jan 2025 08:05:20 -0500 Subject: [PATCH 12/21] Apply suggestions from code review Co-authored-by: Sijawusz Pur Rahnama --- spec/ameba/rule/lint/unused_generic_or_union_spec.cr | 5 ++--- src/ameba/ast/visitors/implicit_return_visitor.cr | 2 +- src/ameba/rule/lint/unused_generic_or_union.cr | 6 ++---- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr index 2f3c16201..c74ce738d 100644 --- a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -1,9 +1,8 @@ require "../../../spec_helper" module Ameba::Rule::Lint - subject = UnusedGenericOrUnion.new - describe UnusedGenericOrUnion do + subject = UnusedGenericOrUnion.new it "passes if generics and unions are used for assign and method calls" do expect_no_issues subject, <<-CRYSTAL my_var : String? = EMPTY_STRING @@ -45,7 +44,7 @@ module Ameba::Rule::Lint CRYSTAL end - it "fails if generics or unions are unused top-level" do + it "fails if generics or unions are unused at top-level" do expect_issue subject, <<-CRYSTAL String? # ^^^^^ error: Generic or union is not used diff --git a/src/ameba/ast/visitors/implicit_return_visitor.cr b/src/ameba/ast/visitors/implicit_return_visitor.cr index 80181e620..2d660ce85 100644 --- a/src/ameba/ast/visitors/implicit_return_visitor.cr +++ b/src/ameba/ast/visitors/implicit_return_visitor.cr @@ -141,7 +141,7 @@ module Ameba::AST end def visit(node : Crystal::ClassDef | Crystal::ModuleDef) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.body.accept(self) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index c501e625e..eca794a32 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -57,10 +57,8 @@ module Ameba::Rule::Lint end private def path_or_generic_union?(node : Crystal::Call) : Bool - return false unless node.name == "|" && node.args.size == 1 && + node.name == "|" && node.args.size == 1 && valid_type_node?(node.obj) && valid_type_node?(node.args.first?) - - true end private def valid_type_node?(node : Crystal::ASTNode?) : Bool @@ -70,7 +68,7 @@ module Ameba::Rule::Lint when Crystal::Var node.name == "self" when Crystal::Call - (node.name == "self") || path_or_generic_union?(node) + path_or_generic_union?(node) else false end From 475c3b52fbf833ed586c990807fa1207517a3082 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sat, 25 Jan 2025 08:09:46 -0500 Subject: [PATCH 13/21] @stack.positive? and formatting --- .../ast/visitors/implicit_return_visitor.cr | 66 +++++++++---------- .../rule/lint/unused_generic_or_union.cr | 2 +- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/ameba/ast/visitors/implicit_return_visitor.cr b/src/ameba/ast/visitors/implicit_return_visitor.cr index 2d660ce85..c5a7bf091 100644 --- a/src/ameba/ast/visitors/implicit_return_visitor.cr +++ b/src/ameba/ast/visitors/implicit_return_visitor.cr @@ -16,7 +16,7 @@ module Ameba::AST # their parents scope. Only the last line in an expressions node can be # captured by their parent node. def visit(node : Crystal::Expressions) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) last_idx = node.expressions.size - 1 @@ -37,7 +37,7 @@ module Ameba::AST end def visit(node : Crystal::BinaryOp) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) if node.right.is_a?(Crystal::Call) incr_stack { node.left.accept(self) } @@ -51,7 +51,7 @@ module Ameba::AST end def visit(node : Crystal::Call) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack do node.obj.try &.accept(self) @@ -65,7 +65,7 @@ module Ameba::AST end def visit(node : Crystal::Arg) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.default_value.try &.accept(self) } @@ -73,7 +73,7 @@ module Ameba::AST end def visit(node : Crystal::EnumDef) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.members.each &.accept(self) @@ -81,7 +81,7 @@ module Ameba::AST end def visit(node : Crystal::Assign | Crystal::OpAssign) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.value.accept(self) } @@ -89,7 +89,7 @@ module Ameba::AST end def visit(node : Crystal::MultiAssign) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.targets.each &.accept(self) incr_stack { node.values.each &.accept(self) } @@ -98,7 +98,7 @@ module Ameba::AST end def visit(node : Crystal::If | Crystal::Unless) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.cond.accept(self) } node.then.accept(self) @@ -108,7 +108,7 @@ module Ameba::AST end def visit(node : Crystal::While | Crystal::Until) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.cond.accept(self) } node.body.accept(self) @@ -117,7 +117,7 @@ module Ameba::AST end def visit(node : Crystal::Def) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack do node.args.each &.accept(self) @@ -149,7 +149,7 @@ module Ameba::AST end def visit(node : Crystal::FunDef) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack do node.args.each &.accept(self) @@ -160,7 +160,7 @@ module Ameba::AST end def visit(node : Crystal::Cast | Crystal::NilableCast) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.obj.accept(self) @@ -168,7 +168,7 @@ module Ameba::AST end def visit(node : Crystal::Annotation) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack do node.args.each &.accept(self) @@ -179,7 +179,7 @@ module Ameba::AST end def visit(node : Crystal::TypeDeclaration) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.value.try &.accept(self) } @@ -187,7 +187,7 @@ module Ameba::AST end def visit(node : Crystal::ArrayLiteral | Crystal::TupleLiteral) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.elements.each &.accept(self) } @@ -195,7 +195,7 @@ module Ameba::AST end def visit(node : Crystal::StringInterpolation) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.expressions.each do |exp| incr_stack { exp.accept(self) } @@ -205,7 +205,7 @@ module Ameba::AST end def visit(node : Crystal::HashLiteral | Crystal::NamedTupleLiteral) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.entries.each &.value.accept(self) } @@ -213,7 +213,7 @@ module Ameba::AST end def visit(node : Crystal::Case) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.cond.try &.accept(self) } node.whens.each &.accept(self) @@ -223,7 +223,7 @@ module Ameba::AST end def visit(node : Crystal::Select) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.whens.each &.accept(self) node.else.try &.accept(self) @@ -232,7 +232,7 @@ module Ameba::AST end def visit(node : Crystal::When) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.conds.each &.accept(self) } node.body.accept(self) @@ -241,7 +241,7 @@ module Ameba::AST end def visit(node : Crystal::Rescue) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.body.accept(self) @@ -249,7 +249,7 @@ module Ameba::AST end def visit(node : Crystal::ExceptionHandler) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) node.body.accept(self) node.rescues.try &.each &.accept(self) @@ -260,7 +260,7 @@ module Ameba::AST end def visit(node : Crystal::ControlExpression) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.exp.try &.accept(self) } @@ -268,7 +268,7 @@ module Ameba::AST end def visit(node : Crystal::RangeLiteral) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack do node.from.accept(self) @@ -279,7 +279,7 @@ module Ameba::AST end def visit(node : Crystal::RegexLiteral) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) # Regex literals either contain string literals or string interpolations, # both of which are "captured" by the parent regex literal @@ -292,13 +292,13 @@ module Ameba::AST node : Crystal::BoolLiteral | Crystal::CharLiteral | Crystal::NumberLiteral | Crystal::StringLiteral | Crystal::SymbolLiteral | Crystal::ProcLiteral, ) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) true end def visit(node : Crystal::Yield) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) incr_stack { node.exps.each &.accept(self) } @@ -306,31 +306,31 @@ module Ameba::AST end def visit(node : Crystal::Generic | Crystal::Path | Crystal::Union) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) false end def visit(node : Crystal::Macro | Crystal::MacroIf | Crystal::MacroFor) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) false end def visit(node : Crystal::UninitializedVar) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) false end def visit(node : Crystal::LibDef) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) false end def visit(node : Crystal::Include | Crystal::Extend) : Bool - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) false end @@ -344,7 +344,7 @@ module Ameba::AST end def visit(node) - @rule.test(@source, node, @stack > 0) + @rule.test(@source, node, @stack.positive?) true end diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index eca794a32..fe4155f4d 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -58,7 +58,7 @@ module Ameba::Rule::Lint private def path_or_generic_union?(node : Crystal::Call) : Bool node.name == "|" && node.args.size == 1 && - valid_type_node?(node.obj) && valid_type_node?(node.args.first?) + valid_type_node?(node.obj) && valid_type_node?(node.args.first?) end private def valid_type_node?(node : Crystal::ASTNode?) : Bool From 7841e1b25d35edec9f91882aad2ed52a94bf2522 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 26 Jan 2025 16:19:50 -0500 Subject: [PATCH 14/21] Update unused_generic_or_union_spec.cr Co-authored-by: Sijawusz Pur Rahnama --- spec/ameba/rule/lint/unused_generic_or_union_spec.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr index c74ce738d..e1b0eb208 100644 --- a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -32,7 +32,7 @@ module Ameba::Rule::Lint CRYSTAL end - it "passes if generics and unions are used for methods" do + it "passes if generics and unions are used for method arguments and return type" do expect_no_issues subject, <<-CRYSTAL def size(a : Float64?) : Float64? 0.1.try(&.+(a)) From 94a05347e3951579566f3abbdbf847cb99bab838 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 26 Jan 2025 17:21:07 -0500 Subject: [PATCH 15/21] Update unused_generic_or_union_spec.cr Co-authored-by: Sijawusz Pur Rahnama --- spec/ameba/rule/lint/unused_generic_or_union_spec.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr index e1b0eb208..a01eabb12 100644 --- a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -22,7 +22,7 @@ module Ameba::Rule::Lint CRYSTAL end - it "passes plain pseudo methods, self, and paths" do + it "passes for plain pseudo methods, self, and paths" do expect_no_issues subject, <<-CRYSTAL _ self From 33aacc08de6b86af5d52b1ad9a317018961b8c8a Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Sun, 26 Jan 2025 18:33:28 -0500 Subject: [PATCH 16/21] Split up generic and union warning messages --- .../rule/lint/unused_generic_or_union_spec.cr | 23 ++++++++++--------- .../rule/lint/unused_generic_or_union.cr | 9 ++++---- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr index a01eabb12..41e076010 100644 --- a/spec/ameba/rule/lint/unused_generic_or_union_spec.cr +++ b/spec/ameba/rule/lint/unused_generic_or_union_spec.cr @@ -3,6 +3,7 @@ require "../../../spec_helper" module Ameba::Rule::Lint describe UnusedGenericOrUnion do subject = UnusedGenericOrUnion.new + it "passes if generics and unions are used for assign and method calls" do expect_no_issues subject, <<-CRYSTAL my_var : String? = EMPTY_STRING @@ -47,11 +48,11 @@ module Ameba::Rule::Lint it "fails if generics or unions are unused at top-level" do expect_issue subject, <<-CRYSTAL String? - # ^^^^^ error: Generic or union is not used + # ^^^^^ error: Generic is not used Int32 | Float64 | Nil - # ^^^^^^^^^^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^^^^^^^^^^ error: Union is not used StaticArray(Int32, 10) - # ^^^^^^^^^^^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^^^^^^^^^^^ error: Generic is not used CRYSTAL end @@ -59,13 +60,13 @@ module Ameba::Rule::Lint expect_issue subject, <<-CRYSTAL def hello Float64? - # ^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^ error: Generic is not used 0.1 end fun fun_name : Int32 Array(String) - # ^^^^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^^^^ error: Generic is not used 1234 end CRYSTAL @@ -75,24 +76,24 @@ module Ameba::Rule::Lint expect_issue subject, <<-CRYSTAL class MyClass String? - # ^^^^^^^ error: Generic or union is not used + # ^^^^^^^ error: Generic is not used Array(self) - # ^^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^^ error: Generic is not used Array(typeof(1)) - # ^^^^^^^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^^^^^^^ error: Generic is not used def hello self | Nil - # ^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^ error: Union is not used typeof(1) | Nil | _ - # ^^^^^^^^^^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^^^^^^^^^^ error: Union is not used "Hello, Gordon!" end end module MyModule Array(Int32) - # ^^^^^^^^^^^^ error: Generic or union is not used + # ^^^^^^^^^^^^ error: Generic is not used end CRYSTAL end diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index fe4155f4d..4597f618e 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -40,7 +40,8 @@ module Ameba::Rule::Lint description "Disallows unused generics or unions" end - MSG = "Generic or union is not used" + GENERIC_MSG = "Generic is not used" + UNION_MSG = "Union is not used" def test(source : Source) AST::ImplicitReturnVisitor.new(self, source) @@ -49,11 +50,11 @@ module Ameba::Rule::Lint def test(source, node : Crystal::Call, node_is_used : Bool) return if node_is_used || !path_or_generic_union?(node) - issue_for node, MSG + issue_for node, UNION_MSG end - def test(source, node : Crystal::Generic | Crystal::Union, node_is_used : Bool) - issue_for node, MSG unless node_is_used + def test(source, node : Crystal::Generic, node_is_used : Bool) + issue_for node, GENERIC_MSG unless node_is_used end private def path_or_generic_union?(node : Crystal::Call) : Bool From f584ce2ec43a84dc37e238fb9f6723a080b3cdd8 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 27 Jan 2025 16:46:09 -0500 Subject: [PATCH 17/21] Update src/ameba/rule/lint/unused_generic_or_union.cr Co-authored-by: Sijawusz Pur Rahnama --- src/ameba/rule/lint/unused_generic_or_union.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 4597f618e..f550d63e4 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -59,7 +59,7 @@ module Ameba::Rule::Lint private def path_or_generic_union?(node : Crystal::Call) : Bool node.name == "|" && node.args.size == 1 && - valid_type_node?(node.obj) && valid_type_node?(node.args.first?) + valid_type_node?(node.obj) && valid_type_node?(node.args.first) end private def valid_type_node?(node : Crystal::ASTNode?) : Bool From 8d0159e7af555ed58d484e64d388e9c203f58173 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 27 Jan 2025 17:32:43 -0500 Subject: [PATCH 18/21] Apply suggestions from code review Co-authored-by: Sijawusz Pur Rahnama --- src/ameba/rule/lint/unused_generic_or_union.cr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index f550d63e4..17bd53d78 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -58,11 +58,11 @@ module Ameba::Rule::Lint end private def path_or_generic_union?(node : Crystal::Call) : Bool - node.name == "|" && node.args.size == 1 && - valid_type_node?(node.obj) && valid_type_node?(node.args.first) + node.name == "|" && node.args.size == 1 && (obj = node.obj) + valid_type_node?(obj) && valid_type_node?(node.args.first) end - private def valid_type_node?(node : Crystal::ASTNode?) : Bool + private def valid_type_node?(node : Crystal::ASTNode) : Bool case node when Crystal::Path, Crystal::Generic, Crystal::Self, Crystal::TypeOf, Crystal::Underscore true From 29bbdebd805f808f659872632ec248a9dfbb7be4 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 27 Jan 2025 17:36:34 -0500 Subject: [PATCH 19/21] Update src/ameba/rule/lint/unused_generic_or_union.cr Co-authored-by: Sijawusz Pur Rahnama --- src/ameba/rule/lint/unused_generic_or_union.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 17bd53d78..336702fee 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -58,7 +58,7 @@ module Ameba::Rule::Lint end private def path_or_generic_union?(node : Crystal::Call) : Bool - node.name == "|" && node.args.size == 1 && (obj = node.obj) + node.name == "|" && node.args.size == 1 && (obj = node.obj) && valid_type_node?(obj) && valid_type_node?(node.args.first) end From 2e7b83f0c28815f7dba21c20effe0d66670e8d57 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 27 Jan 2025 17:39:41 -0500 Subject: [PATCH 20/21] Update src/ameba/rule/lint/unused_generic_or_union.cr Co-authored-by: Sijawusz Pur Rahnama --- src/ameba/rule/lint/unused_generic_or_union.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 336702fee..07761d3b7 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -57,7 +57,7 @@ module Ameba::Rule::Lint issue_for node, GENERIC_MSG unless node_is_used end - private def path_or_generic_union?(node : Crystal::Call) : Bool + private def path_or_generic_union?(node : Crystal::Call) node.name == "|" && node.args.size == 1 && (obj = node.obj) && valid_type_node?(obj) && valid_type_node?(node.args.first) end From 3c28b2b657fb6c5104554c4f71b434d6cf3b1594 Mon Sep 17 00:00:00 2001 From: Margret Riegert Date: Mon, 27 Jan 2025 17:44:35 -0500 Subject: [PATCH 21/21] Update unused_generic_or_union.cr Co-authored-by: Sijawusz Pur Rahnama --- src/ameba/rule/lint/unused_generic_or_union.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ameba/rule/lint/unused_generic_or_union.cr b/src/ameba/rule/lint/unused_generic_or_union.cr index 07761d3b7..0651034a2 100644 --- a/src/ameba/rule/lint/unused_generic_or_union.cr +++ b/src/ameba/rule/lint/unused_generic_or_union.cr @@ -57,8 +57,8 @@ module Ameba::Rule::Lint issue_for node, GENERIC_MSG unless node_is_used end - private def path_or_generic_union?(node : Crystal::Call) - node.name == "|" && node.args.size == 1 && (obj = node.obj) && + private def path_or_generic_union?(node : Crystal::Call) : Bool + node.name == "|" && node.args.size == 1 && !!(obj = node.obj) && valid_type_node?(obj) && valid_type_node?(node.args.first) end