From d051f18c4c98162ec6afd25c916a659caf6ca0f1 Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Wed, 12 Nov 2025 16:07:50 -0800 Subject: [PATCH] Revert "Sema: Prefer the submodule import for access-levels diagnostics" This reverts commit ca8792ca37fb3e8d8db55a6ce2032875ebb4d2db. Revert "[ImportResolution] Gracefully handle importing broken clang module" This reverts commit edb48dff8c6d345b6b66299b35b5887a8352850f. Revert "[ImportResolution] Don't deduplicate scoped imports" This reverts commit da96079eb0460f8f981f05d205dc2ec92e7e88b9. Revert "[ImportResolution] Deduplicate top-level clang modules from import list" This reverts commit 0ea5677aa193ae48db762dc47967acc1d566657f. Reverting due to namelookup errors. rdar://164588082 --- lib/AST/Module.cpp | 20 ++-- lib/Sema/ImportResolution.cpp | 38 ++---- lib/Sema/ResilienceDiagnostics.cpp | 8 +- lib/Sema/TypeCheckAccess.cpp | 2 +- ...g-includes-indirect-explicit-modules.swift | 6 + .../clang-includes-redundant-imports.swift | 17 +++ ...s-level-import-inconsistent-ordering.swift | 109 ------------------ test/Sema/superfluously-public-imports.swift | 4 +- 8 files changed, 48 insertions(+), 156 deletions(-) delete mode 100644 test/Sema/access-level-import-inconsistent-ordering.swift diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp index 2473495be2cfc..de2cfce9f2246 100644 --- a/lib/AST/Module.cpp +++ b/lib/AST/Module.cpp @@ -2990,10 +2990,10 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const { assert(targetModule != getParentModule() && "getImportAccessLevel doesn't support checking for a self-import"); - /// Order of relevancy of `import` to reach `targetModule` assuming the same - /// visibility level. Lower is better/more authoritative. + /// Order of relevancy of `import` to reach `targetModule`. + /// Lower is better/more authoritative. auto rateImport = [&](const ImportAccessLevel import) -> int { - auto importedModule = import->module.importedModule->getTopLevelModule(); + auto importedModule = import->module.importedModule; // Prioritize public names: if (targetModule->getExportAsName() == importedModule->getBaseIdentifier()) @@ -3008,14 +3008,9 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const { if (targetModule == importedModule->getUnderlyingModuleIfOverlay()) return 3; - // Any import in the sources: - if (import->importLoc.isValid()) { - // Prefer clang submodules to their top level modules: - if (import->module.importedModule != importedModule) - return 4; - - return 5; - } + // Any import in the sources. + if (import->importLoc.isValid()) + return 4; return 10; }; @@ -3025,12 +3020,11 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const { auto &imports = getASTContext().getImportCache(); ImportAccessLevel restrictiveImport = std::nullopt; for (auto &import : *Imports) { - auto importedModule = import.module.importedModule->getTopLevelModule(); if ((!restrictiveImport.has_value() || import.accessLevel > restrictiveImport->accessLevel || (import.accessLevel == restrictiveImport->accessLevel && rateImport(import) < rateImport(restrictiveImport))) && - imports.isImportedBy(targetModule, importedModule)) { + imports.isImportedBy(targetModule, import.module.importedModule)) { restrictiveImport = import; } } diff --git a/lib/Sema/ImportResolution.cpp b/lib/Sema/ImportResolution.cpp index 1669633b407cc..bc0881acaad11 100644 --- a/lib/Sema/ImportResolution.cpp +++ b/lib/Sema/ImportResolution.cpp @@ -163,11 +163,6 @@ class ImportResolver final : public DeclVisitor { /// The list of fully bound imports. SmallVector, 16> boundImports; - /// Set of imported top-level clang modules. We normally don't expect - /// duplicated imports, but importing multiple submodules of the same clang - /// TLM would cause the same TLM to be imported once per submodule. - SmallPtrSet seenClangTLMs; - /// All imported modules which should be considered when cross-importing. /// This is basically the transitive import graph, but with only top-level /// modules and without reexports from Objective-C modules. @@ -347,11 +342,6 @@ void swift::performImportResolutionForClangMacroBuffer( SF.ASTStage = SourceFile::ImportsResolved; } -static bool isSubmodule(const ModuleDecl* M) { - auto clangMod = M->findUnderlyingClangModule(); - return clangMod && clangMod->Parent; -} - //===----------------------------------------------------------------------===// // MARK: Import handling generally //===----------------------------------------------------------------------===// @@ -419,23 +409,14 @@ void ImportResolver::bindImport(UnboundImport &&I) { I.validateOptions(topLevelModule, SF); - auto alreadyImportedTLM = [ID,this](const ModuleDecl *MD) { - ASSERT(!isSubmodule(MD)); - // Scoped imports don't import all symbols from the module, so a scoped - // import does not count the module as imported - if (ID && isScopedImportKind(ID.get()->getImportKind())) - return false; - return !seenClangTLMs.insert(MD).second; - }; - if (!M->isNonSwiftModule() || topLevelModule != M || !alreadyImportedTLM(M)) { + if (topLevelModule && topLevelModule != M) { + // If we have distinct submodule and top-level module, add both. + addImport(I, M); + addImport(I, topLevelModule.get()); + } + else { + // Add only the import itself. addImport(I, M); - if (topLevelModule && topLevelModule != M && - !alreadyImportedTLM(topLevelModule.get())) { - // If we have distinct submodule and top-level module, add both. - // Importing the submodule ensures that it gets loaded, but the decls - // are imported to the TLM, so import that for visibility. - addImport(I, topLevelModule.get()); - } } crossImport(M, I); @@ -1655,6 +1636,11 @@ void ImportResolver::findCrossImports( } } +static bool isSubmodule(ModuleDecl* M) { + auto clangMod = M->findUnderlyingClangModule(); + return clangMod && clangMod->Parent; +} + void ImportResolver::addCrossImportableModules( AttributedImport importDesc) { // FIXME: namelookup::getAllImports() doesn't quite do what we need (mainly diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/lib/Sema/ResilienceDiagnostics.cpp index b7bf60be33bb8..cc927feef9a3e 100644 --- a/lib/Sema/ResilienceDiagnostics.cpp +++ b/lib/Sema/ResilienceDiagnostics.cpp @@ -167,8 +167,7 @@ static bool diagnoseTypeAliasDeclRefExportability(SourceLoc loc, ModuleDecl *importedVia = attributedImport.module.importedModule, *sourceModule = D->getModuleContext(); ctx.Diags.diagnose(loc, diag::module_api_import_aliases, D, importedVia, - sourceModule, - importedVia->getTopLevelModule() == sourceModule); + sourceModule, importedVia == sourceModule); }); auto ignoredDowngradeToWarning = DowngradeToWarning::No; @@ -290,8 +289,7 @@ static bool diagnoseValueDeclRefExportability(SourceLoc loc, const ValueDecl *D, ModuleDecl *importedVia = attributedImport.module.importedModule, *sourceModule = D->getModuleContext(); ctx.Diags.diagnose(loc, diag::module_api_import, D, importedVia, - sourceModule, - importedVia->getTopLevelModule() == sourceModule, + sourceModule, importedVia == sourceModule, /*isImplicit*/ false); } }); @@ -439,7 +437,7 @@ TypeChecker::diagnoseConformanceExportability(SourceLoc loc, ctx.Diags.diagnose(loc, diag::module_api_import_conformance, rootConf->getType(), rootConf->getProtocol(), importedVia, sourceModule, - importedVia->getTopLevelModule() == sourceModule); + importedVia == sourceModule); }); auto originKind = getDisallowedOriginKind(ext, where); diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp index 2250437ae0221..2215e225ecd5a 100644 --- a/lib/Sema/TypeCheckAccess.cpp +++ b/lib/Sema/TypeCheckAccess.cpp @@ -2778,7 +2778,7 @@ void swift::recordRequiredImportAccessLevelForDecl(const ValueDecl *decl, *sourceModule = decl->getModuleContext(); dc->getASTContext().Diags.diagnose( diagLoc, diag::module_api_import, decl, importedVia, sourceModule, - importedVia->getTopLevelModule() == sourceModule, loc.isInvalid()); + importedVia == sourceModule, loc.isInvalid()); }); } diff --git a/test/Interop/C/swiftify-import/clang-includes-indirect-explicit-modules.swift b/test/Interop/C/swiftify-import/clang-includes-indirect-explicit-modules.swift index 35a934ec4d2a8..bbf214a7b15c7 100644 --- a/test/Interop/C/swiftify-import/clang-includes-indirect-explicit-modules.swift +++ b/test/Interop/C/swiftify-import/clang-includes-indirect-explicit-modules.swift @@ -153,7 +153,9 @@ d1_t b2d(void * _Nonnull __sized_by(size), int size); // DUMP-NEXT: D1 // DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 // DUMP-NEXT: B2 // DUMP-CXX-NEXT: CxxShim // DUMP-CXX-NEXT: Cxx @@ -168,7 +170,9 @@ d1_t b2d(void * _Nonnull __sized_by(size), int size); // DUMP-NEXT: D1 // DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 // DUMP-NEXT: B2 // DUMP-CXX-NEXT: CxxShim // DUMP-CXX-NEXT: Cxx @@ -183,7 +187,9 @@ d1_t b2d(void * _Nonnull __sized_by(size), int size); // DUMP-NEXT: D1 // DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 // DUMP-NEXT: B2 // DUMP-CXX-NEXT: CxxShim // DUMP-CXX-NEXT: Cxx diff --git a/test/Interop/C/swiftify-import/clang-includes-redundant-imports.swift b/test/Interop/C/swiftify-import/clang-includes-redundant-imports.swift index 0ea13db6ee4c4..e55f638b64411 100644 --- a/test/Interop/C/swiftify-import/clang-includes-redundant-imports.swift +++ b/test/Interop/C/swiftify-import/clang-includes-redundant-imports.swift @@ -38,12 +38,17 @@ import A2.B2.C2 // DUMP-NEXT: SwiftOnoneSupport // DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: D1 +// DUMP-NEXT: A1 // DUMP-NEXT: E1 +// DUMP-NEXT: A1 // DUMP-NEXT: B2 // DUMP-NEXT: A2 // DUMP-NEXT: C2 +// DUMP-NEXT: A2 public func callUnsafe(_ p: UnsafeMutableRawPointer) { let _ = a1(p, 13) @@ -290,7 +295,10 @@ c2_t b2_2(void * _Nonnull __sized_by(size), int size); // DUMP-NEXT: D1 // DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 +// DUMP-NEXT: A1 // DUMP-CXX-NEXT: CxxShim // DUMP-CXX-NEXT: Cxx // DUMP-NEXT: _StringProcessing @@ -305,7 +313,10 @@ c2_t b2_2(void * _Nonnull __sized_by(size), int size); // DUMP-NEXT: D1 // DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 +// DUMP-NEXT: A1 // DUMP-CXX-NEXT: CxxShim // DUMP-CXX-NEXT: Cxx // DUMP-NEXT: _StringProcessing @@ -333,7 +344,10 @@ e2_t c2(void * _Nonnull __sized_by(size), int size); // DUMP-NEXT: D1 // DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 +// DUMP-NEXT: A1 // DUMP-CXX-NEXT: CxxShim // DUMP-CXX-NEXT: Cxx // DUMP-NEXT: _StringProcessing @@ -357,7 +371,10 @@ e2_t d2(void * _Nonnull __sized_by(size), int size); // DUMP-NEXT: D1 // DUMP-NEXT: A1 // DUMP-NEXT: C1 +// DUMP-NEXT: A1 // DUMP-NEXT: B1 +// DUMP-NEXT: A1 +// DUMP-NEXT: A1 // DUMP-CXX-NEXT: CxxShim // DUMP-CXX-NEXT: Cxx // DUMP-NEXT: _StringProcessing diff --git a/test/Sema/access-level-import-inconsistent-ordering.swift b/test/Sema/access-level-import-inconsistent-ordering.swift deleted file mode 100644 index 9d57e916951fd..0000000000000 --- a/test/Sema/access-level-import-inconsistent-ordering.swift +++ /dev/null @@ -1,109 +0,0 @@ -/// The order of imports in sources shouldn't matter for access-levels. - -// RUN: %empty-directory(%t) -// RUN: split-file %s %t --leading-lines - -/// Build the libraries. -// RUN: %target-swift-frontend -emit-module %t/Lib1.swift -module-name Lib1 -o %t -// RUN: %target-swift-frontend -emit-module %t/Lib2.swift -module-name Lib2 -o %t - -/// Test main cases. -// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t -package-name pkg \ -// RUN: -verify -verify-ignore-unrelated -// RUN: %target-swift-frontend -typecheck %t/Client_Clang.swift -I %t \ -// RUN: -verify -verify-ignore-unrelated -// RUN: %target-swift-frontend -typecheck %t/Client_Clang.swift -I %t -DINVERT \ -// RUN: -verify -verify-ignore-unrelated -// RUN: %target-swift-frontend -typecheck %t/Client_Clang_Submodules.swift -I %t \ -// RUN: -verify -verify-ignore-unrelated -// RUN: %target-swift-frontend -typecheck %t/Client_Clang_Submodules.swift -I %t -DINVERT \ -// RUN: -verify -verify-ignore-unrelated - -// REQUIRES: VENDOR=apple - -//--- Lib1.swift -public struct Type1 {} - -//--- Lib2.swift -public struct Type2 {} - -//--- Client.swift - -/// Simple public vs internal. -package import Lib1 // expected-note {{imported 'package' here}} -// expected-note @-1 {{struct 'Type1' imported as 'package' from 'Lib1' here}} -internal import Lib1 // expected-warning {{module 'Lib1' is imported as 'package' from the same file; this 'internal' access level will be ignored}} - -/// Simple package vs internal, inverted. -internal import Lib2 // expected-warning {{module 'Lib2' is imported as 'package' from the same file; this 'internal' access level will be ignored}} -package import Lib2 // expected-note {{imported 'package' here}} -// expected-note @-1 {{struct 'Type2' imported as 'package' from 'Lib2' here}} - -public func dummyAPI(t1: Type1) {} // expected-error {{function cannot be declared public because its parameter uses a package type}} -// expected-note @-1 {{struct 'Type1' is imported by this file as 'package' from 'Lib1'}} - -public func dummyAPI(t2: Type2) {} // expected-error {{function cannot be declared public because its parameter uses a package type}} -// expected-note @-1 {{struct 'Type2' is imported by this file as 'package' from 'Lib2'}} - -//--- Client_Clang.swift - -#if INVERT -private import ClangLib -#endif - -internal import ClangLib2 // expected-note {{struct 'ClangType' imported as 'internal' from 'ClangLib2' here}} - -#if !INVERT -private import ClangLib -#endif - -public func dummyAPI(t2: ClangType) {} -// expected-error @-1 {{function cannot be declared public because its parameter uses an internal type}} -// expected-note @-2 {{struct 'ClangType' is imported by this file as 'internal' from 'ClangLib2'}} - -//--- Client_Clang_Submodules.swift - -#if INVERT -private import ClangLib.Sub1 -#endif - -internal import ClangLib.Sub2 // expected-note {{struct 'SubType' imported as 'internal' from 'Sub2' here}} - -#if !INVERT -private import ClangLib.Sub1 -#endif - -public func dummyAPI(t2: SubType) {} -// expected-error @-1 {{function cannot be declared public because its parameter uses an internal type}} -// expected-note @-2 {{struct 'SubType' is imported by this file as 'internal' from 'Sub2'}} - -//--- module.modulemap - -module ClangLib { - header "ClangLib1.h" - - explicit module Sub1 { - header "Sub1.h" - } - - explicit module Sub2 { - header "Sub2.h" - } -} - -module ClangLib2 { - header "ClangLib2.h" - export * -} - -//--- ClangLib1.h -struct ClangType {}; - -//--- ClangLib2.h -#include - -//--- Sub1.h -struct SubType {}; - -//--- Sub2.h -#include "Sub1.h" diff --git a/test/Sema/superfluously-public-imports.swift b/test/Sema/superfluously-public-imports.swift index 69a20de9b0db1..45731eca4f602 100644 --- a/test/Sema/superfluously-public-imports.swift +++ b/test/Sema/superfluously-public-imports.swift @@ -337,8 +337,8 @@ public import ClangSubmoduleUnused.ClangSubmoduleUnsuedSubmodule // expected-war public import ClangTopModule.ClangTopModuleSubmodule public func clangUser(a: ClangSimpleType) {} // expected-remark {{struct 'ClangSimpleType' is imported via 'ClangSimple'}} -public func clangUser(a: ClangSubmoduleSubmoduleType) {} // expected-remark {{struct 'ClangSubmoduleSubmoduleType' is imported via 'ClangSubmoduleSubmodule'}} -public func clangUser(a: ClangTopModuleType) {} // expected-remark {{struct 'ClangTopModuleType' is imported via 'ClangTopModuleSubmodule'}} +public func clangUser(a: ClangSubmoduleSubmoduleType) {} // expected-remark {{struct 'ClangSubmoduleSubmoduleType' is imported via 'ClangSubmodule'}} +public func clangUser(a: ClangTopModuleType) {} // expected-remark {{struct 'ClangTopModuleType' is imported via 'ClangTopModule'}} //--- ClientOfClangReexportedSubmodules.swift public import ClangReexportedSubmodulePublic.ClangReexportedSubmodulePublicSub