Remove sigs attached to methods defined with modifier calls

[dduugg]

Motivation

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 removes the method but leaves the method's sig
(and any attached comments) behind:

class Foo
  sig { params(value: Integer).void }
  private_class_method def self.bar(value)
    something
  end

  def baz; end
end

Removing bar currently produces:

class Foo
  sig { params(value: Integer).void }

  def baz; end
end

The orphaned sig then attaches to the next method definition (baz). This
goes wrong in a few ways:

• It's a Sorbet error when the leftover sig doesn't match the following method —
  here baz has no value parameter.
• When the following method has its own sig, removal leaves two sigs with no
  method definition between them, which is also a Sorbet error.
• When the leftover sig happens to be compatible with the following method, it's
  arguably worse: it silently re-types an unrelated method with no error at all.

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.
attached_sigs walks the def's siblings, so it never sees the sig (and
comments) that are declared as siblings of the modifier call.

Fix

When removing a def that is wrapped in a modifier call, target the (outermost)
wrapping call instead of the def, so its attached sigs and comments are removed
together with the method.

The wrapping call is detected structurally (a call that takes the method as an
argument, climbing through nested modifiers) rather than by matching a fixed list of
names. This way user-defined modifiers and future ones — e.g. Sorbet's proposed
abstract def — are handled without
maintaining 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 the modifier's name. A call that takes
other arguments — e.g. register(:thing, def bar; end) or memoize def bar; end, ttl: 60
— has its own purpose, so we leave the call (and its sibling arguments) in place and
remove only the def. The only 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.

After the fix the example above becomes:

class Foo
  def baz; end
end

Tests

Added tests covering:

• a visibility modifier (private_class_method def self.bar), an arbitrary custom
  modifier, and nested modifiers (private abstract def bar) — the sole-argument
  cases where the whole wrapping call (and its sigs/comments) is removed;
• a modifier-wrapped def as the last statement in the body;
• single-line and sig-less modifier forms;
• a def passed as a non-sole argument (register(:thing, def bar; end)) and a call
  with a trailing keyword argument (memoize(def bar…, ttl: 60)) — confirming the
  call and its sibling arguments are preserved and only the def is removed.

The full remover suite and srb tc/RuboCop pass.

[KaanOzkan]

I think we should call this method only for DefNode instead of returning early, being more descriptive in the call site will be helpful.

[dduugg]

Done — split the case in apply_edit so Class/ModuleNode delete directly and modifier_call_context is only called for DefNode, which it now takes as a typed parameter. Dropped the internal early-return guard.

[KaanOzkan]

What do you think about iterating the nesting directly? Feels easier to read, something like

#: (Prism::Node) -> NodeContext?
def modifier_call_context(wrapped)
  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
      next
    when Prism::CallNode
      args = ancestor.arguments&.arguments
      break if ancestor.block
      break unless args && args.size == 1 && args.first.equal?(wrapped)

      wrapped = ancestor
      outer_call = ancestor
      outer_nesting = nesting.dup
    else
      break
    end
  end

  return unless outer_call && outer_nesting

  NodeContext.new(@old_source, @node_context.comments, outer_call, outer_nesting)
end

[dduugg]

Done, adopted the pop-based iteration — reads much better, thanks. One small tweak vs. your snippet: wrapped is held in a Prism::Node local since it starts as the DefNode param and gets reassigned to the wrapping CallNode, which would otherwise be a Sorbet type error.