Fix nested generic typerefs applying generic params at the wrong level

Generic params of typerefs are supposed to be "attached" on the level
they belong, not as a flat list, unlike other parts of the system. Fix
the application of bound generic params by checking how many were
already applied in the hierarchy and ignoring those already attached.

Author: augusto2112

include/swift/Remote/MetadataReader.h

@@ -30,12 +30,16 @@
 #include "swift/Runtime/HeapObject.h"
 #include "swift/Basic/Unreachable.h"
 
+#include <type_traits>
 #include <vector>
 #include <unordered_map>
 
 #include <inttypes.h>
 
 namespace swift {
+namespace reflection {
+  class TypeRefBuilder;
+}
 namespace remote {
 
 template <typename BuiltType>
@@ -2905,25 +2909,50 @@ class MetadataReader {
     return builtSubsts;
   }
 
-  BuiltType readNominalTypeFromMetadata(MetadataRef origMetadata,
-                                        bool skipArtificialSubclasses = false) {
-    auto metadata = origMetadata;
-    auto descriptorAddress =
-      readAddressOfNominalTypeDescriptor(metadata,
-                                         skipArtificialSubclasses);
-    if (!descriptorAddress)
+  /// "Generic" version of readNominalTypeFromDescriptor when BuilderType !=
+  /// TypeRefBuilder.
+  template <
+      typename T = BuilderType,
+      std::enable_if_t<!std::is_same<T, reflection::TypeRefBuilder>::value,
+                       bool> = true>
+  BuiltType readNominalTypeFromDescriptor(MetadataRef metadata,
+                                          ContextDescriptorRef descriptor) {
+    if (!descriptor)
       return BuiltType();
 
-    // If we've skipped an artificial subclasses, check the cache at
-    // the superclass.  (This also protects against recursion.)
-    if (skipArtificialSubclasses && metadata != origMetadata) {
-      auto it = TypeCache.find(getAddress(metadata));
-      if (it != TypeCache.end())
-        return it->second;
+    // From that, attempt to resolve a nominal type.
+    BuiltTypeDecl typeDecl = buildNominalTypeDecl(descriptor);
+    if (!typeDecl)
+      return BuiltType();
+
+    // Build the nominal type.
+    BuiltType nominal;
+    if (descriptor->isGeneric()) {
+      // Resolve the generic arguments.
+      auto builtGenerics = getGenericSubst(metadata, descriptor);
+      if (builtGenerics.empty())
+        return BuiltType();
+      nominal = Builder.createBoundGenericType(typeDecl, builtGenerics);
+    } else {
+      nominal = Builder.createNominalType(typeDecl);
     }
 
-    // Read the nominal type descriptor.
-    ContextDescriptorRef descriptor = readContextDescriptor(descriptorAddress);
+    return nominal;
+  }
+
+  /// Specialized version of readNominalTypeFromDescriptor where BuilderType ==
+  /// TypeRefBuilder. There are two main differences in this specialization:
+  /// 1 - We attempt to rebuild the parent typeref and pass that along
+  /// when creating our own typeref.
+  /// 2 - We remove the generic parameters that belong to our typeref's parents
+  /// from the flat array of generic parameters we read from the context
+  /// descriptor, as typerefs store generic parameters per level instead of as a
+  /// single flat array.
+  template <typename T = BuilderType,
+            std::enable_if_t<std::is_same<T, reflection::TypeRefBuilder>::value,
+                             bool> = true>
+  BuiltType readNominalTypeFromDescriptor(MetadataRef metadata,
+                                          ContextDescriptorRef descriptor) {
     if (!descriptor)
       return BuiltType();
 
@@ -2932,21 +2961,81 @@ class MetadataReader {
     if (!typeDecl)
       return BuiltType();
 
+    BuiltType parent;
+    size_t numGenericParamsInParent = 0;
+    if (auto parentContextRef = readParentContextDescriptor(descriptor)) {
+      if (parentContextRef->isResolved()) {
+        if (auto parentContext = parentContextRef->getResolved()) {
+          // Attempt to rebuild the parent as typerefs uses a "nested"
+          // representation to represent the type hierarchy.
+          parent = readNominalTypeFromDescriptor(metadata, parentContext);
+          // Context descriptors use a flat array to store all the generic
+          // parameters, instead of storing them by level, which is what
+          // typerefs do. Store the number of generic parameters in the parent
+          // so we can adjust the number of generic parameters when building our
+          // own typeref. Note that we only need to go one level up the
+          // hierarchy, since the parent will have a flat array of generic
+          // paramaters including all of it's parents parameters.
+          if (auto genericContext = parentContext->getGenericContext()) {
+            auto contextHeader = genericContext->getGenericContextHeader();
+            numGenericParamsInParent = contextHeader.NumParams;
+          }
+        }
+      }
+    }
+
     // Build the nominal type.
     BuiltType nominal;
     if (descriptor->isGeneric()) {
       // Resolve the generic arguments.
       auto builtGenerics = getGenericSubst(metadata, descriptor);
       if (builtGenerics.empty())
         return BuiltType();
-      nominal = Builder.createBoundGenericType(typeDecl, builtGenerics);
+
+      // Erase the generic parameters that belong to the parents.
+      if (numGenericParamsInParent > 0)
+        builtGenerics.erase(
+            builtGenerics.begin(),
+            builtGenerics.begin() +
+                std::min(numGenericParamsInParent, builtGenerics.size()));
+
+      if (parent)
+        nominal =
+            Builder.createBoundGenericType(typeDecl, builtGenerics, parent);
+      else
+        nominal = Builder.createBoundGenericType(typeDecl, builtGenerics);
+    } else if (parent) {
+      nominal = Builder.createNominalType(typeDecl, parent);
     } else {
       nominal = Builder.createNominalType(typeDecl);
     }
 
+    return nominal;
+  }
+
+  BuiltType readNominalTypeFromMetadata(MetadataRef origMetadata,
+                                        bool skipArtificialSubclasses = false) {
+    auto metadata = origMetadata;
+    auto descriptorAddress =
+        readAddressOfNominalTypeDescriptor(metadata, skipArtificialSubclasses);
+    if (!descriptorAddress)
+      return BuiltType();
+
+    // If we've skipped an artificial subclasses, check the cache at
+    // the superclass.  (This also protects against recursion.)
+    if (skipArtificialSubclasses && metadata != origMetadata) {
+      auto it = TypeCache.find(getAddress(metadata));
+      if (it != TypeCache.end())
+        return it->second;
+    }
+
+    // Read the nominal type descriptor.
+    ContextDescriptorRef descriptor = readContextDescriptor(descriptorAddress);
+    auto nominal = readNominalTypeFromDescriptor(origMetadata, descriptor);
+
     if (!nominal)
       return BuiltType();
-    
+
     TypeCache[getAddress(metadata)] = nominal;
 
     // If we've skipped an artificial subclass, remove the
@@ -2958,8 +3047,9 @@ class MetadataReader {
     return nominal;
   }
 
-  BuiltType readNominalTypeFromClassMetadata(MetadataRef origMetadata,
-                                       bool skipArtificialSubclasses = false) {
+  BuiltType
+  readNominalTypeFromClassMetadata(MetadataRef origMetadata,
+                                   bool skipArtificialSubclasses = false) {
     auto classMeta = cast<TargetClassMetadata>(origMetadata);
     if (classMeta->isTypeMetadata())
       return readNominalTypeFromMetadata(origMetadata, skipArtificialSubclasses);

validation-test/Reflection/reflect_nested.swift

@@ -1,7 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: %target-build-swift -lswiftSwiftReflectionTest %s -o %t/reflect_nested
 // RUN: %target-codesign %t/reflect_nested
-// RUN: %target-run %target-swift-reflection-test %t/reflect_nested 2>&1 | %FileCheck %s --check-prefix=CHECK-%target-ptrsize
+// RUN: %target-run %target-swift-reflection-test %t/reflect_nested 2>&1 
 // REQUIRES: reflection_test_support
 // REQUIRES: executable_test
 // UNSUPPORTED: use_os_stdlib
@@ -30,15 +30,19 @@ reflect(object: obj)
 // CHECK-64: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
 // CHECK-64: Type reference:
 // CHECK-64: (bound_generic_class reflect_nested.OuterGeneric.Inner.Innermost
-// CHECK-64-NEXT: (struct Swift.Int)
 // CHECK-64-NEXT: (struct Swift.String)
+// CHECK-64-NEXT: (bound_generic_class reflect_nested.OuterGeneric.Inner
+// CHECK-64-NEXT: (bound_generic_class reflect_nested.OuterGeneric
+// CHECK-64-NEXT: (struct Swift.Int))))
 
 // CHECK-32: Reflecting an object.
 // CHECK-32: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
 // CHECK-32: Type reference:
 // CHECK-32: (bound_generic_class reflect_nested.OuterGeneric.Inner.Innermost
-// CHECK-32-NEXT: (struct Swift.Int)
 // CHECK-32-NEXT: (struct Swift.String)
+// CHECK-32-NEXT: (bound_generic_class reflect_nested.OuterGeneric.Inner
+// CHECK-32-NEXT: (bound_generic_class reflect_nested.OuterGeneric
+// CHECK-32-NEXT: (struct Swift.Int))))
 
 doneReflecting()