Skip to content

AST: Types nested inside a subclass shadow types in the superclass with same name #27365

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 2 commits into from
Sep 26, 2019
Merged

AST: Types nested inside a subclass shadow types in the superclass with same name #27365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51e4792
AST: Types nested inside a subclass shadow types in the superclass wi…
slavapestov Sep 26, 2019
44827bb
IDE: Refactor LookupVisibleDecls to use standard override/shadowing l…
slavapestov Sep 26, 2019
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
34 changes: 27 additions & 7 deletions lib/AST/NameLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ static void recordShadowedDeclsAfterSignatureMatch(
for (unsigned firstIdx : indices(decls)) {
auto firstDecl = decls[firstIdx];
auto firstModule = firstDecl->getModuleContext();
bool firstTopLevel = firstDecl->getDeclContext()->isModuleScopeContext();

auto name = firstDecl->getBaseName();

auto isShadowed = [&](ArrayRef<ModuleDecl::AccessPathTy> paths) {
Expand All @@ -219,15 +221,37 @@ static void recordShadowedDeclsAfterSignatureMatch(
// Determine whether one module takes precedence over another.
auto secondDecl = decls[secondIdx];
auto secondModule = secondDecl->getModuleContext();
bool secondTopLevel = secondDecl->getDeclContext()->isModuleScopeContext();

// For member types, we skip most of the below rules. Instead, we allow
// member types defined in a subclass to shadow member types defined in
// a superclass.
if (isa<TypeDecl>(firstDecl) &&
isa<TypeDecl>(secondDecl) &&
!firstTopLevel &&
!secondTopLevel) {
auto *firstClass = firstDecl->getDeclContext()->getSelfClassDecl();
auto *secondClass = secondDecl->getDeclContext()->getSelfClassDecl();
if (firstClass && secondClass && firstClass != secondClass) {
if (firstClass->isSuperclassOf(secondClass)) {
shadowed.insert(firstDecl);
continue;
} else if (secondClass->isSuperclassOf(firstClass)) {
shadowed.insert(secondDecl);
continue;
}
}

continue;
}

// Top-level type declarations in a module shadow other declarations
// visible through the module's imports.
//
// [Backward compatibility] Note that members of types have the same
// shadowing check, but we do it after dropping unavailable members.
if (firstModule != secondModule &&
firstDecl->getDeclContext()->isModuleScopeContext() &&
secondDecl->getDeclContext()->isModuleScopeContext()) {
firstTopLevel && secondTopLevel) {
auto firstPaths = imports.getAllAccessPathsNotShadowedBy(
firstModule, secondModule, dc);
auto secondPaths = imports.getAllAccessPathsNotShadowedBy(
Expand Down Expand Up @@ -304,8 +328,7 @@ static void recordShadowedDeclsAfterSignatureMatch(
// shadowing check is performed after unavailable candidates have
// already been dropped.
if (firstModule != secondModule &&
!firstDecl->getDeclContext()->isModuleScopeContext() &&
!secondDecl->getDeclContext()->isModuleScopeContext()) {
!firstTopLevel && !secondTopLevel) {
auto firstPaths = imports.getAllAccessPathsNotShadowedBy(
firstModule, secondModule, dc);
auto secondPaths = imports.getAllAccessPathsNotShadowedBy(
Expand Down Expand Up @@ -478,9 +501,6 @@ static void recordShadowedDecls(ArrayRef<ValueDecl *> decls,
// type. This is layering a partial fix upon a total hack.
if (auto asd = dyn_cast<AbstractStorageDecl>(decl))
signature = asd->getOverloadSignatureType();
} else if (decl->getDeclContext()->isTypeContext()) {
// Do not apply shadowing rules for member types.
continue;
}

// Record this declaration based on its signature.
Expand Down
212 changes: 103 additions & 109 deletions lib/Sema/LookupVisibleDecls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,9 +708,10 @@ namespace {

class OverrideFilteringConsumer : public VisibleDeclConsumer {
public:
std::set<ValueDecl *> AllFoundDecls;
std::map<DeclBaseName, std::set<ValueDecl *>> FoundDecls;
llvm::SetVector<FoundDeclTy> DeclsToReport;
llvm::SetVector<FoundDeclTy> Results;
llvm::SmallVector<ValueDecl *, 8> Decls;
llvm::SetVector<FoundDeclTy> FilteredResults;
llvm::DenseMap<DeclBaseName, llvm::SmallVector<ValueDecl *, 2>> DeclsByName;
Type BaseTy;
const DeclContext *DC;

Expand All @@ -723,133 +724,128 @@ class OverrideFilteringConsumer : public VisibleDeclConsumer {

void foundDecl(ValueDecl *VD, DeclVisibilityKind Reason,
DynamicLookupInfo dynamicLookupInfo) override {
if (!AllFoundDecls.insert(VD).second)
if (!Results.insert({VD, Reason, dynamicLookupInfo}))
return;

// If this kind of declaration doesn't participate in overriding, there's
// no filtering to do here.
if (!isa<AbstractFunctionDecl>(VD) &&
!isa<AbstractStorageDecl>(VD) &&
!isa<AssociatedTypeDecl>(VD)) {
DeclsToReport.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
return;
}
DeclsByName[VD->getBaseName()] = {};
Decls.push_back(VD);
}

if (!VD->getInterfaceType()) {
return;
}
void filterDecls(VisibleDeclConsumer &Consumer) {
removeOverriddenDecls(Decls);
removeShadowedDecls(Decls, DC);

if (VD->isInvalid()) {
FoundDecls[VD->getBaseName()].insert(VD);
DeclsToReport.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
return;
}
auto &PossiblyConflicting = FoundDecls[VD->getBaseName()];
size_t index = 0;
for (auto DeclAndReason : Results) {
if (index >= Decls.size())
break;
if (DeclAndReason.D != Decls[index])
continue;

// Check all overridden decls.
{
auto *CurrentVD = VD->getOverriddenDecl();
while (CurrentVD) {
if (!AllFoundDecls.insert(CurrentVD).second)
break;
if (PossiblyConflicting.count(CurrentVD)) {
PossiblyConflicting.erase(CurrentVD);
PossiblyConflicting.insert(VD);
index++;

bool Erased = DeclsToReport.remove(
FoundDeclTy(CurrentVD, DeclVisibilityKind::LocalVariable, {}));
assert(Erased);
(void)Erased;
auto *VD = DeclAndReason.D;
auto Reason = DeclAndReason.Reason;
auto dynamicLookupInfo = DeclAndReason.dynamicLookupInfo;

DeclsToReport.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
return;
}
CurrentVD = CurrentVD->getOverriddenDecl();
// If this kind of declaration doesn't participate in overriding, there's
// no filtering to do here.
if (!isa<AbstractFunctionDecl>(VD) &&
!isa<AbstractStorageDecl>(VD) &&
!isa<AssociatedTypeDecl>(VD)) {
FilteredResults.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
continue;
}
}

// Does it make sense to substitute types?

// If the base type is AnyObject, we might be doing a dynamic
// lookup, so the base type won't match the type of the member's
// context type.
//
// If the base type is not a nominal type, we can't substitute
// the member type.
//
// If the member is a free function and not a member of a type,
// don't substitute either.
bool shouldSubst = (Reason != DeclVisibilityKind::DynamicLookup &&
!BaseTy->isAnyObject() && !BaseTy->hasTypeVariable() &&
!BaseTy->hasUnboundGenericType() &&
(BaseTy->getNominalOrBoundGenericNominal() ||
BaseTy->is<ArchetypeType>()) &&
VD->getDeclContext()->isTypeContext());
ModuleDecl *M = DC->getParentModule();

// Hack; we shouldn't be filtering at this level anyway.
if (!VD->hasInterfaceType()) {
FoundDecls[VD->getBaseName()].insert(VD);
DeclsToReport.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
return;
}
if (!VD->getInterfaceType()) {
continue;
}

auto FoundSignature = VD->getOverloadSignature();
auto FoundSignatureType = VD->getOverloadSignatureType();
if (FoundSignatureType && shouldSubst) {
auto subs = BaseTy->getMemberSubstitutionMap(M, VD);
auto CT = FoundSignatureType.subst(subs);
if (!CT->hasError())
FoundSignatureType = CT->getCanonicalType();
}
auto &PossiblyConflicting = DeclsByName[VD->getBaseName()];

for (auto I = PossiblyConflicting.begin(), E = PossiblyConflicting.end();
I != E; ++I) {
auto *OtherVD = *I;
if (OtherVD->isInvalid() || !OtherVD->hasInterfaceType()) {
// For some invalid decls it might be impossible to compute the
// signature, for example, if the types could not be resolved.
if (VD->isInvalid()) {
FilteredResults.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
PossiblyConflicting.push_back(VD);
continue;
}

auto OtherSignature = OtherVD->getOverloadSignature();
auto OtherSignatureType = OtherVD->getOverloadSignatureType();
if (OtherSignatureType && shouldSubst) {
auto ActualBaseTy = getBaseTypeForMember(M, OtherVD, BaseTy);
auto subs = ActualBaseTy->getMemberSubstitutionMap(M, OtherVD);
auto CT = OtherSignatureType.subst(subs);
// Does it make sense to substitute types?

// If the base type is AnyObject, we might be doing a dynamic
// lookup, so the base type won't match the type of the member's
// context type.
//
// If the base type is not a nominal type, we can't substitute
// the member type.
//
// If the member is a free function and not a member of a type,
// don't substitute either.
bool shouldSubst = (Reason != DeclVisibilityKind::DynamicLookup &&
!BaseTy->isAnyObject() && !BaseTy->hasTypeVariable() &&
!BaseTy->hasUnboundGenericType() &&
(BaseTy->getNominalOrBoundGenericNominal() ||
BaseTy->is<ArchetypeType>()) &&
VD->getDeclContext()->isTypeContext());
ModuleDecl *M = DC->getParentModule();

auto FoundSignature = VD->getOverloadSignature();
auto FoundSignatureType = VD->getOverloadSignatureType();
if (FoundSignatureType && shouldSubst) {
auto subs = BaseTy->getMemberSubstitutionMap(M, VD);
auto CT = FoundSignatureType.subst(subs);
if (!CT->hasError())
OtherSignatureType = CT->getCanonicalType();
FoundSignatureType = CT->getCanonicalType();
}

if (conflicting(M->getASTContext(), FoundSignature, FoundSignatureType,
OtherSignature, OtherSignatureType,
/*wouldConflictInSwift5*/nullptr,
/*skipProtocolExtensionCheck*/true)) {
if (VD->getFormalAccess() > OtherVD->getFormalAccess() ||
//Prefer available one.
(!AvailableAttr::isUnavailable(VD) &&
AvailableAttr::isUnavailable(OtherVD))) {
PossiblyConflicting.erase(I);
PossiblyConflicting.insert(VD);

bool Erased = DeclsToReport.remove(
FoundDeclTy(OtherVD, DeclVisibilityKind::LocalVariable, {}));
assert(Erased);
(void)Erased;

DeclsToReport.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
bool FoundConflicting = false;
for (auto I = PossiblyConflicting.begin(), E = PossiblyConflicting.end();
I != E; ++I) {
auto *OtherVD = *I;
if (OtherVD->isInvalid() || !OtherVD->getInterfaceType()) {
// For some invalid decls it might be impossible to compute the
// signature, for example, if the types could not be resolved.
continue;
}
return;

auto OtherSignature = OtherVD->getOverloadSignature();
auto OtherSignatureType = OtherVD->getOverloadSignatureType();
if (OtherSignatureType && shouldSubst) {
auto ActualBaseTy = getBaseTypeForMember(M, OtherVD, BaseTy);
auto subs = ActualBaseTy->getMemberSubstitutionMap(M, OtherVD);
auto CT = OtherSignatureType.subst(subs);
if (!CT->hasError())
OtherSignatureType = CT->getCanonicalType();
}

if (conflicting(M->getASTContext(), FoundSignature, FoundSignatureType,
OtherSignature, OtherSignatureType,
/*wouldConflictInSwift5*/nullptr,
/*skipProtocolExtensionCheck*/true)) {
FoundConflicting = true;
if (VD->getFormalAccess() > OtherVD->getFormalAccess() ||
//Prefer available one.
(!AvailableAttr::isUnavailable(VD) &&
AvailableAttr::isUnavailable(OtherVD))) {
FilteredResults.remove(
FoundDeclTy(OtherVD, DeclVisibilityKind::LocalVariable, {}));
FilteredResults.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
*I = VD;
}
}
}

if (!FoundConflicting) {
FilteredResults.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
PossiblyConflicting.push_back(VD);
}
}

PossiblyConflicting.insert(VD);
DeclsToReport.insert(FoundDeclTy(VD, Reason, dynamicLookupInfo));
for (auto Result : FilteredResults)
Consumer.foundDecl(Result.D, Result.Reason, Result.dynamicLookupInfo);
}

bool seenBaseName(DeclBaseName name) {
return FoundDecls.find(name) != FoundDecls.end();
return DeclsByName.find(name) != DeclsByName.end();
}
};

Expand Down Expand Up @@ -1003,9 +999,7 @@ static void lookupVisibleMemberDecls(
GSB, Visited, seenDynamicLookup);

// Report the declarations we found to the real consumer.
for (const auto &DeclAndReason : overrideConsumer.DeclsToReport)
Consumer.foundDecl(DeclAndReason.D, DeclAndReason.Reason,
DeclAndReason.dynamicLookupInfo);
overrideConsumer.filterDecls(Consumer);
}

static void lookupVisibleDeclsImpl(VisibleDeclConsumer &Consumer,
Expand Down
4 changes: 2 additions & 2 deletions test/IDE/complete_crashes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,8 @@ extension Foo {
}
#endif
// RDAR_41234606: Begin completion
// RDAR_41234606-DAG: Decl[AssociatedType]/Super: .Element; name=Element
// RDAR_41234606-DAG: Decl[AssociatedType]/Super: .Iterator; name=Iterator
// RDAR_41234606-DAG: Decl[AssociatedType]/CurrNominal: .Element; name=Element
// RDAR_41234606-DAG: Decl[AssociatedType]/CurrNominal: .Iterator; name=Iterator
// RDAR_41234606: End completions

// rdar://problem/41071587
Expand Down
26 changes: 26 additions & 0 deletions test/IDE/complete_member_type.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=A | %FileCheck %s

class A {
typealias T = Int
enum E {
case a
case b
}
}

class B : A {
typealias T = String
struct E {}
}

func foo() {
_ = B.#^A^#
}

// CHECK-LABEL: Begin completions, 5 items
// CHECK-NEXT: Keyword[self]/CurrNominal: self[#B.Type#]; name=self
// CHECK-NEXT: Keyword/CurrNominal: Type[#B.Type#]; name=Type
// CHECK-NEXT: Decl[TypeAlias]/CurrNominal: T[#String#]; name=T
// CHECK-NEXT: Decl[Struct]/CurrNominal: E[#B.E#]; name=E
// CHECK-NEXT: Decl[Constructor]/CurrNominal: init()[#B#]; name=init()
// CHECK-NEXT: End completions
19 changes: 8 additions & 11 deletions test/IDE/complete_value_expr.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,22 +1672,19 @@ func checkOverrideInclusion2(_ arg: Override3) {
// CHECK_NODUP_RESTATED_REQ-NOT: Decl[InstanceVar]/{{Super|CurrNominal}}: doo[#Int#]; name=doo
// CHECK_NODUP_RESTATED_REQ: End completions

// CHECK_NODUP_RESTATED_REQ_NODOT: Begin completions
// CHECK_NODUP_RESTATED_REQ_NODOT: Decl[InstanceMethod]/{{Super|CurrNominal}}: .foo()[#Void#]; name=foo()
// CHECK_NODUP_RESTATED_REQ_NODOT: Decl[InstanceMethod]/{{Super|CurrNominal}}: .roo({#arg1: Int#})[#Void#];
// CHECK_NODUP_RESTATED_REQ_NODOT: Decl[Subscript]/{{Super|CurrNominal}}: [{#(arg): Bool#}][#Bool#]; name=[arg: Bool]
// CHECK_NODUP_RESTATED_REQ_NODOT: Decl[InstanceVar]/{{Super|CurrNominal}}: .doo[#Int#]; name=doo
// CHECK_NODUP_RESTATED_REQ_NODOT: Decl[InstanceMethod]/{{Super|CurrNominal}}: .roo({#arg2: Int#})[#Void#];
// CHECK_NODUP_RESTATED_REQ_NODOT-NOT: Decl[InstanceMethod]/{{Super|CurrNominal}}: .foo()[#Void#]; name=foo()
// CHECK_NODUP_RESTATED_REQ_NODOT-NOT: Decl[Subscript]/{{Super|CurrNominal}}: [{#Bool#}][#Bool#]; name=[Bool]
// CHECK_NODUP_RESTATED_REQ_NODOT-NOT: Decl[InstanceVar]/{{Super|CurrNominal}}: .doo[#Int#]; name=doo
// CHECK_NODUP_RESTATED_REQ_NODOT: Begin completions, 6 items
// CHECK_NODUP_RESTATED_REQ_NODOT-DAG: Decl[InstanceMethod]/{{Super|CurrNominal}}: .foo()[#Void#]; name=foo()
// CHECK_NODUP_RESTATED_REQ_NODOT-DAG: Decl[InstanceMethod]/{{Super|CurrNominal}}: .roo({#arg1: Int#})[#Void#];
// CHECK_NODUP_RESTATED_REQ_NODOT-DAG: Decl[Subscript]/{{Super|CurrNominal}}: [{#(arg): Bool#}][#Bool#]; name=[arg: Bool]
// CHECK_NODUP_RESTATED_REQ_NODOT-DAG: Decl[InstanceVar]/{{Super|CurrNominal}}: .doo[#Int#]; name=doo
// CHECK_NODUP_RESTATED_REQ_NODOT-DAG: Decl[InstanceMethod]/{{Super|CurrNominal}}: .roo({#arg2: Int#})[#Void#];
// CHECK_NODUP_RESTATED_REQ_NODOT: End completions

// CHECK_NODUP_RESTATED_REQ_TYPE1: Begin completions, 6 items
// CHECK_NODUP_RESTATED_REQ_TYPE1: Decl[InstanceMethod]/Super: foo({#(self): NoDupReq6#})[#() -> Void#]; name=foo(self: NoDupReq6)
// CHECK_NODUP_RESTATED_REQ_TYPE1: Decl[InstanceMethod]/Super: roo({#(self): NoDupReq6#})[#(arg1: Int) -> Void#]; name=roo(self: NoDupReq6)
// CHECK_NODUP_RESTATED_REQ_TYPE1: Decl[AssociatedType]/Super: E; name=E
// CHECK_NODUP_RESTATED_REQ_TYPE1: Decl[InstanceMethod]/CurrNominal: foo({#(self): NoDupReq6#})[#() -> Void#]; name=foo(self: NoDupReq6)
// CHECK_NODUP_RESTATED_REQ_TYPE1: Decl[InstanceMethod]/CurrNominal: roo({#(self): NoDupReq6#})[#(arg2: Int) -> Void#]; name=roo(self: NoDupReq6)
// CHECK_NODUP_RESTATED_REQ_TYPE1: Decl[AssociatedType]/CurrNominal: E; name=E
// CHECK_NODUP_RESTATED_REQ_TYPE1: End completions

// CHECK_NODUP_RESTATED_REQ_TYPE2: Begin completions, 6 items
Expand Down
Loading