From 954d3e6e42f321824b16e42c9efb07c77e456fbc Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Tue, 27 Oct 2020 14:08:30 +0100 Subject: [PATCH 01/12] Add a test that verifies that Swift correctly recognizes symbol visibility when dealing with @_implementationOnly imports --- test/Interop/C/implementation-only-imports/Inputs/user_b.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/Interop/C/implementation-only-imports/Inputs/user_b.h b/test/Interop/C/implementation-only-imports/Inputs/user_b.h index 2beac678bf1d2..10b8209d54ff3 100644 --- a/test/Interop/C/implementation-only-imports/Inputs/user_b.h +++ b/test/Interop/C/implementation-only-imports/Inputs/user_b.h @@ -1,3 +1,4 @@ +<<<<<<< HEAD #ifndef TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERB_H #define TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERB_H From 00d4d118026977ac4a704f5a299bda63a0a4e413 Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Tue, 27 Oct 2020 14:16:48 +0100 Subject: [PATCH 02/12] Fix header guards --- test/Interop/C/implementation-only-imports/Inputs/user_b.h | 1 - 1 file changed, 1 deletion(-) diff --git a/test/Interop/C/implementation-only-imports/Inputs/user_b.h b/test/Interop/C/implementation-only-imports/Inputs/user_b.h index 10b8209d54ff3..2beac678bf1d2 100644 --- a/test/Interop/C/implementation-only-imports/Inputs/user_b.h +++ b/test/Interop/C/implementation-only-imports/Inputs/user_b.h @@ -1,4 +1,3 @@ -<<<<<<< HEAD #ifndef TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERB_H #define TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERB_H From 53c4393f565c316ff05b1f0979ceb2f6f00c7586 Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Wed, 28 Oct 2020 14:35:41 +0100 Subject: [PATCH 03/12] Fix SR-13785 --- lib/Sema/ResilienceDiagnostics.cpp | 2 ++ .../Inputs/use-module-a.swift | 1 + .../Inputs/use-module-b.swift | 1 + ...visible-symbols-recursively-inversed.swift | 26 +++++++++++++++++++ .../lookup-visible-symbols-recursively.swift | 26 +++++++++++++++++++ ...er-implementation-only-ones-inversed.swift | 22 ++++++++++++++++ ...symbol-over-implementation-only-ones.swift | 9 ++----- 7 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 test/Interop/C/implementation-only-imports/Inputs/use-module-a.swift create mode 100644 test/Interop/C/implementation-only-imports/Inputs/use-module-b.swift create mode 100644 test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively-inversed.swift create mode 100644 test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively.swift create mode 100644 test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/lib/Sema/ResilienceDiagnostics.cpp index dfade1e1094aa..f6c2dc279835a 100644 --- a/lib/Sema/ResilienceDiagnostics.cpp +++ b/lib/Sema/ResilienceDiagnostics.cpp @@ -21,6 +21,7 @@ #include "swift/AST/Decl.h" #include "swift/AST/DeclContext.h" #include "swift/AST/Initializer.h" +#include "swift/AST/ModuleNameLookup.h" #include "swift/AST/ProtocolConformance.h" #include "swift/AST/SourceFile.h" #include "swift/AST/TypeDeclFinder.h" @@ -130,6 +131,7 @@ TypeChecker::diagnoseDeclRefExportability(SourceLoc loc, if (originKind == DisallowedOriginKind::None) return false; + // TODO: different diagnostics ASTContext &ctx = definingModule->getASTContext(); auto fragileKind = where.getFragileFunctionKind(); diff --git a/test/Interop/C/implementation-only-imports/Inputs/use-module-a.swift b/test/Interop/C/implementation-only-imports/Inputs/use-module-a.swift new file mode 100644 index 0000000000000..f806673847548 --- /dev/null +++ b/test/Interop/C/implementation-only-imports/Inputs/use-module-a.swift @@ -0,0 +1 @@ +@_exported import UserA diff --git a/test/Interop/C/implementation-only-imports/Inputs/use-module-b.swift b/test/Interop/C/implementation-only-imports/Inputs/use-module-b.swift new file mode 100644 index 0000000000000..7708e676ab393 --- /dev/null +++ b/test/Interop/C/implementation-only-imports/Inputs/use-module-b.swift @@ -0,0 +1 @@ +@_exported import UserB diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively-inversed.swift b/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively-inversed.swift new file mode 100644 index 0000000000000..51c35bc083fd9 --- /dev/null +++ b/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively-inversed.swift @@ -0,0 +1,26 @@ +// 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 + + +// If a symbol comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the symbol being hidden. + +// Swift should consider all sources for the symbol and recognize that the +// symbol is not hidden behind @_implementationOnly in all modules. + +// This test, as well as the `lookup-visible-symbols-recursively.swift` check +// that the `getFortyTwo` symbol can be found when at least one of the +// modules is not `@_implementationOnly`. + +@_implementationOnly import UseModuleA +import UseModuleB + +@inlinable +public func callFortyTwo() -> CInt { + return getFortyTwo() +} diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively.swift b/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively.swift new file mode 100644 index 0000000000000..4919f69c05b80 --- /dev/null +++ b/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively.swift @@ -0,0 +1,26 @@ +// 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 + + +// If a symbol comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the symbol being hidden. + +// Swift should consider all sources for the symbol and recognize that the +// symbol is not hidden behind @_implementationOnly in all modules. + +// This test, as well as the `lookup-visible-symbols-recursively-inversed.swift +// check that the `getFortyTwo` symbol can be found when at least one of the +// modules is not `@_implementationOnly`. + +import UseModuleA +@_implementationOnly import UseModuleB + +@inlinable +public func callFortyTwo() -> CInt { + return getFortyTwo() +} diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift new file mode 100644 index 0000000000000..6bc711eb3cff1 --- /dev/null +++ b/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift @@ -0,0 +1,22 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s + +// If a symbol comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the symbol being hidden. + +// Swift should consider all sources for the symbol and recognize that the +// symbol is not hidden behind @_implementationOnly in all modules. + +// In this test case, UserA and UserB both textually include `helper.h`, +// therefore both export `getFortyTwo()`. +// This test verifies that even if Swift chooses UserB.getFortyTwo(), it +// doesn't error out, because the symbol is also exported from UserA. + +import UserA +@_implementationOnly import UserB + +@_inlineable +public func callFortyTwo() -> CInt { + return getFortyTwo() +} diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift index a7a04c655547c..105372993554b 100644 --- a/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift +++ b/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift @@ -1,10 +1,6 @@ // RUN: %empty-directory(%t) // RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -// REQUIRES: SR-13785 - -// TODO: Fix @_implementationOnly to consider all symbol sources - // If a symbol comes from two modules, one of which is marked as // @_implementationOnly, Swift may choose the @_implementationOnly source // and then error out due to the symbol being hidden. @@ -12,11 +8,10 @@ // Swift should consider all sources for the symbol and recognize that the // symbol is not hidden behind @_implementationOnly in all modules. -// E.g: // In this test case, UserA and UserB both textually include `helper.h`, // therefore both export `getFortyTwo()`. -// This test verifies that even though Swift chooses UserA.getFortyTwo(), we -// shouldn't get an error, because the symbol is also exported from UserB. +// This test verifies that even if Swift chooses UserA.getFortyTwo(), it +// doesn't error out, because the symbol is also exported from UserB. @_implementationOnly import UserA import UserB From e20b0f874484cf8bb76f09d326855222f60ad2fb Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Wed, 28 Oct 2020 15:14:37 +0100 Subject: [PATCH 04/12] Minor fixes --- ...-visible-decls-recursively-inversed.swift} | 12 +++++----- ...=> lookup-visible-decls-recursively.swift} | 12 +++++----- ...r-implementation-only-decls-inversed.swift | 22 +++++++++++++++++++ ...-decl-over-implementation-only-decls.swift | 22 +++++++++++++++++++ ...er-implementation-only-ones-inversed.swift | 22 ------------------- ...symbol-over-implementation-only-ones.swift | 22 ------------------- 6 files changed, 56 insertions(+), 56 deletions(-) rename test/Interop/C/implementation-only-imports/{lookup-visible-symbols-recursively-inversed.swift => lookup-visible-decls-recursively-inversed.swift} (67%) rename test/Interop/C/implementation-only-imports/{lookup-visible-symbols-recursively.swift => lookup-visible-decls-recursively.swift} (66%) create mode 100644 test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift create mode 100644 test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift delete mode 100644 test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift delete mode 100644 test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively-inversed.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift similarity index 67% rename from test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively-inversed.swift rename to test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift index 51c35bc083fd9..7827506936b01 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively-inversed.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift @@ -6,15 +6,15 @@ // 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 -// If a symbol comes from two modules, one of which is marked as +// If a decl comes from two modules, one of which is marked as // @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the symbol being hidden. +// and then error out due to the decl being hidden. -// Swift should consider all sources for the symbol and recognize that the -// symbol is not hidden behind @_implementationOnly in all modules. +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. -// This test, as well as the `lookup-visible-symbols-recursively.swift` check -// that the `getFortyTwo` symbol can be found when at least one of the +// This test, as well as `lookup-visible-decls-recursively.swift` checks +// that the `getFortyTwo` decl can be found when at least one of the // modules is not `@_implementationOnly`. @_implementationOnly import UseModuleA diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift similarity index 66% rename from test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively.swift rename to test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift index 4919f69c05b80..cea5566e94ccc 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-symbols-recursively.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift @@ -6,15 +6,15 @@ // 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 -// If a symbol comes from two modules, one of which is marked as +// If a decl comes from two modules, one of which is marked as // @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the symbol being hidden. +// and then error out due to the decl being hidden. -// Swift should consider all sources for the symbol and recognize that the -// symbol is not hidden behind @_implementationOnly in all modules. +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. -// This test, as well as the `lookup-visible-symbols-recursively-inversed.swift -// check that the `getFortyTwo` symbol can be found when at least one of the +// This test, as well as `lookup-visible-decls-recursively-inversed.swift` +// checks that the `getFortyTwo` decl can be found when at least one of the // modules is not `@_implementationOnly`. import UseModuleA diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift new file mode 100644 index 0000000000000..7b8e83da568dd --- /dev/null +++ b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift @@ -0,0 +1,22 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as +// `prefer-a-visible-decl-over-implementation-only-decls.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() +} diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift new file mode 100644 index 0000000000000..da9c72b403884 --- /dev/null +++ b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift @@ -0,0 +1,22 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as +// `prefer-a-visible-decl-over-implementation-only-decls-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() +} diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift deleted file mode 100644 index 6bc711eb3cff1..0000000000000 --- a/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones-inversed.swift +++ /dev/null @@ -1,22 +0,0 @@ -// RUN: %empty-directory(%t) -// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s - -// If a symbol comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the symbol being hidden. - -// Swift should consider all sources for the symbol and recognize that the -// symbol is not hidden behind @_implementationOnly in all modules. - -// In this test case, UserA and UserB both textually include `helper.h`, -// therefore both export `getFortyTwo()`. -// This test verifies that even if Swift chooses UserB.getFortyTwo(), it -// doesn't error out, because the symbol is also exported from UserA. - -import UserA -@_implementationOnly import UserB - -@_inlineable -public func callFortyTwo() -> CInt { - return getFortyTwo() -} diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift deleted file mode 100644 index 105372993554b..0000000000000 --- a/test/Interop/C/implementation-only-imports/prefer-a-visible-symbol-over-implementation-only-ones.swift +++ /dev/null @@ -1,22 +0,0 @@ -// RUN: %empty-directory(%t) -// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s - -// If a symbol comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the symbol being hidden. - -// Swift should consider all sources for the symbol and recognize that the -// symbol is not hidden behind @_implementationOnly in all modules. - -// In this test case, UserA and UserB both textually include `helper.h`, -// therefore both export `getFortyTwo()`. -// This test verifies that even if Swift chooses UserA.getFortyTwo(), it -// doesn't error out, because the symbol is also exported from UserB. - -@_implementationOnly import UserA -import UserB - -@_inlineable -public func callFortyTwo() -> CInt { - return getFortyTwo() -} From a22e65e35add6ef3191af8bebf3acc0a231dd44c Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Thu, 29 Oct 2020 11:09:14 +0100 Subject: [PATCH 05/12] Move logic to TypeCheckAccess.getDisallowedOriginKind --- lib/Sema/TypeCheckAccess.cpp | 40 +++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index dd6cc34657844..9a0c24ed3aba2 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/lib/Sema/TypeCheckAccess.cpp @@ -1447,25 +1447,55 @@ class UsableFromInlineChecker : public AccessControlCheckerBase, /// /// Local variant to swift::getDisallowedOriginKind for downgrade to warnings. DisallowedOriginKind -swift::getDisallowedOriginKind(const Decl *decl, - ExportContext where, +swift::getDisallowedOriginKind(const Decl *decl, ExportContext where, DowngradeToWarning &downgradeToWarning) { downgradeToWarning = DowngradeToWarning::No; ModuleDecl *M = decl->getModuleContext(); + auto *SF = where.getDeclContext()->getParentSourceFile(); if (SF->isImportedImplementationOnly(M)) { + // Temporarily downgrade implementation-only exportability in SPI to // a warning. 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 look at all the imported + // modules and see if we can find the decl in a non @_implementationOnly + // module. + + SmallVector importedModules; + SF->getImportedModules( + importedModules, + ModuleDecl::ImportFilter( + {ModuleDecl::ImportFilterKind::Exported, + ModuleDecl::ImportFilterKind::Default, + ModuleDecl::ImportFilterKind::SPIAccessControl, + ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay})); + auto nlOptions = NL_QualifiedDefault | NL_IncludeUsableFromInline; + if (auto val = dyn_cast(decl)) { + for (auto &importedModule : importedModules) { + SmallVector candidateDecls; + namelookup::lookupInModule( + importedModule.importedModule, val->getName(), candidateDecls, + NLKind::UnqualifiedLookup, namelookup::ResolutionKind::Overloadable, + importedModule.importedModule, nlOptions); + for (auto *candidateDecl : candidateDecls) { + if (candidateDecl->getFormalAccess() >= AccessLevel::Public) { + return DisallowedOriginKind::None; + } + } + } + } // Implementation-only imported, cannot be reexported. return DisallowedOriginKind::ImplementationOnly; } else if (decl->isSPI() && !where.isSPI()) { // SPI can only be exported in SPI. - return where.getDeclContext()->getParentModule() == M ? - DisallowedOriginKind::SPILocal : - DisallowedOriginKind::SPIImported; + return where.getDeclContext()->getParentModule() == M + ? DisallowedOriginKind::SPILocal + : DisallowedOriginKind::SPIImported; } return DisallowedOriginKind::None; From 679ce361aafa773b3da6c1ecd848427eef03f32a Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Thu, 29 Oct 2020 16:55:10 +0100 Subject: [PATCH 06/12] Add more tests --- lib/Sema/ResilienceDiagnostics.cpp | 1 - lib/Sema/TypeCheckAccess.cpp | 1 + ...p-visible-decls-recursively-inversed.swift | 6 ++--- .../lookup-visible-decls-recursively.swift | 6 ++--- .../Inputs/decl-a.h | 8 ++++++ .../Inputs/decl-b.h | 8 ++++++ .../Inputs/helper.h | 15 +++++++++++ .../Inputs/module.modulemap | 19 ++++++++++++++ .../Inputs/use-module-a.swift | 1 + .../Inputs/use-module-b.swift | 1 + .../Inputs/user-a.h | 6 +++++ .../Inputs/user-b.h | 6 +++++ .../check-constructor-visibility.swift | 12 +++++++++ .../check-decls-are-identical.swift | 12 +++++++++ ...p-visible-decls-recursively-inversed.swift | 26 +++++++++++++++++++ .../lookup-visible-decls-recursively.swift | 26 +++++++++++++++++++ ...r-implementation-only-decls-inversed.swift | 22 ++++++++++++++++ ...-decl-over-implementation-only-decls.swift | 22 ++++++++++++++++ 18 files changed, 191 insertions(+), 7 deletions(-) create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/helper.h create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/use-module-a.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/use-module-b.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/user-a.h create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/user-b.h create mode 100644 test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/lib/Sema/ResilienceDiagnostics.cpp index f6c2dc279835a..ab12f569bc545 100644 --- a/lib/Sema/ResilienceDiagnostics.cpp +++ b/lib/Sema/ResilienceDiagnostics.cpp @@ -21,7 +21,6 @@ #include "swift/AST/Decl.h" #include "swift/AST/DeclContext.h" #include "swift/AST/Initializer.h" -#include "swift/AST/ModuleNameLookup.h" #include "swift/AST/ProtocolConformance.h" #include "swift/AST/SourceFile.h" #include "swift/AST/TypeDeclFinder.h" diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index 9a0c24ed3aba2..dabab3120d119 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/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/ModuleNameLookup.h" #include "swift/AST/Pattern.h" #include "swift/AST/ParameterList.h" #include "swift/AST/TypeCheckRequests.h" diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift index 7827506936b01..b748b54f151c0 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift @@ -1,9 +1,9 @@ // 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 -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 -enable-cxx-interop %s +// RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs %s // If a decl comes from two modules, one of which is marked as diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift index cea5566e94ccc..de7615028bb28 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift @@ -1,9 +1,9 @@ // 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 -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 -enable-cxx-interop %s +// RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs %s // If a decl comes from two modules, one of which is marked as diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h new file mode 100644 index 0000000000000..d23c88ac9161c --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h @@ -0,0 +1,8 @@ +#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 diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h new file mode 100644 index 0000000000000..1ed52c69cd52a --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h @@ -0,0 +1,8 @@ +#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 diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h b/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h new file mode 100644 index 0000000000000..0a0dede2cc16b --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h @@ -0,0 +1,15 @@ +#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 { + int _number; +public: + MagicWrapper(){}; + MagicWrapper(int number) : _number(number) {}; +}; + +#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap b/test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap new file mode 100644 index 0000000000000..99e24f87e8418 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap @@ -0,0 +1,19 @@ +module UserA { + header "user-a.h" + export * +} + +module UserB { + header "user-b.h" + export * +} + +module DeclA { + header "decl-a.h" + export * +} + +module DeclB { + header "decl-b.h" + export * +} diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/use-module-a.swift b/test/Interop/Cxx/implementation-only-imports/Inputs/use-module-a.swift new file mode 100644 index 0000000000000..f806673847548 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/use-module-a.swift @@ -0,0 +1 @@ +@_exported import UserA diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/use-module-b.swift b/test/Interop/Cxx/implementation-only-imports/Inputs/use-module-b.swift new file mode 100644 index 0000000000000..7708e676ab393 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/use-module-b.swift @@ -0,0 +1 @@ +@_exported import UserB diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/user-a.h b/test/Interop/Cxx/implementation-only-imports/Inputs/user-a.h new file mode 100644 index 0000000000000..fb7daccebef6d --- /dev/null +++ b/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 diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/user-b.h b/test/Interop/Cxx/implementation-only-imports/Inputs/user-b.h new file mode 100644 index 0000000000000..1daeacb030ee3 --- /dev/null +++ b/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 diff --git a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift new file mode 100644 index 0000000000000..df7c52e62d6ff --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift @@ -0,0 +1,12 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop + +// REQUIRES: SR-13785 + +@_implementationOnly import UserA +import UserB + +@_inlineable +public func createAWrapper() { + let _wrapper = MagicWrapper(); +} diff --git a/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift new file mode 100644 index 0000000000000..5d51cd2e584a6 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift @@ -0,0 +1,12 @@ +// 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 + +@_implementationOnly import DeclA +import DeclB + +@_inlineable +public func callFortySomething() -> CInt { + return getFortySomething() +} + +// CHECK: 'getFortySomething' has different definitions in different modules diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift new file mode 100644 index 0000000000000..7827506936b01 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift @@ -0,0 +1,26 @@ +// 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 + + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as `lookup-visible-decls-recursively.swift` checks +// that the `getFortyTwo` decl can be found when at least one of the +// modules is not `@_implementationOnly`. + +@_implementationOnly import UseModuleA +import UseModuleB + +@inlinable +public func callFortyTwo() -> CInt { + return getFortyTwo() +} diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift new file mode 100644 index 0000000000000..cea5566e94ccc --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift @@ -0,0 +1,26 @@ +// 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 + + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as `lookup-visible-decls-recursively-inversed.swift` +// checks that the `getFortyTwo` decl can be found when at least one of the +// modules is not `@_implementationOnly`. + +import UseModuleA +@_implementationOnly import UseModuleB + +@inlinable +public func callFortyTwo() -> CInt { + return getFortyTwo() +} diff --git a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift new file mode 100644 index 0000000000000..10b00afec08d6 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift @@ -0,0 +1,22 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs -enable-cxx-interop %s + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as +// `prefer-a-visible-decl-over-implementation-only-decls.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() +} diff --git a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift new file mode 100644 index 0000000000000..fa1daf01d61eb --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift @@ -0,0 +1,22 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs -enable-cxx-interop %s + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as +// `prefer-a-visible-decl-over-implementation-only-decls-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() +} From 8062bfcbb88fe19fcb4feb049409e2635aae79b8 Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Thu, 29 Oct 2020 17:07:00 +0100 Subject: [PATCH 07/12] Format files --- lib/Sema/ResilienceDiagnostics.cpp | 1 - lib/Sema/TypeCheckAccess.cpp | 11 +++++------ .../Cxx/implementation-only-imports/Inputs/decl-a.h | 4 +--- .../Cxx/implementation-only-imports/Inputs/decl-b.h | 4 +--- .../Cxx/implementation-only-imports/Inputs/helper.h | 11 +++++------ .../check-constructor-visibility.swift | 2 +- .../check-decls-are-identical.swift | 2 +- .../lookup-visible-decls-recursively-inversed.swift | 2 +- .../lookup-visible-decls-recursively.swift | 2 +- ...decl-over-implementation-only-decls-inversed.swift | 2 +- ...-visible-decl-over-implementation-only-decls.swift | 2 +- 11 files changed, 18 insertions(+), 25 deletions(-) diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/lib/Sema/ResilienceDiagnostics.cpp index ab12f569bc545..dfade1e1094aa 100644 --- a/lib/Sema/ResilienceDiagnostics.cpp +++ b/lib/Sema/ResilienceDiagnostics.cpp @@ -130,7 +130,6 @@ TypeChecker::diagnoseDeclRefExportability(SourceLoc loc, if (originKind == DisallowedOriginKind::None) return false; - // TODO: different diagnostics ASTContext &ctx = definingModule->getASTContext(); auto fragileKind = where.getFragileFunctionKind(); diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index dabab3120d119..2ebd9bad7029c 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/lib/Sema/TypeCheckAccess.cpp @@ -1448,14 +1448,13 @@ class UsableFromInlineChecker : public AccessControlCheckerBase, /// /// Local variant to swift::getDisallowedOriginKind for downgrade to warnings. DisallowedOriginKind -swift::getDisallowedOriginKind(const Decl *decl, ExportContext where, +swift::getDisallowedOriginKind(const Decl *decl, + ExportContext where, DowngradeToWarning &downgradeToWarning) { downgradeToWarning = DowngradeToWarning::No; ModuleDecl *M = decl->getModuleContext(); - auto *SF = where.getDeclContext()->getParentSourceFile(); if (SF->isImportedImplementationOnly(M)) { - // Temporarily downgrade implementation-only exportability in SPI to // a warning. if (where.isSPI()) @@ -1494,9 +1493,9 @@ swift::getDisallowedOriginKind(const Decl *decl, ExportContext where, return DisallowedOriginKind::ImplementationOnly; } else if (decl->isSPI() && !where.isSPI()) { // SPI can only be exported in SPI. - return where.getDeclContext()->getParentModule() == M - ? DisallowedOriginKind::SPILocal - : DisallowedOriginKind::SPIImported; + return where.getDeclContext()->getParentModule() == M ? + DisallowedOriginKind::SPILocal : + DisallowedOriginKind::SPIImported; } return DisallowedOriginKind::None; diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h index d23c88ac9161c..72998faab7e62 100644 --- a/test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-a.h @@ -1,8 +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; -}; +inline int getFortySomething() { return 42; }; #endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_A_H diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h index 1ed52c69cd52a..35839a51051af 100644 --- a/test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/decl-b.h @@ -1,8 +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; -}; +inline int getFortySomething() { return 46; }; #endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_DECL_B_H diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h b/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h index 0a0dede2cc16b..379bc71d2c70a 100644 --- a/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h @@ -1,15 +1,14 @@ #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; -}; +inline int getFortyTwo() { return 42; }; class MagicWrapper { - int _number; + int _number; + public: - MagicWrapper(){}; - MagicWrapper(int number) : _number(number) {}; + MagicWrapper(){}; + MagicWrapper(int number) : _number(number){}; }; #endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_HELPER_H diff --git a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift index df7c52e62d6ff..a0021f6dfe5dc 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift @@ -8,5 +8,5 @@ import UserB @_inlineable public func createAWrapper() { - let _wrapper = MagicWrapper(); + let _wrapper = MagicWrapper() } diff --git a/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift index 5d51cd2e584a6..644d83bb485c0 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift @@ -6,7 +6,7 @@ import DeclB @_inlineable public func callFortySomething() -> CInt { - return getFortySomething() + return getFortySomething() } // CHECK: 'getFortySomething' has different definitions in different modules diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift index 7827506936b01..cd9ff663bff16 100644 --- a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift +++ b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift @@ -22,5 +22,5 @@ import UseModuleB @inlinable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift index cea5566e94ccc..84cb2f2c3ff52 100644 --- a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift +++ b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift @@ -22,5 +22,5 @@ import UseModuleA @inlinable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } diff --git a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift index 10b00afec08d6..214df13928ee5 100644 --- a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift +++ b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift @@ -18,5 +18,5 @@ import UserA @_inlineable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } diff --git a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift index fa1daf01d61eb..8249ef9bd671a 100644 --- a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift +++ b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift @@ -18,5 +18,5 @@ import UserB @_inlineable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } From 81c072163332bc881e070ea2e8ebcb48cea0d8e1 Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Thu, 29 Oct 2020 17:16:12 +0100 Subject: [PATCH 08/12] Format files --- .../C/implementation-only-imports/Inputs/module.modulemap | 4 ++-- test/Interop/C/implementation-only-imports/Inputs/user-a.h | 6 ++++++ test/Interop/C/implementation-only-imports/Inputs/user-b.h | 6 ++++++ test/Interop/C/implementation-only-imports/Inputs/user_a.h | 6 ------ test/Interop/C/implementation-only-imports/Inputs/user_b.h | 6 ------ .../lookup-visible-decls-recursively-inversed.swift | 6 +++--- .../lookup-visible-decls-recursively.swift | 2 +- ...sible-decl-over-implementation-only-decls-inversed.swift | 2 +- ...efer-a-visible-decl-over-implementation-only-decls.swift | 2 +- 9 files changed, 20 insertions(+), 20 deletions(-) create mode 100644 test/Interop/C/implementation-only-imports/Inputs/user-a.h create mode 100644 test/Interop/C/implementation-only-imports/Inputs/user-b.h delete mode 100644 test/Interop/C/implementation-only-imports/Inputs/user_a.h delete mode 100644 test/Interop/C/implementation-only-imports/Inputs/user_b.h diff --git a/test/Interop/C/implementation-only-imports/Inputs/module.modulemap b/test/Interop/C/implementation-only-imports/Inputs/module.modulemap index dd845b25d568c..cd5b5d67c7df1 100644 --- a/test/Interop/C/implementation-only-imports/Inputs/module.modulemap +++ b/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 * } diff --git a/test/Interop/C/implementation-only-imports/Inputs/user-a.h b/test/Interop/C/implementation-only-imports/Inputs/user-a.h new file mode 100644 index 0000000000000..4f6d821f9b7fb --- /dev/null +++ b/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 diff --git a/test/Interop/C/implementation-only-imports/Inputs/user-b.h b/test/Interop/C/implementation-only-imports/Inputs/user-b.h new file mode 100644 index 0000000000000..7b7dbc7a6fd1e --- /dev/null +++ b/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 diff --git a/test/Interop/C/implementation-only-imports/Inputs/user_a.h b/test/Interop/C/implementation-only-imports/Inputs/user_a.h deleted file mode 100644 index bebbd757c5561..0000000000000 --- a/test/Interop/C/implementation-only-imports/Inputs/user_a.h +++ /dev/null @@ -1,6 +0,0 @@ -#ifndef TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERA_H -#define TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERA_H - -#include "helper.h" - -#endif // TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERA_H diff --git a/test/Interop/C/implementation-only-imports/Inputs/user_b.h b/test/Interop/C/implementation-only-imports/Inputs/user_b.h deleted file mode 100644 index 2beac678bf1d2..0000000000000 --- a/test/Interop/C/implementation-only-imports/Inputs/user_b.h +++ /dev/null @@ -1,6 +0,0 @@ -#ifndef TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERB_H -#define TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERB_H - -#include "helper.h" - -#endif // TEST_INTEROP_C_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USERB_H diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift index b748b54f151c0..76d50657e6d0d 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift @@ -17,10 +17,10 @@ // that the `getFortyTwo` decl can be found when at least one of the // modules is not `@_implementationOnly`. -@_implementationOnly import UseModuleA -import UseModuleB +import UseModuleA +@_implementationOnly import UseModuleB @inlinable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift index de7615028bb28..623450f5f5deb 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift @@ -22,5 +22,5 @@ import UseModuleA @inlinable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift index 7b8e83da568dd..9ea3a887cd124 100644 --- a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift +++ b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift @@ -18,5 +18,5 @@ import UserA @_inlineable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift index da9c72b403884..550a4591fcdd6 100644 --- a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift +++ b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift @@ -18,5 +18,5 @@ import UserB @_inlineable public func callFortyTwo() -> CInt { - return getFortyTwo() + return getFortyTwo() } From 3ca701e561d78657ee26c2d0e59c89567778ad5f Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Fri, 6 Nov 2020 14:46:25 +0100 Subject: [PATCH 09/12] Change logic for visible decls lookup --- lib/Sema/TypeCheckAccess.cpp | 91 ++++++++++++++----- ...p-visible-decls-recursively-inversed.swift | 6 +- .../lookup-visible-decls-recursively.swift | 4 +- .../Inputs/helper.h | 14 ++- .../Inputs/module.modulemap | 5 + .../Inputs/user-c.h | 8 ++ ...heck-constructor-visibility-inversed.swift | 21 +++++ .../check-constructor-visibility.swift | 17 +++- .../check-decls-are-identical.swift | 3 + .../check-operator-visibility-inversed.swift | 31 +++++++ .../check-operator-visibility.swift | 30 ++++++ ...p-visible-decls-recursively-inversed.swift | 6 +- .../lookup-visible-decls-recursively.swift | 4 +- .../skip-forward-declarations.swift | 16 ++++ 14 files changed, 215 insertions(+), 41 deletions(-) create mode 100644 test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h create mode 100644 test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index 2ebd9bad7029c..2512b8c6008ad 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/lib/Sema/TypeCheckAccess.cpp @@ -22,10 +22,17 @@ #include "swift/AST/ASTWalker.h" #include "swift/AST/DiagnosticsSema.h" #include "swift/AST/ExistentialLayout.h" -#include "swift/AST/ModuleNameLookup.h" +#include "swift/AST/Import.h" #include "swift/AST/Pattern.h" #include "swift/AST/ParameterList.h" +#include "swift/AST/SourceFile.h" +#include "swift/AST/Type.h" #include "swift/AST/TypeCheckRequests.h" +#include "clang/AST/Type.h" +#include "clang/Basic/Module.h" +#include "llvm/ADT/SmallPtrSet.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/Support/Debug.h" using namespace swift; @@ -1441,6 +1448,46 @@ class UsableFromInlineChecker : public AccessControlCheckerBase, } }; +void getVisibleModules( + llvm::SmallPtrSetImpl &visibleModules, + swift::SourceFile *SF) { + llvm::SmallPtrSet seenModules; + llvm::SmallVector stack; + + auto filter = ModuleDecl::ImportFilter( + {ModuleDecl::ImportFilterKind::Exported, + ModuleDecl::ImportFilterKind::Default, + ModuleDecl::ImportFilterKind::SPIAccessControl, + ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay}); + + SmallVector sfImportedModules; + + SF->getImportedModules(sfImportedModules, filter); + + for (auto importedModule : sfImportedModules) { + stack.push_back(importedModule.importedModule); + seenModules.insert(importedModule.importedModule); + } + + while (!stack.empty()) { + auto module = stack.pop_back_val(); + if (auto clangModule = module->findUnderlyingClangModule()) { + visibleModules.insert(clangModule); + continue; + } + + SmallVector importedModules; + module->getImportedModules(importedModules, filter); + + for (auto &importedModule : importedModules) { + auto moduleDecl = importedModule.importedModule; + if (!seenModules.contains(moduleDecl)) { + seenModules.insert(moduleDecl); + stack.push_back(moduleDecl); + } + } + } +} } // end anonymous namespace /// Returns the kind of origin, implementation-only import or SPI declaration, @@ -1448,12 +1495,13 @@ class UsableFromInlineChecker : public AccessControlCheckerBase, /// /// Local variant to swift::getDisallowedOriginKind for downgrade to warnings. DisallowedOriginKind -swift::getDisallowedOriginKind(const Decl *decl, - ExportContext where, +swift::getDisallowedOriginKind(const Decl *decl, ExportContext where, DowngradeToWarning &downgradeToWarning) { downgradeToWarning = DowngradeToWarning::No; ModuleDecl *M = decl->getModuleContext(); + auto *SF = where.getDeclContext()->getParentSourceFile(); + if (SF->isImportedImplementationOnly(M)) { // Temporarily downgrade implementation-only exportability in SPI to // a warning. @@ -1462,31 +1510,28 @@ swift::getDisallowedOriginKind(const Decl *decl, // 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 look at all the imported + // a non @_implementationOnly module. Thus, we look at all the visible // modules and see if we can find the decl in a non @_implementationOnly // module. + llvm::SmallPtrSet visibleModules; + getVisibleModules(visibleModules, SF); - SmallVector importedModules; - SF->getImportedModules( - importedModules, - ModuleDecl::ImportFilter( - {ModuleDecl::ImportFilterKind::Exported, - ModuleDecl::ImportFilterKind::Default, - ModuleDecl::ImportFilterKind::SPIAccessControl, - ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay})); - auto nlOptions = NL_QualifiedDefault | NL_IncludeUsableFromInline; - if (auto val = dyn_cast(decl)) { - for (auto &importedModule : importedModules) { - SmallVector candidateDecls; - namelookup::lookupInModule( - importedModule.importedModule, val->getName(), candidateDecls, - NLKind::UnqualifiedLookup, namelookup::ResolutionKind::Overloadable, - importedModule.importedModule, nlOptions); - for (auto *candidateDecl : candidateDecls) { - if (candidateDecl->getFormalAccess() >= AccessLevel::Public) { - return DisallowedOriginKind::None; + if (auto clangDecl = decl->getClangDecl()) { + for (auto redecl : clangDecl->redecls()) { + if (!visibleModules.contains(redecl->getOwningModule())) { + redecl->getOwningModule()->dump(); + continue; + } + + if (auto tagRedecl = dyn_cast(redecl)) { + // This is a forward declaration. + // We ignore visibility of these. + if (tagRedecl->getBraceRange().isInvalid()) { + continue; } } + + return DisallowedOriginKind::None; } } // Implementation-only imported, cannot be reexported. diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift index 76d50657e6d0d..0b6daed7a7bf5 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift @@ -13,9 +13,9 @@ // Swift should consider all sources for the decl and recognize that the // decl is not hidden behind @_implementationOnly in all modules. -// This test, as well as `lookup-visible-decls-recursively.swift` checks -// that the `getFortyTwo` decl can be found when at least one of the -// modules is not `@_implementationOnly`. +// This test, as well as `lookup-visible-decls-recursively.swift` +// ensures that Swift looks into the transitive visible modules as well +// when looking for the `getFortyTwo` decl. import UseModuleA @_implementationOnly import UseModuleB diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift index 623450f5f5deb..ad2c1a277d898 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift +++ b/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift @@ -14,8 +14,8 @@ // decl is not hidden behind @_implementationOnly in all modules. // This test, as well as `lookup-visible-decls-recursively-inversed.swift` -// checks that the `getFortyTwo` decl can be found when at least one of the -// modules is not `@_implementationOnly`. +// ensures that Swift looks into the transitive visible modules as well +// when looking for the `getFortyTwo` decl. import UseModuleA @_implementationOnly import UseModuleB diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h b/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h index 379bc71d2c70a..5c4d405bb5d61 100644 --- a/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/helper.h @@ -1,14 +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; }; +inline int getFortyTwo() { return 42; } class MagicWrapper { - int _number; - public: - MagicWrapper(){}; + 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 diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap b/test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap index 99e24f87e8418..b3d9edc456782 100644 --- a/test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/module.modulemap @@ -8,6 +8,11 @@ module UserB { export * } +module UserC { + header "user-c.h" + export * +} + module DeclA { header "decl-a.h" export * diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h b/test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h new file mode 100644 index 0000000000000..5da1e5cf79590 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h @@ -0,0 +1,8 @@ +#ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H +#define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H + +int getFortyTwo(); + +class MagicWrapper; + +#endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H diff --git a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift new file mode 100644 index 0000000000000..38a54879f5922 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift @@ -0,0 +1,21 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the 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() +} diff --git a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift index a0021f6dfe5dc..74c1192454500 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift @@ -1,12 +1,21 @@ // RUN: %empty-directory(%t) // RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop -// REQUIRES: SR-13785 +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. -@_implementationOnly import UserA -import UserB +// Swift should consider all sources for the 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 _wrapper = MagicWrapper() + let _ = MagicWrapper() } diff --git a/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift index 644d83bb485c0..4a8ecf2ab1575 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift @@ -1,6 +1,9 @@ // 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 check that Swift recognizes that the DeclA and DeclB provide +// different implementations for `getFortySomething()` + @_implementationOnly import DeclA import DeclB diff --git a/test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift new file mode 100644 index 0000000000000..7d387c953e84f --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift @@ -0,0 +1,31 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as `check-operator-visibility.swift` checks +// that the operator decl can be found when at least one of the +// modules is not `@_implementationOnly`. + + +import UserA +@_implementationOnly import UserB + +@_inlineable +public func addWrappers() { + let wrapperA = MagicWrapper() + let wrapperB = MagicWrapper() + let _ = wrapperA + wrapperB +} + +@_inlineable +public func subtractWrappers() { + var wrapperA = MagicWrapper() + let wrapperB = MagicWrapper() + let _ = wrapperA - wrapperB +} diff --git a/test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift new file mode 100644 index 0000000000000..ce96b07d96339 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift @@ -0,0 +1,30 @@ +// RUN: %empty-directory(%t) +// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop + +// If a decl comes from two modules, one of which is marked as +// @_implementationOnly, Swift may choose the @_implementationOnly source +// and then error out due to the decl being hidden. + +// Swift should consider all sources for the decl and recognize that the +// decl is not hidden behind @_implementationOnly in all modules. + +// This test, as well as `check-operator-visibility-inversed.swift` checks +// that the operator decl can be found when at least one of the +// modules is not `@_implementationOnly`. + +@_implementationOnly import UserA +import UserB + +@_inlineable +public func addWrappers() { + let wrapperA = MagicWrapper() + let wrapperB = MagicWrapper() + let _ = wrapperA + wrapperB +} + +@_inlineable +public func subtractWrappers() { + var wrapperA = MagicWrapper() + let wrapperB = MagicWrapper() + let _ = wrapperA - wrapperB +} diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift index cd9ff663bff16..4b3ac791fd41e 100644 --- a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift +++ b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift @@ -13,9 +13,9 @@ // Swift should consider all sources for the decl and recognize that the // decl is not hidden behind @_implementationOnly in all modules. -// This test, as well as `lookup-visible-decls-recursively.swift` checks -// that the `getFortyTwo` decl can be found when at least one of the -// modules is not `@_implementationOnly`. +// This test, as well as `lookup-visible-decls-recursively.swift` +// ensures that Swift looks into the transitive visible modules as well +// when looking for the `getFortyTwo` decl. @_implementationOnly import UseModuleA import UseModuleB diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift index 84cb2f2c3ff52..ff537bb496d72 100644 --- a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift +++ b/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift @@ -14,8 +14,8 @@ // decl is not hidden behind @_implementationOnly in all modules. // This test, as well as `lookup-visible-decls-recursively-inversed.swift` -// checks that the `getFortyTwo` decl can be found when at least one of the -// modules is not `@_implementationOnly`. +// ensures that Swift looks into the transitive visible modules as well +// when looking for the `getFortyTwo` decl. import UseModuleA @_implementationOnly import UseModuleB diff --git a/test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift b/test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift new file mode 100644 index 0000000000000..9e59f2f804033 --- /dev/null +++ b/test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift @@ -0,0 +1,16 @@ +// 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 forward declarations are not considered +// when determining the visibility of the decl. + +@_implementationOnly import UserA +import UserC + +@_inlineable +public func createAWrapper() { + let _ = MagicWrapper() + let _ = MagicWrapper(42) +} + +// CHECK: struct 'MagicWrapper' cannot be used in an '@inlinable' function because 'UserA' was imported implementation-only From 58ff402345a4a3361d01e2d633c80bd475675c1b Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Fri, 6 Nov 2020 16:10:25 +0100 Subject: [PATCH 10/12] Remove unnecessary header --- lib/Sema/TypeCheckAccess.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index a0f9c95d7af0f..f8b07934e2ab1 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/lib/Sema/TypeCheckAccess.cpp @@ -32,7 +32,6 @@ #include "clang/Basic/Module.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" -#include "llvm/Support/Debug.h" using namespace swift; From 1a18225ef9f9531565a9a580794e5190bdab646c Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Tue, 10 Nov 2020 11:22:38 +0100 Subject: [PATCH 11/12] Polish PR --- lib/Sema/TypeCheckAccess.cpp | 1 - ...tion-transitive-visibility-inversed.swift} | 11 +++------- ...heck-function-transitive-visibility.swift} | 11 +++------- ... check-function-visibility-inversed.swift} | 9 ++------ .../check-function-visibility.swift | 17 ++++++++++++++ ...-decl-over-implementation-only-decls.swift | 22 ------------------- .../Inputs/user-c.h | 2 -- ...heck-constructor-visibility-inversed.swift | 6 +---- .../check-constructor-visibility.swift | 6 +---- .../check-decls-are-identical.swift | 2 +- ...tion-transitive-visibility-inversed.swift} | 11 +++------- ...heck-function-transitive-visibility.swift} | 10 +++------ .../check-function-visibility-inversed.swift | 17 ++++++++++++++ .../check-function-visibility.swift | 17 ++++++++++++++ .../check-operator-visibility-inversed.swift | 8 +++---- .../check-operator-visibility.swift | 8 +++---- ...r-implementation-only-decls-inversed.swift | 22 ------------------- ...-decl-over-implementation-only-decls.swift | 22 ------------------- .../skip-forward-declarations.swift | 1 - 19 files changed, 74 insertions(+), 129 deletions(-) rename test/Interop/C/implementation-only-imports/{lookup-visible-decls-recursively-inversed.swift => check-function-transitive-visibility-inversed.swift} (68%) rename test/Interop/C/implementation-only-imports/{lookup-visible-decls-recursively.swift => check-function-transitive-visibility.swift} (67%) rename test/Interop/C/implementation-only-imports/{prefer-a-visible-decl-over-implementation-only-decls-inversed.swift => check-function-visibility-inversed.swift} (54%) create mode 100644 test/Interop/C/implementation-only-imports/check-function-visibility.swift delete mode 100644 test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift rename test/Interop/Cxx/implementation-only-imports/{lookup-visible-decls-recursively-inversed.swift => check-function-transitive-visibility-inversed.swift} (69%) rename test/Interop/Cxx/implementation-only-imports/{lookup-visible-decls-recursively.swift => check-function-transitive-visibility.swift} (69%) create mode 100644 test/Interop/Cxx/implementation-only-imports/check-function-visibility-inversed.swift create mode 100644 test/Interop/Cxx/implementation-only-imports/check-function-visibility.swift delete mode 100644 test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift delete mode 100644 test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index f8b07934e2ab1..411de08043bb7 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/lib/Sema/TypeCheckAccess.cpp @@ -1519,7 +1519,6 @@ swift::getDisallowedOriginKind(const Decl *decl, if (auto clangDecl = decl->getClangDecl()) { for (auto redecl : clangDecl->redecls()) { if (!visibleModules.contains(redecl->getOwningModule())) { - redecl->getOwningModule()->dump(); continue; } diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/C/implementation-only-imports/check-function-transitive-visibility-inversed.swift similarity index 68% rename from test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift rename to test/Interop/C/implementation-only-imports/check-function-transitive-visibility-inversed.swift index 0b6daed7a7bf5..6746f49d7a49b 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift +++ b/test/Interop/C/implementation-only-imports/check-function-transitive-visibility-inversed.swift @@ -5,17 +5,12 @@ // RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs %s - -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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 `lookup-visible-decls-recursively.swift` +// 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. +// when looking for the `getFortyTwo()` decl. import UseModuleA @_implementationOnly import UseModuleB diff --git a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/C/implementation-only-imports/check-function-transitive-visibility.swift similarity index 67% rename from test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift rename to test/Interop/C/implementation-only-imports/check-function-transitive-visibility.swift index ad2c1a277d898..35515bfbcde54 100644 --- a/test/Interop/C/implementation-only-imports/lookup-visible-decls-recursively.swift +++ b/test/Interop/C/implementation-only-imports/check-function-transitive-visibility.swift @@ -5,17 +5,12 @@ // RUN: %target-swift-frontend -typecheck -swift-version 5 -I %t/use_module_a -I %t/use_module_b -I %S/Inputs %s - -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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 `lookup-visible-decls-recursively-inversed.swift` +// 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. +// when looking for the `getFortyTwo()` decl. import UseModuleA @_implementationOnly import UseModuleB diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift b/test/Interop/C/implementation-only-imports/check-function-visibility-inversed.swift similarity index 54% rename from test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift rename to test/Interop/C/implementation-only-imports/check-function-visibility-inversed.swift index 9ea3a887cd124..974349324ea83 100644 --- a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift +++ b/test/Interop/C/implementation-only-imports/check-function-visibility-inversed.swift @@ -1,15 +1,10 @@ // RUN: %empty-directory(%t) // RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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 -// `prefer-a-visible-decl-over-implementation-only-decls.swift` +// 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`. diff --git a/test/Interop/C/implementation-only-imports/check-function-visibility.swift b/test/Interop/C/implementation-only-imports/check-function-visibility.swift new file mode 100644 index 0000000000000..b427725e1e155 --- /dev/null +++ b/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() +} diff --git a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift b/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift deleted file mode 100644 index 550a4591fcdd6..0000000000000 --- a/test/Interop/C/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift +++ /dev/null @@ -1,22 +0,0 @@ -// RUN: %empty-directory(%t) -// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s - -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the -// decl is not hidden behind @_implementationOnly in all modules. - -// This test, as well as -// `prefer-a-visible-decl-over-implementation-only-decls-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() -} diff --git a/test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h b/test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h index 5da1e5cf79590..db273238536a2 100644 --- a/test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h +++ b/test/Interop/Cxx/implementation-only-imports/Inputs/user-c.h @@ -1,8 +1,6 @@ #ifndef TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H #define TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H -int getFortyTwo(); - class MagicWrapper; #endif // TEST_INTEROP_CXX_IMPLEMENTATION_ONLY_IMPORTS_INPUTS_USER_C_H diff --git a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift index 38a54879f5922..40ed6da45458f 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility-inversed.swift @@ -1,11 +1,7 @@ // RUN: %empty-directory(%t) // RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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 diff --git a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift index 74c1192454500..e2fe2fe0ee3c2 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-constructor-visibility.swift @@ -1,11 +1,7 @@ // RUN: %empty-directory(%t) // RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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 diff --git a/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift index 4a8ecf2ab1575..d43ce85a1c15b 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-decls-are-identical.swift @@ -1,7 +1,7 @@ // 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 check that Swift recognizes that the DeclA and DeclB provide +// This test checks that Swift recognizes that the DeclA and DeclB provide // different implementations for `getFortySomething()` @_implementationOnly import DeclA diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift b/test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility-inversed.swift similarity index 69% rename from test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift rename to test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility-inversed.swift index 4b3ac791fd41e..77def9dd320ae 100644 --- a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively-inversed.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility-inversed.swift @@ -5,17 +5,12 @@ // 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 - -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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 `lookup-visible-decls-recursively.swift` +// 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. +// when looking for the `getFortyTwo()` decl. @_implementationOnly import UseModuleA import UseModuleB diff --git a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift b/test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility.swift similarity index 69% rename from test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift rename to test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility.swift index ff537bb496d72..efb75d73401d2 100644 --- a/test/Interop/Cxx/implementation-only-imports/lookup-visible-decls-recursively.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-function-transitive-visibility.swift @@ -6,16 +6,12 @@ // 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 -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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 `lookup-visible-decls-recursively-inversed.swift` +// 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. +// when looking for the `getFortyTwo()` decl. import UseModuleA @_implementationOnly import UseModuleB diff --git a/test/Interop/Cxx/implementation-only-imports/check-function-visibility-inversed.swift b/test/Interop/Cxx/implementation-only-imports/check-function-visibility-inversed.swift new file mode 100644 index 0000000000000..0f24a16bf8567 --- /dev/null +++ b/test/Interop/Cxx/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 -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-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() +} diff --git a/test/Interop/Cxx/implementation-only-imports/check-function-visibility.swift b/test/Interop/Cxx/implementation-only-imports/check-function-visibility.swift new file mode 100644 index 0000000000000..2b3d6989851d0 --- /dev/null +++ b/test/Interop/Cxx/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 -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-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() +} diff --git a/test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift index 7d387c953e84f..bec834521a146 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility-inversed.swift @@ -1,11 +1,7 @@ // RUN: %empty-directory(%t) // RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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-operator-visibility.swift` checks @@ -16,6 +12,7 @@ import UserA @_implementationOnly import UserB +// Operator `+` is a non-member function. @_inlineable public func addWrappers() { let wrapperA = MagicWrapper() @@ -23,6 +20,7 @@ public func addWrappers() { let _ = wrapperA + wrapperB } +// Operator `-` is a member function. @_inlineable public func subtractWrappers() { var wrapperA = MagicWrapper() diff --git a/test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift index ce96b07d96339..40ea146ce442e 100644 --- a/test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift +++ b/test/Interop/Cxx/implementation-only-imports/check-operator-visibility.swift @@ -1,11 +1,7 @@ // RUN: %empty-directory(%t) // RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs %s -enable-cxx-interop -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the +// 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-operator-visibility-inversed.swift` checks @@ -15,6 +11,7 @@ @_implementationOnly import UserA import UserB +// Operator `+` is a non-member function. @_inlineable public func addWrappers() { let wrapperA = MagicWrapper() @@ -22,6 +19,7 @@ public func addWrappers() { let _ = wrapperA + wrapperB } +// Operator `-` is a member function. @_inlineable public func subtractWrappers() { var wrapperA = MagicWrapper() diff --git a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift deleted file mode 100644 index 214df13928ee5..0000000000000 --- a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls-inversed.swift +++ /dev/null @@ -1,22 +0,0 @@ -// RUN: %empty-directory(%t) -// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs -enable-cxx-interop %s - -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the -// decl is not hidden behind @_implementationOnly in all modules. - -// This test, as well as -// `prefer-a-visible-decl-over-implementation-only-decls.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() -} diff --git a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift b/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift deleted file mode 100644 index 8249ef9bd671a..0000000000000 --- a/test/Interop/Cxx/implementation-only-imports/prefer-a-visible-decl-over-implementation-only-decls.swift +++ /dev/null @@ -1,22 +0,0 @@ -// RUN: %empty-directory(%t) -// RUN: %target-swift-frontend -emit-module -o %t/FortyTwo.swiftmodule -I %S/Inputs -enable-cxx-interop %s - -// If a decl comes from two modules, one of which is marked as -// @_implementationOnly, Swift may choose the @_implementationOnly source -// and then error out due to the decl being hidden. - -// Swift should consider all sources for the decl and recognize that the -// decl is not hidden behind @_implementationOnly in all modules. - -// This test, as well as -// `prefer-a-visible-decl-over-implementation-only-decls-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() -} diff --git a/test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift b/test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift index 9e59f2f804033..5655581e3df2c 100644 --- a/test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift +++ b/test/Interop/Cxx/implementation-only-imports/skip-forward-declarations.swift @@ -10,7 +10,6 @@ import UserC @_inlineable public func createAWrapper() { let _ = MagicWrapper() - let _ = MagicWrapper(42) } // CHECK: struct 'MagicWrapper' cannot be used in an '@inlinable' function because 'UserA' was imported implementation-only From 6898aae7dc9a64f468bb06901ada5098fd7fcd3c Mon Sep 17 00:00:00 2001 From: Rosica Dejanovska Date: Fri, 13 Nov 2020 18:58:06 +0100 Subject: [PATCH 12/12] Make use of ModuleDecl::isImportedImplementationOnly() when searching for visible access path between 2 modules --- lib/Sema/TypeCheckAccess.cpp | 90 ++++++++++-------------------------- 1 file changed, 25 insertions(+), 65 deletions(-) diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index 411de08043bb7..59ccf4f94b74c 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/lib/Sema/TypeCheckAccess.cpp @@ -25,13 +25,7 @@ #include "swift/AST/Import.h" #include "swift/AST/Pattern.h" #include "swift/AST/ParameterList.h" -#include "swift/AST/SourceFile.h" -#include "swift/AST/Type.h" #include "swift/AST/TypeCheckRequests.h" -#include "clang/AST/Type.h" -#include "clang/Basic/Module.h" -#include "llvm/ADT/SmallPtrSet.h" -#include "llvm/ADT/SmallVector.h" using namespace swift; @@ -1446,47 +1440,6 @@ class UsableFromInlineChecker : public AccessControlCheckerBase, } } }; - -void getVisibleModules( - llvm::SmallPtrSetImpl &visibleModules, - swift::SourceFile *SF) { - llvm::SmallPtrSet seenModules; - llvm::SmallVector stack; - - auto filter = ModuleDecl::ImportFilter( - {ModuleDecl::ImportFilterKind::Exported, - ModuleDecl::ImportFilterKind::Default, - ModuleDecl::ImportFilterKind::SPIAccessControl, - ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay}); - - SmallVector sfImportedModules; - - SF->getImportedModules(sfImportedModules, filter); - - for (auto importedModule : sfImportedModules) { - stack.push_back(importedModule.importedModule); - seenModules.insert(importedModule.importedModule); - } - - while (!stack.empty()) { - auto module = stack.pop_back_val(); - if (auto clangModule = module->findUnderlyingClangModule()) { - visibleModules.insert(clangModule); - continue; - } - - SmallVector importedModules; - module->getImportedModules(importedModules, filter); - - for (auto &importedModule : importedModules) { - auto moduleDecl = importedModule.importedModule; - if (!seenModules.contains(moduleDecl)) { - seenModules.insert(moduleDecl); - stack.push_back(moduleDecl); - } - } - } -} } // end anonymous namespace /// Returns the kind of origin, implementation-only import or SPI declaration, @@ -1499,9 +1452,7 @@ swift::getDisallowedOriginKind(const Decl *decl, DowngradeToWarning &downgradeToWarning) { downgradeToWarning = DowngradeToWarning::No; ModuleDecl *M = decl->getModuleContext(); - auto *SF = where.getDeclContext()->getParentSourceFile(); - if (SF->isImportedImplementationOnly(M)) { // Temporarily downgrade implementation-only exportability in SPI to // a warning. @@ -1510,27 +1461,36 @@ swift::getDisallowedOriginKind(const Decl *decl, // 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 look at all the visible - // modules and see if we can find the decl in a non @_implementationOnly - // module. - llvm::SmallPtrSet visibleModules; - getVisibleModules(visibleModules, SF); - + // 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 sfImportedModules; + SF->getImportedModules(sfImportedModules, filter); if (auto clangDecl = decl->getClangDecl()) { for (auto redecl : clangDecl->redecls()) { - if (!visibleModules.contains(redecl->getOwningModule())) { - continue; - } - - if (auto tagRedecl = dyn_cast(redecl)) { - // This is a forward declaration. - // We ignore visibility of these. - if (tagRedecl->getBraceRange().isInvalid()) { + if (auto tagReDecl = dyn_cast(redecl)) { + // This is a forward declaration. We ignore visibility of those. + if (tagReDecl->getBraceRange().isInvalid()) { continue; } } - - return DisallowedOriginKind::None; + 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.