From 5a78009bf77685d1cac55165c5fc193f4f286afb Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sun, 7 Jun 2026 14:27:48 -0700 Subject: [PATCH 1/3] Remove sigs attached to methods defined with modifier calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the deadcode remover removes a method defined as the argument of a modifier call — e.g. `private def foo; end`, `private_class_method def self.foo; end`, or `abstract def foo; end` — it left the method's `sig` (and any attached comments) behind, producing an orphaned `sig` that Sorbet then flags (`Unused type annotation`). The cause: for `private_class_method def self.bar`, the `def` node's parent in the nesting is the modifier call's arguments rather than the enclosing class/module body, so `attached_sigs` (which walks the def's siblings) never sees the sig and comments declared alongside the modifier call. Fix: when removing a `def` wrapped in a modifier call, target the outermost wrapping call instead, so its attached sigs and comments are removed together with the method. The wrapping call is detected structurally (climbing through nested modifiers) rather than from a fixed list of names, so user-defined and future modifiers — e.g. Sorbet's proposed `abstract def` (https://github.com/sorbet/sorbet/pull/10276) — are handled without an allowlist. To stay safe, a call only counts as a modifier when the def is its sole argument (and it takes no block): such a call exists only to wrap that one method, so removing it whole is correct regardless of name. A call with other arguments — e.g. `register(:thing, def bar; end)` or `memoize def bar; end, ttl: 60` — keeps its call and sibling arguments; only the def is removed. Adds tests for visibility, custom, and nested modifiers; a modifier-wrapped def as the last statement; single-line and sig-less forms; and non-sole-argument calls whose siblings must be preserved. Regenerates rbi/spoom.rbi for the new method. --- lib/spoom/deadcode/remover.rb | 50 +++++++- rbi/spoom.rbi | 3 + test/spoom/deadcode/remover_test.rb | 180 ++++++++++++++++++++++++++++ 3 files changed, 232 insertions(+), 1 deletion(-) diff --git a/lib/spoom/deadcode/remover.rb b/lib/spoom/deadcode/remover.rb index 6d4ff6b0..7b389905 100644 --- a/lib/spoom/deadcode/remover.rb +++ b/lib/spoom/deadcode/remover.rb @@ -49,7 +49,7 @@ def apply_edit node = @node_context.node case node when Prism::ClassNode, Prism::ModuleNode, Prism::DefNode - delete_node_and_comments_and_sigs(@node_context) + delete_node_and_comments_and_sigs(modifier_call_context || @node_context) when Prism::ConstantWriteNode, Prism::ConstantOperatorWriteNode, Prism::ConstantAndWriteNode, Prism::ConstantOrWriteNode, Prism::ConstantPathWriteNode, Prism::ConstantPathOperatorWriteNode, @@ -65,6 +65,54 @@ def apply_edit private + # When a method is defined as the argument of a modifier call (e.g. `private def foo; end`, + # `private_class_method def self.foo; end`, or `abstract def foo; end`), the `def` node's + # parent is the modifier call rather than the enclosing class or module body. The sigs and + # comments attached to the method are siblings of the outermost such call, so return a + # context targeting that call to remove them together with the method. + # + # Any modifier is matched structurally (rather than from a fixed list) so user-defined and + # future modifiers are handled too. To stay safe, a call only counts as a modifier when the + # wrapped node is its *sole* argument: such a call exists only to wrap that one method, so + # removing it whole is correct regardless of the modifier's name. A call that takes other + # arguments (e.g. `register(:thing, def foo; end)`) has its own purpose, so we leave it in + # place and remove only the `def`. The remaining ambiguous case — a single-argument + # user-defined macro with a side effect — is structurally indistinguishable from a real + # modifier and vanishingly rare, so we treat it like one. + #: -> NodeContext? + def modifier_call_context + wrapped = @node_context.node #: Prism::Node + return unless wrapped.is_a?(Prism::DefNode) + + nesting = @node_context.nesting + call_index = nil #: Integer? + + index = nesting.size - 1 + while index >= 0 + ancestor = nesting.fetch(index) + case ancestor + when Prism::ArgumentsNode + # An arguments node sits between a call and its arguments, keep climbing. + when Prism::CallNode + # Stop unless this call wraps the node as its sole argument (and takes no block), so we + # never delete sibling arguments or a call doing more than wrapping the method. + args = ancestor.arguments&.arguments + break if ancestor.block + break unless args && args.size == 1 && args.first.equal?(wrapped) + + wrapped = ancestor + call_index = index + else + break + end + index -= 1 + end + return unless call_index + + call = nesting.fetch(call_index) + NodeContext.new(@old_source, @node_context.comments, call, nesting[0...call_index] || []) + end + #: (NodeContext context) -> void def delete_constant_assignment(context) case context.node diff --git a/rbi/spoom.rbi b/rbi/spoom.rbi index ef1a2327..2b010b60 100644 --- a/rbi/spoom.rbi +++ b/rbi/spoom.rbi @@ -1519,6 +1519,9 @@ class Spoom::Deadcode::Remover::NodeRemover end def insert_accessor(node, send_context, was_removed:); end + sig { returns(T.nilable(::Spoom::Deadcode::Remover::NodeContext)) } + def modifier_call_context; end + sig { params(start_char: ::Integer, end_char: ::Integer, replacement: ::String).void } def replace_chars(start_char, end_char, replacement); end diff --git a/test/spoom/deadcode/remover_test.rb b/test/spoom/deadcode/remover_test.rb index 4e38b71b..4187a5d8 100644 --- a/test/spoom/deadcode/remover_test.rb +++ b/test/spoom/deadcode/remover_test.rb @@ -940,6 +940,186 @@ def baz; end RB end + def test_removes_node_sig_with_visibility_modifier + res = remove(<<~RB, "bar") + class Foo + def foo; end + + sig { void } + private_class_method def self.bar + something + end + + def baz; end + end + RB + + assert_equal(<<~RB, res) + class Foo + def foo; end + + def baz; end + end + RB + end + + def test_removes_node_sig_with_custom_modifier + res = remove(<<~RB, "bar") + class Foo + def foo; end + + # A comment + sig { void } + some_custom_modifier def bar + something + end + + def baz; end + end + RB + + assert_equal(<<~RB, res) + class Foo + def foo; end + + def baz; end + end + RB + end + + def test_removes_node_sig_with_nested_modifiers + res = remove(<<~RB, "bar") + class Foo + def foo; end + + sig { void } + private abstract def bar + something + end + + def baz; end + end + RB + + assert_equal(<<~RB, res) + class Foo + def foo; end + + def baz; end + end + RB + end + + def test_removes_node_sig_with_modifier_as_last_statement + res = remove(<<~RB, "bar") + class Foo + def foo; end + + sig { void } + private def bar + something + end + end + RB + + assert_equal(<<~RB, res) + class Foo + def foo; end + end + RB + end + + def test_removes_single_line_def_with_modifier + res = remove(<<~RB, "bar") + class Foo + def foo; end + private def bar; end + def baz; end + end + RB + + assert_equal(<<~RB, res) + class Foo + def foo; end + def baz; end + end + RB + end + + def test_removes_def_with_modifier_without_sig + res = remove(<<~RB, "bar") + class Foo + def foo; end + + private_class_method def self.bar + something + end + + def baz; end + end + RB + + assert_equal(<<~RB, res) + class Foo + def foo; end + + def baz; end + end + RB + end + + def test_keeps_call_when_def_is_not_the_sole_argument + # `bar` is a positional argument to `register`, not wrapped by a modifier, so only the `def` + # is removed and the call (along with its other arguments) is preserved. + res = remove(<<~RB, "bar") + class Foo + register( + :thing, + def bar + x + end, + ) + + def keep; end + end + RB + + assert_equal(<<~RB, res) + class Foo + register( + :thing, + ) + + def keep; end + end + RB + end + + def test_keeps_sibling_arguments_when_def_is_not_the_sole_argument + res = remove(<<~RB, "bar") + class Foo + memoize( + def bar + x + end, + ttl: 60, + ) + + def keep; end + end + RB + + assert_equal(<<~RB, res) + class Foo + memoize( + ttl: 60, + ) + + def keep; end + end + RB + end + def test_removes_node_sig_and_comments res = remove(<<~RB, "bar") class Foo From b130f0e0c15d0dd2e781cf37fb4c06dff442b7c6 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 10 Jun 2026 16:59:03 -0700 Subject: [PATCH 2/3] Address review: split DefNode call site, iterate nesting via pop --- lib/spoom/deadcode/remover.rb | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/lib/spoom/deadcode/remover.rb b/lib/spoom/deadcode/remover.rb index 7b389905..52d1918e 100644 --- a/lib/spoom/deadcode/remover.rb +++ b/lib/spoom/deadcode/remover.rb @@ -48,8 +48,10 @@ def apply_edit node = @node_context.node case node - when Prism::ClassNode, Prism::ModuleNode, Prism::DefNode - delete_node_and_comments_and_sigs(modifier_call_context || @node_context) + when Prism::ClassNode, Prism::ModuleNode + delete_node_and_comments_and_sigs(@node_context) + when Prism::DefNode + delete_node_and_comments_and_sigs(modifier_call_context(node) || @node_context) when Prism::ConstantWriteNode, Prism::ConstantOperatorWriteNode, Prism::ConstantAndWriteNode, Prism::ConstantOrWriteNode, Prism::ConstantPathWriteNode, Prism::ConstantPathOperatorWriteNode, @@ -79,20 +81,18 @@ def apply_edit # place and remove only the `def`. The remaining ambiguous case — a single-argument # user-defined macro with a side effect — is structurally indistinguishable from a real # modifier and vanishingly rare, so we treat it like one. - #: -> NodeContext? - def modifier_call_context - wrapped = @node_context.node #: Prism::Node - return unless wrapped.is_a?(Prism::DefNode) - - nesting = @node_context.nesting - call_index = nil #: Integer? - - index = nesting.size - 1 - while index >= 0 - ancestor = nesting.fetch(index) + #: (Prism::DefNode def_node) -> NodeContext? + def modifier_call_context(def_node) + wrapped = def_node #: Prism::Node + nesting = @node_context.nesting.dup + outer_call = nil #: Prism::CallNode? + outer_nesting = nil #: Array[Prism::Node]? + + while (ancestor = nesting.pop) case ancestor when Prism::ArgumentsNode # An arguments node sits between a call and its arguments, keep climbing. + next when Prism::CallNode # Stop unless this call wraps the node as its sole argument (and takes no block), so we # never delete sibling arguments or a call doing more than wrapping the method. @@ -101,16 +101,15 @@ def modifier_call_context break unless args && args.size == 1 && args.first.equal?(wrapped) wrapped = ancestor - call_index = index + outer_call = ancestor + outer_nesting = nesting.dup else break end - index -= 1 end - return unless call_index + return unless outer_call && outer_nesting - call = nesting.fetch(call_index) - NodeContext.new(@old_source, @node_context.comments, call, nesting[0...call_index] || []) + NodeContext.new(@old_source, @node_context.comments, outer_call, outer_nesting) end #: (NodeContext context) -> void From 5baedd949a650772b8933326c9a65514956d1ae6 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 10 Jun 2026 17:02:43 -0700 Subject: [PATCH 3/3] Tighten comments to match repo style --- lib/spoom/deadcode/remover.rb | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/lib/spoom/deadcode/remover.rb b/lib/spoom/deadcode/remover.rb index 52d1918e..ec95e40f 100644 --- a/lib/spoom/deadcode/remover.rb +++ b/lib/spoom/deadcode/remover.rb @@ -67,20 +67,15 @@ def apply_edit private - # When a method is defined as the argument of a modifier call (e.g. `private def foo; end`, - # `private_class_method def self.foo; end`, or `abstract def foo; end`), the `def` node's - # parent is the modifier call rather than the enclosing class or module body. The sigs and - # comments attached to the method are siblings of the outermost such call, so return a - # context targeting that call to remove them together with the method. + # When a method is defined as the argument of a modifier call (e.g. `private def foo; end` or + # `private_class_method def self.foo; end`), the `def` node's parent is the call rather than the + # enclosing class or module body, so its sigs and comments are siblings of the call. Return a + # context targeting the outermost such call so they are removed together with the method. # - # Any modifier is matched structurally (rather than from a fixed list) so user-defined and - # future modifiers are handled too. To stay safe, a call only counts as a modifier when the - # wrapped node is its *sole* argument: such a call exists only to wrap that one method, so - # removing it whole is correct regardless of the modifier's name. A call that takes other - # arguments (e.g. `register(:thing, def foo; end)`) has its own purpose, so we leave it in - # place and remove only the `def`. The remaining ambiguous case — a single-argument - # user-defined macro with a side effect — is structurally indistinguishable from a real - # modifier and vanishingly rare, so we treat it like one. + # Modifiers are matched structurally rather than from a fixed list, so user-defined ones are + # handled too. A call only counts as a modifier when the `def` is its sole argument and it takes + # no block, so we never remove a call that does more than wrap the method (e.g. + # `register(:thing, def foo; end)`). #: (Prism::DefNode def_node) -> NodeContext? def modifier_call_context(def_node) wrapped = def_node #: Prism::Node @@ -91,11 +86,11 @@ def modifier_call_context(def_node) while (ancestor = nesting.pop) case ancestor when Prism::ArgumentsNode - # An arguments node sits between a call and its arguments, keep climbing. + # An arguments node sits between a call and its arguments, keep climbing next when Prism::CallNode - # Stop unless this call wraps the node as its sole argument (and takes no block), so we - # never delete sibling arguments or a call doing more than wrapping the method. + # Stop unless the call wraps the node as its sole argument and takes no block, otherwise + # it does more than wrap the method and we must not remove it args = ancestor.arguments&.arguments break if ancestor.block break unless args && args.size == 1 && args.first.equal?(wrapped)