Merge pull request #16951 from DougGregor/overlay-shadow-reexported-module

[Type checking] A declaration in an overlay can shadow an imported declaration.

Author: DougGregor

lib/AST/NameLookup.cpp

@@ -15,7 +15,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "NameLookupImpl.h"
-#include "swift/Basic/Statistic.h"
+#include "swift/ClangImporter/ClangImporter.h"
 #include "swift/AST/NameLookup.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/ASTScope.h"
@@ -26,6 +26,7 @@
 #include "swift/AST/Initializer.h"
 #include "swift/AST/ReferencedNameTracker.h"
 #include "swift/Basic/SourceManager.h"
+#include "swift/Basic/Statistic.h"
 #include "swift/Basic/STLExtras.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/TinyPtrVector.h"
@@ -281,20 +282,35 @@ bool swift::removeShadowedDecls(SmallVectorImpl<ValueDecl*> &decls,
         // module.
         // FIXME: This is a hack. We should query a (lazily-built, cached)
         // module graph to determine shadowing.
-        if ((firstModule == curModule) == (secondModule == curModule))
-          continue;
+        if ((firstModule == curModule) != (secondModule == curModule)) {
+          // If the first module is the current module, the second declaration
+          // is shadowed by the first.
+          if (firstModule == curModule) {
+            shadowed.insert(secondDecl);
+            continue;
+          }
 
-        // If the first module is the current module, the second declaration
-        // is shadowed by the first.
-        if (firstModule == curModule) {
-          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;
         }
 
-        // 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 an overlay to similar declarations in
+        // the Clang module it customizes.
+        if (firstDecl->hasClangNode() != secondDecl->hasClangNode()) {
+          if (isInOverlayModuleForImportedModule(firstDecl->getDeclContext(),
+                                                 secondDecl->getDeclContext())){
+            shadowed.insert(secondDecl);
+            continue;
+          }
+
+          if (isInOverlayModuleForImportedModule(secondDecl->getDeclContext(),
+                                                 firstDecl->getDeclContext())) {
+            shadowed.insert(firstDecl);
+            break;
+          }
+        }
       }
     }
   }

lib/Sema/TypeCheckNameLookup.cpp

@@ -75,19 +75,37 @@ namespace {
         Options |= NameLookupFlags::IgnoreAccessControl;
     }
 
+    /// Determine whether we should filter out the results by removing
+    /// overridden and shadowed declarations.
+    /// FIXME: We should *always* do this, but there are weird assumptions
+    /// about the results of unqualified name lookup, e.g., that a local
+    /// variable not having a type indicates that it hasn't been seen yet.
+    bool shouldFilterResults() const {
+      // Member lookups always filter results.
+      if (IsMemberLookup) return true;
+
+      bool allAreInOtherModules = true;
+      auto currentModule = DC->getParentModule();
+      for (const auto &found : Result) {
+        // We found a member, so we need to filter.
+        if (found.getBaseDecl() != nullptr)
+          return true;
+
+        // We found something in our own module.
+        if (found.getValueDecl()->getDeclContext()->getParentModule() ==
+              currentModule)
+          allAreInOtherModules = false;
+      }
+
+      // FIXME: Only perform shadowing if we found things from other modules.
+      // This prevents us from introducing additional type-checking work
+      // during name lookup.
+      return allAreInOtherModules;
+    }
+
     ~LookupResultBuilder() {
-      // If any of the results have a base, we need to remove
-      // overridden and shadowed declarations.
-      // FIXME: We should *always* remove overridden and shadowed declarations,
-      // but there are weird assumptions about the results of unqualified
-      // name lookup, e.g., that a local variable not having a type indicates
-      // that it hasn't been seen yet.
-      if (!IsMemberLookup &&
-          std::find_if(Result.begin(), Result.end(),
-                       [](const LookupResultEntry &found) {
-                         return found.getBaseDecl() != nullptr;
-                       }) == Result.end())
-        return;
+      // Check whether we should do this filtering aat all.
+      if (!shouldFilterResults()) return;
 
       // Remove any overridden declarations from the found-declarations set.
       removeOverriddenDecls(FoundDecls);

test/ClangImporter/Inputs/privateframeworks/bridging-somekit.h

@@ -0,0 +1 @@
+#import <SomeKit/SKWidget.h>

test/ClangImporter/Inputs/privateframeworks/bridging-somekitcore.h

@@ -0,0 +1 @@
+#import <SomeKitCore/SKWidget.h>

test/ClangImporter/Inputs/privateframeworks/overlay/SomeKit.swift

@@ -47,3 +47,10 @@ public func inlineWidgetOperations(_ widget: SKWidget) {
   someKitGlobalFunc()
   hao.anObject = widget
 }
+
+@available(swift, deprecated: 4.2)
+public func someKitOtherGlobalFunc() { }
+
+extension SKWidget {
+  public var name: String { return "blah" }
+}

test/ClangImporter/Inputs/privateframeworks/withoutprivate/SomeKit.framework/Headers/SKWidget.h

@@ -24,3 +24,8 @@ typedef enum __attribute__((ns_error_domain(SKWidgetErrorDomain))) __attribute__
 @end
 
 extern void someKitGlobalFunc(void);
+
+static inline void someKitOtherGlobalFunc(void) { }
+
+extern NSString * _Nonnull someKitGetWidgetName(SKWidget * _Nonnull)
+  __attribute__((swift_name("getter:SKWidget.name(self:)")));

test/ClangImporter/Inputs/privateframeworks/withoutprivate/SomeKit.framework/Headers/SomeKit.apinotes

@@ -0,0 +1,10 @@
+---
+Name: SomeKit
+Functions:
+- Name: someKitOtherGlobalFunc
+  SwiftPrivate: true
+SwiftVersions:
+- Version: 4
+  Functions:
+  - Name: someKitOtherGlobalFunc
+    SwiftPrivate: false

test/ClangImporter/Inputs/privateframeworks/withprivate/SomeKit.framework/Headers/SomeKit.apinotes

@@ -0,0 +1,7 @@
+---
+Name: SomeKit
+SwiftVersions:
+- Version: 4
+  Functions:
+  - Name: someKitOtherGlobalFunc
+    SwiftPrivate: false

test/ClangImporter/Inputs/privateframeworks/withprivate/SomeKitCore.framework/Headers/SKWidget.h

@@ -25,3 +25,8 @@ typedef enum __attribute__((ns_error_domain(SKWidgetErrorDomain))) __attribute__
 @end
 
 extern void someKitGlobalFunc(void);
+
+static inline void someKitOtherGlobalFunc(void) { }
+
+extern NSString * _Nonnull someKitGetWidgetName(SKWidget * _Nonnull)
+  __attribute__((swift_name("getter:SKWidget.name(self:)")));

test/ClangImporter/Inputs/privateframeworks/withprivate/SomeKitCore.framework/Headers/SomeKitCore.apinotes

@@ -0,0 +1,10 @@
+---
+Name: SomeKitCore
+Functions:
+- Name: someKitOtherGlobalFunc
+  SwiftPrivate: true
+SwiftVersions:
+- Version: 4
+  Functions:
+  - Name: someKitOtherGlobalFunc
+    SwiftPrivate: false

test/ClangImporter/private_frameworks.swift

@@ -9,19 +9,19 @@
 // RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-module -F %S/Inputs/privateframeworks/withprivate -o %t %S/Inputs/privateframeworks/overlay/SomeKit.swift
 
 // Use the overlay with private frameworks.
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate %s -verify
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekitcore.h -verify
 
 // Use the overlay without private frameworks.
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate -I %t %s
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate -I %t -swift-version 4 -import-objc-header %S/Inputs/privateframeworks/bridging-somekit.h %s
 
 // Build the overlay with public frameworks.
 // RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-module -F %S/Inputs/privateframeworks/withoutprivate -o %t %S/Inputs/privateframeworks/overlay/SomeKit.swift
 
 // Use the overlay with private frameworks.
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate %s -verify
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withprivate -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekitcore.h -verify
 
 // Use the overlay without private frameworks.
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate -I %t %s
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -emit-sil -o /dev/null -F %S/Inputs/privateframeworks/withoutprivate -I %t -swift-version 4 %s -import-objc-header %S/Inputs/privateframeworks/bridging-somekit.h 
 
 // REQUIRES: objc_interop
 
@@ -36,10 +36,17 @@ func testWidget(widget: SKWidget) {
 
   widget.doSomethingElse(widget)
   inlineWidgetOperations(widget)
+
+  let _ = widget.name
 }
 
 func testError(widget: SKWidget) {
   let c: SKWidget.Error.Code = SKWidget.Error(.boom).getCode(from: widget)
   if c.isBoom { }
 }
 
+func testGlobals() {
+  someKitGlobalFunc()
+  SomeKit.someKitOtherGlobalFunc()
+  someKitOtherGlobalFunc()
+}