From 15bb8f5331c2e36e5bd7c49926be53c60d4ab1e0 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Mon, 28 May 2018 11:48:07 +0300 Subject: [PATCH] Detect shadowing outer local vars --- .../formatter/disabled_formatter_spec.cr | 12 +- spec/ameba/formatter/dot_formatter_spec.cr | 6 +- .../rule/shadowing_local_outer_var_spec.cr | 140 ++++++++++++++++++ spec/ameba/runner_spec.cr | 4 +- src/ameba/ast/branch.cr | 6 +- src/ameba/rule/shadowing_local_outer_var.cr | 62 ++++++++ 6 files changed, 216 insertions(+), 14 deletions(-) create mode 100644 spec/ameba/rule/shadowing_local_outer_var_spec.cr create mode 100644 src/ameba/rule/shadowing_local_outer_var.cr diff --git a/spec/ameba/formatter/disabled_formatter_spec.cr b/spec/ameba/formatter/disabled_formatter_spec.cr index 5d38899c..052d336c 100644 --- a/spec/ameba/formatter/disabled_formatter_spec.cr +++ b/spec/ameba/formatter/disabled_formatter_spec.cr @@ -15,9 +15,9 @@ module Ameba::Formatter Colorize.enabled = false path = "source.cr" - s = Source.new("", path).tap do |s| - s.error(ErrorRule.new, 1, 2, "ErrorRule", :disabled) - s.error(NamedRule.new, 2, 2, "NamedRule", :disabled) + s = Source.new("", path).tap do |source| + source.error(ErrorRule.new, 1, 2, "ErrorRule", :disabled) + source.error(NamedRule.new, 2, 2, "NamedRule", :disabled) end subject.finished [s] log = output.to_s @@ -29,9 +29,9 @@ module Ameba::Formatter end it "does not write not-disabled rules" do - s = Source.new("", "source.cr").tap do |s| - s.error(ErrorRule.new, 1, 2, "ErrorRule") - s.error(NamedRule.new, 2, 2, "NamedRule", :disabled) + s = Source.new("", "source.cr").tap do |source| + source.error(ErrorRule.new, 1, 2, "ErrorRule") + source.error(NamedRule.new, 2, 2, "NamedRule", :disabled) end subject.finished [s] output.to_s.should_not contain ErrorRule.name diff --git a/spec/ameba/formatter/dot_formatter_spec.cr b/spec/ameba/formatter/dot_formatter_spec.cr index ed1d9c57..eac49dcf 100644 --- a/spec/ameba/formatter/dot_formatter_spec.cr +++ b/spec/ameba/formatter/dot_formatter_spec.cr @@ -39,9 +39,9 @@ module Ameba::Formatter context "when errors found" do it "writes each error" do - s = Source.new("").tap do |s| - s.error(DummyRule.new, 1, 1, "DummyRuleError") - s.error(NamedRule.new, 1, 2, "NamedRuleError") + s = Source.new("").tap do |source| + source.error(DummyRule.new, 1, 1, "DummyRuleError") + source.error(NamedRule.new, 1, 2, "NamedRuleError") end subject.finished [s] log = output.to_s diff --git a/spec/ameba/rule/shadowing_local_outer_var_spec.cr b/spec/ameba/rule/shadowing_local_outer_var_spec.cr new file mode 100644 index 00000000..75e19f35 --- /dev/null +++ b/spec/ameba/rule/shadowing_local_outer_var_spec.cr @@ -0,0 +1,140 @@ +require "../../spec_helper" + +module Ameba::Rule + describe ShadowingOuterLocalVar do + subject = ShadowingOuterLocalVar.new + + it "doesn't report if there is no shadowing" do + source = Source.new %( + def some_method + foo = 1 + + 3.times do |bar| + bar + end + + -> (baz : Int32) {} + + -> (bar : String) {} + end + ) + + subject.catch(source).should be_valid + end + + it "reports if there is a shadowing in a block" do + source = Source.new %( + def some_method + foo = 1 + + 3.times do |foo| + end + end + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is a shadowing in a proc" do + source = Source.new %( + def some_method + foo = 1 + + -> (foo : Int32) {} + end + ) + subject.catch(source).should_not be_valid + end + + it "reports if there is a shadowing in an inner scope" do + source = Source.new %( + def foo + foo = 1 + + 3.times do |i| + 3.times { |foo| foo } + end + end + ) + subject.catch(source).should_not be_valid + end + + it "reports if variable is shadowed twice" do + source = Source.new %( + foo = 1 + + 3.times do |foo| + -> (foo : Int32) { foo + 1 } + end + ) + subject.catch(source).should_not be_valid + + source.errors.size.should eq 2 + end + + it "reports if a splat block argument shadows local var" do + source = Source.new %( + foo = 1 + + 3.times do |*foo| + end + ) + subject.catch(source).should_not be_valid + end + + it "reports if a &block argument is shadowed" do + source = Source.new %( + def method_with_block(a, &block) + 3.times do |block| + end + end + ) + subject.catch(source).should_not be_valid + source.errors.first.message.should eq "Shadowing outer local variable `block`" + end + + it "reports if there are multiple args and one shadows local var" do + source = Source.new %( + foo = 1 + [1, 2, 3].each_with_index do |i, foo| + i + foo + end + ) + subject.catch(source).should_not be_valid + source.errors.first.message.should eq "Shadowing outer local variable `foo`" + end + + it "doesn't report if an outer var is reassigned in a block" do + source = Source.new %( + def foo + foo = 1 + 3.times do |i| + foo = 2 + end + end + ) + subject.catch(source).should be_valid + end + + it "doesn't report if an argument is a black hole '_'" do + source = Source.new %( + _ = 1 + 3.times do |_| + end + ) + subject.catch(source).should be_valid + end + + it "reports rule, location and message" do + source = Source.new %( + foo = 1 + 3.times { |foo| foo + 1 } + ), "source.cr" + subject.catch(source).should_not be_valid + + error = source.errors.first + error.rule.should_not be_nil + error.location.to_s.should eq "source.cr:3:20" + error.message.should eq "Shadowing outer local variable `foo`" + end + end +end diff --git a/spec/ameba/runner_spec.cr b/spec/ameba/runner_spec.cr index bd31e6fe..880f00a3 100644 --- a/spec/ameba/runner_spec.cr +++ b/spec/ameba/runner_spec.cr @@ -43,13 +43,13 @@ module Ameba path = "source.cr" source = Source.new "", path - rules = ([] of Rule::Base).tap do |rules| + all_rules = ([] of Rule::Base).tap do |rules| rule = ErrorRule.new rule.excluded = [path] rules << rule end - Runner.new(rules, [source], formatter).run.success?.should be_true + Runner.new(all_rules, [source], formatter).run.success?.should be_true end context "invalid syntax" do diff --git a/src/ameba/ast/branch.cr b/src/ameba/ast/branch.cr index 081b3031..322cd3f4 100644 --- a/src/ameba/ast/branch.cr +++ b/src/ameba/ast/branch.cr @@ -83,10 +83,10 @@ module Ameba::AST private def on_branchable_start(node, branches : Array | Tuple) @branchable = Branchable.new(node, @branchable) - branches.each do |node| + branches.each do |branch_node| break if branch # branch found - @current_branch = node if node && !node.nop? - node.try &.accept(self) + @current_branch = branch_node if branch_node && !branch_node.nop? + branch_node.try &.accept(self) end false diff --git a/src/ameba/rule/shadowing_local_outer_var.cr b/src/ameba/rule/shadowing_local_outer_var.cr new file mode 100644 index 00000000..3f9fe164 --- /dev/null +++ b/src/ameba/rule/shadowing_local_outer_var.cr @@ -0,0 +1,62 @@ +module Ameba::Rule + # A rule that disallows the usage of the same name as outer local variables + # for block or proc arguments. + # + # For example, this is considered incorrect: + # + # ``` + # def some_method + # foo = 1 + # + # 3.times do |foo| # shadowing outer `foo` + # end + # end + # ``` + # + # and should be written as: + # + # ``` + # def some_method + # foo = 1 + # + # 3.times do |bar| + # end + # end + # ``` + # + # YAML configuration example: + # + # ``` + # ShadowingOuterLocalVar: + # Enabled: true + # ``` + # + struct ShadowingOuterLocalVar < Base + properties do + description "Disallows the usage of the same name as outer local variables" \ + " for block or proc arguments." + end + + MSG = "Shadowing outer local variable `%s`" + + def test(source) + AST::ScopeVisitor.new self, source + end + + def test(source, node : Crystal::ProcLiteral, scope : AST::Scope) + find_shadowing source, scope + end + + def test(source, node : Crystal::Block, scope : AST::Scope) + find_shadowing source, scope + end + + private def find_shadowing(source, scope) + scope.arguments.each do |arg| + if scope.outer_scope.try &.find_variable(arg.name) + source.error self, arg.location, MSG % arg.name + end + end + end + end +end