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

Fix infinite recursion bug in .inherited sigs #1636

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions lib/tapioca/runtime/generic_type_registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def create_generic_type(constant, name)
# the generic class `Foo[Bar]` is still a `Foo`. That is:
Copy link
Collaborator

Choose a reason for hiding this comment

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

^ Typo in commit message

# `Foo[Bar].new.is_a?(Foo)` should be true, which isn't the case
# if we just clone the class. But subclassing works just fine.
create_safe_subclass(constant)
create_safe_subclass(constant, name)
else
# This can only be a module and it is fine to just clone modules
# since they can't have instances and will not have `is_a?` relationships.
Expand Down Expand Up @@ -151,21 +151,30 @@ def create_generic_type(constant, name)
generic_type
end

sig { params(constant: T::Class[T.anything]).returns(T::Class[T.anything]) }
def create_safe_subclass(constant)
sig { params(constant: T::Class[T.anything], name: String).returns(T::Class[T.anything]) }
def create_safe_subclass(constant, name)
# Lookup the "inherited" class method
inherited_method = constant.method(:inherited)
# and the module that defines it
owner = inherited_method.owner

# If no one has overriden the inherited method yet, just subclass
# If no one has overridden the inherited method yet, just subclass
return Class.new(constant) if Class == owner

# Capture this Hash locally, to mutate it in the `inherited` callback below.
generic_instances = @generic_instances

begin
# Otherwise, some inherited method could be preventing us
# from creating subclasses, so let's override it and rescue
owner.send(:define_method, :inherited) do |s|
inherited_method.call(s)
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. 🔁

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


# Call the original `.inherited` method, but rescue any errors that might be raised,
# which would have otherwise prevented our subclass from being created.
inherited_method.call(new_subclass)
rescue
# Ignoring errors
end
Expand Down
8 changes: 2 additions & 6 deletions spec/tapioca/runtime/generic_type_registry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,10 @@ class GenericTypeRegistrySpec < Minitest::Spec
_ = HasNonRecursiveInheritedSig[Object]
end

it "FIXME: breaks from infinite recursion if the sig on .inherited uses the generic type" do
it "works for classes whose .inherited sig reference the generic type itself" 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
HasRecursiveInheritedSig[Object]
end
_ = HasRecursiveInheritedSig[Object]
vinistock marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
Expand Down Expand Up @@ -120,8 +118,6 @@ class HasNonRecursiveInheritedSig
class << self
extend T::Sig

# The correct type would be `T::Class[SampleGenericClass[T.anything]]`, but that would crash Tapioca.
# That's not honey Pooh, that's recursion!
sig { params(subclass: T::Class[T.anything]).void }
def inherited(subclass)
super
Expand Down