Skip to content

Commit dbe495f

Browse files
committed
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.
1 parent 2477135 commit dbe495f

2 files changed

Lines changed: 79 additions & 5 deletions

File tree

lib/error_highlight/base.rb

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -235,17 +235,21 @@ def spot
235235
spot_op_cdecl
236236

237237
when :DEFN
238-
raise NotImplementedError if @point_type != :name
238+
# There is nothing to highlight for the arguments of a method
239+
# definition, so just return nil instead of raising.
240+
return nil if @point_type != :name
239241
spot_defn
240242

241243
when :DEFS
242-
raise NotImplementedError if @point_type != :name
244+
return nil if @point_type != :name
243245
spot_defs
244246

245247
when :LAMBDA
248+
return nil if @point_type != :name
246249
spot_lambda
247250

248251
when :ITER
252+
return nil if @point_type != :name
249253
spot_iter
250254

251255
when :call_node
@@ -294,23 +298,30 @@ def spot
294298
when :name
295299
prism_spot_def_for_name
296300
when :args
297-
raise NotImplementedError
301+
# There is nothing to highlight for the arguments of a method
302+
# definition (e.g. an ArgumentError for a missing required keyword
303+
# is spotted with point_type: :args). Return nil instead of raising
304+
# NotImplementedError, which would otherwise escape
305+
# Exception#detailed_message / #full_message.
306+
return nil
298307
end
299308

300309
when :lambda_node
301310
case @point_type
302311
when :name
303312
prism_spot_lambda_for_name
304313
when :args
305-
raise NotImplementedError
314+
# See the comment for :def_node above.
315+
return nil
306316
end
307317

308318
when :block_node
309319
case @point_type
310320
when :name
311321
prism_spot_block_for_name
312322
when :args
313-
raise NotImplementedError
323+
# See the comment for :def_node above.
324+
return nil
314325
end
315326

316327
end

test/test_error_highlight.rb

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1752,6 +1752,69 @@ def test_singleton_method_multiple_missing_keywords
17521752
end
17531753
end
17541754

1755+
def def_with_required_keyword(x:)
1756+
x
1757+
end
1758+
1759+
def test_spot_with_args_point_type_on_def_node_returns_nil
1760+
# A method with a required keyword argument called without it raises an
1761+
# ArgumentError whose first backtrace location maps to the `def` node.
1762+
# ErrorHighlight.spot defaults point_type to :args for ArgumentError, and
1763+
# there is no way to highlight "the arguments" of a definition, so spot
1764+
# must return nil instead of raising NotImplementedError.
1765+
begin
1766+
def_with_required_keyword
1767+
rescue ArgumentError => exc
1768+
end
1769+
1770+
assert_nil(ErrorHighlight.spot(exc))
1771+
end
1772+
1773+
def test_spot_with_args_point_type_on_lambda_node_returns_nil
1774+
l = lambda { |x:| x }
1775+
begin
1776+
l.call
1777+
rescue ArgumentError => exc
1778+
end
1779+
1780+
assert_nil(ErrorHighlight.spot(exc))
1781+
end
1782+
1783+
define_method(:block_with_required_keyword) { |x:| x }
1784+
1785+
def test_spot_with_args_point_type_on_block_node_returns_nil
1786+
begin
1787+
block_with_required_keyword
1788+
rescue ArgumentError => exc
1789+
end
1790+
1791+
assert_nil(ErrorHighlight.spot(exc))
1792+
end
1793+
1794+
def test_detailed_message_does_not_raise_when_argument_error_is_rewrapped
1795+
# This reproduces a real-world crash: an ArgumentError originating from a
1796+
# missing required keyword (which maps to the `def` node) is re-raised with
1797+
# a custom message. The custom message no longer matches the keyword regex
1798+
# in CoreExt#generate_snippet, so the :args branch is taken. The spotter
1799+
# must not raise NotImplementedError (which is not a StandardError and would
1800+
# escape detailed_message / full_message).
1801+
begin
1802+
def_with_required_keyword
1803+
rescue ArgumentError => original
1804+
exc = original.exception("a custom message that is not a keyword error")
1805+
end
1806+
1807+
msg = nil
1808+
assert_nothing_raised do
1809+
msg = exc.detailed_message(highlight: false)
1810+
end
1811+
assert_match("a custom message that is not a keyword error", msg)
1812+
1813+
assert_nothing_raised do
1814+
exc.full_message(highlight: false)
1815+
end
1816+
end
1817+
17551818
private
17561819

17571820
def find_node_by_id(node, node_id)

0 commit comments

Comments
 (0)