From d71091a40c1d652cfc1ed22b37b539d0ff1e1065 Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 22 Jan 2021 02:31:18 +0100 Subject: [PATCH 1/2] Add Performance/AnyInsteadOfEmpty rule --- .../performance/any_instead_of_empty_spec.cr | 46 +++++++++++++++++++ .../rule/performance/any_instead_of_empty.cr | 44 ++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 spec/ameba/rule/performance/any_instead_of_empty_spec.cr create mode 100644 src/ameba/rule/performance/any_instead_of_empty.cr diff --git a/spec/ameba/rule/performance/any_instead_of_empty_spec.cr b/spec/ameba/rule/performance/any_instead_of_empty_spec.cr new file mode 100644 index 00000000..2a5f8227 --- /dev/null +++ b/spec/ameba/rule/performance/any_instead_of_empty_spec.cr @@ -0,0 +1,46 @@ +require "../../../spec_helper" + +module Ameba::Rule::Performance + subject = AnyInsteadOfEmpty.new + + describe AnyInsteadOfEmpty do + it "passes if there is no potential performance improvements" do + source = Source.new %( + [1, 2, 3].any?(&.zero?) + [1, 2, 3].any?(String) + [1, 2, 3].any?(1..3) + [1, 2, 3].any? { |e| e > 1 } + ) + subject.catch(source).should be_valid + end + + it "reports if there is any? call without a block nor argument" do + source = Source.new %( + [1, 2, 3].any? + ) + subject.catch(source).should_not be_valid + end + + context "macro" do + it "reports in macro scope" do + source = Source.new %( + {{ [1, 2, 3].any? }} + ) + subject.catch(source).should_not be_valid + end + end + + it "reports rule, pos and message" do + source = Source.new path: "source.cr", code: %( + [1, 2, 3].any? + ) + subject.catch(source).should_not be_valid + issue = source.issues.first + + issue.rule.should_not be_nil + issue.location.to_s.should eq "source.cr:1:11" + issue.end_location.to_s.should eq "source.cr:1:15" + issue.message.should eq "Use `!{...}.empty?` instead of `{...}.any?`" + end + end +end diff --git a/src/ameba/rule/performance/any_instead_of_empty.cr b/src/ameba/rule/performance/any_instead_of_empty.cr new file mode 100644 index 00000000..6dd7a2ba --- /dev/null +++ b/src/ameba/rule/performance/any_instead_of_empty.cr @@ -0,0 +1,44 @@ +module Ameba::Rule::Performance + # This rule is used to identify usage of arg-less `Enumerable#any?` calls. + # + # Using `Enumerable#any?` instead of `Enumerable#empty?` might lead to an + # unexpected results (like `[nil, false].any? # => false`). In some cases + # it also might be less efficient, since it iterates until the block will + # return a _truthy_ value, instead of just checking if there's at least + # one value present. + # + # For example, this is considered invalid: + # + # ``` + # [1, 2, 3].any? + # ``` + # + # And it should be written as this: + # + # ``` + # ![1, 2, 3].empty? + # ``` + # + # YAML configuration example: + # + # ``` + # Performance/AnyInsteadOfEmpty: + # Enabled: true + # ``` + class AnyInsteadOfEmpty < Base + properties do + description "Identifies usage of arg-less `any?` calls." + end + + ANY_NAME = "any?" + MSG = "Use `!{...}.empty?` instead of `{...}.any?`" + + def test(source, node : Crystal::Call) + return unless node.name == ANY_NAME + return unless node.block.nil? && node.args.empty? + return unless node.obj + + issue_for node.name_location, node.name_end_location, MSG + end + end +end From 7b3c814914052ed023832cdb464757607b7891cb Mon Sep 17 00:00:00 2001 From: Sijawusz Pur Rahnama Date: Fri, 22 Jan 2021 02:37:11 +0100 Subject: [PATCH 2/2] Fix newly found issues --- spec/ameba/ast/scope_spec.cr | 2 +- spec/ameba/ast/variabling/variable_spec.cr | 4 ++-- src/ameba/ast/variabling/variable.cr | 2 +- src/ameba/ast/visitors/flow_expression_visitor.cr | 2 +- src/ameba/cli/cmd.cr | 2 +- src/ameba/rule/lint/unneeded_disable_directive.cr | 2 +- src/ameba/rule/performance/first_last_after_filter.cr | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/ameba/ast/scope_spec.cr b/spec/ameba/ast/scope_spec.cr index 72951b14..00f3061c 100644 --- a/spec/ameba/ast/scope_spec.cr +++ b/spec/ameba/ast/scope_spec.cr @@ -51,7 +51,7 @@ module Ameba::AST it "adds a new variable to the scope" do scope = Scope.new as_node("") scope.add_variable(Crystal::Var.new "foo") - scope.variables.any?.should be_true + scope.variables.empty?.should be_false end end diff --git a/spec/ameba/ast/variabling/variable_spec.cr b/spec/ameba/ast/variabling/variable_spec.cr index fc39ef0d..66b1a188 100644 --- a/spec/ameba/ast/variabling/variable_spec.cr +++ b/spec/ameba/ast/variabling/variable_spec.cr @@ -48,7 +48,7 @@ module Ameba::AST it "assigns the variable (creates a new assignment)" do variable = Variable.new(var_node, scope) variable.assign(assign_node, scope) - variable.assignments.any?.should be_true + variable.assignments.empty?.should be_false end it "can create multiple assignments" do @@ -64,7 +64,7 @@ module Ameba::AST variable = Variable.new(var_node, scope) variable.assign(as_node("foo=1"), scope) variable.reference(var_node, scope) - variable.references.any?.should be_true + variable.references.empty?.should be_false end it "adds a reference to the scope" do diff --git a/src/ameba/ast/variabling/variable.cr b/src/ameba/ast/variabling/variable.cr index ea741e82..b3dab305 100644 --- a/src/ameba/ast/variabling/variable.cr +++ b/src/ameba/ast/variabling/variable.cr @@ -58,7 +58,7 @@ module Ameba::AST # variable.referenced? # => true # ``` def referenced? - references.any? + !references.empty? end # Creates a reference to this variable in some scope. diff --git a/src/ameba/ast/visitors/flow_expression_visitor.cr b/src/ameba/ast/visitors/flow_expression_visitor.cr index 74592705..69ec1e25 100644 --- a/src/ameba/ast/visitors/flow_expression_visitor.cr +++ b/src/ameba/ast/visitors/flow_expression_visitor.cr @@ -60,7 +60,7 @@ module Ameba::AST end private def in_loop? - @loop_stack.any? + !@loop_stack.empty? end end end diff --git a/src/ameba/cli/cmd.cr b/src/ameba/cli/cmd.cr index 3f80899e..821f2845 100644 --- a/src/ameba/cli/cmd.cr +++ b/src/ameba/cli/cmd.cr @@ -61,7 +61,7 @@ module Ameba::Cli if f.size == 1 && f.first =~ /.+:\d+:\d+/ configure_explain_opts(f.first, opts) else - opts.globs = f if f.any? + opts.globs = f unless f.empty? end end diff --git a/src/ameba/rule/lint/unneeded_disable_directive.cr b/src/ameba/rule/lint/unneeded_disable_directive.cr index d6d8fc2c..4a55f446 100644 --- a/src/ameba/rule/lint/unneeded_disable_directive.cr +++ b/src/ameba/rule/lint/unneeded_disable_directive.cr @@ -36,7 +36,7 @@ module Ameba::Rule::Lint next unless token.type == :COMMENT next unless directive = source.parse_inline_directive(token.value.to_s) next unless names = unneeded_disables(source, directive, token.location) - next unless names.any? + next if names.empty? issue_for token, MSG % names.join(", ") end diff --git a/src/ameba/rule/performance/first_last_after_filter.cr b/src/ameba/rule/performance/first_last_after_filter.cr index 4333bd6d..f3368027 100644 --- a/src/ameba/rule/performance/first_last_after_filter.cr +++ b/src/ameba/rule/performance/first_last_after_filter.cr @@ -45,7 +45,7 @@ module Ameba::Rule::Performance def test(source, node : Crystal::Call) return unless node.name.in?(CALL_NAMES) && (obj = node.obj) return unless obj.is_a?(Crystal::Call) && obj.block - return if !node.block.nil? || node.args.any? + return unless node.block.nil? && node.args.empty? return unless obj.name.in?(filter_names) message = node.name.includes?(CALL_NAMES.first) ? MSG : MSG_REVERSE