Merge pull request #2657 from Shopify/codex/extract-generic-type-fix

Fix generic type registry cache collisions

Author: st0012

lib/tapioca/internal.rb

@@ -12,6 +12,7 @@
 require "tapioca/sorbet_ext/backcompat_patches"
 require "tapioca/sorbet_ext/name_patch"
 require "tapioca/sorbet_ext/generic_name_patch"
+require "tapioca/sorbet_ext/generic_type_patch"
 require "tapioca/sorbet_ext/proc_bind_patch"
 require "tapioca/sorbet_ext/void_patch"
 require "tapioca/runtime/generic_type_registry"

lib/tapioca/runtime/generic_type_registry.rb

@@ -6,7 +6,7 @@ module Runtime
     # This class is responsible for storing and looking up information related to generic types.
     #
     # The class stores 2 different kinds of data, in two separate lookup tables:
-    #   1. a lookup of generic type instances by name: `@generic_instances`
+    #   1. a lookup of generic type instances by constant and name: `@generic_instances`
     #   2. a lookup of type variable serializer by constant and type variable
     #      instance: `@type_variables`
     #
@@ -21,7 +21,7 @@ module Runtime
     # variable to type variable serializers. This allows us to associate type variables
     # to the constant names that represent them, easily.
     module GenericTypeRegistry
-      @generic_instances = {} #: Hash[String, Module[top]]
+      @generic_instances = {}.compare_by_identity #: Hash[Module[top], Hash[String, Module[top]]]
 
       @type_variables = {}.compare_by_identity #: Hash[Module[top], Array[TypeVariableModule]]
 
@@ -45,8 +45,9 @@ class << self
         # and cloning the given constant so that we can return a type that is the same
         # as the current type but is a different instance and has a different name method.
         #
-        # We cache those cloned instances by their name in `@generic_instances`, so that
-        # we don't keep instantiating a new type every single time it is referenced.
+        # We cache those cloned instances by their original constant and their name in
+        # `@generic_instances`, so that we don't keep instantiating a new type every single
+        # time it is referenced.
         # For example, `[Foo[Integer], Foo[Integer], Foo[Integer], Foo[String]]` will only
         # result in 2 clones (1 for `Foo[Integer]` and another for `Foo[String]`) and
         # 2 hash lookups (for the other two `Foo[Integer]`s).
@@ -64,12 +65,15 @@ def register_type(constant, types)
           #
           # Also, we try to memoize the generic type based on the name, so that
           # we don't have to keep recreating them all the time.
-          @generic_instances[name] ||= create_generic_type(constant, name)
+          generic_instances = @generic_instances[constant] ||= {}
+          generic_instances[name] ||= create_generic_type(constant, name)
         end
 
         #: (Object instance) -> bool
         def generic_type_instance?(instance)
-          @generic_instances.values.any? { |generic_type| generic_type === instance }
+          @generic_instances.values.any? do |generic_instances|
+            generic_instances.values.any? { |generic_type| generic_type === instance }
+          end
         end
 
         #: (Module[top] constant) -> Array[TypeVariableModule]?
@@ -88,7 +92,7 @@ def lookup_type_variables(constant)
         # can return it from the original methods as well.
         #: (untyped constant, TypeVariableModule type_variable) -> void
         def register_type_variable(constant, type_variable)
-          type_variables = lookup_or_initialize_type_variables(constant)
+          type_variables = @type_variables[constant] ||= []
 
           type_variables << type_variable
         end
@@ -163,11 +167,6 @@ def create_safe_subclass(constant)
             owner.send(:define_method, :inherited, inherited_method)
           end
         end
-
-        #: (Module[top] constant) -> Array[TypeVariableModule]
-        def lookup_or_initialize_type_variables(constant)
-          @type_variables[constant] ||= []
-        end
       end
     end
   end

lib/tapioca/sorbet_ext/generic_name_patch.rb

@@ -82,26 +82,6 @@ def name
       prepend GenericPatch
     end
   end
-
-  module Utils
-    module Private
-      module PrivateCoercePatch
-        def coerce_and_check_module_types(val, check_val, check_module_type)
-          if val.is_a?(Tapioca::TypeVariableModule)
-            val.coerce_to_type_variable
-          elsif val.respond_to?(:__tapioca_override_type)
-            val.__tapioca_override_type
-          else
-            super
-          end
-        end
-      end
-
-      class << self
-        prepend(PrivateCoercePatch)
-      end
-    end
-  end
 end
 
 module Tapioca

lib/tapioca/sorbet_ext/generic_type_patch.rb

@@ -0,0 +1,48 @@
+# typed: true
+# frozen_string_literal: true
+
+module T
+  module Utils
+    module Private
+      # Preserve Tapioca's generic type variables and instantiated generic
+      # names when Sorbet coerces them into runtime types.
+      module TapiocaGenericTypeCoercePatch
+        def coerce_and_check_module_types(val, check_val, check_module_type)
+          if val.is_a?(Tapioca::TypeVariableModule)
+            val.coerce_to_type_variable
+          elsif val.respond_to?(:__tapioca_override_type)
+            val.__tapioca_override_type
+          else
+            super
+          end
+        end
+      end
+
+      class << self
+        prepend(TapiocaGenericTypeCoercePatch)
+      end
+    end
+  end
+
+  module Private
+    module Casts
+      module TapiocaGenericTypeCastPatch
+        # https://github.com/sorbet/sorbet/commit/b8d64c7fd9a08e2b9159b5d592bc2de6d586b44a
+        # inlines the Module fast path in `T.let`, `T.cast`, `T.bind`, and
+        # `T.assert_type!`, so generic module clones can reach this cast path
+        # without going through `T::Utils::Private::TapiocaGenericTypeCoercePatch`.
+        def cast(value, type, cast_method)
+          if type.respond_to?(:__tapioca_override_type)
+            type = type.__tapioca_override_type
+          end
+
+          super(value, type, cast_method)
+        end
+      end
+
+      class << self
+        prepend(TapiocaGenericTypeCastPatch)
+      end
+    end
+  end
+end

spec/tapioca/runtime/generic_type_registry_spec.rb

@@ -57,6 +57,28 @@ class GenericTypeRegistrySpec < Minitest::Spec
           end
         end
 
+        describe ".register_type" do
+          it "allows generic interface implementations to be cast to generic interface types" do
+            T.let(SampleGenericInterfaceImplementation.new, SampleGenericInterface[Object])
+          end
+
+          it "does not reuse generic instances for redefined constants with the same name" do
+            first_constant, first_generic_type = register_reloadable_generic
+
+            self.class.send(:remove_const, :ReloadableGeneric) # rubocop:disable RSpec/RemoveConst
+
+            second_constant, second_generic_type = register_reloadable_generic
+
+            refute_same(first_generic_type, second_generic_type)
+            assert_operator(first_generic_type, :<, first_constant)
+            assert_operator(second_generic_type, :<, second_constant)
+          ensure
+            if self.class.const_defined?(:ReloadableGeneric, false)
+              self.class.send(:remove_const, :ReloadableGeneric) # rubocop:disable RSpec/RemoveConst
+            end
+          end
+        end
+
         describe "the patch for .inherited on generic classes" do
           # This is more of an internal detail and cross-cutting concern of all the public APIs,
           # but it's easier to test here on its own.
@@ -74,12 +96,37 @@ class GenericTypeRegistrySpec < Minitest::Spec
         end
       end
 
+      def register_reloadable_generic
+        constant = T.let(Class.new, T.untyped)
+        constant.extend(T::Generic)
+        constant.const_set(:Element, constant.type_member)
+
+        self.class.const_set(:ReloadableGeneric, constant)
+        [constant, constant[Object]]
+      end
+
       class SampleGenericClass
         extend T::Generic
 
         Element = type_member
       end
 
+      module SampleGenericInterface
+        extend T::Generic
+
+        interface!
+
+        Element = type_member
+      end
+
+      class SampleGenericInterfaceImplementation
+        extend T::Generic
+
+        include SampleGenericInterface
+
+        Element = type_member
+      end
+
       class RaisesInInheritedCallback
         extend T::Generic