Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Active Support Module#delegate DSL #2148

Closed
wants to merge 6 commits into from

Conversation

stephenprater
Copy link

@stephenprater stephenprater commented Jan 10, 2025

Motivation

When using ActiveSupport Module gains a delegate method that creates methods delegated to targets - very much like Stdlib's Forwardable module. Unfortunately - Sorbet does not understand either delegate or Forwardable and all delegated methods return T.untyped - even if the delegate can be statically analyzed. In order to retain typing information, you need to implement "stub" methods like def delegated = target.delegated: this actually runs afoul of Rubocop style guidelines (the Ruby/Delegate cop.)

BecauseModule#delegate actually creates methods on the receiver class, it's fairly straightforward to put these generated methods into an RBI file and get typing support for methods called through delegation.

Implementation

The meta-programming done by Module#delegate doesn't leave any obvious markers on the method - so we prepend an override for delegate to Module so that we can both detect Classes which contain delegation as well as keep track of which methods have been delegated and to where.

Perfect typing is not a goal - so we only do the lowest hanging fruit of the DSL: methods delegated to either other methods within the receiving class, or to other classes. AFAICT it's not possible to determine the type of instance variables or non-class constants without executing the method bodies or re-parsing the source code. That's a little bit extra - so if you attempt to delegate to a class, meta-programmed method, or runtime-only value - the DSL will not generate a method stub in the RBI file.

The delegate method has a number of different behaviors like allow_nil and prefix - which are covered by the DSL and shown in the tests. The same ActiveSupport extension also defines delegate_missing_to - which delegates all methods not responded to by a receiver to a given target. method_missing shenanigans are poorly supported by Sorbet - so rather than hack "support" for this in via RBI files, we'll leave it to Sorbet to support

Tests

Tests are completed - additional thoughts about uncovered cases are welcome!

@stephenprater stephenprater requested a review from a team as a code owner January 10, 2025 02:09
@stephenprater stephenprater force-pushed the activesupport-delegate-compiler branch 2 times, most recently from 996421d to b3c4173 Compare January 10, 2025 02:13
@stephenprater stephenprater added the enhancement New feature or request label Jan 10, 2025
@stephenprater stephenprater force-pushed the activesupport-delegate-compiler branch 2 times, most recently from 92cf6ae to dd37149 Compare January 10, 2025 02:27
@stephenprater stephenprater force-pushed the activesupport-delegate-compiler branch from dd37149 to 40830d9 Compare January 10, 2025 02:29
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I just realized (or remembered) that Sorbet has a rewriter for delegate already. Given that that exists, what's really the benefit of this DSL compiler? If it is just for typing, then I am not sure if it is worth it.

lib/tapioca/dsl/compilers/delegate.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/delegate.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/delegate.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/delegate.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/delegate.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/delegate.rb Outdated Show resolved Hide resolved
lib/tapioca/dsl/compilers/delegate.rb Outdated Show resolved Hide resolved
Co-authored-by: Ufuk Kayserilioglu <[email protected]>
@stephenprater
Copy link
Author

Hmm, I just realized (or remembered) that Sorbet has a rewriter for delegate already. Given that that exists, what's really the benefit of this DSL compiler? If it is just for typing, then I am not sure if it is worth it.

Isn't the point of any of the compilers to generate typing information for metaprogramming like delegate? I'm not sure I understand. You can get around it by implementing "stub" methods, but then Rubocop complains at you that you should be using delegate

@paracycle
Copy link
Member

Hmm, I just realized (or remembered) that Sorbet has a rewriter for delegate already. Given that that exists, what's really the benefit of this DSL compiler? If it is just for typing, then I am not sure if it is worth it.

Isn't the point of any of the compilers to generate typing information for metaprogramming like delegate? I'm not sure I understand. You can get around it by implementing "stub" methods, but then Rubocop complains at you that you should be using delegate

No, it is not. The point of DSL compilers is to make methods/constants that are created at runtime to be statically visible to the type checker. That is the whole idea behind DSL compilers. The signature and better typing is there as a side-effect, and is best effort.

That's why, if Sorbet already has a mechanism, internally, to surface the delegate methods, then I am not sure if the DSL compiler is needed. I am not interested in the type information of the generated methods are correct or not.

If we really want better type information for delegate methods, then we could always add annotations in shim RBI files, which won't make RuboCop complain about them.

@stephenprater
Copy link
Author

I see. I'd still advocate for this change from the point of view of "principle of least surprise" - because the Sorbet delegate stuff leads to type erasure of methods you did specify sigs for. Like if I attach a sig to the target, and then delegate to it, Sorbet's delegation strips it and my signatured method is now "untyped" without me intentionally going through a known escape hatch.

That said: if compilers shouldn't be used to preserve that kind of information - that's fine. Sorry for the misunderstanding.

@stephenprater stephenprater force-pushed the activesupport-delegate-compiler branch from 3bc4bd9 to 5d5aaea Compare January 10, 2025 19:28
@stephenprater stephenprater force-pushed the activesupport-delegate-compiler branch from 5d5aaea to 0f330aa Compare January 10, 2025 19:30
@stephenprater
Copy link
Author

Alright - I reworked it so it's less imperative - you sort of do need all of those parts to generate the "right" RBI - but replacing the twisty if conditions with the polymorpish is definitely clearer.

Although - if it's a dumb idea in the first place - I guess it doesn't really matter.

@paracycle
Copy link
Member

Alright - I reworked it so it's less imperative - you sort of do need all of those parts to generate the "right" RBI - but replacing the twisty if conditions with the polymorpish is definitely clearer.

Thanks, this is indeed clearer.

Although - if it's a dumb idea in the first place - I guess it doesn't really matter.

I don't think it is a "dumb" idea, tbh, but there is a cost to adding a DSL compiler like this, which is more churn on generated files and longer times for generating them in the first place.

Knowing that Sorbet internally handles these methods, albeit without signatures, is a good enough trade-off in my opinion. Anyone who wants better signatures can add them in shim RBI files, or do the delegation explicitly.

I don't think we should perform an operation like this over all modules when that alternative exists.

So, thank you for the work here, but I don't think we should have this as a DSL compiler in Tapioca.

@paracycle paracycle closed this Jan 10, 2025
@paracycle paracycle deleted the activesupport-delegate-compiler branch January 13, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants