Skip to content

Fix name lookup in nested protocol inheritance clauses #72713

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 1 commit into from
Apr 29, 2024
Merged

Fix name lookup in nested protocol inheritance clauses #72713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 54 additions & 27 deletions lib/AST/NameLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,8 @@ static DirectlyReferencedTypeDecls
directReferencesForTypeRepr(Evaluator &evaluator, ASTContext &ctx,
TypeRepr *typeRepr, DeclContext *dc,
bool allowUsableFromInline,
bool rhsOfSelfRequirement);
bool rhsOfSelfRequirement,
bool allowProtocolMembers);
Copy link
Contributor

@slavapestov slavapestov Mar 30, 2024

Choose a reason for hiding this comment

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

It’s probably time to replace these boolean flags with an OptionSet.

Copy link
Contributor Author

@karwa karwa Apr 10, 2024

Choose a reason for hiding this comment

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

@slavapestov There are a couple of reasons I wanted to avoid doing that with this PR:

  1. In my opinion, this kind of cycle should be avoided in a more structured way by ASTScope lookup. It is obvious that if it walks up to a parent scope and enumerates the children, it will find the scope it just visited, and depending on which methods happen to be called, it is incredibly easy for it to get in to a request cycle. If it knew the child it had just visited, it would be able to avoid this.

    I tried to implement this, but there's quite a bit of plumbing to do, and of course I don't know if there are any longer-term plans to rework this lookup so I wasn't sure if it was worth doing right now. I'd like to keep this change low-risk so it can be cherry-picked to 6.0, and major refactoring of name lookup seems to me higher risk than is absolutely necessary to fix this issue.

  2. I haven't audited all of the places which pass allowProtocolMembers as true. Right now, everywhere passes it as true in order to maintain the behaviour before this patch, but I suspect some places could afford to turn it off for a slight performance improvement.

Also, could you run the smoke test again? There was a failure on Mac, but the data isn't available on CI any more (I think it was something to do with LLDB? Possibly unrelated to this change. I tried the LLDB tests locally and don't see anything related)

Copy link
Contributor Author

@karwa karwa Apr 15, 2024

Choose a reason for hiding this comment

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

Ping @slavapestov


/// Retrieve the set of type declarations that are directly referenced from
/// the given type.
Expand Down Expand Up @@ -1148,7 +1149,8 @@ SelfBounds SelfBoundsFromWhereClauseRequest::evaluate(
rhsDecls = directReferencesForTypeRepr(evaluator, ctx, typeRepr,
const_cast<DeclContext *>(dc),
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/true);
/*rhsOfSelfRequirement=*/true,
/*allowProtocolMembers=*/true);
}

SmallVector<ModuleDecl *, 2> modulesFound;
Expand Down Expand Up @@ -1212,7 +1214,8 @@ TypeDeclsFromWhereClauseRequest::evaluate(Evaluator &evaluator,
auto resolve = [&](TypeRepr *typeRepr) {
auto decls = directReferencesForTypeRepr(evaluator, ctx, typeRepr, ext,
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers=*/true);
result.first.insert(result.first.end(),
decls.first.begin(),
decls.first.end());
Expand Down Expand Up @@ -2834,10 +2837,11 @@ directReferencesForUnqualifiedTypeLookup(DeclNameRef name,
SourceLoc loc, DeclContext *dc,
LookupOuterResults lookupOuter,
bool allowUsableFromInline,
bool rhsOfSelfRequirement) {
UnqualifiedLookupOptions options =
UnqualifiedLookupFlags::TypeLookup |
UnqualifiedLookupFlags::AllowProtocolMembers;
bool rhsOfSelfRequirement,
bool allowProtocolMembers) {
UnqualifiedLookupOptions options = UnqualifiedLookupFlags::TypeLookup;
if (allowProtocolMembers)
options |= UnqualifiedLookupFlags::AllowProtocolMembers;
if (lookupOuter == LookupOuterResults::Included)
options |= UnqualifiedLookupFlags::IncludeOuterResults;

Expand Down Expand Up @@ -2951,11 +2955,12 @@ static DirectlyReferencedTypeDecls
directReferencesForDeclRefTypeRepr(Evaluator &evaluator, ASTContext &ctx,
DeclRefTypeRepr *repr, DeclContext *dc,
bool allowUsableFromInline,
bool rhsOfSelfRequirement) {
bool rhsOfSelfRequirement,
bool allowProtocolMembers) {
if (auto *qualIdentTR = dyn_cast<QualifiedIdentTypeRepr>(repr)) {
auto result = directReferencesForTypeRepr(
evaluator, ctx, qualIdentTR->getBase(), dc,
allowUsableFromInline, rhsOfSelfRequirement);
allowUsableFromInline, rhsOfSelfRequirement, allowProtocolMembers);

// For a qualified identifier, perform qualified name lookup.
result.first = directReferencesForQualifiedTypeLookup(
Expand All @@ -2968,14 +2973,15 @@ directReferencesForDeclRefTypeRepr(Evaluator &evaluator, ASTContext &ctx,
// For an unqualified identifier, perform unqualified name lookup.
return directReferencesForUnqualifiedTypeLookup(
repr->getNameRef(), repr->getLoc(), dc, LookupOuterResults::Excluded,
allowUsableFromInline, rhsOfSelfRequirement);
allowUsableFromInline, rhsOfSelfRequirement, allowProtocolMembers);
}

static DirectlyReferencedTypeDecls
directReferencesForTypeRepr(Evaluator &evaluator,
ASTContext &ctx, TypeRepr *typeRepr,
DeclContext *dc, bool allowUsableFromInline,
bool rhsOfSelfRequirement) {
bool rhsOfSelfRequirement,
bool allowProtocolMembers) {
DirectlyReferencedTypeDecls result;

switch (typeRepr->getKind()) {
Expand All @@ -2988,7 +2994,8 @@ directReferencesForTypeRepr(Evaluator &evaluator,
return directReferencesForTypeRepr(evaluator, ctx,
attributed->getTypeRepr(), dc,
allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);
}

case TypeReprKind::Composition: {
Expand All @@ -2997,7 +3004,8 @@ directReferencesForTypeRepr(Evaluator &evaluator,
auto componentResult =
directReferencesForTypeRepr(evaluator, ctx, component, dc,
allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);
result.first.insert(result.first.end(),
componentResult.first.begin(),
componentResult.first.end());
Expand All @@ -3013,7 +3021,8 @@ directReferencesForTypeRepr(Evaluator &evaluator,
return directReferencesForDeclRefTypeRepr(evaluator, ctx,
cast<DeclRefTypeRepr>(typeRepr),
dc, allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);

case TypeReprKind::Dictionary:
result.first.push_back(ctx.getDictionaryDecl());
Expand All @@ -3025,7 +3034,8 @@ directReferencesForTypeRepr(Evaluator &evaluator,
result = directReferencesForTypeRepr(evaluator, ctx,
tupleRepr->getElementType(0), dc,
allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);
} else {
result.first.push_back(ctx.getBuiltinTupleDecl());
}
Expand All @@ -3037,23 +3047,26 @@ directReferencesForTypeRepr(Evaluator &evaluator,
return directReferencesForTypeRepr(evaluator, ctx,
packExpansionRepr->getElementType(), dc,
allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);
}

case TypeReprKind::PackExpansion: {
auto packExpansionRepr = cast<PackExpansionTypeRepr>(typeRepr);
return directReferencesForTypeRepr(evaluator, ctx,
packExpansionRepr->getPatternType(), dc,
allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);
}

case TypeReprKind::PackElement: {
auto packReferenceRepr = cast<PackElementTypeRepr>(typeRepr);
return directReferencesForTypeRepr(evaluator, ctx,
packReferenceRepr->getPackType(), dc,
allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);
}

case TypeReprKind::Inverse: {
Expand All @@ -3063,7 +3076,8 @@ directReferencesForTypeRepr(Evaluator &evaluator,
auto innerResult = directReferencesForTypeRepr(evaluator, ctx,
inverseRepr->getConstraint(), dc,
allowUsableFromInline,
rhsOfSelfRequirement);
rhsOfSelfRequirement,
allowProtocolMembers);
if (innerResult.first.size() == 1) {
if (auto *proto = dyn_cast<ProtocolDecl>(innerResult.first[0])) {
if (auto ip = proto->getInvertibleProtocolKind()) {
Expand Down Expand Up @@ -3173,10 +3187,16 @@ DirectlyReferencedTypeDecls InheritedDeclsReferencedRequest::evaluate(
else
dc = (DeclContext *)decl.get<const ExtensionDecl *>();

// If looking at a protocol's inheritance list,
// do not look at protocol members to avoid circularity.
// Protocols cannot inherit from any protocol members anyway.
bool allowProtocolMembers = (dc->getSelfProtocolDecl() == nullptr);

return directReferencesForTypeRepr(evaluator, dc->getASTContext(), typeRepr,
const_cast<DeclContext *>(dc),
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
allowProtocolMembers);
}

// Fall back to semantic types.
Expand All @@ -3197,7 +3217,8 @@ DirectlyReferencedTypeDecls UnderlyingTypeDeclsReferencedRequest::evaluate(
return directReferencesForTypeRepr(evaluator, typealias->getASTContext(),
typeRepr, typealias,
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers=*/true);
}

// Fall back to semantic types.
Expand Down Expand Up @@ -3356,7 +3377,8 @@ ExtendedNominalRequest::evaluate(Evaluator &evaluator,
DirectlyReferencedTypeDecls referenced =
directReferencesForTypeRepr(evaluator, ctx, typeRepr, ext->getParent(),
ext->isInSpecializeExtensionContext(),
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers=*/true);

// Resolve those type declarations to nominal type declarations.
SmallVector<ModuleDecl *, 2> modulesFound;
Expand Down Expand Up @@ -3406,7 +3428,8 @@ bool TypeRepr::isProtocolOrProtocolComposition(DeclContext *dc) {
auto &ctx = dc->getASTContext();
auto references = directReferencesForTypeRepr(ctx.evaluator, ctx, this, dc,
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers=*/true);
return declsAreProtocols(references.first);
}

Expand Down Expand Up @@ -3441,7 +3464,8 @@ createTupleExtensionGenericParams(ASTContext &ctx,
extendedTypeRepr,
ext->getParent(),
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers=*/true);
assert(referenced.second.empty() && "Implement me");
if (referenced.first.size() != 1 || !isa<TypeAliasDecl>(referenced.first[0]))
return nullptr;
Expand Down Expand Up @@ -3716,7 +3740,8 @@ CustomAttrNominalRequest::evaluate(Evaluator &evaluator,
decls = directReferencesForTypeRepr(
evaluator, ctx, typeRepr, dc,
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers=*/true);
} else if (Type type = attr->getType()) {
decls = directReferencesForType(type);
}
Expand Down Expand Up @@ -3747,7 +3772,8 @@ CustomAttrNominalRequest::evaluate(Evaluator &evaluator,
decls = directReferencesForUnqualifiedTypeLookup(
name, loc, dc, LookupOuterResults::Included,
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers*/true);
nominals = resolveTypeDeclsToNominal(evaluator, ctx, decls.first,
ResolveToNominalOptions(),
modulesFound, anyObject);
Expand Down Expand Up @@ -3982,7 +4008,8 @@ ProtocolDecl *ImplementsAttrProtocolRequest::evaluate(
DirectlyReferencedTypeDecls referenced =
directReferencesForTypeRepr(evaluator, ctx, typeRepr, dc,
/*allowUsableFromInline=*/false,
/*rhsOfSelfRequirement=*/false);
/*rhsOfSelfRequirement=*/false,
/*allowProtocolMembers=*/true);

// Resolve those type declarations to nominal type declarations.
SmallVector<ModuleDecl *, 2> modulesFound;
Expand Down
42 changes: 41 additions & 1 deletion test/decl/nested/protocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,46 @@ class OuterClass {
protocol InnerProtocol : OuterClass { }
}

// Name lookup circularity tests.

protocol SomeBaseProtocol {}

struct ConformanceOnDecl: ConformanceOnDecl.P {
protocol P: SomeBaseProtocol {}
}
struct ConformanceOnDecl_2: ConformanceOnDecl_2.P {
protocol P where Self: SomeBaseProtocol {}
}
struct ConformanceOnDecl_3: Self.P {
protocol P: SomeBaseProtocol {}
}
struct ConformanceOnDecl_4: ConformanceOnDecl_4.Q {
protocol P: SomeBaseProtocol {}
protocol Q: P {}
}


struct ConformanceInExtension {
protocol P: SomeBaseProtocol {}
}
extension ConformanceInExtension: ConformanceInExtension.P {}

struct ConformanceInExtension_2 {
protocol P where Self: SomeBaseProtocol {}
}
extension ConformanceInExtension_2: ConformanceInExtension_2.P {}

struct ConformanceInExtension_3 {
protocol P: SomeBaseProtocol {}
}
extension ConformanceInExtension_3: Self.P {}

struct ConformanceInExtension_4 {
protocol P: SomeBaseProtocol {}
protocol Q: P {}
}
extension ConformanceInExtension_4: ConformanceInExtension_4.Q {}

// Protocols can be nested in actors.

@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
Expand Down Expand Up @@ -315,4 +355,4 @@ struct Outer {
typealias B = Int
}
}
}
}