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 all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 15 additions & 6 deletions lib/tapioca/runtime/generic_type_registry.rb
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
42 changes: 42 additions & 0 deletions spec/tapioca/runtime/generic_type_registry_spec.rb
Expand Up @@ -77,6 +77,17 @@ class GenericTypeRegistrySpec < Minitest::Spec
refute_same(result, RaisesInInheritedCallback)
Copy link
Collaborator

Choose a reason for hiding this comment

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

^ There is a typo in the commit message

assert_operator(result, :<, RaisesInInheritedCallback)
end

it "works for classes that specify a 'loose' sig on .inherited" do
# By "loose", we mean `T::Class[T.anything]` instead of `T::Class[SampleGenericClass[T.anything]]`
_ = HasNonRecursiveInheritedSig[Object]
end

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.
_ = HasRecursiveInheritedSig[Object]
vinistock marked this conversation as resolved.
Show resolved Hide resolved
end
end
end

Expand All @@ -98,6 +109,37 @@ def inherited(subclass)
end
end
end

class HasNonRecursiveInheritedSig
extend T::Generic

Element = type_member

class << self
extend T::Sig

sig { params(subclass: T::Class[T.anything]).void }
def inherited(subclass)
super
end
end
end

class HasRecursiveInheritedSig
extend T::Generic

Element = type_member

class << self
extend T::Sig

# This sig references an instantiation of this class itself.
sig { params(subclass: T::Class[HasRecursiveInheritedSig[T.anything]]).void }
def inherited(subclass)
super
end
end
end
end
end
end