Use new, not initialize, to enforce runtime abstract
#6888
Conversation
| sig {override.void} | ||
| def initialize; end |
There was a problem hiding this comment.
Since initialize is kind of special, do you think it's worth testing at least an error case or two where we are overriding initialize incompatibly with the abstract base class? (I realize we have tests for other instance methods above, but perhaps the special-ness of initialize is worth doing some "duplicate" testing.)
| if mod.method(:new).owner == mod | ||
| raise "You must call `abstract!` *before* defining a `new` method" |
There was a problem hiding this comment.
WDYT about also warning people that you shouldn't define new on abstract! classes?
There was a problem hiding this comment.
It's interesting. I think it's probably a good check to have? I'm struggling to think of a case where you'd want to define self.new and not initialize.
But also I'm generally afraid of rocking the boat too much on runtime changes like this, because people do weird things.
So I'd probably prefer to investigate something like raising an error for any self.new method defined in an abstract class in a follow up change. Or alternatively, if we do just want to emit a warning, not an error, we could get rid of the alias_method trick we do later in the file, which would then use the Ruby VM's built in method redefinition warnings to surface this to the user.
There was a problem hiding this comment.
Rails defines self.new in order to implement single-table inheritance. new is called on the STI base class, which delegates to the appropriate child class based on the value of the type attribute.
Consider this example:
Market.has_many :regionsRegion < ActiveRecord::Baseand is markedabstract!Country < RegionProvince < Region
Before this change (desired behavior IMO)
[1] pry(main)> market.regions.build
RuntimeError: Region is declared as abstract; it cannot be instantiated
[2] pry(main)> market.regions.build(type: Country)
=> #<Country:0x00007f97c3b1a3c0 ... >
After this change:
[1] pry(main)> market.regions.build
RuntimeError: Region is declared as abstract; it cannot be instantiated
[2] pry(main)> market.regions.build(type: Country)
RuntimeError: Region is declared as abstract; it cannot be instantiated
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: |
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: → https://go/builds/bui_No3dBatUPfwnyR |
jez
force-pushed
the
jez-abstract-initialize
branch
from
a21ca94
to
285ae0e
Compare
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: → https://go/builds/bui_NrM8Klkuvtef99 This change also has sorbet-runtime changes. Those tests: |
Some Ruby code uses the `new` method as a type factory to instantiate
a type that is related to but not the same as the type of the receiver. That means, those classes cannot be marked as `abstract!`, since doing that would prevent the use of `new` on the class immediately.
This is specifically how Active Record handles STI, which is now broken. Consider the following example:
```ruby
class Market < ActiveRecord::Base
has_many :regions
end
class Region < ActiveRecord::Base
abstract!
end
class Country < Region
end
class Province < Region
end
```
Before Sorbet started patching `new` (pre sorbet#6888):
```ruby
[1] pry(main)> market.regions.build
RuntimeError: Region is declared as abstract; it cannot be instantiated
[2] pry(main)> market.regions.build(type: Country)
=> #<Country:0x00007f97c3b1a3c0 ... >
```
After Sorbet started patching `new` (post sorbet#6888):
```ruby
[1] pry(main)> market.regions.build
RuntimeError: Region is declared as abstract; it cannot be instantiated
[2] pry(main)> market.regions.build(type: Country)
RuntimeError: Region is declared as abstract; it cannot be instantiated
```
The difference is due to how [Active Record redefines `new`](https://github.com/rails/rails/blob/ef04fbb3b256beececfa44c47c4ec93ac6945e59/activerecord/lib/active_record/inheritance.rb#L56-L78) to return an instance of a subtype of the receiver as specified by the `type` argument. Sorbet's patching of `new` prevents that from happening, so the `type` argument is ignored and an exception is raised instead.
However, what we really want to prevent is for `new` to return an instance of the abstract type, not prevent it from returning a non-abstract type since the receiver was abstract. So this commit changes the behaviour of the `new` method `abstract!` installs to call `super` first, and then check if the returned instance is an instance of the abstract type. If it is, it raises an exception, otherwise it returns the instance.
…7079) Some Ruby code uses the `new` method as a type factory to instantiate a type that is related to but not the same as the type of the receiver. That means, those classes cannot be marked as `abstract!`, since doing that would prevent the use of `new` on the class immediately. This is specifically how Active Record handles STI, which is now broken. Consider the following example: ```ruby class Market < ActiveRecord::Base has_many :regions end class Region < ActiveRecord::Base abstract! end class Country < Region end class Province < Region end ``` Before Sorbet started patching `new` (pre #6888): ```ruby [1] pry(main)> market.regions.build RuntimeError: Region is declared as abstract; it cannot be instantiated [2] pry(main)> market.regions.build(type: Country) => #<Country:0x00007f97c3b1a3c0 ... > ``` After Sorbet started patching `new` (post #6888): ```ruby [1] pry(main)> market.regions.build RuntimeError: Region is declared as abstract; it cannot be instantiated [2] pry(main)> market.regions.build(type: Country) RuntimeError: Region is declared as abstract; it cannot be instantiated ``` The difference is due to how [Active Record redefines `new`](https://github.com/rails/rails/blob/ef04fbb3b256beececfa44c47c4ec93ac6945e59/activerecord/lib/active_record/inheritance.rb#L56-L78) to return an instance of a subtype of the receiver as specified by the `type` argument. Sorbet's patching of `new` prevents that from happening, so the `type` argument is ignored and an exception is raised instead. However, what we really want to prevent is for `new` to return an instance of the abstract type, not prevent it from returning a non-abstract type since the receiver was abstract. So this commit changes the behaviour of the `new` method `abstract!` installs to call `super` first, and then check if the returned instance is an instance of the abstract type. If it is, it raises an exception, otherwise it returns the instance.
* Use `new`, not `initialize`, to enforce runtime abstract * Add a big note about initialize special case * Add a test * fix rubocop * Add another test * This error is useless
…7079) Some Ruby code uses the `new` method as a type factory to instantiate a type that is related to but not the same as the type of the receiver. That means, those classes cannot be marked as `abstract!`, since doing that would prevent the use of `new` on the class immediately. This is specifically how Active Record handles STI, which is now broken. Consider the following example: ```ruby class Market < ActiveRecord::Base has_many :regions end class Region < ActiveRecord::Base abstract! end class Country < Region end class Province < Region end ``` Before Sorbet started patching `new` (pre #6888): ```ruby [1] pry(main)> market.regions.build RuntimeError: Region is declared as abstract; it cannot be instantiated [2] pry(main)> market.regions.build(type: Country) => #<Country:0x00007f97c3b1a3c0 ... > ``` After Sorbet started patching `new` (post #6888): ```ruby [1] pry(main)> market.regions.build RuntimeError: Region is declared as abstract; it cannot be instantiated [2] pry(main)> market.regions.build(type: Country) RuntimeError: Region is declared as abstract; it cannot be instantiated ``` The difference is due to how [Active Record redefines `new`](https://github.com/rails/rails/blob/ef04fbb3b256beececfa44c47c4ec93ac6945e59/activerecord/lib/active_record/inheritance.rb#L56-L78) to return an instance of a subtype of the receiver as specified by the `type` argument. Sorbet's patching of `new` prevents that from happening, so the `type` argument is ignored and an exception is raised instead. However, what we really want to prevent is for `new` to return an instance of the abstract type, not prevent it from returning a non-abstract type since the receiver was abstract. So this commit changes the behaviour of the `new` method `abstract!` installs to call `super` first, and then check if the returned instance is an instance of the abstract type. If it is, it raises an exception, otherwise it returns the instance.
Note to users: this PR might cause new errors in any code that was
previously defining
initializein anabstract!class and then trying toinstantiate it at runtime.
Doing that would have had the effect of redefining the method that
sorbet-runtimepreviously defined to enforce that abstract classes can't beinstantiated, which would have allowed the instantiation. This will not happen
anymore.
Motivation
Fixes #3388
The general idea is that people want both:
constructor
By moving the "no new" enforcement to happen in
self.newinstead ofinitialize, we can accomplish this. It's far, far more rare for people to havevalid use cases for overriding
self.new.This also unblocks being able to override
initializeat runtime and mark itabstract, so that it can be used to enforce that all subclasses conform to astandard contract for constructing new instances.
Test plan
See included automated tests.
I tested this against Stripe's codebase and 17 tests failed that were errantly
attempting to instantiate an abstract class at runtime in a way that Sorbet
couldn't catch statically.