From a86869c23c46ce01511495f1cf01e9b41c75aeea Mon Sep 17 00:00:00 2001
From: Will Fleming <will@flemi.ng>
Date: Tue, 17 Nov 2015 22:27:00 -0500
Subject: [PATCH] improve derivation of lines from Sexp

This started as a hunt into why an issue was reported for just a single
line saying `end`, with the only other instance of the duplication being
the same line: the source code in the spec is a reduced instance of the
triggering code.

The poor behavior results from how RubyParser/Sexp handle certain
structures: the "line" of the `rescue..end` expression is parsed as the
location of the `end`. This seems wrong, but I can't figure out a quick
fix within those libraries.

The way child sexprs get lines seems generally sane: at the very small level,
lines seem pretty reliable. Strange behavior emerges at the higher
level. The way we were calculating the end line of an sexpr already
implicitly acknowledged this by walking the whole tree to find the
highest line.

This change adapts that previous algorithm to apply the same principle
to finding both the start & end lines: the start line is basically the
smallest line of the sexpr or any of its children, and the end line is
the maximum.
---
 lib/cc/engine/analyzers/sexp_lines.rb       | 28 ++++++++++++++
 lib/cc/engine/analyzers/violation.rb        | 17 ++------
 lib/cc/engine/duplication.rb                |  1 +
 spec/cc/engine/analyzers/sexp_lines_spec.rb | 43 +++++++++++++++++++++
 4 files changed, 76 insertions(+), 13 deletions(-)
 create mode 100644 lib/cc/engine/analyzers/sexp_lines.rb
 create mode 100644 spec/cc/engine/analyzers/sexp_lines_spec.rb

diff --git a/lib/cc/engine/analyzers/sexp_lines.rb b/lib/cc/engine/analyzers/sexp_lines.rb
new file mode 100644
index 00000000..6871ce31
--- /dev/null
+++ b/lib/cc/engine/analyzers/sexp_lines.rb
@@ -0,0 +1,28 @@
+module CC
+  module Engine
+    module Analyzers
+      class SexpLines
+        attr_reader :begin_line, :end_line
+
+        def initialize(root_sexp)
+          @root_sexp = root_sexp
+          calculate
+        end
+
+        private
+
+        attr_reader :root_sexp
+
+        def calculate
+          @begin_line = root_sexp.line
+          @end_line = root_sexp.end_line || root_sexp.line
+
+          root_sexp.deep_each do |sexp|
+            @begin_line = [@begin_line, sexp.line].min
+            @end_line = [@end_line, sexp.end_line || sexp.line].max
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/cc/engine/analyzers/violation.rb b/lib/cc/engine/analyzers/violation.rb
index ae68eac3..b562550a 100644
--- a/lib/cc/engine/analyzers/violation.rb
+++ b/lib/cc/engine/analyzers/violation.rb
@@ -69,25 +69,16 @@ def format_other_locations
         end
 
         def format_sexp(sexp)
+          lines = SexpLines.new(sexp)
           {
             "path": sexp.file.gsub(%r(^./), ""),
             "lines": {
-              "begin": sexp.line,
-              "end": sexp.end_line || sexp_max_line(sexp, sexp.line)
-            }
+              "begin": lines.begin_line,
+              "end": lines.end_line,
+            },
           }
         end
 
-        def sexp_max_line(sexp_tree, default)
-          max = default
-
-          sexp_tree.deep_each do |sexp|
-            max = sexp.line if sexp.line > max
-          end
-
-          max
-        end
-
         def content_body
           @_content_body ||= { "body": File.read(read_up_path) }
         end
diff --git a/lib/cc/engine/duplication.rb b/lib/cc/engine/duplication.rb
index fcc339b5..803b5e15 100644
--- a/lib/cc/engine/duplication.rb
+++ b/lib/cc/engine/duplication.rb
@@ -6,6 +6,7 @@
 require 'cc/engine/analyzers/reporter'
 require 'cc/engine/analyzers/engine_config'
 require 'cc/engine/analyzers/sexp'
+require 'cc/engine/analyzers/sexp_lines'
 require 'flay'
 require 'json'
 
diff --git a/spec/cc/engine/analyzers/sexp_lines_spec.rb b/spec/cc/engine/analyzers/sexp_lines_spec.rb
new file mode 100644
index 00000000..ef7fa0db
--- /dev/null
+++ b/spec/cc/engine/analyzers/sexp_lines_spec.rb
@@ -0,0 +1,43 @@
+require "spec_helper"
+require "cc/engine/duplication"
+
+module CC::Engine::Analyzers
+  RSpec.describe SexpLines do
+    describe "violation location" do
+      it "gets appropriate locations for rescue blocks" do
+        source = <<-SOURCE
+          begin
+            foo
+          rescue SyntaxError => e
+            Jekyll.logger.warn "YAML Exception reading \#{File.join(base, name)}: \#{e.message}"
+          rescue Exception => e
+            Jekyll.logger.warn "Error reading file \#{File.join(base, name)}: \#{e.message}"
+          end
+        SOURCE
+        flay = Flay.new({
+          diff: false,
+          mass: CC::Engine::Analyzers::Ruby::Main::DEFAULT_MASS_THRESHOLD,
+          summary: false,
+          verbose: false,
+          number: true,
+          timeout: 10,
+          liberal: false,
+          fuzzy: false,
+          only: nil,
+        })
+
+        sexp = RubyParser.new.process(source, "file.rb")
+        flay.process_sexp(sexp)
+        report = flay.analyze[0]
+        sexps = flay.hashes[report.structural_hash]
+        locations = sexps.map { |sexp| SexpLines.new(sexp) }
+
+        expect(locations.count).to eq 2
+        expect(locations[0].begin_line).to eq(3)
+        expect(locations[0].end_line).to eq(7)
+        expect(locations[1].begin_line).to eq(5)
+        expect(locations[1].end_line).to eq(7)
+      end
+    end
+  end
+end