From dbe495f8a948384206108691af2c221cdbe428bf Mon Sep 17 00:00:00 2001 From: Randy Stauner Date: Wed, 24 Jun 2026 11:23:56 -0700 Subject: [PATCH] Return nil instead of raising NotImplementedError for def/lambda/block args ErrorHighlight.spot defaults point_type to :args for ArgumentError. When an ArgumentError originates from argument binding (e.g. a missing required keyword), its first backtrace location maps to a def/lambda/block node. The spotter had no implementation for the :args point type on those nodes and did `raise NotImplementedError`. This is a regression from 0.7.0 (and in Ruby 4.0 compared to Ruby 3.4). ErrorHighlight.spot only rescues SyntaxError/SystemCallError/ArgumentError, and NotImplementedError is not a StandardError, so it escaped through CoreExt#generate_snippet -> Exception#detailed_message / #full_message and replaced the original exception. This is normally hidden because the VM's "missing keyword" message matches the keyword regex in generate_snippet and takes the safe :name branch, but it surfaces as soon as the ArgumentError is re-raised/wrapped with a custom message that falls into the :args else branch. Return nil (the documented "no spot" result) for these cases instead, matching the graceful-degradation behavior of the surrounding feature. The fix is applied consistently across both compilers: - prism: def_node / lambda_node / block_node - parse.y: DEFN / DEFS (raised NotImplementedError) and LAMBDA / ITER (silently returned a spot for :args, inconsistent with prism) All of these branches were introduced together by the experimental "wrong number of arguments" snippet feature and are only intended for point_type :name, so returning nil for any other point_type keeps the two parsers in sync. --- lib/error_highlight/base.rb | 21 +++++++++--- test/test_error_highlight.rb | 63 ++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/lib/error_highlight/base.rb b/lib/error_highlight/base.rb index 5fffe5e..e1a9c0e 100644 --- a/lib/error_highlight/base.rb +++ b/lib/error_highlight/base.rb @@ -235,17 +235,21 @@ def spot spot_op_cdecl when :DEFN - raise NotImplementedError if @point_type != :name + # There is nothing to highlight for the arguments of a method + # definition, so just return nil instead of raising. + return nil if @point_type != :name spot_defn when :DEFS - raise NotImplementedError if @point_type != :name + return nil if @point_type != :name spot_defs when :LAMBDA + return nil if @point_type != :name spot_lambda when :ITER + return nil if @point_type != :name spot_iter when :call_node @@ -294,7 +298,12 @@ def spot when :name prism_spot_def_for_name when :args - raise NotImplementedError + # There is nothing to highlight for the arguments of a method + # definition (e.g. an ArgumentError for a missing required keyword + # is spotted with point_type: :args). Return nil instead of raising + # NotImplementedError, which would otherwise escape + # Exception#detailed_message / #full_message. + return nil end when :lambda_node @@ -302,7 +311,8 @@ def spot when :name prism_spot_lambda_for_name when :args - raise NotImplementedError + # See the comment for :def_node above. + return nil end when :block_node @@ -310,7 +320,8 @@ def spot when :name prism_spot_block_for_name when :args - raise NotImplementedError + # See the comment for :def_node above. + return nil end end diff --git a/test/test_error_highlight.rb b/test/test_error_highlight.rb index 5f664de..5de70f5 100644 --- a/test/test_error_highlight.rb +++ b/test/test_error_highlight.rb @@ -1752,6 +1752,69 @@ def test_singleton_method_multiple_missing_keywords end end + def def_with_required_keyword(x:) + x + end + + def test_spot_with_args_point_type_on_def_node_returns_nil + # A method with a required keyword argument called without it raises an + # ArgumentError whose first backtrace location maps to the `def` node. + # ErrorHighlight.spot defaults point_type to :args for ArgumentError, and + # there is no way to highlight "the arguments" of a definition, so spot + # must return nil instead of raising NotImplementedError. + begin + def_with_required_keyword + rescue ArgumentError => exc + end + + assert_nil(ErrorHighlight.spot(exc)) + end + + def test_spot_with_args_point_type_on_lambda_node_returns_nil + l = lambda { |x:| x } + begin + l.call + rescue ArgumentError => exc + end + + assert_nil(ErrorHighlight.spot(exc)) + end + + define_method(:block_with_required_keyword) { |x:| x } + + def test_spot_with_args_point_type_on_block_node_returns_nil + begin + block_with_required_keyword + rescue ArgumentError => exc + end + + assert_nil(ErrorHighlight.spot(exc)) + end + + def test_detailed_message_does_not_raise_when_argument_error_is_rewrapped + # This reproduces a real-world crash: an ArgumentError originating from a + # missing required keyword (which maps to the `def` node) is re-raised with + # a custom message. The custom message no longer matches the keyword regex + # in CoreExt#generate_snippet, so the :args branch is taken. The spotter + # must not raise NotImplementedError (which is not a StandardError and would + # escape detailed_message / full_message). + begin + def_with_required_keyword + rescue ArgumentError => original + exc = original.exception("a custom message that is not a keyword error") + end + + msg = nil + assert_nothing_raised do + msg = exc.detailed_message(highlight: false) + end + assert_match("a custom message that is not a keyword error", msg) + + assert_nothing_raised do + exc.full_message(highlight: false) + end + end + private def find_node_by_id(node, node_id)