Fix infinite recursion bug in .inherited sigs
#1636
Conversation
amomchilov
changed the title
.inherited sigs
amomchilov
force-pushed
the
bug/GenericTypeRegistry-recursion
branch
from
8581a3b
to
8b9189b
Compare
amomchilov
force-pushed
the
GenericTypeRegistrySpec
branch
from
9bfa651
to
9e68657
Compare
amomchilov
force-pushed
the
bug/GenericTypeRegistry-recursion
branch
from
8b9189b
to
d5b7c98
Compare
| owner.send(:define_method, :inherited) do |new_subclass| | ||
| # Register this new subclass ASAP, to prevent re-entry into the `create_safe_subclass` code-path. | ||
| # This can happen if the sig of the original `.inherited` method references the generic type itself. | ||
| generic_instances[name] ||= new_subclass |
There was a problem hiding this comment.
Admittedly, this is a bit odd.
I would have preferred keeping the simplicity of the "create the thing, then register the thing" as two clean, separate steps, but I don't think that's possible.
Registering the "completed" subclass requires it to first be created, creating it entails running its sigs, which attempts to register it. 🔁
There was a problem hiding this comment.
You could separate the process in 2 steps. 1. to register the class and 2. to call the original .inherited. But this looks good enough 👍
Should we update the register_type comment to add a note about recursiveness?
| @@ -77,6 +77,19 @@ class GenericTypeRegistrySpec < Minitest::Spec | |||
| refute_same(result, RaisesInInheritedCallback) | |||
There was a problem hiding this comment.
^ There is a typo in the commit message
| it "FIXME: breaks from infinite recursion if the sig on .inherited uses the generic type" do | ||
| # Our swizzled implementation of the `.inherited` method needs to be carefully implemented to not fall into | ||
| # infinite recursion when the sig for the method references the class that it's defined on. | ||
| assert_raises(SystemStackError) do |
There was a problem hiding this comment.
Let's try to not commit code that is being changed by a commit in the same PR. You can introduce a test that breaks, and fix it in the next commit, TDD like.
| @@ -117,7 +117,7 @@ def create_generic_type(constant, name) | |||
| # the generic class `Foo[Bar]` is still a `Foo`. That is: | |||
| owner.send(:define_method, :inherited) do |new_subclass| | ||
| # Register this new subclass ASAP, to prevent re-entry into the `create_safe_subclass` code-path. | ||
| # This can happen if the sig of the original `.inherited` method references the generic type itself. | ||
| generic_instances[name] ||= new_subclass |
There was a problem hiding this comment.
You could separate the process in 2 steps. 1. to register the class and 2. to call the original .inherited. But this looks good enough 👍
Should we update the register_type comment to add a note about recursiveness?
Motivation
Fixes #1634
Implementation
The implementation isn't the most elegant, but it's a working starting point.
The main point was to register our new subclass in our lookup table asap, inside of our swizzled
.inheritedimplementation. This puts it in place early enough so that if the.inheritedmethod'ssigreferences the generic class, it will be found (rather than hitting thecreate_generic_typecode path and recursing infinitely).At that point, our subclass is in an intermediate state. It exists, its superclass is set, but none of our customizations in
create_generic_type(e.g. definine.name,__tapioca_override_type, etc.) have been done yet. I'm not sure what the impact of this could be.I added the "help-wanted" label to solicit feedback on this part.
Tests
Check the commit history. I started with a characterization tests that ensured the test scenario would hit the expected
SystemStackError, then replaced it with a test that ensures the fix works. And it does :)