From 2813326a82cd055e92a0bf693001af45dde9d7ee Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Tue, 18 Aug 2015 11:09:41 -0400 Subject: [PATCH 1/4] add spec for cyclomatic complexity issues --- spec/cc/engine/rubocop_spec.rb | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/spec/cc/engine/rubocop_spec.rb b/spec/cc/engine/rubocop_spec.rb index 145ef39f..d5449fc7 100644 --- a/spec/cc/engine/rubocop_spec.rb +++ b/spec/cc/engine/rubocop_spec.rb @@ -161,6 +161,46 @@ def method assert_equal location, result["location"] end + it "includes complete method body for cyclomatic complexity issue" do + create_source_file("my_script", <<-EORUBY) + #!/usr/bin/env ruby + + def method(a,b,c,d,e,f,g) + r = 1 + if a + if !b + if c + if !d + if e + if !f + (1..g).each do |n| + r = (r * n) - n + end + end + end + end + end + end + end + r + end + EORUBY + output = run_engine + assert includes_check?(output, "Metrics/CyclomaticComplexity") + + json = JSON.parse('[' + output.split("\u0000").join(',') + ']') + + result = json.select { |i| i && i["check_name"] =~ /Metrics\/CyclomaticComplexity/ }.first + location = { + "path" => "my_script", + "positions" => { + "begin" => { "column"=>11, "line"=>3 }, + "end" => { "column"=>14, "line"=>21 } + } + } + assert_equal location, result["location"] + end + def includes_check?(output, cop_name) issues = output.split("\0").map { |x| JSON.parse(x) } From 00b63edf2098908ffad9ea23203032ace75c0b41 Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Tue, 18 Aug 2015 12:16:08 -0400 Subject: [PATCH 2/4] Monkey Patch Rubocop to fix complexity report location This is a horrible no good very bad take at fixing the issue with being unable to see method source in reports for cyclomatic complexity issues, but the alternative is a major pain, so I decided to at least try this first to confirm it really fixes the issue. So, the underlying reason that cyclomatic complexity violations repoorted by RuboCop weren't coming through with the full method def (I think) is because for complexity violations RuboCop reports the `location` of the violation as just the method definition line, not the whole method definition. Getting the full method location is pretty easy if you have the AST node. Unfortunately, RuboCop doesn't actually keep the AST `node` attached to the `Offense` instances: they get dropped when `Cop` instances generate their `Offense` instances. So by the time Rubocop's results come back to our code, there is no AST `node` for us to look at: just a string buffer of file contents & a location pointing at a method name. So this monkey patch basically just overrides the `Offense` creation on complexity cops to pass along the location as the whole method def instead of just the method def line. The alternative, to do it in our own code would, I think be: 1. Parse the string buffer ourself ourself to get an AST 2. Descend into AST to find node corresponding to the location RuboCop gave us 3. Map that node back to a location of the entire method. --- lib/cc/engine/rubocop.rb | 1 + lib/rubocop/cop/patch_method_complexity.rb | 10 ++++++++++ 2 files changed, 11 insertions(+) create mode 100644 lib/rubocop/cop/patch_method_complexity.rb diff --git a/lib/cc/engine/rubocop.rb b/lib/cc/engine/rubocop.rb index 49d92d5a..109a8384 100644 --- a/lib/cc/engine/rubocop.rb +++ b/lib/cc/engine/rubocop.rb @@ -1,5 +1,6 @@ require "json" require "pathname" +require "rubocop/cop/patch_method_complexity" require "rubocop" require "cc/engine/category_parser" diff --git a/lib/rubocop/cop/patch_method_complexity.rb b/lib/rubocop/cop/patch_method_complexity.rb new file mode 100644 index 00000000..43b35252 --- /dev/null +++ b/lib/rubocop/cop/patch_method_complexity.rb @@ -0,0 +1,10 @@ +# Monkey patching! Here be dragons. +module RuboCop + module Cop + module MethodComplexity + def add_offense(node, loc, message = nil, severity = nil) + super(node, node.loc, message, severity) + end + end + end +end From 66a1a62e3634759109ca77c51f5bba62b690fcc0 Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Tue, 18 Aug 2015 13:43:53 -0400 Subject: [PATCH 3/4] rename file --- lib/cc/engine/rubocop.rb | 2 +- .../{patch_method_complexity.rb => method_complexity_patch.rb} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename lib/rubocop/cop/{patch_method_complexity.rb => method_complexity_patch.rb} (100%) diff --git a/lib/cc/engine/rubocop.rb b/lib/cc/engine/rubocop.rb index 109a8384..49173168 100644 --- a/lib/cc/engine/rubocop.rb +++ b/lib/cc/engine/rubocop.rb @@ -1,6 +1,6 @@ require "json" require "pathname" -require "rubocop/cop/patch_method_complexity" +require "rubocop/cop/method_complexity_patch" require "rubocop" require "cc/engine/category_parser" diff --git a/lib/rubocop/cop/patch_method_complexity.rb b/lib/rubocop/cop/method_complexity_patch.rb similarity index 100% rename from lib/rubocop/cop/patch_method_complexity.rb rename to lib/rubocop/cop/method_complexity_patch.rb From 5b079a37a213d1ef90d82ecccbe759df6f951ba4 Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Tue, 18 Aug 2015 14:31:00 -0400 Subject: [PATCH 4/4] explain the monkey patching --- lib/rubocop/cop/method_complexity_patch.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/rubocop/cop/method_complexity_patch.rb b/lib/rubocop/cop/method_complexity_patch.rb index 43b35252..e16a345a 100644 --- a/lib/rubocop/cop/method_complexity_patch.rb +++ b/lib/rubocop/cop/method_complexity_patch.rb @@ -2,6 +2,14 @@ module RuboCop module Cop module MethodComplexity + # RuboCop's default implementation of `add_offense` in `Cop` only gets the + # location of the keyword associated with the problem which, for things + # like complexity checkers, is just the method def line. This isn't very + # useful for checkers where the entire method body is relevant. Fetching + # this information from an `Offense` instance is difficult, since the + # original AST is no longer available. So it's easier to monkey-path + # this method on complexity checkers to send the location of the entire + # method to the created `Offense`. def add_offense(node, loc, message = nil, severity = nil) super(node, node.loc, message, severity) end