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.

Author: wfleming

lib/cc/engine/analyzers/sexp_lines.rb

@@ -0,0 +1,32 @@
+module CC
+  module Engine
+    module Analyzers
+      class SexpLines
+        attr_reader :begin_line, :end_line
+
+        def initialize(root_sexp)
+          @root_sexp = root_sexp
+          calculate
+        end
+
+        def to_h
+          {"begin": begin_line, "end": end_line}
+        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

lib/cc/engine/analyzers/violation.rb

@@ -71,23 +71,10 @@ def format_other_locations
         def format_sexp(sexp)
           {
             "path": sexp.file.gsub(%r(^./), ""),
-            "lines": {
-              "begin": sexp.line,
-              "end": sexp.end_line || sexp_max_line(sexp, sexp.line)
-            }
+            "lines": SexpLines.new(sexp).to_h,
           }
         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

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'
 

spec/cc/engine/analyzers/sexp_lines_spec.rb

@@ -0,0 +1,48 @@
+require "spec_helper"
+require "cc/engine/duplication"
+
+module CC::Engine::Analyzers
+  RSpec.describe SexpLines do
+    let(:source) do
+      <<-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
+    end
+    let(:flay) do
+      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,
+      })
+    end
+
+    describe "violation location" do
+      it "gets appropriate locations for rescue blocks" do
+        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].to_h).not_to eq locations[1].to_h
+        expect(locations[0].to_h).to eq({:begin => 3, :end => 7})
+        expect(locations[1].to_h).to eq({:begin => 5, :end => 7})
+      end
+    end
+  end
+end