[RemoteMirror] Fix lookup of generic type parameters #65332

tbkka · 2023-04-20T21:37:39Z

PR #62854 identified an issue with generic type arguments. Unfortunately, that fix didn't quite work, since it inadvertently renumbered those arguments, leading to failures when we tried to look up generic type arguments when we had nested generic and non-generic types interspersed.

This is (I hope) a more correct fix. It changes how the parent types are recursively reconstructed to ensure that we number the nesting levels the same way the compiler does. I've also built some additional tests that verify not only the type reference (name) is correctly printed, but that the built types have the correct layout (which implies that the various generic type arguments are being correctly handled).

Resolves rdar://106790077

…hen building decl" (Keeps the test, but reverts the code changes; the test will be updated later.) This reverts most of commit 893d83b from PR swiftlang#62854

This is a more correct fix for the issue that inspired PR swiftlang#62854. In particular, this does not change the numbering of generic argument levels, and so does not break generic type parameter lookups. Added a new test case to verify that nested types that mix generics and non-generics in different ways consistently identify the correct generic argument labels.

The new logic no longer includes non-generic parent types as part of the type reference. This also combines the 32- and 64-bit tests validations, since they were identical for this test anyway.

tbkka

A few comments on the code here to help reviewers.

tbkka · 2023-04-20T21:38:50Z

include/swift/Remote/MetadataReader.h

-            } else {
-              // If there is no generic context, this is a non-generic type
-              // which has 0 generic parameters.
-              paramsPerLevel.emplace_back(0);


This was the key bug introduced by PR #62854: Adding a 0 params marker here caused non-generic types to get counted as generic types, throwing off the generic type parameter matching.

tbkka · 2023-04-20T21:39:44Z

include/swift/RemoteInspection/TypeRefBuilder.h

@@ -567,62 +567,58 @@ class TypeRefBuilder {
    return nullptr;
  }

+  // Recursively constructs TypeRef for a bound generic, including
+  // the enclosing (parent) generic contexts


I've significantly rearranged the logic here to make it more cleanly recursive (which allowed me to eliminate the iterative walk to find a parent type).

tbkka · 2023-04-20T21:40:43Z

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 | %FileCheck %s --check-prefix=CHECK


Since the CHECK-64 and CHECK-32 lines here are all identical, I just eliminated the target-ptrsize distinction.

tbkka · 2023-04-20T21:41:26Z

validation-test/Reflection/reflect_nested.swift

-// CHECK-64: Type reference:
-// CHECK-64: (bound_generic_class reflect_nested.OuterGeneric.Inner.Innermost
-// CHECK-64-NEXT:   (struct Swift.String)
-// CHECK-64-NEXT:   (bound_generic_class reflect_nested.OuterGeneric.Inner


The one actual change in this test: reflect_nested.OuterGeneric.Inner is not a bound_generic_class, so does not need to be in the list of generic parent types.

tbkka · 2023-04-20T21:43:20Z

validation-test/Reflection/reflect_nested_generic.swift

+struct Outer2 {
+  enum E<T: P> {
+    struct Inner {
+      enum F<U: P> {


Changing this to a multi-payload Enum fails. I think that's just because all the MPE fixes from #64852 haven't landed yet.

tbkka · 2023-04-20T21:44:23Z

@swift-ci Please test

mikeash

Nice, that's a subtle bug.

include/swift/RemoteInspection/TypeRefBuilder.h

The recursive structure made it a little awkward to correctly distinguish between the root node (which has to be included regardless of whether it has direct generic params) and the parent nodes (which must only be included if they have direct params). So I rewrote this to do a simple two-pass iteration: * The first pass walks the parent list collecting candidates * The second pass walks the list backwards, assigning generic params We then just stack the start node onto the end of the list regardless of whether it has generic params.

Handle the top node directly in `createBoundGenericType` so that we can be sure to always include it regardless of whether it has direct generic type params or not. The helper function can now focus only on handling parents (where we must only include ones that actually contain generic type params). This separates the two different cases and makes it a little easier to follow. I refactored `reconstructParentsOfBoundGenericType` to be iterative rather than recursive, though I'm not sure that really matters.

tbkka · 2023-04-22T20:42:05Z

include/swift/RemoteInspection/TypeRefBuilder.h

+  const BoundGenericTypeRef *reconstructParentsOfBoundGenericType(
+      const NodePointer startNode,
+      const std::vector<size_t> &genericParamsPerLevel,
+      const llvm::ArrayRef<const TypeRef *> &args)


After fussing around with different structures here, I finally reduced this to only handle reconstructing the parent list. That means it can consistently ignore nodes with no direct generic params. I also reworked it to be iterative rather than recursive, though that's not particularly relevant -- I just found it a little easier to follow that way.

tbkka · 2023-04-22T20:43:48Z

include/swift/RemoteInspection/TypeRefBuilder.h

+    // We're now going to build the type tree from the
+    // outermost parent in, which matches the order of
+    // the generic parameter list and genericParamsPerLevel.
+    std::reverse(nodes.begin(), nodes.end());


Above, we walk the parent list from the innermost to outermost. That is, for Foo<Int>.Bar<Double>.Baz<String>.Quux, we visit Baz, then Bar, then Foo (skipping Quux, since that's the start node, not one of the parents we're collecting here.

I then need to build the new list in the opposite order, which is why I reverse the list here.

tbkka · 2023-04-22T20:44:13Z

include/swift/RemoteInspection/TypeRefBuilder.h

+      }
+      auto argEnd = argBegin + numGenericArgs;
+      std::vector<const TypeRef *> params(argBegin, argEnd);
+      argBegin = argEnd;


Walking the list in order makes it a lot easier to track which arguments go with with parent node.

tbkka · 2023-04-22T20:45:58Z

include/swift/RemoteInspection/TypeRefBuilder.h

+    // Sanity:  Verify that the generic params per level add
+    // up exactly to the number of args we were provided, and
+    // that we don't have a rediculous number of either one
+    auto genericParamsPerLevel = *maybeGenericParamsPerLevel;


There were a lot of checks to ensure that the shape list (params-per-level) and the actual args list made sense together. I found it easier to pull that together into a single unified check: Ensure that the total number of params mentioned in params-per-level adds up exactly to the number of args we have. I also imposed an arbitrary limit of 1000 nested types and 1000 generic args in order to be comfortably certain that we'll never have problems from integer wrapping.

tbkka · 2023-04-22T20:46:45Z

include/swift/RemoteInspection/TypeRefBuilder.h

+    std::vector<const TypeRef *> params(argBegin, args.end());
+
+    // Build and return the top typeref
+    return BoundGenericTypeRef::create(*this, mangling.result(), params, parents);


The innermost node always gets included here, since that's the node we're actually constructing the typeref for. This happens even if the innermost node has no explicit generic type params.

tbkka · 2023-04-22T20:47:31Z

validation-test/Reflection/reflect_nested_generic.swift

+//CHECK: Enum value:
+//CHECK-NEXT: (enum_value name=b index=2)
+
+reflect(enum: Outer2.E<S1>.Inner.F<S2>.Innerer?.none)


As @augusto2112 suggested, I added a test where the innermost type has no generic type parameters.

tbkka · 2023-04-22T20:47:52Z

@swift-ci Please test

augusto2112

This looks much better than the original!

mikeash · 2023-04-24T13:27:00Z

include/swift/RemoteInspection/TypeRefBuilder.h

+    // Collect the first N parents that potentially have generic args
+    // (Ignore the last genericParamPerLevel, which
+    // applies to the startNode itself.)
+    std::vector<NodePointer> nodes;


Fairly unimportant, but if you use a SmallVector here (and a couple other places), it'll avoid allocating in the common case where there number of parents is small.

tbkka added 3 commits April 20, 2023 14:30

Partially Revert "Fix wrong calculation of generic params per level w…

89f98e9

…hen building decl" (Keeps the test, but reverts the code changes; the test will be updated later.) This reverts most of commit 893d83b from PR swiftlang#62854

Correct and simplify the test from PR swiftlang#62854

16c0729

The new logic no longer includes non-generic parent types as part of the type reference. This also combines the 32- and 64-bit tests validations, since they were identical for this test anyway.

tbkka requested review from mikeash, augusto2112 and btroller April 20, 2023 21:37

tbkka commented Apr 20, 2023

View reviewed changes

mikeash approved these changes Apr 21, 2023

View reviewed changes

augusto2112 reviewed Apr 21, 2023

View reviewed changes

include/swift/RemoteInspection/TypeRefBuilder.h Outdated Show resolved Hide resolved

tbkka added 3 commits April 22, 2023 11:52

Add a test case where the innermost type has no generic parameters

ed28d8e

tbkka commented Apr 22, 2023

View reviewed changes

augusto2112 approved these changes Apr 24, 2023

View reviewed changes

mikeash reviewed Apr 24, 2023

View reviewed changes

tbkka merged commit db14ae8 into swiftlang:main Apr 24, 2023

This was referenced Apr 24, 2023

[RemoteMirror] Fix handling of generic MPEs #64852

Merged

5.9 [RemoteMirror] Fix lookup of generic type parameters #65524

Merged

tbkka deleted the tbkka-RemoteMirror-nested-generics branch August 1, 2024 16:37

[RemoteMirror] Fix lookup of generic type parameters #65332

[RemoteMirror] Fix lookup of generic type parameters #65332

Uh oh!

Conversation

tbkka commented Apr 20, 2023

Uh oh!

tbkka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbkka Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbkka commented Apr 20, 2023

Uh oh!

mikeash left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbkka commented Apr 22, 2023

Uh oh!

augusto2112 left a comment

Choose a reason for hiding this comment

Uh oh!

mikeash Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tbkka Apr 20, 2023 •

edited

Loading

mikeash Apr 24, 2023 •

edited

Loading