Skip to content

[RemoteMirror] Fix lookup of generic type parameters #65332

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 24, 2023
Merged

[RemoteMirror] Fix lookup of generic type parameters #65332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89f98e9
Partially Revert "Fix wrong calculation of generic params per level w…
tbkka Apr 20, 2023
02160e6
Recursively reconstruct nested generics, skipping non-generic
tbkka Apr 20, 2023
16c0729
Correct and simplify the test from PR #62854
tbkka Apr 20, 2023
ed28d8e
Add a test case where the innermost type has no generic parameters
tbkka Apr 22, 2023
5350cd5
Rework createBoundGenericTypeReconstructingParent
tbkka Apr 22, 2023
488fb82
Refactor to handle the parent list separately
tbkka Apr 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 7 additions & 13 deletions include/swift/Remote/MetadataReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2959,19 +2959,13 @@ class MetadataReader {
// Only consider generic contexts of type class, enum or struct.
// There are other context types that can be generic, but they should
// not affect the generic shape.
if (current->getKind() == ContextDescriptorKind::Class ||
current->getKind() == ContextDescriptorKind::Enum ||
current->getKind() == ContextDescriptorKind::Struct) {
if (genericContext) {
auto contextHeader = genericContext->getGenericContextHeader();
paramsPerLevel.emplace_back(contextHeader.NumParams -
runningCount);
runningCount += paramsPerLevel.back();
} else {
// If there is no generic context, this is a non-generic type
// which has 0 generic parameters.
paramsPerLevel.emplace_back(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
if (genericContext &&
(current->getKind() == ContextDescriptorKind::Class ||
current->getKind() == ContextDescriptorKind::Enum ||
current->getKind() == ContextDescriptorKind::Struct)) {
auto contextHeader = genericContext->getGenericContextHeader();
paramsPerLevel.emplace_back(contextHeader.NumParams - runningCount);
runningCount += paramsPerLevel.back();
}
};
countLevels(descriptor, runningCount);
Expand Down
164 changes: 103 additions & 61 deletions include/swift/RemoteInspection/TypeRefBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,65 +567,66 @@ class TypeRefBuilder {
return nullptr;
}

const BoundGenericTypeRef *createBoundGenericTypeReconstructingParent(
const NodePointer node, const TypeRefDecl &decl, size_t shapeIndex,
const llvm::ArrayRef<const TypeRef *> &args, size_t argsIndex) {
if (!node || !node->hasChildren())
return nullptr;

auto maybeGenericParamsPerLevel = decl.genericParamsPerLevel;
if (!maybeGenericParamsPerLevel)
return nullptr;

auto genericParamsPerLevel = *maybeGenericParamsPerLevel;

auto kind = node->getKind();
// Kinds who have a "BoundGeneric..." variant.
if (kind != Node::Kind::Class && kind != Node::Kind::Structure &&
kind != Node::Kind::Enum)
return nullptr;
auto mangling = Demangle::mangleNode(node);
if (!mangling.isSuccess())
return nullptr;

if (shapeIndex >= genericParamsPerLevel.size())
return nullptr;

auto numGenericArgs = genericParamsPerLevel[shapeIndex];

auto startOffsetFromEnd = argsIndex + numGenericArgs;
auto endOffsetFromEnd = argsIndex;
if (startOffsetFromEnd > args.size() || endOffsetFromEnd > args.size())
return nullptr;

std::vector<const TypeRef *> genericParams(
args.end() - startOffsetFromEnd, args.end() - endOffsetFromEnd);

const BoundGenericTypeRef *parent = nullptr;
if (node->hasChildren()) {
// Skip over nodes that are not of type class, enum or struct
auto parentNode = node->getFirstChild();
while (parentNode->getKind() != Node::Kind::Class &&
parentNode->getKind() != Node::Kind::Structure &&
parentNode->getKind() != Node::Kind::Enum) {
if (parentNode->hasChildren()) {
parentNode = parentNode->getFirstChild();
} else {
parentNode = nullptr;
break;
}
// Construct a bound generic type ref along with the parent type info
// The parent list contains every parent type with at least 1 generic
// type parameter.
const BoundGenericTypeRef *reconstructParentsOfBoundGenericType(
const NodePointer startNode,
const std::vector<size_t> &genericParamsPerLevel,
const llvm::ArrayRef<const TypeRef *> &args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

@mikeash mikeash Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

NodePointer node = startNode;
while (nodes.size() < genericParamsPerLevel.size() - 1) {
if (!node || !node->hasChildren()) {
return nullptr;
}
if (parentNode) {
if (shapeIndex > 0)
parent = createBoundGenericTypeReconstructingParent(
parentNode, decl, --shapeIndex, args, argsIndex + numGenericArgs);
else
return nullptr;
node = node->getFirstChild();
switch (node->getKind()) {
case Node::Kind::Class:
case Node::Kind::Structure:
case Node::Kind::Enum:
nodes.push_back(node);
break;
default:
break;
}
}
assert(nodes.size() == genericParamsPerLevel.size() - 1);

// 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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// Walk the list of parent types together with
// the generic argument list...
const BoundGenericTypeRef *typeref = nullptr;
auto argBegin = args.begin();
for (size_t i = 0; i < nodes.size(); i++) {
// Get the mangling for this node
auto mangling = Demangle::mangleNode(nodes[i]);
if (!mangling.isSuccess()) {
return nullptr;
}

return BoundGenericTypeRef::create(*this, mangling.result(), genericParams,
parent);
// Use the next N params for this node
auto numGenericArgs = genericParamsPerLevel[i];
// Skip nodes that don't have any actual type params.
if (numGenericArgs == 0) {
continue;
}
auto argEnd = argBegin + numGenericArgs;
std::vector<const TypeRef *> params(argBegin, argEnd);
argBegin = argEnd;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// Extend the typeref list towards the innermost type
typeref = BoundGenericTypeRef::create(*this, mangling.result(), params, typeref);
}
return typeref;
}

const BoundGenericTypeRef *
Expand All @@ -634,17 +635,58 @@ class TypeRefBuilder {
if (!builtTypeDecl)
return nullptr;

if (!builtTypeDecl->genericParamsPerLevel)
// If there aren't generic params on the parent types, we just emit
// a single BG typeref with all the generic args
auto maybeGenericParamsPerLevel = builtTypeDecl->genericParamsPerLevel;
if (!maybeGenericParamsPerLevel) {
return BoundGenericTypeRef::create(*this, builtTypeDecl->mangledName, args, nullptr);
}


// Otherwise, work from a full demangle tree to produce a
// typeref that includes information about parent generic args
auto node = Dem.demangleType(builtTypeDecl->mangledName);
if (!node || !node->hasChildren() || node->getKind() != Node::Kind::Type)
if (!node || !node->hasChildren() || node->getKind() != Node::Kind::Type) {
return nullptr;
}
auto startNode = node->getFirstChild();
auto mangling = Demangle::mangleNode(startNode);
if (!mangling.isSuccess()) {
return nullptr;
}

// 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (genericParamsPerLevel.size() > 1000 || args.size() > 1000) {
return nullptr;
}
size_t totalParams = 0;
for (size_t i = 0; i < genericParamsPerLevel.size(); i++) {
if (genericParamsPerLevel[i] > args.size()) {
return nullptr;
}
totalParams += genericParamsPerLevel[i];
}
if (totalParams != args.size()) {
return nullptr;
}

auto type = node->getFirstChild();
return createBoundGenericTypeReconstructingParent(
type, *builtTypeDecl, builtTypeDecl->genericParamsPerLevel->size() - 1, args, 0);
// Reconstruct all parents that have non-zero generic params
auto parents = reconstructParentsOfBoundGenericType(startNode, genericParamsPerLevel, args);

// Collect the final set of generic params for the
// innermost type. Note: This will sometimes be empty:
// consider `Foo<Int, String>.Bar.Baz<Double>.Quux`
// which has 2 parents in the parent list
// (`Foo<Int,String>`, `Baz<Double>`), and the
// startNode is `Quux` with no params.
auto numGenericArgs = genericParamsPerLevel[genericParamsPerLevel.size() - 1];
auto argBegin = args.end() - numGenericArgs;
std::vector<const TypeRef *> params(argBegin, args.end());

// Build and return the top typeref
return BoundGenericTypeRef::create(*this, mangling.result(), params, parents);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

const BoundGenericTypeRef *
Expand Down
51 changes: 15 additions & 36 deletions validation-test/Reflection/reflect_nested.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// REQUIRES: reflection_test_support
// REQUIRES: executable_test
// UNSUPPORTED: use_os_stdlib
Expand All @@ -26,24 +26,14 @@ var obj = OuterGeneric.Inner.Innermost(x: 17, y: "hello")

reflect(object: obj)

// CHECK-64: Reflecting an object.
// 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.String)
// CHECK-64-NEXT: (bound_generic_class reflect_nested.OuterGeneric.Inner
Copy link
Contributor Author

@tbkka tbkka Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

// CHECK-64-NEXT: (bound_generic_class reflect_nested.OuterGeneric
// CHECK-64-NEXT: (struct Swift.Int))))
// CHECK: Reflecting an object.
// CHECK: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
// CHECK: Type reference:
// CHECK: (bound_generic_class reflect_nested.OuterGeneric.Inner.Innermost
// CHECK-NEXT: (struct Swift.String)
// CHECK-NEXT: (bound_generic_class reflect_nested.OuterGeneric
// CHECK-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.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))))

class OuterNonGeneric {
class InnerGeneric<T, U> {
var x: T
Expand All @@ -59,24 +49,13 @@ class OuterNonGeneric {
var obj2 = OuterNonGeneric.InnerGeneric(x: 17, y: "hello")
reflect(object: obj2)

// CHECK-64: Reflecting an object.
// CHECK-64: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
// CHECK-64: Type reference:
// CHECK-64: (bound_generic_class reflect_nested.OuterNonGeneric.InnerGeneric
// CHECK-64-NEXT: (struct Swift.Int)
// CHECK-64-NEXT: (struct Swift.String)
// CHECK-64-NEXT: (bound_generic_class reflect_nested.OuterNonGeneric))

// 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.OuterNonGeneric.InnerGeneric
// CHECK-32-NEXT: (struct Swift.Int)
// CHECK-32-NEXT: (struct Swift.String)
// CHECK-32-NEXT: (bound_generic_class reflect_nested.OuterNonGeneric))
// CHECK: Reflecting an object.
// CHECK: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
// CHECK: Type reference:
// CHECK: (bound_generic_class reflect_nested.OuterNonGeneric.InnerGeneric
// CHECK-NEXT: (struct Swift.Int)
// CHECK-NEXT: (struct Swift.String))

doneReflecting()

// CHECK-64: Done.

// CHECK-32: Done.
// CHECK: Done.
Loading