From 3beb2aff3be712e44c34a588fbf35b79c0246ca5 Mon Sep 17 00:00:00 2001
From: Yusuke Endoh <mame@ruby-lang.org>
Date: Wed, 10 Aug 2022 18:16:25 +0900
Subject: [PATCH] Use error_highlight gem to locate the columns where an error
 was raised

This change incorporates to Rails a feature called error_highlight that
has been available since Ruby 3.1. This allow Rails' error report screen
to display the fine-grained location where an error occurred (not only a
line number but also beginning and end column numbers of the code
fragment).

For ErrorHighlight, see https://bugs.ruby-lang.org/issues/17930 in
detail.

To use error_highlight, ExceptionWrapper now prefers
`Exception#backtrace_locations` (since Ruby 2.1), which returns an array
of `Thread::Backtrace::Location`s, instead of `Exception#backtrace`.
This is because error_highlight requires `Thread::Backtrace::Location`
to locate the column where an error was raised.

Co-Authored-By: John Hawthorn <john@hawthorn.email>
Co-Authored-By: Jean Boussier <jean.boussier@gmail.com>
---
 Gemfile                                       |  7 ++
 Gemfile.lock                                  |  2 +
 .../middleware/debug_exceptions.rb            |  1 +
 .../middleware/exception_wrapper.rb           | 69 ++++++++++++++++---
 .../templates/rescues/_source.html.erb        |  9 ++-
 .../templates/rescues/diagnostics.html.erb    |  4 +-
 .../rescues/invalid_statement.html.erb        |  2 +-
 .../middleware/templates/rescues/layout.erb   | 12 ++++
 .../rescues/missing_template.html.erb         |  2 +-
 .../templates/rescues/template_error.html.erb |  2 +-
 .../test/dispatch/exception_wrapper_test.rb   | 19 +++++
 .../lib/active_support/backtrace_cleaner.rb   |  6 +-
 .../generators/rails/app/templates/Gemfile.tt |  4 ++
 13 files changed, 120 insertions(+), 19 deletions(-)

diff --git a/Gemfile b/Gemfile
index 8d80a0cd14a..9d06796df17 100644
--- a/Gemfile
+++ b/Gemfile
@@ -176,3 +176,10 @@ end
 
 gem "tzinfo-data", platforms: [:mingw, :mswin, :x64_mingw, :jruby]
 gem "wdm", ">= 0.1.0", platforms: [:mingw, :mswin, :x64_mingw, :mswin64]
+
+# The error_highlight gem only works on CRuby 3.1 or later.
+# Also, Rails depends on a new API available since error_highlight 0.4.0.
+# (Note that Ruby 3.1 bundles error_highlight 0.3.0.)
+if RUBY_VERSION >= "3.1"
+  gem "error_highlight", ">= 0.4.0", platforms: [:ruby]
+end
diff --git a/Gemfile.lock b/Gemfile.lock
index 1217c899779..969fe3847fb 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -205,6 +205,7 @@ GEM
       http_parser.rb (>= 0.6.0)
     em-socksify (0.3.2)
       eventmachine (>= 1.0.0.beta.4)
+    error_highlight (0.4.0)
     erubi (1.11.0)
     et-orbi (1.2.6)
       tzinfo
@@ -572,6 +573,7 @@ DEPENDENCIES
   debug (>= 1.1.0)
   delayed_job
   delayed_job_active_record
+  error_highlight (>= 0.4.0)
   google-cloud-storage (~> 1.11)
   image_processing (~> 1.2)
   importmap-rails
diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
index 2ac604d5e12..7d48eb703b1 100644
--- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
+++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb
@@ -121,6 +121,7 @@ module ActionDispatch
           trace_to_show: wrapper.trace_to_show,
           routes_inspector: routes_inspector(wrapper.exception),
           source_extracts: wrapper.source_extracts,
+          error_highlight_available: wrapper.error_highlight_available?,
           line_number: wrapper.line_number,
           file: wrapper.file
         )
diff --git a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
index 03190e7d29d..d3d725321fe 100644
--- a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
+++ b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
@@ -124,15 +124,18 @@ module ActionDispatch
 
     def source_extracts
       backtrace.map do |trace|
-        file, line_number = extract_file_and_line_number(trace)
-
-        {
-          code: source_fragment(file, line_number),
-          line_number: line_number
-        }
+        extract_source(trace)
       end
     end
 
+    def error_highlight_available?
+      # ErrorHighlight.spot with backtrace_location keyword is available since error_highlight 0.4.0
+      unless defined?(@@error_highlight_available)
+        @@error_highlight_available = defined?(ErrorHighlight) && Gem::Version.new(ErrorHighlight::VERSION) >= Gem::Version.new("0.4.0")
+      end
+      @@error_highlight_available
+    end
+
     def trace_to_show
       if traces["Application Trace"].empty? && rescue_template != "routing_error"
         "Full Trace"
@@ -147,7 +150,17 @@ module ActionDispatch
 
     private
       def backtrace
-        Array(@exception.backtrace)
+        backtrace_locations = @exception.backtrace_locations
+        backtrace = @exception.backtrace
+
+        if backtrace_locations && backtrace_locations.size == backtrace.size
+          # Prefer #backtrace_locations as it looks consistent with #backtrace
+          Array(backtrace_locations)
+        else
+          # Conservatively fallback to #backtrace as they are inconsistent;
+          # probably #set_backtrace is used somewhere?
+          Array(backtrace)
+        end
       end
 
       def causes_for(exception)
@@ -168,19 +181,55 @@ module ActionDispatch
         end
       end
 
+      def extract_source(trace)
+        if error_highlight_available?
+          spot = ErrorHighlight.spot(@exception, backtrace_location: trace)
+          if spot
+            line = spot[:first_lineno]
+            code = extract_source_fragment_lines(spot[:script_lines], line)
+
+            if line == spot[:last_lineno]
+              code[line] = [
+                code[line][0, spot[:first_column]],
+                code[line][spot[:first_column]...spot[:last_column]],
+                code[line][spot[:last_column]..-1],
+              ]
+            end
+
+            return {
+              code: code,
+              line_number: line
+            }
+          end
+        end
+
+        file, line_number = extract_file_and_line_number(trace)
+
+        {
+          code: source_fragment(file, line_number),
+          line_number: line_number
+        }
+      end
+
+      def extract_source_fragment_lines(source_lines, line)
+        start = [line - 3, 0].max
+        lines = source_lines.drop(start).take(6)
+        Hash[*(start + 1..(lines.count + start)).zip(lines).flatten]
+      end
+
       def source_fragment(path, line)
         return unless Rails.respond_to?(:root) && Rails.root
         full_path = Rails.root.join(path)
         if File.exist?(full_path)
           File.open(full_path, "r") do |file|
-            start = [line - 3, 0].max
-            lines = file.each_line.drop(start).take(6)
-            Hash[*(start + 1..(lines.count + start)).zip(lines).flatten]
+            extract_source_fragment_lines(file.each_line, line)
           end
         end
       end
 
       def extract_file_and_line_number(trace)
+        return [trace.path, trace.lineno] if Thread::Backtrace::Location === trace
+
         # Split by the first colon followed by some digits, which works for both
         # Windows and Unix path styles.
         file, line = trace.match(/^(.+?):(\d+).*$/, &:captures) || trace
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb
index 88a8e6ad833..d0d7a0663ed 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb
@@ -18,12 +18,19 @@
             </td>
 <td width="100%">
 <pre>
-<% source_extract[:code].each do |line, source| -%><div class="line<%= " active" if line == source_extract[:line_number] -%>"><%= source -%></div><% end -%>
+<% source_extract[:code].each do |line, source| -%>
+<div class="line<%= " active" if line == source_extract[:line_number] -%>"><% if source.is_a?(Array) -%><%= source[0] -%><span class="error_highlight"><%= source[1] -%></span><%= source[2] -%>
+<% else -%>
+<%= source -%>
+<% end -%></div><% end -%>
 </pre>
 </td>
           </tr>
         </table>
       </div>
+      <%- if defined?(ErrorHighlight) && !error_highlight_available -%>
+        <p class="error_highlight_tip">Tip: You may want to add <code>gem 'error_highlight', '&gt;= 0.4.0'</code> into your Gemfile, which will display the fine-grained error location.</p>
+      <%- end -%>
     </div>
   <% end %>
 <% end %>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
index a68eaab8d78..10f9077e33a 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb
@@ -11,7 +11,7 @@
   <%= render "rescues/message_and_suggestions", exception: @exception %>
   <%= render "rescues/actions", exception: @exception, request: @request %>
 
-  <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx, error_index: 0 %>
+  <%= render "rescues/source", source_extracts: @source_extracts, error_highlight_available: @error_highlight_available, show_source_idx: @show_source_idx, error_index: 0 %>
   <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show, error_index: 0 %>
 
   <% if @exception.cause %>
@@ -26,7 +26,7 @@
     </div>
 
     <div id="<%= wrapper.exception.object_id %>" class="hidden">
-      <%= render "rescues/source", source_extracts: wrapper.source_extracts, show_source_idx: wrapper.source_to_show_id, error_index: index %>
+      <%= render "rescues/source", source_extracts: wrapper.source_extracts, error_highlight_available: wrapper.error_highlight_available?, show_source_idx: wrapper.source_to_show_id, error_index: index %>
       <%= render "rescues/trace", traces: wrapper.traces, trace_to_show: wrapper.trace_to_show, error_index: index %>
     </div>
   <% end %>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb
index 3743994e483..15269f9aa2f 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb
@@ -18,7 +18,7 @@
     <% end %>
   </h2>
 
-  <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx %>
+  <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx, error_highlight_available: nil %>
   <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show %>
   <%= render template: "rescues/_request_and_response" %>
 </main>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb
index 19dbabe5a29..0a8face4839 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/layout.erb
@@ -148,6 +148,18 @@
       background-color: #FCC;
     }
 
+    .error_highlight {
+      display: inline-block;
+      background-color: #FF9;
+      text-decoration: #F00 wavy underline;
+    }
+
+    .error_highlight_tip {
+      color: #666;
+      padding: 2px 2px;
+      font-size: 10px;
+    }
+
     .button_to {
       display: inline-block;
       margin-top: 0.75em;
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb
index 576d2e35978..6b3916769b0 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb
@@ -5,7 +5,7 @@
 <main role="main" id="container">
   <h2><%= h @exception.message %></h2>
 
-  <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx %>
+  <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx, error_highlight_available: nil %>
   <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show %>
   <%= render template: "rescues/_request_and_response" %>
 </main>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb
index 8bf661ad22f..f73569599b0 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb
@@ -11,7 +11,7 @@
   </p>
   <pre><code><%= h @exception.message %></code></pre>
 
-  <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx %>
+  <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx, error_highlight_available: nil %>
 
   <p><%= @exception.sub_template_message %></p>
 
diff --git a/actionpack/test/dispatch/exception_wrapper_test.rb b/actionpack/test/dispatch/exception_wrapper_test.rb
index a3898559cec..5552588a10c 100644
--- a/actionpack/test/dispatch/exception_wrapper_test.rb
+++ b/actionpack/test/dispatch/exception_wrapper_test.rb
@@ -53,6 +53,25 @@ module ActionDispatch
       end
     end
 
+    if defined?(ErrorHighlight) && Gem::Version.new(ErrorHighlight::VERSION) >= Gem::Version.new("0.4.0")
+      test "#source_extracts works with error_highlight" do
+        lineno = __LINE__
+        begin
+          1.time
+        rescue NameError => exc
+        end
+
+        wrapper = ExceptionWrapper.new(nil, exc)
+
+        code = {}
+        File.foreach(__FILE__).to_a.drop(lineno - 1).take(6).each_with_index do |line, i|
+          code[lineno + i] = line
+        end
+        code[lineno + 2] = ["          1", ".time", "\n"]
+        assert_equal({ code: code, line_number: lineno + 2 }, wrapper.source_extracts.first)
+      end
+    end
+
     test "#application_trace returns traces only from the application" do
       exception = TestError.new(caller.prepend("lib/file.rb:42:in `index'"))
       wrapper = ExceptionWrapper.new(@cleaner, exception)
diff --git a/activesupport/lib/active_support/backtrace_cleaner.rb b/activesupport/lib/active_support/backtrace_cleaner.rb
index 3c054d3f395..9abf3fb10f9 100644
--- a/activesupport/lib/active_support/backtrace_cleaner.rb
+++ b/activesupport/lib/active_support/backtrace_cleaner.rb
@@ -106,7 +106,7 @@ module ActiveSupport
 
       def filter_backtrace(backtrace)
         @filters.each do |f|
-          backtrace = backtrace.map { |line| f.call(line) }
+          backtrace = backtrace.map { |line| f.call(line.to_s) }
         end
 
         backtrace
@@ -114,7 +114,7 @@ module ActiveSupport
 
       def silence(backtrace)
         @silencers.each do |s|
-          backtrace = backtrace.reject { |line| s.call(line) }
+          backtrace = backtrace.reject { |line| s.call(line.to_s) }
         end
 
         backtrace
@@ -123,7 +123,7 @@ module ActiveSupport
       def noise(backtrace)
         backtrace.select do |line|
           @silencers.any? do |s|
-            s.call(line)
+            s.call(line.to_s)
           end
         end
       end
diff --git a/railties/lib/rails/generators/rails/app/templates/Gemfile.tt b/railties/lib/rails/generators/rails/app/templates/Gemfile.tt
index 06b47ff016a..878b6d720ee 100644
--- a/railties/lib/rails/generators/rails/app/templates/Gemfile.tt
+++ b/railties/lib/rails/generators/rails/app/templates/Gemfile.tt
@@ -56,6 +56,10 @@ group :development do
 <%- end -%>
   # Speed up commands on slow machines / big apps [https://github.com/rails/spring]
   # gem "spring"
+
+<%- if RUBY_VERSION >= "3.1" -%>
+  gem "error_highlight", ">= 0.4.0", platforms: [:ruby]
+<%- end -%>
 end
 
 <%- if depends_on_system_test? -%>