Skip to content

Commit 2211824

Browse files
hnrklssnhnrklssn
authored
Merge pull request #85468 from hnrklssn/revert-dedup-clang-module-imports
Revert "Sema: Prefer the submodule import for access-levels diagnostics"
2 parents cf535d8 + ba7b9b0 commit 2211824

9 files changed

+79
-156
lines changed

‎lib/AST/Module.cpp‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2990,10 +2990,10 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const {
29902990
assert(targetModule != getParentModule() &&
29912991
"getImportAccessLevel doesn't support checking for a self-import");
29922992

2993-
/// Order of relevancy of `import` to reach `targetModule` assuming the same
2994-
/// visibility level. Lower is better/more authoritative.
2993+
/// Order of relevancy of `import` to reach `targetModule`.
2994+
/// Lower is better/more authoritative.
29952995
auto rateImport = [&](const ImportAccessLevel import) -> int {
2996-
auto importedModule = import->module.importedModule->getTopLevelModule();
2996+
auto importedModule = import->module.importedModule;
29972997

29982998
// Prioritize public names:
29992999
if (targetModule->getExportAsName() == importedModule->getBaseIdentifier())
@@ -3008,14 +3008,9 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const {
30083008
if (targetModule == importedModule->getUnderlyingModuleIfOverlay())
30093009
return 3;
30103010

3011-
// Any import in the sources:
3012-
if (import->importLoc.isValid()) {
3013-
// Prefer clang submodules to their top level modules:
3014-
if (import->module.importedModule != importedModule)
3015-
return 4;
3016-
3017-
return 5;
3018-
}
3011+
// Any import in the sources.
3012+
if (import->importLoc.isValid())
3013+
return 4;
30193014

30203015
return 10;
30213016
};
@@ -3025,12 +3020,11 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const {
30253020
auto &imports = getASTContext().getImportCache();
30263021
ImportAccessLevel restrictiveImport = std::nullopt;
30273022
for (auto &import : *Imports) {
3028-
auto importedModule = import.module.importedModule->getTopLevelModule();
30293023
if ((!restrictiveImport.has_value() ||
30303024
import.accessLevel > restrictiveImport->accessLevel ||
30313025
(import.accessLevel == restrictiveImport->accessLevel &&
30323026
rateImport(import) < rateImport(restrictiveImport))) &&
3033-
imports.isImportedBy(targetModule, importedModule)) {
3027+
imports.isImportedBy(targetModule, import.module.importedModule)) {
30343028
restrictiveImport = import;
30353029
}
30363030
}

‎lib/Sema/ImportResolution.cpp‎

Lines changed: 12 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -163,11 +163,6 @@ class ImportResolver final : public DeclVisitor<ImportResolver> {
163163
/// The list of fully bound imports.
164164
SmallVector<AttributedImport<ImportedModule>, 16> boundImports;
165165

166-
/// Set of imported top-level clang modules. We normally don't expect
167-
/// duplicated imports, but importing multiple submodules of the same clang
168-
/// TLM would cause the same TLM to be imported once per submodule.
169-
SmallPtrSet<const ModuleDecl*, 16> seenClangTLMs;
170-
171166
/// All imported modules which should be considered when cross-importing.
172167
/// This is basically the transitive import graph, but with only top-level
173168
/// modules and without reexports from Objective-C modules.
@@ -347,11 +342,6 @@ void swift::performImportResolutionForClangMacroBuffer(
347342
SF.ASTStage = SourceFile::ImportsResolved;
348343
}
349344

350-
static bool isSubmodule(const ModuleDecl* M) {
351-
auto clangMod = M->findUnderlyingClangModule();
352-
return clangMod && clangMod->Parent;
353-
}
354-
355345
//===----------------------------------------------------------------------===//
356346
// MARK: Import handling generally
357347
//===----------------------------------------------------------------------===//
@@ -419,23 +409,14 @@ void ImportResolver::bindImport(UnboundImport &&I) {
419409

420410
I.validateOptions(topLevelModule, SF);
421411

422-
auto alreadyImportedTLM = [ID,this](const ModuleDecl *MD) {
423-
ASSERT(!isSubmodule(MD));
424-
// Scoped imports don't import all symbols from the module, so a scoped
425-
// import does not count the module as imported
426-
if (ID && isScopedImportKind(ID.get()->getImportKind()))
427-
return false;
428-
return !seenClangTLMs.insert(MD).second;
429-
};
430-
if (!M->isNonSwiftModule() || topLevelModule != M || !alreadyImportedTLM(M)) {
412+
if (topLevelModule && topLevelModule != M) {
413+
// If we have distinct submodule and top-level module, add both.
414+
addImport(I, M);
415+
addImport(I, topLevelModule.get());
416+
}
417+
else {
418+
// Add only the import itself.
431419
addImport(I, M);
432-
if (topLevelModule && topLevelModule != M &&
433-
!alreadyImportedTLM(topLevelModule.get())) {
434-
// If we have distinct submodule and top-level module, add both.
435-
// Importing the submodule ensures that it gets loaded, but the decls
436-
// are imported to the TLM, so import that for visibility.
437-
addImport(I, topLevelModule.get());
438-
}
439420
}
440421

441422
crossImport(M, I);
@@ -1655,6 +1636,11 @@ void ImportResolver::findCrossImports(
16551636
}
16561637
}
16571638

1639+
static bool isSubmodule(ModuleDecl* M) {
1640+
auto clangMod = M->findUnderlyingClangModule();
1641+
return clangMod && clangMod->Parent;
1642+
}
1643+
16581644
void ImportResolver::addCrossImportableModules(
16591645
AttributedImport<ImportedModule> importDesc) {
16601646
// FIXME: namelookup::getAllImports() doesn't quite do what we need (mainly

‎lib/Sema/ResilienceDiagnostics.cpp‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -167,8 +167,7 @@ static bool diagnoseTypeAliasDeclRefExportability(SourceLoc loc,
167167
ModuleDecl *importedVia = attributedImport.module.importedModule,
168168
*sourceModule = D->getModuleContext();
169169
ctx.Diags.diagnose(loc, diag::module_api_import_aliases, D, importedVia,
170-
sourceModule,
171-
importedVia->getTopLevelModule() == sourceModule);
170+
sourceModule, importedVia == sourceModule);
172171
});
173172

174173
auto ignoredDowngradeToWarning = DowngradeToWarning::No;
@@ -290,8 +289,7 @@ static bool diagnoseValueDeclRefExportability(SourceLoc loc, const ValueDecl *D,
290289
ModuleDecl *importedVia = attributedImport.module.importedModule,
291290
*sourceModule = D->getModuleContext();
292291
ctx.Diags.diagnose(loc, diag::module_api_import, D, importedVia,
293-
sourceModule,
294-
importedVia->getTopLevelModule() == sourceModule,
292+
sourceModule, importedVia == sourceModule,
295293
/*isImplicit*/ false);
296294
}
297295
});
@@ -439,7 +437,7 @@ TypeChecker::diagnoseConformanceExportability(SourceLoc loc,
439437
ctx.Diags.diagnose(loc, diag::module_api_import_conformance,
440438
rootConf->getType(), rootConf->getProtocol(),
441439
importedVia, sourceModule,
442-
importedVia->getTopLevelModule() == sourceModule);
440+
importedVia == sourceModule);
443441
});
444442

445443
auto originKind = getDisallowedOriginKind(ext, where);

‎lib/Sema/TypeCheckAccess.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2777,7 +2777,7 @@ void swift::recordRequiredImportAccessLevelForDecl(const ValueDecl *decl,
27772777
*sourceModule = decl->getModuleContext();
27782778
dc->getASTContext().Diags.diagnose(
27792779
diagLoc, diag::module_api_import, decl, importedVia, sourceModule,
2780-
importedVia->getTopLevelModule() == sourceModule, loc.isInvalid());
2780+
importedVia == sourceModule, loc.isInvalid());
27812781
});
27822782
}
27832783

‎test/Interop/C/swiftify-import/clang-includes-indirect-explicit-modules.swift‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,9 @@ d1_t b2d(void * _Nonnull __sized_by(size), int size);
153153
// DUMP-NEXT: D1
154154
// DUMP-NEXT: A1
155155
// DUMP-NEXT: C1
156+
// DUMP-NEXT: A1
156157
// DUMP-NEXT: B1
158+
// DUMP-NEXT: A1
157159
// DUMP-NEXT: B2
158160
// DUMP-CXX-NEXT: CxxShim
159161
// DUMP-CXX-NEXT: Cxx
@@ -168,7 +170,9 @@ d1_t b2d(void * _Nonnull __sized_by(size), int size);
168170
// DUMP-NEXT: D1
169171
// DUMP-NEXT: A1
170172
// DUMP-NEXT: C1
173+
// DUMP-NEXT: A1
171174
// DUMP-NEXT: B1
175+
// DUMP-NEXT: A1
172176
// DUMP-NEXT: B2
173177
// DUMP-CXX-NEXT: CxxShim
174178
// DUMP-CXX-NEXT: Cxx
@@ -183,7 +187,9 @@ d1_t b2d(void * _Nonnull __sized_by(size), int size);
183187
// DUMP-NEXT: D1
184188
// DUMP-NEXT: A1
185189
// DUMP-NEXT: C1
190+
// DUMP-NEXT: A1
186191
// DUMP-NEXT: B1
192+
// DUMP-NEXT: A1
187193
// DUMP-NEXT: B2
188194
// DUMP-CXX-NEXT: CxxShim
189195
// DUMP-CXX-NEXT: Cxx

‎test/Interop/C/swiftify-import/clang-includes-redundant-imports.swift‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,17 @@ import A2.B2.C2
3838
// DUMP-NEXT: SwiftOnoneSupport
3939
// DUMP-NEXT: A1
4040
// DUMP-NEXT: B1
41+
// DUMP-NEXT: A1
4142
// DUMP-NEXT: C1
43+
// DUMP-NEXT: A1
4244
// DUMP-NEXT: D1
45+
// DUMP-NEXT: A1
4346
// DUMP-NEXT: E1
47+
// DUMP-NEXT: A1
4448
// DUMP-NEXT: B2
4549
// DUMP-NEXT: A2
4650
// DUMP-NEXT: C2
51+
// DUMP-NEXT: A2
4752

4853
public func callUnsafe(_ p: UnsafeMutableRawPointer) {
4954
let _ = a1(p, 13)
@@ -290,7 +295,10 @@ c2_t b2_2(void * _Nonnull __sized_by(size), int size);
290295
// DUMP-NEXT: D1
291296
// DUMP-NEXT: A1
292297
// DUMP-NEXT: C1
298+
// DUMP-NEXT: A1
293299
// DUMP-NEXT: B1
300+
// DUMP-NEXT: A1
301+
// DUMP-NEXT: A1
294302
// DUMP-CXX-NEXT: CxxShim
295303
// DUMP-CXX-NEXT: Cxx
296304
// DUMP-NEXT: _StringProcessing
@@ -305,7 +313,10 @@ c2_t b2_2(void * _Nonnull __sized_by(size), int size);
305313
// DUMP-NEXT: D1
306314
// DUMP-NEXT: A1
307315
// DUMP-NEXT: C1
316+
// DUMP-NEXT: A1
308317
// DUMP-NEXT: B1
318+
// DUMP-NEXT: A1
319+
// DUMP-NEXT: A1
309320
// DUMP-CXX-NEXT: CxxShim
310321
// DUMP-CXX-NEXT: Cxx
311322
// DUMP-NEXT: _StringProcessing
@@ -333,7 +344,10 @@ e2_t c2(void * _Nonnull __sized_by(size), int size);
333344
// DUMP-NEXT: D1
334345
// DUMP-NEXT: A1
335346
// DUMP-NEXT: C1
347+
// DUMP-NEXT: A1
336348
// DUMP-NEXT: B1
349+
// DUMP-NEXT: A1
350+
// DUMP-NEXT: A1
337351
// DUMP-CXX-NEXT: CxxShim
338352
// DUMP-CXX-NEXT: Cxx
339353
// DUMP-NEXT: _StringProcessing
@@ -357,7 +371,10 @@ e2_t d2(void * _Nonnull __sized_by(size), int size);
357371
// DUMP-NEXT: D1
358372
// DUMP-NEXT: A1
359373
// DUMP-NEXT: C1
374+
// DUMP-NEXT: A1
360375
// DUMP-NEXT: B1
376+
// DUMP-NEXT: A1
377+
// DUMP-NEXT: A1
361378
// DUMP-CXX-NEXT: CxxShim
362379
// DUMP-CXX-NEXT: Cxx
363380
// DUMP-NEXT: _StringProcessing

‎test/Sema/access-level-import-both-public-and-private.swift‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Regression test for rdar://164588082
2+
3+
// RUN: %empty-directory(%t)
4+
// RUN: split-file %s %t
5+
6+
// RUN: %target-swift-frontend -verify -verify-additional-file %t%{fs-sep}bar.h -typecheck %t/test.swift -I %t
7+
8+
//--- test.swift
9+
private import Foo.Bar
10+
public import Foo
11+
12+
public func test(x: Baz) {}
13+
14+
//--- foo.h
15+
#pragma once
16+
#include "bar.h"
17+
18+
//--- bar.h
19+
#pragma once
20+
struct Baz {};
21+
22+
//--- module.modulemap
23+
module Foo {
24+
header "foo.h"
25+
export *
26+
27+
explicit module Bar {
28+
header "bar.h"
29+
export *
30+
}
31+
}

‎test/Sema/access-level-import-inconsistent-ordering.swift‎

Lines changed: 0 additions & 109 deletions
This file was deleted.

‎test/Sema/superfluously-public-imports.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,8 +337,8 @@ public import ClangSubmoduleUnused.ClangSubmoduleUnsuedSubmodule // expected-war
337337
public import ClangTopModule.ClangTopModuleSubmodule
338338

339339
public func clangUser(a: ClangSimpleType) {} // expected-remark {{struct 'ClangSimpleType' is imported via 'ClangSimple'}}
340-
public func clangUser(a: ClangSubmoduleSubmoduleType) {} // expected-remark {{struct 'ClangSubmoduleSubmoduleType' is imported via 'ClangSubmoduleSubmodule'}}
341-
public func clangUser(a: ClangTopModuleType) {} // expected-remark {{struct 'ClangTopModuleType' is imported via 'ClangTopModuleSubmodule'}}
340+
public func clangUser(a: ClangSubmoduleSubmoduleType) {} // expected-remark {{struct 'ClangSubmoduleSubmoduleType' is imported via 'ClangSubmodule'}}
341+
public func clangUser(a: ClangTopModuleType) {} // expected-remark {{struct 'ClangTopModuleType' is imported via 'ClangTopModule'}}
342342

343343
//--- ClientOfClangReexportedSubmodules.swift
344344
public import ClangReexportedSubmodulePublic.ClangReexportedSubmodulePublicSub

0 commit comments

Comments
 (0)