From e850bff60f082cd3a252a19a63688c8b2f1e2e74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=A4ufler?= Date: Sat, 16 Feb 2019 20:03:44 +0100 Subject: [PATCH] Hk cyclomatic complexity (#92) * Proof of concept for cyclomatic complexity * Enable configurability of rule * Use the same nodes to increment the complexity as rubocop * Fix typo in test description * Properly indent code and simplify macro * Move metric into metrics * Cover a violation supressed by increased threshold * Extract visitor into its own file * Document cyclomatic complexity rule and visitor * Refactor specs to use a macro * Indent code inside macro * Replace array with tuple for string formatting. `Tuple` is stack based, whereas `Array` is allocated on the heap increasing GC pressure. * Fix formatting * Enable cyclomatic complexity rule by default --- .../ast/visitors/counting_visitor_spec.cr | 39 +++++++++++++++ .../metrics/cyclomatic_complexity_spec.cr | 47 +++++++++++++++++++ src/ameba/ast/visitors/counting_visitor.cr | 31 ++++++++++++ .../rule/metrics/cyclomatic_complexity.cr | 32 +++++++++++++ 4 files changed, 149 insertions(+) create mode 100644 spec/ameba/ast/visitors/counting_visitor_spec.cr create mode 100644 spec/ameba/rule/metrics/cyclomatic_complexity_spec.cr create mode 100644 src/ameba/ast/visitors/counting_visitor.cr create mode 100644 src/ameba/rule/metrics/cyclomatic_complexity.cr diff --git a/spec/ameba/ast/visitors/counting_visitor_spec.cr b/spec/ameba/ast/visitors/counting_visitor_spec.cr new file mode 100644 index 00000000..f870be71 --- /dev/null +++ b/spec/ameba/ast/visitors/counting_visitor_spec.cr @@ -0,0 +1,39 @@ +require "../../../spec_helper" + +module Ameba::AST + describe CountingVisitor do + describe "#visit" do + it "allow to visit ASTNode" do + node = Crystal::Parser.new("").parse + visitor = CountingVisitor.new node + node.accept visitor + end + end + + describe "#count" do + it "is 1 for an empty method" do + node = Crystal::Parser.new("def hello; end").parse + visitor = CountingVisitor.new node + + visitor.count.should eq 1 + end + + {% for pair in [ + {code: "if true; end", description: "conditional"}, + {code: "while true; end", description: "while loop"}, + {code: "until 1 < 2; end", description: "until loop"}, + {code: "begin; rescue; end", description: "rescue"}, + {code: "case 1 when 1; end", description: "when"}, + {code: "true || false", description: "or"}, + {code: "true && false", description: "and"}, + ] %} + it "increases count for every {{ pair[:description].id }}" do + node = Crystal::Parser.new("def hello; {{ pair[:code].id }} end").parse + visitor = CountingVisitor.new node + + visitor.count.should eq 2 + end + {% end %} + end + end +end diff --git a/spec/ameba/rule/metrics/cyclomatic_complexity_spec.cr b/spec/ameba/rule/metrics/cyclomatic_complexity_spec.cr new file mode 100644 index 00000000..3d771127 --- /dev/null +++ b/spec/ameba/rule/metrics/cyclomatic_complexity_spec.cr @@ -0,0 +1,47 @@ +require "../../../spec_helper" + +module Ameba::Rule::Metrics + subject = CyclomaticComplexity.new + complex_method = <<-CODE + def hello(a, b, c) + if a && b && c + begin + while true + return if false && b + end + "" + rescue + "" + end + end + end + CODE + + describe CyclomaticComplexity do + it "passes for empty methods" do + source = Source.new %( + def hello + end + ) + subject.catch(source).should be_valid + end + + it "reports one issue for a complex method" do + subject.max_complexity = 5 + source = Source.new(complex_method, "source.cr") + subject.catch(source).should_not be_valid + + issue = source.issues.first + issue.rule.should eq subject + issue.location.to_s.should eq "source.cr:1:1" + issue.end_location.to_s.should eq "source.cr:12:3" + issue.message.should eq "Cyclomatic complexity too high [8/5]" + end + + it "doesn't report an issue for an increased threshold" do + subject.max_complexity = 100 + source = Source.new(complex_method, "source.cr") + subject.catch(source).should be_valid + end + end +end diff --git a/src/ameba/ast/visitors/counting_visitor.cr b/src/ameba/ast/visitors/counting_visitor.cr new file mode 100644 index 00000000..32999bec --- /dev/null +++ b/src/ameba/ast/visitors/counting_visitor.cr @@ -0,0 +1,31 @@ +module Ameba::AST + # AST Visitor that counts occurences of certain keywords + class CountingVisitor < Crystal::Visitor + @complexity = 1 + + # Creates a new counting visitor + def initialize(@scope : Crystal::ASTNode) + end + + # :nodoc: + def visit(node : Crystal::ASTNode) + true + end + + # Returns the number of keywords that were found in the node + def count + @scope.accept(self) + @complexity + end + + # Uses the same logic than rubocop. See + # https://github.com/rubocop-hq/rubocop/blob/master/lib/rubocop/cop/metrics/cyclomatic_complexity.rb#L21 + # Except "for", because crystal doesn't have a "for" loop. + {% for node in %i(if while until rescue when or and) %} + # :nodoc: + def visit(node : Crystal::{{ node.id.capitalize }}) + @complexity += 1 + end + {% end %} + end +end diff --git a/src/ameba/rule/metrics/cyclomatic_complexity.cr b/src/ameba/rule/metrics/cyclomatic_complexity.cr new file mode 100644 index 00000000..9c1e0f89 --- /dev/null +++ b/src/ameba/rule/metrics/cyclomatic_complexity.cr @@ -0,0 +1,32 @@ +module Ameba::Rule::Metrics + # A rule that disallows methods with a cyclomatic complexity higher than `MaxComplexity` + # + # YAML configuration example: + # + # ``` + # Metrics/CyclomaticComplexity: + # Enabled: true + # MaxComplexity: 10 + # ``` + # + struct CyclomaticComplexity < Base + properties do + description "Disallows methods with a cyclomatic complexity higher than `MaxComplexity`" + max_complexity 10 + end + + MSG = "Cyclomatic complexity too high [%d/%d]" + + def test(source) + AST::NodeVisitor.new self, source + end + + def test(source, node : Crystal::Def) + complexity = AST::CountingVisitor.new(node).count + + if complexity > max_complexity + issue_for(node, MSG % {complexity, max_complexity}) + end + end + end +end