Merge pull request #34476 from scentini/implementation-only-symbol-visibility-fix

[cxx-interop] Fix SR-13785

Author: CodaFi

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/Inputs/user_a.h

@@ -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/Inputs/user_b.h

@@ -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()
+}