diff --git a/spec/ameba/ast/variabling/type_def_variable_spec.cr b/spec/ameba/ast/variabling/type_dec_variable_spec.cr similarity index 100% rename from spec/ameba/ast/variabling/type_def_variable_spec.cr rename to spec/ameba/ast/variabling/type_dec_variable_spec.cr diff --git a/src/ameba/ast/branch.cr b/src/ameba/ast/branch.cr index 9008c4a2..73ee8dab 100644 --- a/src/ameba/ast/branch.cr +++ b/src/ameba/ast/branch.cr @@ -62,7 +62,7 @@ module Ameba::AST # Branch.of(assign_node, def_node) # ``` def self.of(node : Crystal::ASTNode, parent_node : Crystal::ASTNode) - BranchVisitor.new(node).tap(&.accept parent_node).branch + BranchVisitor.new(node).tap(&.accept(parent_node)).branch end # :nodoc: @@ -84,6 +84,7 @@ module Ameba::AST branches.each do |branch_node| break if branch # branch found + @current_branch = branch_node if branch_node && !branch_node.nop? branch_node.try &.accept(self) end @@ -108,19 +109,11 @@ module Ameba::AST true end - def visit(node : Crystal::If) + def visit(node : Crystal::If | Crystal::Unless) on_branchable_start node, node.cond, node.then, node.else end - def end_visit(node : Crystal::If) - on_branchable_end node - end - - def visit(node : Crystal::Unless) - on_branchable_start node, node.cond, node.then, node.else - end - - def end_visit(node : Crystal::Unless) + def end_visit(node : Crystal::If | Crystal::Unless) on_branchable_end node end @@ -140,19 +133,11 @@ module Ameba::AST on_branchable_end node end - def visit(node : Crystal::While) + def visit(node : Crystal::While | Crystal::Until) on_branchable_start node, node.cond, node.body end - def end_visit(node : Crystal::While) - on_branchable_end node - end - - def visit(node : Crystal::Until) - on_branchable_start node, node.cond, node.body - end - - def end_visit(node : Crystal::Until) + def end_visit(node : Crystal::While | Crystal::Until) on_branchable_end node end diff --git a/src/ameba/ast/branchable.cr b/src/ameba/ast/branchable.cr index 6b4b576a..faeea67c 100644 --- a/src/ameba/ast/branchable.cr +++ b/src/ameba/ast/branchable.cr @@ -15,14 +15,15 @@ module Ameba::AST class Branchable include Util - getter branches = [] of Crystal::ASTNode - - # The actual Crystal node. - getter node : Crystal::ASTNode - # Parent branchable (if any) getter parent : Branchable? + # Array of branches + getter branches = [] of Crystal::ASTNode + + # The actual Crystal node + getter node : Crystal::ASTNode + delegate to_s, to: @node delegate location, to: @node delegate end_location, to: @node diff --git a/src/ameba/ast/flow_expression.cr b/src/ameba/ast/flow_expression.cr index 174111f2..fe71c699 100644 --- a/src/ameba/ast/flow_expression.cr +++ b/src/ameba/ast/flow_expression.cr @@ -53,8 +53,11 @@ module Ameba::AST case current_node = node when Crystal::Expressions control_flow_found = false + current_node.expressions.each do |exp| - unreachable_nodes << exp if control_flow_found + if control_flow_found + unreachable_nodes << exp + end control_flow_found ||= !loop?(exp) && flow_expression?(exp, in_loop?) end when Crystal::BinaryOp diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 9d727409..08b42931 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -46,7 +46,7 @@ module Ameba::AST # scope = Scope.new(class_node, nil) # ``` def initialize(@node, @outer_scope = nil) - @outer_scope.try &.inner_scopes.<<(self) + @outer_scope.try &.inner_scopes.<< self end # Creates a new variable in the current scope. @@ -97,7 +97,8 @@ module Ameba::AST # scope.find_variable("foo") # ``` def find_variable(name : String) - variables.find(&.name.==(name)) || outer_scope.try &.find_variable(name) + variables.find(&.name.==(name)) || + outer_scope.try &.find_variable(name) end # Creates a new assignment for the variable. @@ -113,7 +114,8 @@ module Ameba::AST # Returns `true` if current scope represents a block (or proc), # `false` otherwise. def block? - node.is_a?(Crystal::Block) || node.is_a?(Crystal::ProcLiteral) + node.is_a?(Crystal::Block) || + node.is_a?(Crystal::ProcLiteral) end # Returns `true` if current scope represents a spawn block, e. g. @@ -129,7 +131,9 @@ module Ameba::AST # Returns `true` if current scope sits inside a macro. def in_macro? - (node.is_a?(Crystal::Macro) || node.is_a?(Crystal::MacroFor)) || + (node.is_a?(Crystal::Macro) || + node.is_a?(Crystal::MacroIf) || + node.is_a?(Crystal::MacroFor)) || !!outer_scope.try(&.in_macro?) end @@ -141,7 +145,8 @@ module Ameba::AST # Returns `true` if type declaration variable is assigned in this scope. def assigns_type_dec?(name) - type_dec_variables.any?(&.name.== name) || !!outer_scope.try(&.assigns_type_dec?(name)) + type_dec_variables.any?(&.name.== name) || + !!outer_scope.try(&.assigns_type_dec?(name)) end # Returns `true` if and only if current scope represents some @@ -149,6 +154,7 @@ module Ameba::AST def type_definition? node.is_a?(Crystal::ClassDef) || node.is_a?(Crystal::ModuleDef) || + node.is_a?(Crystal::EnumDef) || node.is_a?(Crystal::LibDef) || node.is_a?(Crystal::FunDef) || node.is_a?(Crystal::TypeDef) || @@ -159,8 +165,8 @@ module Ameba::AST # `false` otherwise. def references?(variable : Variable, check_inner_scopes = true) variable.references.any? do |reference| - return true if reference.scope == self - check_inner_scopes && inner_scopes.any?(&.references?(variable)) + (reference.scope == self) || + (check_inner_scopes && inner_scopes.any?(&.references?(variable))) end || variable.used_in_macro? end diff --git a/src/ameba/ast/variabling/type_def_variable.cr b/src/ameba/ast/variabling/type_dec_variable.cr similarity index 100% rename from src/ameba/ast/variabling/type_def_variable.cr rename to src/ameba/ast/variabling/type_dec_variable.cr diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 1a9d2943..a0b66e04 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -170,7 +170,7 @@ module Ameba::AST private class MacroReferenceFinder < Crystal::Visitor property? references = false - def initialize(node, @reference : String = reference) + def initialize(node, @reference : String) node.accept self end @@ -179,10 +179,6 @@ module Ameba::AST val.to_s.includes?(@reference) end - def visit(node : Crystal::ASTNode) - true - end - def visit(node : Crystal::MacroLiteral) !(@references ||= includes_reference?(node.value)) end @@ -201,14 +197,20 @@ module Ameba::AST includes_reference?(node.then) || includes_reference?(node.else)) end + + def visit(node : Crystal::ASTNode) + true + end end private def update_assign_reference! - if @assign_before_reference.nil? && - references.size <= assignments.size && - assignments.none?(&.op_assign?) - @assign_before_reference = assignments.find { |ass| !ass.in_branch? }.try &.node - end + return if @assign_before_reference + return if references.size > assignments.size + return if assignments.any?(&.op_assign?) + + @assign_before_reference = assignments.find { |ass| + !ass.in_branch? + }.try &.node end end end diff --git a/src/ameba/ast/visitors/flow_expression_visitor.cr b/src/ameba/ast/visitors/flow_expression_visitor.cr index 69ec1e25..f23d8de7 100644 --- a/src/ameba/ast/visitors/flow_expression_visitor.cr +++ b/src/ameba/ast/visitors/flow_expression_visitor.cr @@ -8,11 +8,6 @@ module Ameba::AST @loop_stack = [] of Crystal::ASTNode - # Creates a new flow expression visitor. - def initialize(@rule, @source) - @source.ast.accept self - end - # :nodoc: def visit(node) if flow_expression?(node, in_loop?) @@ -22,12 +17,7 @@ module Ameba::AST end # :nodoc: - def visit(node : Crystal::While) - on_loop_started(node) - end - - # :nodoc: - def visit(node : Crystal::Until) + def visit(node : Crystal::While | Crystal::Until) on_loop_started(node) end @@ -37,12 +27,7 @@ module Ameba::AST end # :nodoc: - def end_visit(node : Crystal::While) - on_loop_ended(node) - end - - # :nodoc: - def end_visit(node : Crystal::Until) + def end_visit(node : Crystal::While | Crystal::Until) on_loop_ended(node) end diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 28a563ba..95b550cf 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -16,6 +16,7 @@ module Ameba::AST ProcLiteral, Block, Macro, + MacroIf, MacroFor, } @@ -25,21 +26,25 @@ module Ameba::AST @scope_queue = [] of Scope @current_scope : Scope @current_assign : Crystal::ASTNode? - @visibility_modifier : Crystal::Visibility? + @current_visibility : Crystal::Visibility? @skip : Array(Crystal::ASTNode.class)? def initialize(@rule, @source, skip = nil) - @skip = skip.try &.map(&.as(Crystal::ASTNode.class)) @current_scope = Scope.new(@source.ast) # top level scope - @source.ast.accept self - @scope_queue.each { |scope| @rule.test @source, scope.node, scope } + @skip = skip.try &.map(&.as(Crystal::ASTNode.class)) + + super @rule, @source + + @scope_queue.each do |scope| + @rule.test @source, scope.node, scope + end end private def on_scope_enter(node) return if skip?(node) scope = Scope.new(node, @current_scope) - scope.visibility = @visibility_modifier + scope.visibility = @current_visibility @current_scope = scope end @@ -47,11 +52,12 @@ module Ameba::AST private def on_scope_end(node) @scope_queue << @current_scope - @visibility_modifier = nil + @current_visibility = nil # go up if this is not a top level scope - return unless outer_scope = @current_scope.outer_scope - @current_scope = outer_scope + if outer_scope = @current_scope.outer_scope + @current_scope = outer_scope + end end private def on_assign_end(target, node) @@ -73,7 +79,7 @@ module Ameba::AST # :nodoc: def visit(node : Crystal::VisibilityModifier) - @visibility_modifier = node.exp.visibility = node.modifier + @current_visibility = node.exp.visibility = node.modifier true end @@ -96,6 +102,7 @@ module Ameba::AST def end_visit(node : Crystal::Assign | Crystal::OpAssign) on_assign_end(node.target, node) @current_assign = nil + on_scope_end(node) if @current_scope.eql?(node) end @@ -103,6 +110,7 @@ module Ameba::AST def end_visit(node : Crystal::MultiAssign) node.targets.each { |target| on_assign_end(target, node) } @current_assign = nil + on_scope_end(node) if @current_scope.eql?(node) end @@ -110,6 +118,7 @@ module Ameba::AST def end_visit(node : Crystal::UninitializedVar) on_assign_end(node.var, node) @current_assign = nil + on_scope_end(node) if @current_scope.eql?(node) end @@ -119,7 +128,8 @@ module Ameba::AST @current_scope.add_variable(var) @current_scope.add_type_dec_variable(node) - @current_assign = node.value unless node.value.nil? + + @current_assign = node.value if node.value end # :nodoc: @@ -128,6 +138,7 @@ module Ameba::AST on_assign_end(var, node) @current_assign = nil + on_scope_end(node) if @current_scope.eql?(node) end @@ -165,7 +176,9 @@ module Ameba::AST if node.name.in?(SPECIAL_NODE_NAMES) && node.args.empty? @current_scope.arguments.each do |arg| variable = arg.variable - variable.reference(variable.node, @current_scope).explicit = false + + ref = variable.reference(variable.node, @current_scope) + ref.explicit = false end end true diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 5b426195..3d3a3c71 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -246,7 +246,7 @@ class Ameba::Config {% if block.body.is_a? Assign %} {% definitions << {var: block.body.target, value: block.body.value} %} {% elsif block.body.is_a? Call %} - {% definitions << {var: block.body.name, value: block.body.args.first} %} + {% definitions << {var: block.body.name, value: block.body.args.first} %} {% elsif block.body.is_a? TypeDeclaration %} {% definitions << {var: block.body.var, value: block.body.value, type: block.body.type} %} {% elsif block.body.is_a? Expressions %} @@ -274,7 +274,7 @@ class Ameba::Config {% converter = SeverityYamlConverter %} {% end %} - {% if type == nil %} + {% unless type %} {% if value.is_a? BoolLiteral %} {% type = Bool %} {% elsif value.is_a? StringLiteral %} @@ -284,23 +284,23 @@ class Ameba::Config {% type = Int32 %} {% elsif value.kind == :i64 %} {% type = Int64 %} + {% elsif value.kind == :i128 %} + {% type = Int128 %} {% elsif value.kind == :f32 %} {% type = Float32 %} {% elsif value.kind == :f64 %} {% type = Float64 %} {% end %} {% end %} - - {% type = Nil if type == nil %} {% end %} {% properties[name] = {key: key, default: value, type: type, converter: converter} %} - @[YAML::Field(key: {{ key }}, converter: {{ converter }}, type: {{ type }})] + @[YAML::Field(key: {{ key }}, converter: {{ converter }})] {% if type == Bool %} - property? {{ name }} : {{ type }} = {{ value }} + property? {{ name }}{{ " : #{type}".id if type }} = {{ value }} {% else %} - property {{ name }} : {{ type }} = {{ value }} + property {{ name }}{{ " : #{type}".id if type }} = {{ value }} {% end %} {% end %} diff --git a/src/ameba/rule/lint/debug_calls.cr b/src/ameba/rule/lint/debug_calls.cr index 0e02be29..fc4e3e2f 100644 --- a/src/ameba/rule/lint/debug_calls.cr +++ b/src/ameba/rule/lint/debug_calls.cr @@ -18,7 +18,7 @@ module Ameba::Rule::Lint class DebugCalls < Base properties do description "Disallows debug-related calls" - method_names : Array(String) = %w(p p! pp pp!) + method_names %w(p p! pp pp!) end MSG = "Possibly forgotten debug-related `%s` call detected" diff --git a/src/ameba/rule/lint/documentation.cr b/src/ameba/rule/lint/documentation.cr index 5e6d134b..f08a2a0f 100644 --- a/src/ameba/rule/lint/documentation.cr +++ b/src/ameba/rule/lint/documentation.cr @@ -7,6 +7,12 @@ module Ameba::Rule::Lint # ``` # Lint/Documentation: # Enabled: true + # IgnoreClasses: false + # IgnoreModules: true + # IgnoreEnums: false + # IgnoreDefs: true + # IgnoreMacros: false + # IgnoreMacroHooks: true # ``` class Documentation < Base properties do @@ -18,6 +24,7 @@ module Ameba::Rule::Lint ignore_enums false ignore_defs true ignore_macros false + ignore_macro_hooks true end MSG = "Missing documentation" @@ -50,14 +57,21 @@ module Ameba::Rule::Lint end def test(source, node : Crystal::Macro, scope : AST::Scope) - node.name.in?(MACRO_HOOK_NAMES) || - ignore_macros? || check_missing_doc(source, node, scope) + return if ignore_macro_hooks? && node.name.in?(MACRO_HOOK_NAMES) + + ignore_macros? || check_missing_doc(source, node, scope) end private def check_missing_doc(source, node, scope) - visibility = scope.visibility + # bail out if the node is not public, + # i.e. `private def foo` + return if !node.visibility.public? - return if visibility && !visibility.public? + # bail out if the scope is not public, + # i.e. `def bar` inside `private class Foo` + return if (visibility = scope.visibility) && !visibility.public? + + # bail out if the node has the documentation present return if node.doc.presence issue_for(node, MSG) diff --git a/src/ameba/rule/lint/unused_argument.cr b/src/ameba/rule/lint/unused_argument.cr index 864b982f..d001a1aa 100644 --- a/src/ameba/rule/lint/unused_argument.cr +++ b/src/ameba/rule/lint/unused_argument.cr @@ -42,23 +42,26 @@ module Ameba::Rule::Lint end def test(source, node : Crystal::ProcLiteral, scope : AST::Scope) - ignore_procs? || find_unused_arguments source, scope + ignore_procs? || find_unused_arguments(source, scope) end def test(source, node : Crystal::Block, scope : AST::Scope) - ignore_blocks? || find_unused_arguments source, scope + ignore_blocks? || find_unused_arguments(source, scope) end def test(source, node : Crystal::Def, scope : AST::Scope) + arguments = scope.arguments.dup + # `Lint/UnusedBlockArgument` rule covers this case explicitly if block_arg = node.block_arg - scope.arguments.reject!(&.node.== block_arg) + arguments.reject!(&.node.== block_arg) end - ignore_defs? || find_unused_arguments source, scope + + ignore_defs? || find_unused_arguments(source, scope, arguments) end - private def find_unused_arguments(source, scope) - scope.arguments.each do |argument| + private def find_unused_arguments(source, scope, arguments = scope.arguments) + arguments.each do |argument| next if argument.anonymous? || argument.ignored? next if scope.references?(argument.variable) diff --git a/src/ameba/rule/lint/useless_condition_in_when.cr b/src/ameba/rule/lint/useless_condition_in_when.cr index 2d52f480..da3f47ab 100644 --- a/src/ameba/rule/lint/useless_condition_in_when.cr +++ b/src/ameba/rule/lint/useless_condition_in_when.cr @@ -37,11 +37,11 @@ module Ameba::Rule::Lint MSG = "Useless condition in when detected" - # TODO: condition.cond may be a complex ASTNode with + # TODO: condition *cond* may be a complex ASTNode with # useless inner conditions. We might need to improve this # simple implementation in future. protected def check_node(source, when_node, cond) - cond_s = cond.to_s + return unless cond_s = cond.to_s.presence return if when_node.conds.none?(&.to_s.==(cond_s)) issue_for cond, MSG diff --git a/src/ameba/rule/performance/any_after_filter.cr b/src/ameba/rule/performance/any_after_filter.cr index f40fbbca..3dafb598 100644 --- a/src/ameba/rule/performance/any_after_filter.cr +++ b/src/ameba/rule/performance/any_after_filter.cr @@ -29,7 +29,7 @@ module Ameba::Rule::Performance class AnyAfterFilter < Base properties do description "Identifies usage of `any?` calls that follow filters" - filter_names : Array(String) = %w(select reject) + filter_names %w(select reject) end ANY_NAME = "any?" diff --git a/src/ameba/rule/performance/chained_call_with_no_bang.cr b/src/ameba/rule/performance/chained_call_with_no_bang.cr index d9285726..ef505b73 100644 --- a/src/ameba/rule/performance/chained_call_with_no_bang.cr +++ b/src/ameba/rule/performance/chained_call_with_no_bang.cr @@ -45,7 +45,7 @@ module Ameba::Rule::Performance # All of those have bang method variants returning `self` # and are not modifying the receiver type (like `compact` does), # thus are safe to switch to the bang variant. - call_names : Array(String) = %w(uniq sort sort_by shuffle reverse) + call_names %w(uniq sort sort_by shuffle reverse) end # All these methods are allocating a new object diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index a995fcae..9127ccd5 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -28,7 +28,7 @@ module Ameba::Rule::Performance class FirstLastAfterFilter < Base properties do description "Identifies usage of `first/last/first?/last?` calls that follow filters" - filter_names : Array(String) = %w(select) + filter_names %w(select) end CALL_NAMES = %w(first last first? last?) diff --git a/src/ameba/rule/performance/size_after_filter.cr b/src/ameba/rule/performance/size_after_filter.cr index ba952586..2e5dbbb0 100644 --- a/src/ameba/rule/performance/size_after_filter.cr +++ b/src/ameba/rule/performance/size_after_filter.cr @@ -35,7 +35,7 @@ module Ameba::Rule::Performance class SizeAfterFilter < Base properties do description "Identifies usage of `size` calls that follow filter" - filter_names : Array(String) = %w(select reject) + filter_names %w(select reject) end SIZE_NAME = "size" diff --git a/src/ameba/rule/style/guard_clause.cr b/src/ameba/rule/style/guard_clause.cr index 618c4c36..348fd85c 100644 --- a/src/ameba/rule/style/guard_clause.cr +++ b/src/ameba/rule/style/guard_clause.cr @@ -63,7 +63,9 @@ module Ameba::Rule::Style "code inside a conditional expression." def test(source) - AST::NodeVisitor.new self, source, skip: [Crystal::Assign] + AST::NodeVisitor.new self, source, skip: [ + Crystal::Assign, + ] end def test(source, node : Crystal::Def) @@ -118,6 +120,7 @@ module Ameba::Rule::Style issue_for keyword_loc, keyword_end_loc, MSG % example do |corrector| replacement = "#{scope_exiting_keyword} #{conditional_keyword}" + corrector.replace(keyword_loc, keyword_end_loc, replacement) corrector.remove(end_loc, end_end_loc) end diff --git a/src/ameba/rule/style/is_a_filter.cr b/src/ameba/rule/style/is_a_filter.cr index 8b07f3a9..97c131fa 100644 --- a/src/ameba/rule/style/is_a_filter.cr +++ b/src/ameba/rule/style/is_a_filter.cr @@ -41,7 +41,7 @@ module Ameba::Rule::Style class IsAFilter < Base properties do description "Identifies usage of `is_a?/nil?` calls within filters" - filter_names : Array(String) = %w(select reject any? all? none? one?) + filter_names %w(select reject any? all? none? one?) end MSG = "Use `%s` instead of `%s`"