Skip to content

Commit ca8792c

Browse files
xymusxymus
committed
Sema: Prefer the submodule import for access-levels diagnostics
Update logic finding the import to point in diagnostics to avoid relying on the implicit import of the top-level module when importing a clang submodule. A recent change avoids duplicating the implicit imports and broke the old logic. rdar://162151660
1 parent a956308 commit ca8792c

File tree

5 files changed

+130
-13
lines changed

5 files changed

+130
-13
lines changed

‎lib/AST/Module.cpp‎

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

2974-
/// Order of relevancy of `import` to reach `targetModule`.
2975-
/// Lower is better/more authoritative.
2974+
/// Order of relevancy of `import` to reach `targetModule` assuming the same
2975+
/// visibility level. Lower is better/more authoritative.
29762976
auto rateImport = [&](const ImportAccessLevel import) -> int {
2977-
auto importedModule = import->module.importedModule;
2977+
auto importedModule = import->module.importedModule->getTopLevelModule();
29782978

29792979
// Prioritize public names:
29802980
if (targetModule->getExportAsName() == importedModule->getBaseIdentifier())
@@ -2989,9 +2989,14 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const {
29892989
if (targetModule == importedModule->getUnderlyingModuleIfOverlay())
29902990
return 3;
29912991

2992-
// Any import in the sources.
2993-
if (import->importLoc.isValid())
2994-
return 4;
2992+
// Any import in the sources:
2993+
if (import->importLoc.isValid()) {
2994+
// Prefer clang submodules to their top level modules:
2995+
if (import->module.importedModule != importedModule)
2996+
return 4;
2997+
2998+
return 5;
2999+
}
29953000

29963001
return 10;
29973002
};
@@ -3001,11 +3006,12 @@ SourceFile::getImportAccessLevel(const ModuleDecl *targetModule) const {
30013006
auto &imports = getASTContext().getImportCache();
30023007
ImportAccessLevel restrictiveImport = std::nullopt;
30033008
for (auto &import : *Imports) {
3009+
auto importedModule = import.module.importedModule->getTopLevelModule();
30043010
if ((!restrictiveImport.has_value() ||
30053011
import.accessLevel > restrictiveImport->accessLevel ||
30063012
(import.accessLevel == restrictiveImport->accessLevel &&
30073013
rateImport(import) < rateImport(restrictiveImport))) &&
3008-
imports.isImportedBy(targetModule, import.module.importedModule)) {
3014+
imports.isImportedBy(targetModule, importedModule)) {
30093015
restrictiveImport = import;
30103016
}
30113017
}

‎lib/Sema/ResilienceDiagnostics.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,8 @@ static bool diagnoseTypeAliasDeclRefExportability(SourceLoc loc,
162162
ModuleDecl *importedVia = attributedImport.module.importedModule,
163163
*sourceModule = D->getModuleContext();
164164
ctx.Diags.diagnose(loc, diag::module_api_import_aliases, D, importedVia,
165-
sourceModule, importedVia == sourceModule);
165+
sourceModule,
166+
importedVia->getTopLevelModule() == sourceModule);
166167
});
167168

168169
auto ignoredDowngradeToWarning = DowngradeToWarning::No;
@@ -283,7 +284,8 @@ static bool diagnoseValueDeclRefExportability(SourceLoc loc, const ValueDecl *D,
283284
ModuleDecl *importedVia = attributedImport.module.importedModule,
284285
*sourceModule = D->getModuleContext();
285286
ctx.Diags.diagnose(loc, diag::module_api_import, D, importedVia,
286-
sourceModule, importedVia == sourceModule,
287+
sourceModule,
288+
importedVia->getTopLevelModule() == sourceModule,
287289
/*isImplicit*/ false);
288290
}
289291
});
@@ -426,7 +428,7 @@ TypeChecker::diagnoseConformanceExportability(SourceLoc loc,
426428
ctx.Diags.diagnose(loc, diag::module_api_import_conformance,
427429
rootConf->getType(), rootConf->getProtocol(),
428430
importedVia, sourceModule,
429-
importedVia == sourceModule);
431+
importedVia->getTopLevelModule() == sourceModule);
430432
});
431433

432434
auto originKind = getDisallowedOriginKind(ext, where);

‎lib/Sema/TypeCheckAccess.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2768,7 +2768,7 @@ void swift::recordRequiredImportAccessLevelForDecl(const ValueDecl *decl,
27682768
*sourceModule = decl->getModuleContext();
27692769
dc->getASTContext().Diags.diagnose(
27702770
diagLoc, diag::module_api_import, decl, importedVia, sourceModule,
2771-
importedVia == sourceModule, loc.isInvalid());
2771+
importedVia->getTopLevelModule() == sourceModule, loc.isInvalid());
27722772
});
27732773
}
27742774

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

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
/// The order of imports in sources shouldn't matter for access-levels.
2+
3+
// RUN: %empty-directory(%t)
4+
// RUN: split-file %s %t --leading-lines
5+
6+
/// Build the libraries.
7+
// RUN: %target-swift-frontend -emit-module %t/Lib1.swift -module-name Lib1 -o %t
8+
// RUN: %target-swift-frontend -emit-module %t/Lib2.swift -module-name Lib2 -o %t
9+
10+
/// Test main cases.
11+
// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t -package-name pkg \
12+
// RUN: -verify -verify-ignore-unrelated
13+
// RUN: %target-swift-frontend -typecheck %t/Client_Clang.swift -I %t \
14+
// RUN: -verify -verify-ignore-unrelated
15+
// RUN: %target-swift-frontend -typecheck %t/Client_Clang.swift -I %t -DINVERT \
16+
// RUN: -verify -verify-ignore-unrelated
17+
// RUN: %target-swift-frontend -typecheck %t/Client_Clang_Submodules.swift -I %t \
18+
// RUN: -verify -verify-ignore-unrelated
19+
// RUN: %target-swift-frontend -typecheck %t/Client_Clang_Submodules.swift -I %t -DINVERT \
20+
// RUN: -verify -verify-ignore-unrelated
21+
22+
// REQUIRES: VENDOR=apple
23+
24+
//--- Lib1.swift
25+
public struct Type1 {}
26+
27+
//--- Lib2.swift
28+
public struct Type2 {}
29+
30+
//--- Client.swift
31+
32+
/// Simple public vs internal.
33+
package import Lib1 // expected-note {{imported 'package' here}}
34+
// expected-note @-1 {{struct 'Type1' imported as 'package' from 'Lib1' here}}
35+
internal import Lib1 // expected-warning {{module 'Lib1' is imported as 'package' from the same file; this 'internal' access level will be ignored}}
36+
37+
/// Simple package vs internal, inverted.
38+
internal import Lib2 // expected-warning {{module 'Lib2' is imported as 'package' from the same file; this 'internal' access level will be ignored}}
39+
package import Lib2 // expected-note {{imported 'package' here}}
40+
// expected-note @-1 {{struct 'Type2' imported as 'package' from 'Lib2' here}}
41+
42+
public func dummyAPI(t1: Type1) {} // expected-error {{function cannot be declared public because its parameter uses a package type}}
43+
// expected-note @-1 {{struct 'Type1' is imported by this file as 'package' from 'Lib1'}}
44+
45+
public func dummyAPI(t2: Type2) {} // expected-error {{function cannot be declared public because its parameter uses a package type}}
46+
// expected-note @-1 {{struct 'Type2' is imported by this file as 'package' from 'Lib2'}}
47+
48+
//--- Client_Clang.swift
49+
50+
#if INVERT
51+
private import ClangLib
52+
#endif
53+
54+
internal import ClangLib2 // expected-note {{struct 'ClangType' imported as 'internal' from 'ClangLib2' here}}
55+
56+
#if !INVERT
57+
private import ClangLib
58+
#endif
59+
60+
public func dummyAPI(t2: ClangType) {}
61+
// expected-error @-1 {{function cannot be declared public because its parameter uses an internal type}}
62+
// expected-note @-2 {{struct 'ClangType' is imported by this file as 'internal' from 'ClangLib2'}}
63+
64+
//--- Client_Clang_Submodules.swift
65+
66+
#if INVERT
67+
private import ClangLib.Sub1
68+
#endif
69+
70+
internal import ClangLib.Sub2 // expected-note {{struct 'SubType' imported as 'internal' from 'Sub2' here}}
71+
72+
#if !INVERT
73+
private import ClangLib.Sub1
74+
#endif
75+
76+
public func dummyAPI(t2: SubType) {}
77+
// expected-error @-1 {{function cannot be declared public because its parameter uses an internal type}}
78+
// expected-note @-2 {{struct 'SubType' is imported by this file as 'internal' from 'Sub2'}}
79+
80+
//--- module.modulemap
81+
82+
module ClangLib {
83+
header "ClangLib1.h"
84+
85+
explicit module Sub1 {
86+
header "Sub1.h"
87+
}
88+
89+
explicit module Sub2 {
90+
header "Sub2.h"
91+
}
92+
}
93+
94+
module ClangLib2 {
95+
header "ClangLib2.h"
96+
export *
97+
}
98+
99+
//--- ClangLib1.h
100+
struct ClangType {};
101+
102+
//--- ClangLib2.h
103+
#include <ClangLib1.h>
104+
105+
//--- Sub1.h
106+
struct SubType {};
107+
108+
//--- Sub2.h
109+
#include "Sub1.h"

‎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 'ClangSubmodule'}}
341-
public func clangUser(a: ClangTopModuleType) {} // expected-remark {{struct 'ClangTopModuleType' is imported via 'ClangTopModule'}}
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'}}
342342

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

0 commit comments

Comments
 (0)