Merge pull request rails#53763 from martinemde/martinemde/highlight-multiline-template-errors-in-blocks

Improve error highlighting of multiline methods in ERB templates

Author: byroot

actionpack/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   In ExceptionWrapper, match backtrace lines with built templates more often,
+    allowing improved highlighting of errors within do-end blocks in templates.
+    Fix for Ruby 3.4 to match new method labels in backtrace.
+
+    *Martin Emde*
+
 *   Allow setting content type with a symbol of the Mime type.
 
     ```ruby

actionpack/lib/action_dispatch/middleware/exception_wrapper.rb

@@ -261,13 +261,13 @@ def build_backtrace
         end
 
         (@exception.backtrace_locations || []).map do |loc|
-          if built_methods.key?(loc.label.to_s)
+          if built_methods.key?(loc.base_label)
             thread_backtrace_location = if loc.respond_to?(:__getobj__)
               loc.__getobj__
             else
               loc
             end
-            SourceMapLocation.new(thread_backtrace_location, built_methods[loc.label.to_s])
+            SourceMapLocation.new(thread_backtrace_location, built_methods[loc.base_label])
           else
             loc
           end

actionpack/test/dispatch/exception_wrapper_test.rb

@@ -28,6 +28,24 @@ def backtrace
       end
     end
 
+    class TestTemplate
+      attr_reader :method_name
+
+      def initialize(method_name)
+        @method_name = method_name
+      end
+
+      def spot(backtrace_location)
+        { first_lineno: 1, script_lines: ["compiled @ #{backtrace_location.base_label}:#{backtrace_location.lineno}"] }
+      end
+
+      def translate_location(backtrace_location, spot)
+        # note: extract_source_fragment_lines pulls lines from script_lines for indexes near first_lineno
+        # since we're mocking the behavior, we need to leave the first_lineno close to 1
+        { first_lineno: 1, script_lines: ["translated @ #{backtrace_location.base_label}:#{backtrace_location.lineno}"] }
+      end
+    end
+
     setup do
       @cleaner = ActiveSupport::BacktraceCleaner.new
       @cleaner.remove_filters!
@@ -70,7 +88,7 @@ def backtrace
     end
 
     class_eval "def throw_syntax_error; eval %(
-      'abc' + pluralize 'def'
+      pluralize { # create a syntax error without a parser warning
     ); end", "lib/file.rb", 42
 
     test "#source_extracts works with eval syntax error" do
@@ -79,8 +97,8 @@ def backtrace
       wrapper = ExceptionWrapper.new(nil, TopErrorProxy.new(exception, 1))
 
       assert_called_with(wrapper, :source_fragment, ["lib/file.rb", 42], returns: "foo") do
-       assert_equal [ code: "foo", line_number: 42 ], wrapper.source_extracts
-     end
+        assert_equal [ code: "foo", line_number: 42 ], wrapper.source_extracts
+      end
     end
 
     test "#source_extracts works with nil backtrace_locations" do
@@ -108,6 +126,62 @@ def backtrace
       assert_equal({ code: code, line_number: lineno + 2 }, wrapper.source_extracts.first)
     end
 
+    class_eval "def _app_views_tests_show_html_erb;
+      raise TestError; end", "app/views/tests/show.html.erb", 2
+
+    test "#source_extracts wraps template lines in a SourceMapLocation" do
+      exception = begin _app_views_tests_show_html_erb; rescue TestError => ex; ex; end
+
+      template = TestTemplate.new("_app_views_tests_show_html_erb")
+      resolver = Data.define(:built_templates).new(built_templates: [template])
+
+      wrapper = nil
+      assert_called(ActionView::PathRegistry, :all_resolvers, nil, returns: [resolver]) do
+        wrapper = ExceptionWrapper.new(nil, TopErrorProxy.new(exception, 1))
+      end
+
+      assert_equal [{
+        code: { 1 => "translated @ _app_views_tests_show_html_erb:3" },
+        line_number: 1
+      }], wrapper.source_extracts
+    end
+
+    class_eval "def _app_views_tests_nested_html_erb;
+      [1].each do
+        [2].each do
+          raise TestError
+        end
+      end
+    end", "app/views/tests/nested.html.erb", 2
+
+    test "#source_extracts works with nested template code" do
+      exception = begin _app_views_tests_nested_html_erb; rescue TestError => ex; ex; end
+
+      template = TestTemplate.new("_app_views_tests_nested_html_erb")
+      resolver = Data.define(:built_templates).new(built_templates: [template])
+
+      wrapper = nil
+      assert_called(ActionView::PathRegistry, :all_resolvers, nil, returns: [resolver]) do
+        wrapper = ExceptionWrapper.new(nil, TopErrorProxy.new(exception, 5))
+      end
+
+      extracts = wrapper.source_extracts
+      assert_equal({
+        code: { 1 => "translated @ _app_views_tests_nested_html_erb:5" },
+        line_number: 1
+      }, extracts[0])
+      # extracts[1] is Array#each (unreliable backtrace across rubies)
+      assert_equal({
+        code: { 1 => "translated @ _app_views_tests_nested_html_erb:4" },
+        line_number: 1
+      }, extracts[2])
+      # extracts[3] is Array#each (unreliable backtrace across rubies)
+      assert_equal({
+        code: { 1 => "translated @ _app_views_tests_nested_html_erb:3" },
+        line_number: 1
+      }, extracts[4])
+    end
+
     test "#application_trace returns traces only from the application" do
       exception = begin index; rescue TestError => ex; ex; end
       wrapper = ExceptionWrapper.new(@cleaner, TopErrorProxy.new(exception, 1))

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
@@ -152,8 +176,9 @@ def find_offset(compiled, source_tokens, error_column)
 
         def offset_source_tokens(source_tokens)
           source_offset = 0
-          with_offset = source_tokens.filter_map do |(name, str)|
-            result = [name, str, source_offset] if name == :CODE || name == :TEXT
+          with_offset = source_tokens.filter_map do |name, str|
+            result = [:CODE, str, source_offset] if name == :CODE || name == :PLAIN
+            result = [:TEXT, str, source_offset] if name == :TEXT
             source_offset += str.bytesize
             result
           end

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

activesupport/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Alter `ERB::Util.tokenize` to return :PLAIN token with full input string when string doesn't contain ERB tags.
+
+    *Martin Emde*
+
 *   Fix a bug in `ERB::Util.tokenize` that causes incorrect tokenization when ERB tags are preceeded by multibyte characters.
 
     *Martin Emde*

activesupport/lib/active_support/core_ext/erb/util.rb

@@ -169,7 +169,7 @@ def self.tokenize(source) # :nodoc:
       while !source.eos?
         pos = source.pos
         source.scan_until(/(?:#{start_re}|#{finish_re})/)
-        raise NotImplementedError if source.matched.nil?
+        return [[:PLAIN, source.string]] unless source.matched?
         len = source.pos - source.matched.bytesize - pos
 
         case source.matched

activesupport/lib/active_support/syntax_error_proxy.rb

@@ -18,6 +18,9 @@ def spot(_)
 
       def label
       end
+
+      def base_label
+      end
     end
 
     class BacktraceLocationProxy < DelegateClass(Thread::Backtrace::Location) # :nodoc: