[cxx-interop] Fix SR-13785

lib/Sema/TypeCheckAccess.cpp

@@ -22,6 +22,7 @@
 #include "swift/AST/ASTWalker.h"
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/ExistentialLayout.h"
+#include "swift/AST/Import.h"
 #include "swift/AST/Pattern.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/TypeCheckRequests.h"
@@ -1439,7 +1440,6 @@ class UsableFromInlineChecker : public AccessControlCheckerBase,
     }
   }
 };
-
 } // end anonymous namespace
 
 /// Returns the kind of origin, implementation-only import or SPI declaration,
@@ -1459,6 +1459,40 @@ swift::getDisallowedOriginKind(const Decl *decl,
     if (where.isSPI())
       downgradeToWarning = DowngradeToWarning::Yes;
 
+    // Even if the current module is @_implementationOnly, Swift should
+    // not report an error in the cases where the decl is also exported from
+    // a non @_implementationOnly module. Thus, we check to see if there is
+    // a visible access path to the Clang decl, and only error out in case
+    // there is none.
+    auto filter = ModuleDecl::ImportFilter(
+        {ModuleDecl::ImportFilterKind::Exported,
+         ModuleDecl::ImportFilterKind::Default,
+         ModuleDecl::ImportFilterKind::SPIAccessControl,
+         ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay});
+    SmallVector<ImportedModule, 4> sfImportedModules;
+    SF->getImportedModules(sfImportedModules, filter);
+    if (auto clangDecl = decl->getClangDecl()) {
+      for (auto redecl : clangDecl->redecls()) {
+        if (auto tagReDecl = dyn_cast<clang::TagDecl>(redecl)) {
+          // This is a forward declaration. We ignore visibility of those.
+          if (tagReDecl->getBraceRange().isInvalid()) {
+            continue;
+          }
+        }
+        auto moduleWrapper =
+            decl->getASTContext().getClangModuleLoader()->getWrapperForModule(
+                redecl->getOwningModule());
+        auto visibleAccessPath =
+            find_if(sfImportedModules, [&moduleWrapper](auto importedModule) {
+              return importedModule.importedModule == moduleWrapper ||
+                     !importedModule.importedModule
+                          ->isImportedImplementationOnly(moduleWrapper);
+            });
+        if (visibleAccessPath != sfImportedModules.end()) {
+          return DisallowedOriginKind::None;
+        }
+      }
+    }
     // Implementation-only imported, cannot be reexported.
     return DisallowedOriginKind::ImplementationOnly;
   } else if (decl->isSPI() && !where.isSPI()) {

test/Interop/C/implementation-only-imports/Inputs/module.modulemap

@@ -1,9 +1,9 @@
 module UserA {
-    header "user_a.h"
+    header "user-a.h"
     export *
 }
 
 module UserB {
-    header "user_b.h"
+    header "user-b.h"
     export *
 }

test/Interop/C/implementation-only-imports/Inputs/use-module-a.swift

@@ -0,0 +1 @@
+@_exported import UserA

test/Interop/C/implementation-only-imports/Inputs/use-module-b.swift

@@ -0,0 +1 @@
+@_exported import UserB

test/Interop/C/implementation-only-imports/Inputs/user-a.h

@@ -0,0 +1,6 @@
+#ifndef TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_A_H
+#define TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_A_H
+
+#include "helper.h"
+
+#endif // TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_A_H

test/Interop/C/implementation-only-imports/Inputs/user-b.h

@@ -0,0 +1,6 @@
+#ifndef TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_B_H
+#define TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_B_H
+
+#include "helper.h"
+
+#endif // TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_B_H

test/Interop/C/implementation-only-imports/check-function-transitive-visibility-inversed.swift

@@ -0,0 +1,21 @@
+// RUN: %empty-directory(%t)
+// RUN: mkdir %t/use_module_a %t/use_module_b
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_a/UseModuleA.swiftmodule %S/Inputs/use-module-a.swift -I %S/Inputs
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_b/UseModuleB.swiftmodule %S/Inputs/use-module-b.swift -I %S/Inputs
+
+// RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs %s
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-function-transitive-visibility.swift`
+// ensures that Swift looks into the transitive visible modules as well
+// when looking for the `getFortyTwo()` decl. 
+
+import UseModuleA
+@_implementationOnly import UseModuleB
+
+@inlinable
+public func callFortyTwo() -> CInt {
+  return getFortyTwo()
+}

test/Interop/C/implementation-only-imports/check-function-transitive-visibility.swift

@@ -0,0 +1,21 @@
+// RUN: %empty-directory(%t)
+// RUN: mkdir %t/use_module_a %t/use_module_b
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_a/UseModuleA.swiftmodule %S/Inputs/use-module-a.swift -I %S/Inputs
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_b/UseModuleB.swiftmodule %S/Inputs/use-module-b.swift -I %S/Inputs
+
+// RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs %s
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-function-transitive-visibility-inversed.swift`
+// ensures that Swift looks into the transitive visible modules as well
+// when looking for the `getFortyTwo()` decl. 
+
+import UseModuleA
+@_implementationOnly import UseModuleB
+
+@inlinable
+public func callFortyTwo() -> CInt {
+  return getFortyTwo()
+}

test/Interop/C/implementation-only-imports/check-function-visibility-inversed.swift

@@ -0,0 +1,17 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-function-visibility.swift`
+// checks that the `getFortyTwo` decl can be found when at least one of the
+// modules is not `@_implementationOnly`.
+
+import UserA
+@_implementationOnly import UserB
+
+@_inlineable
+public func callFortyTwo() -> CInt {
+  return getFortyTwo()
+}

test/Interop/C/implementation-only-imports/check-function-visibility.swift

@@ -0,0 +1,17 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-function-visibility-inversed.swift`
+// checks that the `getFortyTwo()` decl can be found when at least one of the
+// modules is not `@_implementationOnly`.
+
+@_implementationOnly import UserA
+import UserB
+
+@_inlineable
+public func callFortyTwo() -> CInt {
+  return getFortyTwo()
+}

test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h

@@ -0,0 +1,6 @@
+#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_A_H
+#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_A_H
+
+inline int getFortySomething() { return 42; };
+
+#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_A_H

test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h

@@ -0,0 +1,6 @@
+#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_B_H
+#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_B_H
+
+inline int getFortySomething() { return 46; };
+
+#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_B_H

test/Interop/Cxx/implementation-only-imports/Inputs/helper.h

@@ -0,0 +1,20 @@
+#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H
+#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H
+
+inline int getFortyTwo() { return 42; }
+
+class MagicWrapper {
+public:
+  int _number;
+  MagicWrapper(){_number = 2;};
+  MagicWrapper(int number) : _number(number){};
+  MagicWrapper operator - (MagicWrapper other) {
+      return MagicWrapper{_number - other._number};
+  }
+};
+
+inline MagicWrapper operator + (MagicWrapper lhs, MagicWrapper rhs) {
+  return MagicWrapper{lhs._number + rhs._number};
+}
+
+#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H

test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap

@@ -0,0 +1,24 @@
+module UserA {
+    header "user-a.h"
+    export *
+}
+
+module UserB {
+    header "user-b.h"
+    export *
+}
+
+module UserC {
+    header "user-c.h"
+    export *
+}
+
+module DeclA {
+    header "decl-a.h"
+    export *
+}
+
+module DeclB {
+    header "decl-b.h"
+    export *
+}

test/Interop/Cxx/implementation-only-imports/Inputs/use-module-a.swift

@@ -0,0 +1 @@
+@_exported import UserA

test/Interop/Cxx/implementation-only-imports/Inputs/use-module-b.swift

@@ -0,0 +1 @@
+@_exported import UserB

test/Interop/Cxx/implementation-only-imports/Inputs/user-a.h

@@ -0,0 +1,6 @@
+#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_A_H
+#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_A_H
+
+#include "helper.h"
+
+#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_A_H

test/Interop/Cxx/implementation-only-imports/Inputs/user-b.h

@@ -0,0 +1,6 @@
+#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_B_H
+#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_B_H
+
+#include "helper.h"
+
+#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_B_H

test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h

@@ -0,0 +1,6 @@
+#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H
+#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H
+
+class MagicWrapper;
+
+#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H

test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift

@@ -0,0 +1,17 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-constructor-visibility.swift` checks
+// that the constructor decl can be found when at least one of the
+// modules is not `@_implementationOnly`.
+
+@_implementationOnly import UserA
+import UserB
+
+@_inlineable
+public func createAWrapper() {
+  let _ = MagicWrapper()
+}

test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift

@@ -0,0 +1,17 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-constructor-visibility-inversed.swift` checks
+// that the constructor decl can be found when at least one of the
+// modules is not `@_implementationOnly`.
+
+import UserA
+@_implementationOnly import UserB
+
+@_inlineable
+public func createAWrapper() {
+  let _ = MagicWrapper()
+}

test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift

@@ -0,0 +1,15 @@
+// RUN: %empty-directory(%t)
+// RUN: not %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs -enable-cxx-interop %s 2>&1 | %FileCheck %s
+
+// This test checks that Swift recognizes that the DeclA and DeclB provide
+// different implementations for `getFortySomething()`
+
+@_implementationOnly import DeclA
+import DeclB
+
+@_inlineable
+public func callFortySomething() -> CInt {
+  return getFortySomething()
+}
+
+// CHECK: 'getFortySomething' has different definitions in different modules

test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility-inversed.swift

@@ -0,0 +1,21 @@
+// RUN: %empty-directory(%t)
+// RUN: mkdir %t/use_module_a %t/use_module_b
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_a/UseModuleA.swiftmodule %S/Inputs/use-module-a.swift -I %S/Inputs -enable-cxx-interop
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_b/UseModuleB.swiftmodule %S/Inputs/use-module-b.swift -I %S/Inputs -enable-cxx-interop
+
+// RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs -enable-cxx-interop %s
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-function-transitive-visibility.swift`
+// ensures that Swift looks into the transitive visible modules as well
+// when looking for the `getFortyTwo()` decl. 
+
+@_implementationOnly import UseModuleA
+import UseModuleB
+
+@inlinable
+public func callFortyTwo() -> CInt {
+  return getFortyTwo()
+}

test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility.swift

@@ -0,0 +1,22 @@
+// RUN: %empty-directory(%t)
+// RUN: mkdir %t/use_module_a %t/use_module_b
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_a/UseModuleA.swiftmodule %S/Inputs/use-module-a.swift -I %S/Inputs -enable-cxx-interop
+// RUN: %target-swift-frontend -enable-library-evolution -swift-version 5 -emit-module -o %t/use_module_b/UseModuleB.swiftmodule %S/Inputs/use-module-b.swift -I %S/Inputs -enable-cxx-interop
+
+// RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs -enable-cxx-interop %s
+
+
+// Swift should consider all sources for a decl and recognize that the
+// decl is not hidden behind @_implementationOnly in all modules.
+
+// This test, as well as `check-function-transitive-visibility-inversed.swift`
+// ensures that Swift looks into the transitive visible modules as well
+// when looking for the `getFortyTwo()` decl. 
+
+import UseModuleA
+@_implementationOnly import UseModuleB
+
+@inlinable
+public func callFortyTwo() -> CInt {
+  return getFortyTwo()
+}