Merge pull request #26864 from slavapestov/new-shadowing-rule-part-2

slavapestov · web-flow · commit 016acf3ac906 · 2019-08-29T10:20:09.000-04:00
New shadowing rule, part 2
diff --git a/include/swift/AST/ModuleNameLookup.h b/include/swift/AST/ModuleNameLookup.h
@@ -42,24 +42,25 @@ enum class ResolutionKind {
   TypesOnly
 };
 
-/// Performs a lookup into the given module and, if necessary, its
-/// reexports, observing proper shadowing rules.
+/// Performs a lookup into the given module and it's imports.
+///
+/// If 'moduleOrFile' is a ModuleDecl, we search the module and it's
+/// public imports. If 'moduleOrFile' is a SourceFile, we search the
+/// file's parent module, the module's public imports, and the source
+/// file's private imports.
 ///
-/// \param module The module that will contain the name.
-/// \param accessPath The import scope on \p module.
+/// \param moduleOrFile The module or file unit whose imports to search.
 /// \param name The name to look up.
 /// \param[out] decls Any found decls will be added to this vector.
 /// \param lookupKind Whether this lookup is qualified or unqualified.
 /// \param resolutionKind What sort of decl is expected.
 /// \param moduleScopeContext The top-level context from which the lookup is
 ///        being performed, for checking access. This must be either a
 ///        FileUnit or a Module.
-/// \param extraImports Private imports to include in this search.
-void lookupInModule(ModuleDecl *module, ModuleDecl::AccessPathTy accessPath,
+void lookupInModule(const DeclContext *moduleOrFile,
                     DeclName name, SmallVectorImpl<ValueDecl *> &decls,
                     NLKind lookupKind, ResolutionKind resolutionKind,
-                    const DeclContext *moduleScopeContext,
-                    ArrayRef<ModuleDecl::ImportedModule> extraImports = {});
+                    const DeclContext *moduleScopeContext);
 
 template <typename Fn>
 void forAllVisibleModules(const DeclContext *DC, const Fn &fn) {
@@ -74,12 +75,12 @@ void forAllVisibleModules(const DeclContext *DC, const Fn &fn) {
 /// Performs a qualified lookup into the given module and, if necessary, its
 /// reexports, observing proper shadowing rules.
 void
-lookupVisibleDeclsInModule(ModuleDecl *M, ModuleDecl::AccessPathTy accessPath,
+lookupVisibleDeclsInModule(const DeclContext *moduleOrFile,
+                           ModuleDecl::AccessPathTy accessPath,
                            SmallVectorImpl<ValueDecl *> &decls,
                            NLKind lookupKind,
                            ResolutionKind resolutionKind,
-                           const DeclContext *moduleScopeContext,
-                           ArrayRef<ModuleDecl::ImportedModule> extraImports = {});
+                           const DeclContext *moduleScopeContext);
 
 } // end namespace namelookup
 
diff --git a/lib/AST/ModuleNameLookup.cpp b/lib/AST/ModuleNameLookup.cpp
@@ -444,38 +444,59 @@ void ModuleNameLookup<LookupStrategy>::lookupInModule(
   cache.try_emplace(cacheKey, lookupResults);
 }
 
-void namelookup::lookupInModule(ModuleDecl *startModule,
-                                ModuleDecl::AccessPathTy topAccessPath,
+void namelookup::lookupInModule(const DeclContext *moduleOrFile,
                                 DeclName name,
                                 SmallVectorImpl<ValueDecl *> &decls,
                                 NLKind lookupKind,
                                 ResolutionKind resolutionKind,
-                                const DeclContext *moduleScopeContext,
-                                ArrayRef<ModuleDecl::ImportedModule> extraImports) {
+                                const DeclContext *moduleScopeContext) {
+  assert(moduleScopeContext->isModuleScopeContext());
+
+  auto *startModule = moduleOrFile->getParentModule();
   auto &ctx = startModule->getASTContext();
   auto *stats = ctx.Stats;
   if (stats)
     stats->getFrontendCounters().NumLookupInModule++;
 
   FrontendStatsTracer tracer(stats, "lookup-in-module");
 
-  assert(moduleScopeContext && moduleScopeContext->isModuleScopeContext());
+  // Add private imports to the extra search list.
+  SmallVector<ModuleDecl::ImportedModule, 8> extraImports;
+  if (auto *file = dyn_cast<FileUnit>(moduleOrFile)) {
+    ModuleDecl::ImportFilter importFilter;
+    importFilter |= ModuleDecl::ImportFilterKind::Private;
+    importFilter |= ModuleDecl::ImportFilterKind::ImplementationOnly;
+    file->getImportedModules(extraImports, importFilter);
+  }
+
   LookupByName lookup(ctx, resolutionKind, name, lookupKind);
-  lookup.lookupInModule(decls, startModule, topAccessPath, moduleScopeContext,
+  lookup.lookupInModule(decls, startModule, {}, moduleScopeContext,
                         extraImports);
 }
 
 void namelookup::lookupVisibleDeclsInModule(
-    ModuleDecl *M,
+    const DeclContext *moduleOrFile,
     ModuleDecl::AccessPathTy accessPath,
     SmallVectorImpl<ValueDecl *> &decls,
     NLKind lookupKind,
     ResolutionKind resolutionKind,
-    const DeclContext *moduleScopeContext,
-    ArrayRef<ModuleDecl::ImportedModule> extraImports) {
-  auto &ctx = M->getASTContext();
-  assert(moduleScopeContext && moduleScopeContext->isModuleScopeContext());
+    const DeclContext *moduleScopeContext) {
+  assert(moduleScopeContext->isModuleScopeContext());
+
+  auto *startModule = moduleOrFile->getParentModule();
+  auto &ctx = startModule->getASTContext();
+
+  // Add private imports to the extra search list.
+  SmallVector<ModuleDecl::ImportedModule, 8> extraImports;
+  if (auto *file = dyn_cast<FileUnit>(moduleOrFile)) {
+    ModuleDecl::ImportFilter importFilter;
+    importFilter |= ModuleDecl::ImportFilterKind::Private;
+    importFilter |= ModuleDecl::ImportFilterKind::ImplementationOnly;
+    file->getImportedModules(extraImports, importFilter);
+  }
+
   LookupVisibleDecls lookup(ctx, resolutionKind, lookupKind);
-  lookup.lookupInModule(decls, M, accessPath, moduleScopeContext, extraImports);
+  lookup.lookupInModule(decls, startModule, accessPath, moduleScopeContext,
+                        extraImports);
 }
 
diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp
@@ -206,6 +206,17 @@ static void recordShadowedDeclsAfterSignatureMatch(
       if (firstModule != secondModule &&
           firstDecl->getDeclContext()->isModuleScopeContext() &&
           secondDecl->getDeclContext()->isModuleScopeContext()) {
+        // First, scoped imports shadow unscoped imports.
+        bool firstScoped = imports.isScopedImport(firstModule, name, dc);
+        bool secondScoped = imports.isScopedImport(secondModule, name, dc);
+        if (!firstScoped && secondScoped) {
+          shadowed.insert(firstDecl);
+          break;
+        } else if (firstScoped && !secondScoped) {
+          shadowed.insert(secondDecl);
+          continue;
+        }
+
         // Now check if one module shadows the other.
         if (imports.isShadowedBy(firstModule, secondModule, name, dc)) {
           shadowed.insert(firstDecl);
@@ -261,22 +272,19 @@ static void recordShadowedDeclsAfterSignatureMatch(
           isa<ProtocolDecl>(secondDecl->getDeclContext()))
         continue;
 
-      // Prefer declarations in the current module over those in another
-      // module.
-      // FIXME: This is a hack. We should query a (lazily-built, cached)
-      // module graph to determine shadowing.
-      if ((firstModule == curModule) != (secondModule == curModule)) {
-        // If the first module is the current module, the second declaration
-        // is shadowed by the first.
-        if (firstModule == curModule) {
+      // [Backward compatibility] For members of types, the general module
+      // shadowing check is performed after unavailable candidates have
+      // already been dropped.
+      if (firstModule != secondModule &&
+          !firstDecl->getDeclContext()->isModuleScopeContext() &&
+          !secondDecl->getDeclContext()->isModuleScopeContext()) {
+        if (imports.isShadowedBy(firstModule, secondModule, dc)) {
+          shadowed.insert(firstDecl);
+          break;
+        } else if (imports.isShadowedBy(secondModule, firstModule, dc)) {
           shadowed.insert(secondDecl);
           continue;
         }
-
-        // Otherwise, the first declaration is shadowed by the second. There is
-        // no point in continuing to compare the first declaration to others.
-        shadowed.insert(firstDecl);
-        break;
       }
 
       // Prefer declarations in the any module over those in the standard
@@ -1645,7 +1653,7 @@ bool DeclContext::lookupQualified(ModuleDecl *module, DeclName member,
     if (tracker) {
       recordLookupOfTopLevelName(topLevelScope, member, isLookupCascading);
     }
-    lookupInModule(module, /*accessPath=*/{}, member, decls,
+    lookupInModule(module, member, decls,
                    NLKind::QualifiedLookup, kind, topLevelScope);
   } else {
     // Note: This is a lookup into another module. Unless we're compiling
@@ -1660,7 +1668,7 @@ bool DeclContext::lookupQualified(ModuleDecl *module, DeclName member,
                      [&](ModuleDecl::AccessPathTy accessPath) {
                        return ModuleDecl::matchesAccessPath(accessPath, member);
                      })) {
-      lookupInModule(module, {}, member, decls,
+      lookupInModule(module, member, decls,
                      NLKind::QualifiedLookup, kind, topLevelScope);
     }
   }
diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp
@@ -963,21 +963,12 @@ void UnqualifiedLookupFactory::recordDependencyOnTopLevelName(
 }
 
 void UnqualifiedLookupFactory::addImportedResults(DeclContext *const dc) {
-  // Add private imports to the extra search list.
-  SmallVector<ModuleDecl::ImportedModule, 8> extraImports;
-  if (auto FU = dyn_cast<FileUnit>(dc)) {
-    ModuleDecl::ImportFilter importFilter;
-    importFilter |= ModuleDecl::ImportFilterKind::Private;
-    importFilter |= ModuleDecl::ImportFilterKind::ImplementationOnly;
-    FU->getImportedModules(extraImports, importFilter);
-  }
-
   using namespace namelookup;
   SmallVector<ValueDecl *, 8> CurModuleResults;
   auto resolutionKind = isOriginallyTypeLookup ? ResolutionKind::TypesOnly
                                                : ResolutionKind::Overloadable;
-  lookupInModule(&M, {}, Name, CurModuleResults, NLKind::UnqualifiedLookup,
-                 resolutionKind, dc, extraImports);
+  lookupInModule(dc, Name, CurModuleResults, NLKind::UnqualifiedLookup,
+                 resolutionKind, dc);
 
   // Always perform name shadowing for type lookup.
   if (options.contains(Flags::TypeLookup)) {
diff --git a/lib/Sema/LookupVisibleDecls.cpp b/lib/Sema/LookupVisibleDecls.cpp
@@ -1018,7 +1018,6 @@ static void lookupVisibleMemberDecls(
 static void lookupVisibleDeclsImpl(VisibleDeclConsumer &Consumer,
                                    const DeclContext *DC,
                                    bool IncludeTopLevel, SourceLoc Loc) {
-  const ModuleDecl &M = *DC->getParentModule();
   const SourceManager &SM = DC->getASTContext().SourceMgr;
   auto Reason = DeclVisibilityKind::MemberOfCurrentNominal;
 
@@ -1121,7 +1120,6 @@ static void lookupVisibleDeclsImpl(VisibleDeclConsumer &Consumer,
     Reason = DeclVisibilityKind::MemberOfOutsideNominal;
   }
 
-  SmallVector<ModuleDecl::ImportedModule, 8> extraImports;
   if (auto SF = dyn_cast<SourceFile>(DC)) {
     if (Loc.isValid()) {
       // Look for local variables in top-level code; normally, the parser
@@ -1137,22 +1135,16 @@ static void lookupVisibleDeclsImpl(VisibleDeclConsumer &Consumer,
           Consumer.foundDecl(result, DeclVisibilityKind::VisibleAtTopLevel);
         return;
       }
-
-      ModuleDecl::ImportFilter importFilter;
-      importFilter |= ModuleDecl::ImportFilterKind::Private;
-      importFilter |= ModuleDecl::ImportFilterKind::ImplementationOnly;
-      SF->getImportedModules(extraImports, importFilter);
     }
   }
 
   if (IncludeTopLevel) {
     using namespace namelookup;
     SmallVector<ValueDecl *, 0> moduleResults;
-    auto &mutableM = const_cast<ModuleDecl&>(M);
-    lookupVisibleDeclsInModule(&mutableM, {}, moduleResults,
+    lookupVisibleDeclsInModule(DC, {}, moduleResults,
                                NLKind::UnqualifiedLookup,
                                ResolutionKind::Overloadable,
-                               DC, extraImports);
+                               DC);
     for (auto result : moduleResults)
       Consumer.foundDecl(result, DeclVisibilityKind::VisibleAtTopLevel);
 
diff --git a/lib/Sema/NameBinding.cpp b/lib/Sema/NameBinding.cpp
@@ -295,7 +295,7 @@ void NameBinder::addImport(
     // FIXME: Doesn't handle scoped testable imports correctly.
     assert(declPath.size() == 1 && "can't handle sub-decl imports");
     SmallVector<ValueDecl *, 8> decls;
-    lookupInModule(topLevelModule, declPath, declPath.front().first, decls,
+    lookupInModule(topLevelModule, declPath.front().first, decls,
                    NLKind::QualifiedLookup, ResolutionKind::Overloadable,
                    &SF);
 
diff --git a/test/NameBinding/import-resolution.swift b/test/NameBinding/import-resolution.swift
@@ -32,8 +32,8 @@ letters.asdf.A // expected-error{{module 'letters' has no member named 'asdf'}}
 var uA : A // expected-error {{'A' is ambiguous for type lookup in this context}}
 var uB : B = abcde.B()
 var uC : C // expected-error {{'C' is ambiguous for type lookup in this context}}
-var uD : D // expected-error {{'D' is ambiguous for type lookup in this context}}
-var uE : E // expected-error {{'E' is ambiguous for type lookup in this context}}
+var uD : D = asdf.D()
+var uE : E = aeiou.E()
 var uF : F = letters.F()
 
 var qA1 : abcde.A // okay
