Skip to content

Commit 7f4a1e1

Browse files
committed
[ruby/error_highlight] 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. ruby/error_highlight@ca126db2db [Bug #22129] (Backport for 4.0) (cherry picked from 12ef8a6)
1 parent c552f2b commit 7f4a1e1

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/error_highlight/test_error_highlight.rb

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

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

17561819
def find_node_by_id(node, node_id)

0 commit comments

Comments
 (0)