Support override with prepended modules at runtime

[jplaisted]

Problem

The sorbet runtime does not support overriding methods from prepended modules (link). I think there's at least one very common use case for this that doesn't violate any type safety; which is to wrap methods. I would think a common use case of this is when instrumenting / tracing methods.

Assume we have some module like this

module MyTracer
  def instrument(name)
    proxy = Module.new
    
    proxy.define_method(name) do |*args|
      # Tracing code goes here
      puts "pre-call #{name}"
      super(*args)
      puts "post-call #{name}"
    end

    prepend proxy
  end
end

And then we have an interface where an implementation also wants to instrument the method like this:

module MyInterface
  extend T::Sig
  extend T::Helpers
  interface!

  sig {abstract.returns(String)}
  def foo; end
end

class MyClass
  extend T::Sig
  extend MyTracer
  include MyInterface

  instrument :foo

  sig {override.returns(String)}
  def foo
    puts 'override called'
    'foo'
  end
end

MyClass.new.foo

If you run this, you get the error that is linked above (with the "we don't support this" message). If you comment out the sig with the override it runs fine.

pre-call foo
override called
post-call foo

It isn't clear to me what this sig is trying to do at runtime, and why it can't support prepend. Seems like maybe its trying to, ironically, wrap the method to verify it actually overrides a method. It just takes a different approach that doesn't support prepend... (edit: see second post, has nothing to do with override)

Proposed solution(s)

I still don't claim to fully understand the problem, but if I'm reading this comment correctly...

        # If we get here, that means the method we're trying to replace exists on a *prepended*
        # mixin, which means in order to supersede it, we'd need to create a method on a new
        # module that we'd prepend before `ancestor`. The problem with that approach is there'd
        # be no way to remove that new module after prepending it, so we'd be left with these
        # empty anonymous modules in the ancestor chain after calling `restore`.
        #
        # That's not necessarily a deal breaker, but for now, we're keeping it as unsupported.

Then either:

• It "isn't a deal breaker", so just do it.
• Maybe modify the implementation of ReplacedMethod to throw if restore is called in this case. So replacing is supported, just not restoring.
• It doesn't appear that restore is called anyway (this util stuff was copied from elsewhere), so maybe just remove that method, and this comment, and allow supporting this? If this would make restore ugly, but restore is never used... does it matter?

[jplaisted]

Oh, you don't even need the interface to reproduce, so it has nothing to do with override. My mistake!

# typed: strict

require 'sorbet-runtime'

module MyTracer
  def instrument(name)
    proxy = Module.new
    
    proxy.define_method(name) do |*args|
      # Tracing code goes here
      puts "pre-call #{name}"
      super(*args)
      puts "post-call #{name}"
    end

    prepend proxy
  end
end

class MyClass
  extend T::Sig
  extend MyTracer

  instrument :foo

  sig {returns(String)}
  def foo
    puts 'override called'
    'foo'
  end
end

MyClass.new.foo

[s-soroosh]

Bumping this issue!
We have the exact use case and it's kind of pity that we already can't use the prepended modules.
Can you folks please reconsider it and add this feature given that it's outlined as non deal breaker?

[ianks]

Is there a way around this issue? checked(:never) doesnt seem to help either :/

[eabartlett]

We also have this exact use case and it would be great to have this added as a feature.

[oleg-vinted]

One workaround for this is to prepend modules after running sig, in your case:

class MyClass
  extend T::Sig
  extend MyTracer

  sig {returns(String)}
  def foo
    puts 'override called'
    'foo'
  end

  instrument :foo # <--- wrap `#foo` after defining it
end

[mclark]

From what I can tell, the ClassUtils#replace_method iterates through the ancestors of the mod whose method is being replaced. If it finds the mod first (which will be the first ancestor for any module that hasn't been prepended) then it continues on its merry way. If it gets to the "original owner" first, which in the case of a module with prepended overrides will be any of those prepends with the method defined, then it raises the error.

In the following graphic, it would find PrependB#bar first and raise an error:

The reason it raises the error is that in order to "supersede" that prepended method it would need to create another anonymous prepend that could be left empty if the original method got restored. This makes sense.

However, I wonder if we really need to supersede the prepended module definition at all in this case? Instead, could we not simply replace the method MyClass#bar and not worry about type checking the prepended module's matching methods? Prepended methods are almost always decorators, and in my experience it's acceptable if they don't have type safety as they are usually written in quite a generic fashion anyway.

This way we wouldn't need to worry about any orphaned empty modules. The downside is that from the caller's perspective, a method with a prepended override might not be getting runtime type checked at all if the prepend never calls super.

I think that downside is an acceptable trade for decorators that work like Ruby developers expect them to. If the prepended method has its own sig block then there is no problem anyway. If it calls super then both its sig and MyClass's sig will have runtime validation (I think).

Does this make sense? Am I missing something? If not, I'd be happy to try throwing together a PR for this change.

[oleg-vinted]

This proposed behavior makes sense to me, considering that the sig is for MyClass#bar rather than PrependB#bar. If PrependB#bar also had a sig, I'd expect it to be type checked too.

I've tried messing around with sorbet-runtime's source around a year ago to work around this issue, and I think the tricky part is implementing this correctly, as MyClass.instance_method(:bar) returns PrependB#bar, and this breaks a lot of assumptions that sorbet-runtime has, and this is what that the error message is trying to prevent. You'd have to find a way to get the MyClass#bar method object even when #bar exists in a prepended module.

I'm not absolutely sure, but I believe such behavior already happens if you prepend a module after defining a method with a sig, like I mentioned in #5025 (comment). So if you manage to patch sorbet-runtime, we'd get the same behavior no matter what the order is (prepend then sig VS. sig then prepend).

(I'm not a Sorbet developer, just another Sorbet user that has run into the same issue.)