From 67173070fa1128de924af7d233844c44183ac9a2 Mon Sep 17 00:00:00 2001 From: Alexander Momchilov Date: Fri, 1 Sep 2023 12:47:33 -0400 Subject: [PATCH 1/2] Add characterization test for `.inhertited` bug --- .../runtime/generic_type_registry_spec.rb | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/spec/tapioca/runtime/generic_type_registry_spec.rb b/spec/tapioca/runtime/generic_type_registry_spec.rb index f7de31b58..e48f19089 100644 --- a/spec/tapioca/runtime/generic_type_registry_spec.rb +++ b/spec/tapioca/runtime/generic_type_registry_spec.rb @@ -77,6 +77,19 @@ class GenericTypeRegistrySpec < Minitest::Spec refute_same(result, RaisesInInheritedCallback) 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 "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 + HasRecursiveInheritedSig[Object] + end + end end end @@ -98,6 +111,39 @@ def inherited(subclass) end end end + + class HasNonRecursiveInheritedSig + extend T::Generic + + Element = type_member + + 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 + 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 From d5b7c98feb407675cbc46f6c6b270b89a089c086 Mon Sep 17 00:00:00 2001 From: Alexander Momchilov Date: Fri, 1 Sep 2023 17:58:25 -0400 Subject: [PATCH 2/2] Fix `.inhertited` infinite recursion bug --- lib/tapioca/runtime/generic_type_registry.rb | 21 +++++++++++++------ .../runtime/generic_type_registry_spec.rb | 8 ++----- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/tapioca/runtime/generic_type_registry.rb b/lib/tapioca/runtime/generic_type_registry.rb index 6de1c417c..d13f7df17 100644 --- a/lib/tapioca/runtime/generic_type_registry.rb +++ b/lib/tapioca/runtime/generic_type_registry.rb @@ -117,7 +117,7 @@ def create_generic_type(constant, name) # the generic class `Foo[Bar]` is still a `Foo`. That is: # `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. @@ -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 + + # 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 diff --git a/spec/tapioca/runtime/generic_type_registry_spec.rb b/spec/tapioca/runtime/generic_type_registry_spec.rb index e48f19089..012b4e790 100644 --- a/spec/tapioca/runtime/generic_type_registry_spec.rb +++ b/spec/tapioca/runtime/generic_type_registry_spec.rb @@ -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] end end end @@ -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