From 65ab317a3bb716cc9973fd5ea3e8e01254b9a61d Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 27 Dec 2023 19:28:56 +0100 Subject: [PATCH 1/5] Merge `delegate` calls --- src/ameba/ast/scope.cr | 5 ++--- src/ameba/ast/variabling/argument.cr | 5 ++--- src/ameba/ast/variabling/assignment.cr | 5 ++--- src/ameba/ast/variabling/ivariable.cr | 6 ++---- src/ameba/ast/variabling/type_dec_variable.cr | 5 ++--- src/ameba/ast/variabling/variable.cr | 6 ++---- 6 files changed, 12 insertions(+), 20 deletions(-) diff --git a/src/ameba/ast/scope.cr b/src/ameba/ast/scope.cr index 97b71483b..3e0b4573b 100644 --- a/src/ameba/ast/scope.cr +++ b/src/ameba/ast/scope.cr @@ -34,9 +34,8 @@ module Ameba::AST # The actual AST node that represents a current scope. getter node : Crystal::ASTNode - delegate to_s, to: node - delegate location, to: node - delegate end_location, to: node + delegate location, end_location, to_s, + to: @node def_equals_and_hash node, location diff --git a/src/ameba/ast/variabling/argument.cr b/src/ameba/ast/variabling/argument.cr index 59b546618..d080e9a17 100644 --- a/src/ameba/ast/variabling/argument.cr +++ b/src/ameba/ast/variabling/argument.cr @@ -19,9 +19,8 @@ module Ameba::AST # Variable of this argument (may be the same node) getter variable : Variable - delegate location, to: @node - delegate end_location, to: @node - delegate to_s, to: @node + delegate location, end_location, to_s, + to: @node # Creates a new argument. # diff --git a/src/ameba/ast/variabling/assignment.cr b/src/ameba/ast/variabling/assignment.cr index 7168f5cba..ece19b804 100644 --- a/src/ameba/ast/variabling/assignment.cr +++ b/src/ameba/ast/variabling/assignment.cr @@ -19,9 +19,8 @@ module Ameba::AST # A scope assignment belongs to getter scope : Scope - delegate to_s, to: @node - delegate location, to: @node - delegate end_location, to: @node + delegate location, end_location, to_s, + to: @node # Creates a new assignment. # diff --git a/src/ameba/ast/variabling/ivariable.cr b/src/ameba/ast/variabling/ivariable.cr index 836ad345f..db6f3bd36 100644 --- a/src/ameba/ast/variabling/ivariable.cr +++ b/src/ameba/ast/variabling/ivariable.cr @@ -2,10 +2,8 @@ module Ameba::AST class InstanceVariable getter node : Crystal::InstanceVar - delegate location, to: @node - delegate end_location, to: @node - delegate name, to: @node - delegate to_s, to: @node + delegate location, end_location, name, to_s, + to: @node def initialize(@node) end diff --git a/src/ameba/ast/variabling/type_dec_variable.cr b/src/ameba/ast/variabling/type_dec_variable.cr index 3376b37fa..74cc3fbba 100644 --- a/src/ameba/ast/variabling/type_dec_variable.cr +++ b/src/ameba/ast/variabling/type_dec_variable.cr @@ -2,9 +2,8 @@ module Ameba::AST class TypeDecVariable getter node : Crystal::TypeDeclaration - delegate location, to: @node - delegate end_location, to: @node - delegate to_s, to: @node + delegate location, end_location, to_s, + to: @node def initialize(@node) end diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index c2eee962b..615b588eb 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -17,10 +17,8 @@ module Ameba::AST # Node of the first assignment which can be available before any reference. getter assign_before_reference : Crystal::ASTNode? - delegate location, to: @node - delegate end_location, to: @node - delegate name, to: @node - delegate to_s, to: @node + delegate location, end_location, name, to_s, + to: @node # Creates a new variable(in the scope). # From 6d0b12c70fb10f586aebc3863247475a1d6aa386 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 27 Dec 2023 19:38:44 +0100 Subject: [PATCH 2/5] Cleanup docs --- src/ameba/formatter/base_formatter.cr | 6 +++--- src/ameba/rule/base.cr | 5 +++-- src/ameba/rule/naming/block_parameter_name.cr | 2 +- src/ameba/rule/style/verbose_block.cr | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/ameba/formatter/base_formatter.cr b/src/ameba/formatter/base_formatter.cr index 4009c9921..a6ceafdf0 100644 --- a/src/ameba/formatter/base_formatter.cr +++ b/src/ameba/formatter/base_formatter.cr @@ -17,13 +17,13 @@ module Ameba::Formatter # A list of sources to inspect is passed as an argument. def started(sources); end - # Callback that indicates when source inspection is finished. + # Callback that indicates when source inspection is started. # A corresponding source is passed as an argument. - def source_finished(source : Source); end + def source_started(source : Source); end # Callback that indicates when source inspection is finished. # A corresponding source is passed as an argument. - def source_started(source : Source); end + def source_finished(source : Source); end # Callback that indicates when inspection is finished. # A list of inspected sources is passed as an argument. diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index f28c83d0e..242c86f0b 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -32,14 +32,15 @@ module Ameba::Rule # This method is designed to test the source passed in. If source has issues # that are tested by this rule, it should add an issue. # - # Be default it uses a node visitor to traverse all the nodes in the source. + # By default it uses a node visitor to traverse all the nodes in the source. + # # NOTE: Must be overridden for other type of rules. def test(source : Source) AST::NodeVisitor.new self, source end + # NOTE: Can't be abstract def test(source : Source, node : Crystal::ASTNode, *opts) - # can't be abstract end # A convenient addition to `#test` method that does the same diff --git a/src/ameba/rule/naming/block_parameter_name.cr b/src/ameba/rule/naming/block_parameter_name.cr index 2311b8900..71e6a6a87 100644 --- a/src/ameba/rule/naming/block_parameter_name.cr +++ b/src/ameba/rule/naming/block_parameter_name.cr @@ -41,7 +41,7 @@ module Ameba::Rule::Naming end private def valid_name?(name) - return true if name.blank? # happens with compound names like `(arg1, arg2)` + return true if name.blank? # TODO: handle unpacked variables return true if name.in?(allowed_names) return false if name.in?(forbidden_names) diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index fb69f14fe..fee2cecd3 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -230,7 +230,7 @@ module Ameba::Rule::Style # we filter out the blocks that are of call type - `i.to_i64.odd?` return unless (body = block.body).is_a?(Crystal::Call) - # we need to "unwind" the chain calls, so the final receiver object + # we need to "unwind" the call chain, so the final receiver object # ends up being a variable - `i` obj = body.obj while obj.is_a?(Crystal::Call) From e99a69765fc7483e3dd9b0554dea9eda46c2992e Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 27 Dec 2023 19:42:50 +0100 Subject: [PATCH 3/5] Few refactors --- spec/ameba/ast/scope_spec.cr | 28 +++++++++------- spec/ameba/ast/variabling/variable_spec.cr | 13 +++++--- spec/ameba/config_spec.cr | 2 +- .../rule/lint/literals_comparison_spec.cr | 2 ++ src/ameba/ast/variabling/variable.cr | 8 ++--- src/ameba/ast/visitors/scope_visitor.cr | 4 +-- src/ameba/config.cr | 9 ++--- src/ameba/formatter/todo_formatter.cr | 33 +++++++++++-------- src/ameba/rule/style/verbose_block.cr | 33 +++++++++---------- 9 files changed, 74 insertions(+), 58 deletions(-) diff --git a/spec/ameba/ast/scope_spec.cr b/spec/ameba/ast/scope_spec.cr index c36ef62d0..3d8b6ba30 100644 --- a/spec/ameba/ast/scope_spec.cr +++ b/spec/ameba/ast/scope_spec.cr @@ -57,13 +57,15 @@ module Ameba::AST end end CRYSTAL - scope = Scope.new nodes.def_nodes.first + var_node = nodes.var_nodes.first - scope.add_variable var_node + + scope = Scope.new nodes.def_nodes.first + scope.add_variable(var_node) scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope) variable = Variable.new(var_node, scope) - variable.reference nodes.var_nodes.first, scope.inner_scopes.first + variable.reference(nodes.var_nodes.first, scope.inner_scopes.first) scope.references?(variable).should be_true end @@ -77,13 +79,15 @@ module Ameba::AST end end CRYSTAL - scope = Scope.new nodes.def_nodes.first + var_node = nodes.var_nodes.first - scope.add_variable var_node + + scope = Scope.new nodes.def_nodes.first + scope.add_variable(var_node) scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope) variable = Variable.new(var_node, scope) - variable.reference nodes.var_nodes.first, scope.inner_scopes.first + variable.reference(nodes.var_nodes.first, scope.inner_scopes.first) scope.references?(variable, check_inner_scopes: false).should be_false end @@ -98,9 +102,11 @@ module Ameba::AST end end CRYSTAL - scope = Scope.new nodes.def_nodes.first + var_node = nodes.var_nodes.first - scope.add_variable var_node + + scope = Scope.new nodes.def_nodes.first + scope.add_variable(var_node) scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope) variable = Variable.new(var_node, scope) @@ -120,7 +126,7 @@ module Ameba::AST describe "#find_variable" do it "returns the variable in the scope by name" do scope = Scope.new as_node("foo = 1") - scope.add_variable Crystal::Var.new "foo" + scope.add_variable(Crystal::Var.new "foo") scope.find_variable("foo").should_not be_nil end @@ -133,7 +139,7 @@ module Ameba::AST describe "#assign_variable" do it "creates a new assignment" do scope = Scope.new as_node("foo = 1") - scope.add_variable Crystal::Var.new "foo" + scope.add_variable(Crystal::Var.new "foo") scope.assign_variable("foo", Crystal::Var.new "foo") var = scope.find_variable("foo").should_not be_nil var.assignments.size.should eq 1 @@ -141,7 +147,7 @@ module Ameba::AST it "does not create the assignment if variable is wrong" do scope = Scope.new as_node("foo = 1") - scope.add_variable Crystal::Var.new "foo" + scope.add_variable(Crystal::Var.new "foo") scope.assign_variable("bar", Crystal::Var.new "bar") var = scope.find_variable("foo").should_not be_nil var.assignments.size.should eq 0 diff --git a/spec/ameba/ast/variabling/variable_spec.cr b/spec/ameba/ast/variabling/variable_spec.cr index 83792e2ef..97f72f84e 100644 --- a/spec/ameba/ast/variabling/variable_spec.cr +++ b/spec/ameba/ast/variabling/variable_spec.cr @@ -85,13 +85,16 @@ module Ameba::AST 3.times { |i| a = a + i } end CRYSTAL - scope = Scope.new nodes.def_nodes.first + var_node = nodes.var_nodes.first - scope.add_variable var_node + + scope = Scope.new(nodes.def_nodes.first) + scope.add_variable(var_node) scope.inner_scopes << Scope.new(nodes.block_nodes.first, scope) variable = Variable.new(var_node, scope) - variable.reference nodes.var_nodes.last, scope.inner_scopes.last + variable.reference(nodes.var_nodes.last, scope.inner_scopes.last) + variable.captured_by_block?.should be_truthy end @@ -101,8 +104,10 @@ module Ameba::AST a = 1 end CRYSTAL - scope.add_variable Crystal::Var.new "a" + + scope.add_variable(Crystal::Var.new "a") variable = scope.variables.first + variable.captured_by_block?.should be_falsey end end diff --git a/spec/ameba/config_spec.cr b/spec/ameba/config_spec.cr index 4d6f95f04..678da23ba 100644 --- a/spec/ameba/config_spec.cr +++ b/spec/ameba/config_spec.cr @@ -85,7 +85,7 @@ module Ameba end it "raises when custom config file doesn't exist" do - expect_raises(Exception, "Unable to load config file: Config file does not exist foo.yml") do + expect_raises(Exception, "Unable to load config file: Config file does not exist") do Config.load "foo.yml" end end diff --git a/spec/ameba/rule/lint/literals_comparison_spec.cr b/spec/ameba/rule/lint/literals_comparison_spec.cr index ded948e66..ef75b6824 100644 --- a/spec/ameba/rule/lint/literals_comparison_spec.cr +++ b/spec/ameba/rule/lint/literals_comparison_spec.cr @@ -10,6 +10,8 @@ module Ameba::Rule::Lint ["foo"] === [foo] "foo" == foo "foo" != foo + "foo" == FOO + FOO == "foo" foo == "foo" foo != "foo" CRYSTAL diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 615b588eb..5380d8faa 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -52,7 +52,7 @@ module Ameba::AST # # ``` # variable = Variable.new(node, scope) - # variable.reference(var_node) + # variable.reference(var_node, some_scope) # variable.referenced? # => true # ``` def referenced? @@ -206,9 +206,9 @@ module Ameba::AST return if references.size > assignments.size return if assignments.any?(&.op_assign?) - @assign_before_reference = assignments.find { |ass| - !ass.in_branch? - }.try &.node + @assign_before_reference = assignments + .find(&.in_branch?.!) + .try(&.node) end end end diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index 0b93cdb9c..a7f132dbd 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -153,7 +153,7 @@ module Ameba::AST # :nodoc: def visit(node : Crystal::Var) - variable = @current_scope.find_variable node.name + variable = @current_scope.find_variable(node.name) case when @current_scope.arg?(node) # node is an argument @@ -161,7 +161,7 @@ module Ameba::AST when variable.nil? && @current_assign # node is a variable @current_scope.add_variable(node) when variable # node is a reference - reference = variable.reference node, @current_scope + reference = variable.reference(node, @current_scope) if @current_assign.is_a?(Crystal::OpAssign) || !reference.target_of?(@current_assign) variable.reference_assignments! end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index a8db5c669..195c70587 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -97,8 +97,9 @@ class Ameba::Config @excluded = load_array_section(config, "Excluded") @globs = load_array_section(config, "Globs", DEFAULT_GLOBS) - return unless formatter_name = load_formatter_name(config) - self.formatter = formatter_name + if formatter_name = load_formatter_name(config) + self.formatter = formatter_name + end end # Loads YAML configuration file by `path`. @@ -120,8 +121,8 @@ class Ameba::Config protected def self.read_config(path = nil) if path - raise ArgumentError.new("Config file does not exist #{path}") unless File.exists?(path) - return File.read(path) + return File.read(path) if File.exists?(path) + raise "Config file does not exist" end each_config_path do |config_path| return File.read(config_path) if File.exists?(config_path) diff --git a/src/ameba/formatter/todo_formatter.cr b/src/ameba/formatter/todo_formatter.cr index 5dc797e86..00a2d28f1 100644 --- a/src/ameba/formatter/todo_formatter.cr +++ b/src/ameba/formatter/todo_formatter.cr @@ -26,16 +26,21 @@ module Ameba::Formatter end private def generate_todo_config(issues) - file = File.new(@config_path, mode: "w") - file << header - rule_issues_map(issues).each do |rule, rule_issues| - file << "\n# Problems found: #{rule_issues.size}" - file << "\n# Run `ameba --only #{rule.name}` for details" - file << rule_todo(rule, rule_issues).gsub("---", "") + File.open(@config_path, mode: "w") do |file| + file << header + + rule_issues_map(issues).each do |rule, rule_issues| + rule_todo = rule_todo(rule, rule_issues) + rule_todo = + {rule_todo.name => rule_todo} + .to_yaml.gsub("---", "") + + file << "\n# Problems found: #{rule_issues.size}" + file << "\n# Run `ameba --only #{rule.name}` for details" + file << rule_todo + end + file end - file - ensure - file.close if file end private def rule_issues_map(issues) @@ -60,11 +65,11 @@ module Ameba::Formatter end private def rule_todo(rule, issues) - rule.excluded = issues - .compact_map(&.location.try &.filename.try &.to_s) - .uniq! - - {rule.name => rule}.to_yaml + rule.dup.tap do |rule_todo| + rule_todo.excluded = issues + .compact_map(&.location.try &.filename.try &.to_s) + .uniq! + end end end end diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index fee2cecd3..fa4ae961e 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -110,23 +110,22 @@ module Ameba::Rule::Style i end - protected def args_to_s(io : IO, node : Crystal::Call, short_block = nil, skip_last_arg = false) - node.args.dup.tap do |args| - args.pop? if skip_last_arg - args.join io, ", " - - named_args = node.named_args - if named_args - io << ", " unless args.empty? || named_args.empty? - named_args.join io, ", " do |arg, inner_io| - inner_io << arg.name << ": " << arg.value - end + protected def args_to_s(io : IO, node : Crystal::Call, short_block = nil, skip_last_arg = false) : Nil + args = node.args.dup + args.pop? if skip_last_arg + args.join io, ", " + + named_args = node.named_args + if named_args + io << ", " unless args.empty? || named_args.empty? + named_args.join io, ", " do |arg, inner_io| + inner_io << arg.name << ": " << arg.value end + end - if short_block - io << ", " unless args.empty? && (named_args.nil? || named_args.empty?) - io << short_block - end + if short_block + io << ", " unless args.empty? && (named_args.nil? || named_args.empty?) + io << short_block end end @@ -164,9 +163,7 @@ module Ameba::Rule::Style return unless block_end_location = block.body.end_location block_code = source_between(block_location, block_end_location, source.lines) - return unless block_code.try(&.starts_with?("&.")) - - block_code + block_code if block_code.try(&.starts_with?("&.")) end protected def call_code(source, call, body) From 444b07c1796594f94da85ab727bf8972d4103530 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Wed, 27 Dec 2023 22:51:36 +0100 Subject: [PATCH 4/5] Bump version to 1.6.1 --- shard.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shard.yml b/shard.yml index 5ebd6c8b5..74ce7c671 100644 --- a/shard.yml +++ b/shard.yml @@ -1,5 +1,5 @@ name: ameba -version: 1.6.0 +version: 1.6.1 authors: - Vitalii Elenhaupt From ce3f2b7e4b6f728d29bf8ff15db5ffd210ea4c6b Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Thu, 28 Dec 2023 04:01:05 +0100 Subject: [PATCH 5/5] Add QoL `Variable#reference(scope)` method --- src/ameba/ast/variabling/variable.cr | 5 +++++ src/ameba/ast/visitors/scope_visitor.cr | 4 +--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index 5380d8faa..8b513948c 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -72,6 +72,11 @@ module Ameba::AST end end + # :ditto: + def reference(scope : Scope) + reference(node, scope) + end + # Reference variable's assignments. # # ``` diff --git a/src/ameba/ast/visitors/scope_visitor.cr b/src/ameba/ast/visitors/scope_visitor.cr index a7f132dbd..53ea39ebe 100644 --- a/src/ameba/ast/visitors/scope_visitor.cr +++ b/src/ameba/ast/visitors/scope_visitor.cr @@ -178,9 +178,7 @@ module Ameba::AST when scope.type_definition? && accessor_macro?(node) then return false when scope.def? && special_node?(node) scope.arguments.each do |arg| - variable = arg.variable - - ref = variable.reference(variable.node, scope) + ref = arg.variable.reference(scope) ref.explicit = false end end