Improve error template highlighting when backtrace lineno is wrong

Author: martinemde

actionview/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Improve error highlighting of multi-line methods in ERB templates or
+    templates where the error occurs within a do-end block.
+
+    *Martin Emde*
+
 *   Fix a crash in ERB template error highlighting when the error occurs on a
     line in the compiled template that is past the end of the source template.
 

actionview/lib/action_view/template/handlers/erb.rb

@@ -40,17 +40,19 @@ def handles_encoding?
 
         # Translate an error location returned by ErrorHighlight to the correct
         # source location inside the template.
-        def translate_location(spot, backtrace_location, source)
-          # Tokenize the source line
+        def translate_location(spot, _backtrace_location, source)
+          compiled = spot[:script_lines]
+          highlight = compiled[spot[:first_lineno] - 1]&.byteslice((spot[:first_column] - 1)...spot[:last_column])
+          return nil if highlight.blank?
+
           source_lines = source.lines
-          return nil if source_lines.size < backtrace_location.lineno
-          tokens = ::ERB::Util.tokenize(source_lines[backtrace_location.lineno - 1])
-          new_first_column = find_offset(spot[:snippet], tokens, spot[:first_column])
-          lineno_delta = spot[:first_lineno] - backtrace_location.lineno
+          lineno_delta = find_lineno_offset(compiled, source_lines, highlight, spot[:first_lineno])
+
+          tokens = ::ERB::Util.tokenize(source_lines[spot[:first_lineno] - lineno_delta - 1])
+          column_delta = find_offset(spot[:snippet], tokens, spot[:first_column])
+
           spot[:first_lineno] -= lineno_delta
           spot[:last_lineno] -= lineno_delta
-
-          column_delta = spot[:first_column] - new_first_column
           spot[:first_column] -= column_delta
           spot[:last_column] -= column_delta
           spot[:script_lines] = source_lines
@@ -107,6 +109,28 @@ def valid_encoding(string, encoding)
           raise WrongEncodingError.new(string, string.encoding)
         end
 
+        # Return the offset between the error lineno and the source lineno.
+        # Searches in reverse from the backtrace lineno so we have a better
+        # chance of finding the correct line
+        #
+        # The compiled template is likely to be longer than the source.
+        # Use the difference between the compiled and source sizes to
+        # determine the earliest line that could contain the highlight.
+        def find_lineno_offset(compiled, source_lines, highlight, error_lineno)
+          first_index = error_lineno - 1 - compiled.size + source_lines.size
+          first_index = 0 if first_index < 0
+
+          last_index = error_lineno - 1
+          last_index = source_lines.size - 1 if last_index >= source_lines.size
+
+          last_index.downto(first_index) do |line_index|
+            next unless source_lines[line_index].include?(highlight)
+            return error_lineno - 1 - line_index
+          end
+
+          raise LocationParsingError, "Couldn't find code snippet"
+        end
+
         # Find which token in the source template spans the byte range that
         # contains the error_column, then return the offset compared to the
         # original source template.
@@ -137,7 +161,7 @@ def find_offset(compiled, source_tokens, error_column)
                 matched_str = true
 
                 if name == :CODE && compiled.pos <= error_column && compiled.pos + str.bytesize >= error_column
-                  return error_column - compiled.pos + offset
+                  return compiled.pos - offset
                 end
 
                 compiled.pos += str.bytesize

actionview/test/template/template_test.rb

@@ -67,8 +67,8 @@ def render(implicit_locals: [], **locals)
 
   def spot_highlight(compiled, highlight, first_column: nil, **options)
     # rindex by default since our tests usually put the highlight last
-    first_column ||= compiled.rindex(highlight) || 999
-    last_column = first_column + highlight.size
+    first_column ||= compiled.byterindex(highlight) || 999
+    last_column = first_column + highlight.bytesize
     spot = {
       first_column:, last_column:, snippet: compiled,
       first_lineno: 1, last_lineno: 1, script_lines: compiled.lines,
@@ -332,121 +332,99 @@ def test_template_translate_location
     source = "<%= nomethoderror %>"
     compiled = "'.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight)
     expected = spot_highlight(source, highlight, snippet: compiled)
 
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 
-  # merely tests for the case where the backtrace and spot disagree about lineno
-  def test_template_translate_location_lineno_offset
-    highlight = "nomethoderror"
-    source = "<%= nomethoderror %>"
-    compiled = "'.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='"
-
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
-    spot = spot_highlight(compiled, highlight, first_lineno: 2, last_lineno: 2)
-    expected = spot_highlight(source, highlight, snippet: compiled)
-
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
-  end
-
-  # We are testing the failure case here. `find_offset` doesn't correctly handle the case
-  # where the line number is not the same in the backtrace and template.
   def test_template_translate_location_with_multiline_code_source
     highlight = "nomethoderror"
     source = "<%=\ngood(\n nomethoderror\n) %>"
     extracted_line = " nomethoderror\n"
-    compiled = "ValidatedOutputBuffer.wrap(@output_buffer, ({}), ' \ngood(\n nomethoderror\n" \
-               ") '.freeze, true).safe_none_append=( \ngood(\n nomethoderror\n) );\n@output_buffer"
+    compiled = "ValidatedOutputBuffer.wrap(@output_buffer, ({}), '\ngood(\n nomethoderror\n) '.freeze, true).safe_none_append=(\ngood(\n nomethoderror\n) );\n@output_buffer"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 6)
     spot = spot_highlight(compiled, highlight, first_column: 1, first_lineno: 6, last_lineno: 6, snippet: extracted_line)
+    expected = spot_highlight(source, highlight, first_column: 1, first_lineno: 3, last_lineno: 3, snippet: extracted_line)
 
-    assert_equal spot, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 
   def test_template_translate_location_with_multibye_string_before_highlight
-    highlight = "nomethoderror"
-    source = String.new("\u{a5}<%= nomethoderror %>", encoding: Encoding::UTF_8) # yen symbol
-    compiled = String.new("\u{a5}'.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n", encoding: Encoding::UTF_8)
+    highlight = "nope"
+    # ensure the byte offset is enough to make us miss the highlight if wrong
+    multibyte = String.new("\u{a5}\u{a5}\u{a5}\u{a5}\u{a5}\u{a5}\u{a5}", encoding: Encoding::UTF_8) # yen symbol
+    source = "#{multibyte}<%= nope %>"
+    compiled = "#{multibyte}'.freeze; @output_buffer.append=  nope ; @output_buffer.safe_append='\n"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight)
     expected = spot_highlight(source, highlight, snippet: compiled)
 
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 
   def test_template_translate_location_no_match_in_compiled
     highlight = "nomatch"
     source = "<%= nomatch %>"
     compiled = "this source does not contain the highlight, so the original spot is returned"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight, first_column: 50)
 
-    assert_equal spot, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal spot, new_template(source).translate_location(nil, spot)
   end
 
   def test_template_translate_location_text_includes_highlight
     highlight = "nomethoderror"
     source = " nomethoderror <%= nomethoderror %>"
     compiled = " nomethoderror '.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight)
     expected = spot_highlight(source, highlight, snippet: compiled)
 
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 
   def test_template_translate_location_space_separated_erb_tags
     highlight = "nomethoderror"
     source = "<%= goodcode %> <%= nomethoderror %>"
     compiled = "'.freeze; @output_buffer.append=  goodcode ; @output_buffer.safe_append=' '.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight)
     expected = spot_highlight(source, highlight, snippet: compiled)
 
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 
   def test_template_translate_location_consecutive_erb_tags
     highlight = "nomethoderror"
     source = "<%= goodcode %><%= nomethoderror %>"
     compiled = "'.freeze; @output_buffer.append=  goodcode ; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight)
     expected = spot_highlight(source, highlight, snippet: compiled)
 
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 
   def test_template_translate_location_repeated_highlight_in_compiled_template
     highlight = "nomethoderror"
     source = "<%= nomethoderror %>"
     compiled = "ValidatedOutputBuffer.wrap(@output_buffer, ({}), ' nomethoderror '.freeze, true).safe_none_append=  nomethoderror ; @output_buffer.safe_append='\n"
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight)
     expected = spot_highlight(source, highlight, snippet: compiled)
 
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 
   def test_template_translate_location_flaky_pathological_template
     highlight = "flakymethod"
     source = "<%= flakymethod %> flakymethod <%= flakymethod " # fails on second call, no tailing %>
     compiled = "ValidatedOutputBuffer.wrap(@output_buffer, ({}), ' flakymethod '.freeze, true).safe_none_append=( flakymethod );@output_buffer.safe_append=' flakymethod '.freeze;ValidatedOutputBuffer.wrap(@output_buffer, ({}), ' flakymethod '.freeze, true).safe_none_append=( flakymethod "
 
-    backtrace_location = Data.define(:lineno).new(lineno: 1)
     spot = spot_highlight(compiled, highlight)
     expected = spot_highlight(source, highlight, snippet: compiled)
 
-    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+    assert_equal expected, new_template(source).translate_location(nil, spot)
   end
 end