Merge pull request #27365 from slavapestov/member-type-shadowing-rule

slavapestov · web-flow · commit c0a57b6e1783 · 2019-09-26T02:20:40.000-04:00
AST: Types nested inside a subclass shadow types in the superclass with same name
diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp
@@ -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) {
@@ -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(
@@ -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(
@@ -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.
diff --git a/lib/Sema/LookupVisibleDecls.cpp b/lib/Sema/LookupVisibleDecls.cpp
@@ -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;
 
@@ -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();
   }
 };
 
@@ -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,
diff --git a/test/IDE/complete_crashes.swift b/test/IDE/complete_crashes.swift
@@ -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
diff --git a/test/IDE/complete_member_type.swift b/test/IDE/complete_member_type.swift
@@ -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
diff --git a/test/IDE/complete_value_expr.swift b/test/IDE/complete_value_expr.swift
@@ -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
diff --git a/test/NameBinding/member_type_shadowing.swift b/test/NameBinding/member_type_shadowing.swift
