Skip to content

Commit 73904ec

Browse files
xymusxymus
committed
Sema: report inconsistent access levels on imports of one module from one file
Warn on imports of the same target module from the same source file when they have different access-levels. This situation can lead to unexpected behavior as the compiler will only take into account the most permissive import. In the following example, the module Foundation in functionally imported as public. The `internal` modifier will be ignored. The new diagnostic reports it as such. ``` public import Foundation internal import Foundation // warning: module 'Foundation' is imported as 'public' from the same file; this 'internal' access level will be ignored ``` This could be an easy mistake to do when using scoped imports where this diagnostic will also help. rdar://117855481
1 parent d976ea6 commit 73904ec

File tree

6 files changed

+209
-0
lines changed

6 files changed

+209
-0
lines changed

‎include/swift/AST/DiagnosticsSema.def‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3635,6 +3635,13 @@ NOTE(inconsistent_implicit_access_level_on_import_here,none,
36353635
FIXIT(inconsistent_implicit_access_level_on_import_fixit,
36363636
"%select{private|fileprivate|internal|package|public|%error}0 ",
36373637
(AccessLevel))
3638+
WARNING(inconsistent_import_access_levels,none,
3639+
"module %0 is imported as "
3640+
"'%select{private|fileprivate|internal|package|public|%error}1' "
3641+
"from the same file; "
3642+
"this '%select{private|fileprivate|internal|package|public|%error}2' "
3643+
"access level will be ignored",
3644+
(const ModuleDecl *, AccessLevel, AccessLevel))
36383645

36393646
ERROR(implementation_only_decl_non_override,none,
36403647
"'@_implementationOnly' can only be used on overrides", ())

‎include/swift/AST/TypeCheckRequests.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3676,6 +3676,25 @@ class CheckInconsistentSPIOnlyImportsRequest
36763676
bool isCached() const { return true; }
36773677
};
36783678

3679+
// Check that imports of the same module from the same file have the same
3680+
// access-level.
3681+
class CheckInconsistentAccessLevelOnImportSameFileRequest
3682+
: public SimpleRequest<CheckInconsistentAccessLevelOnImportSameFileRequest,
3683+
evaluator::SideEffect(SourceFile *),
3684+
RequestFlags::Cached> {
3685+
public:
3686+
using SimpleRequest::SimpleRequest;
3687+
3688+
private:
3689+
friend SimpleRequest;
3690+
3691+
evaluator::SideEffect evaluate(Evaluator &evaluator, SourceFile *mod) const;
3692+
3693+
public:
3694+
// Cached.
3695+
bool isCached() const { return true; }
3696+
};
3697+
36793698
/// Report default imports if other imports of the same target from this
36803699
/// module have an explicitly defined access level. In such a case, all imports
36813700
/// of the target module need an explicit access level or it may be made public

‎include/swift/AST/TypeCheckerTypeIDZone.def‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ SWIFT_REQUEST(TypeChecker, CheckInconsistentSPIOnlyImportsRequest,
3737
evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
3838
SWIFT_REQUEST(TypeChecker, CheckInconsistentAccessLevelOnImport,
3939
evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
40+
SWIFT_REQUEST(TypeChecker, CheckInconsistentAccessLevelOnImportSameFileRequest,
41+
evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
4042
SWIFT_REQUEST(TypeChecker, CheckInconsistentWeakLinkedImportsRequest,
4143
evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
4244
SWIFT_REQUEST(TypeChecker, CheckRedeclarationRequest,

‎lib/Sema/ImportResolution.cpp‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -970,6 +970,58 @@ CheckInconsistentSPIOnlyImportsRequest::evaluate(
970970
return {};
971971
}
972972

973+
evaluator::SideEffect
974+
CheckInconsistentAccessLevelOnImportSameFileRequest::evaluate(
975+
Evaluator &evaluator, SourceFile *SF) const {
976+
977+
// Gather the most permissive import decl for each imported module.
978+
llvm::DenseMap<ModuleDecl *, const ImportDecl *> mostPermissiveImports;
979+
for (auto *topLevelDecl : SF->getTopLevelDecls()) {
980+
auto *importDecl = dyn_cast<ImportDecl>(topLevelDecl);
981+
if (!importDecl)
982+
continue;
983+
984+
ModuleDecl *importedModule = importDecl->getModule();
985+
if (!importedModule)
986+
continue;
987+
988+
auto otherImportDecl = mostPermissiveImports.find(importedModule);
989+
if (otherImportDecl == mostPermissiveImports.end() ||
990+
otherImportDecl->second->getAccessLevel() <
991+
importDecl->getAccessLevel()) {
992+
mostPermissiveImports[importedModule] = importDecl;
993+
}
994+
}
995+
996+
// Report import decls that are not the most permissive.
997+
auto &diags = SF->getASTContext().Diags;
998+
for (auto *topLevelDecl : SF->getTopLevelDecls()) {
999+
auto *importDecl = dyn_cast<ImportDecl>(topLevelDecl);
1000+
if (!importDecl)
1001+
continue;
1002+
1003+
ModuleDecl *importedModule = importDecl->getModule();
1004+
if (!importedModule)
1005+
continue;
1006+
1007+
auto otherImportDecl = mostPermissiveImports.find(importedModule);
1008+
if (otherImportDecl != mostPermissiveImports.end() &&
1009+
otherImportDecl->second != importDecl &&
1010+
otherImportDecl->second->getAccessLevel() >
1011+
importDecl->getAccessLevel()) {
1012+
diags.diagnose(importDecl, diag::inconsistent_import_access_levels,
1013+
importedModule,
1014+
otherImportDecl->second->getAccessLevel(),
1015+
importDecl->getAccessLevel());
1016+
diags.diagnose(otherImportDecl->second,
1017+
diag::inconsistent_implicit_access_level_on_import_here,
1018+
otherImportDecl->second->getAccessLevel());
1019+
}
1020+
}
1021+
1022+
return {};
1023+
}
1024+
9731025
evaluator::SideEffect
9741026
CheckInconsistentAccessLevelOnImport::evaluate(
9751027
Evaluator &evaluator, SourceFile *SF) const {

‎lib/Sema/TypeChecker.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,23 +306,34 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
306306
diagnoseUnnecessaryPublicImports(*SF);
307307

308308
// Check to see if there are any inconsistent imports.
309+
// Whole-module @_implementationOnly imports.
309310
evaluateOrDefault(
310311
Ctx.evaluator,
311312
CheckInconsistentImplementationOnlyImportsRequest{SF->getParentModule()},
312313
{});
313314

315+
// Whole-module @_spiOnly imports.
314316
evaluateOrDefault(
315317
Ctx.evaluator,
316318
CheckInconsistentSPIOnlyImportsRequest{SF},
317319
{});
318320

321+
// Whole-module ambiguous bare imports defaulting to public, when other
322+
// imports are marked 'internal'.
319323
if (!Ctx.LangOpts.hasFeature(Feature::InternalImportsByDefault)) {
320324
evaluateOrDefault(
321325
Ctx.evaluator,
322326
CheckInconsistentAccessLevelOnImport{SF},
323327
{});
324328
}
325329

330+
// Per-file inconsistent access-levels on imports of the same module.
331+
evaluateOrDefault(
332+
Ctx.evaluator,
333+
CheckInconsistentAccessLevelOnImportSameFileRequest{SF},
334+
{});
335+
336+
// Whole-module inconsistent @_weakLinked.
326337
evaluateOrDefault(
327338
Ctx.evaluator,
328339
CheckInconsistentWeakLinkedImportsRequest{SF->getParentModule()}, {});

‎test/Sema/access-level-import-inconsistent-same-file.swift‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: split-file %s %t
3+
4+
/// Build the library.
5+
// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib1 -o %t
6+
// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib2 -o %t
7+
// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib3 -o %t
8+
// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib4 -o %t
9+
// RUN: %target-swift-frontend -emit-module %t/Lib.swift -module-name Lib5 -o %t
10+
11+
/// Test main cases.
12+
// RUN: %target-swift-frontend -typecheck -verify %t/Client.swift -I %t
13+
// RUN: %target-swift-frontend -typecheck -verify %t/Client_Scoped.swift -I %t
14+
// RUN: %target-swift-frontend -typecheck -verify %t/Client_Clang.swift -I %t
15+
16+
/// Test language mode specific variants.
17+
// RUN: %target-swift-frontend -typecheck -verify %t/Client_Swift6.swift -I %t \
18+
// RUN: -enable-upcoming-feature InternalImportsByDefault
19+
// RUN: %target-swift-frontend -typecheck -verify %t/Client_Swift5.swift -I %t \
20+
// RUN: -swift-version 5
21+
22+
//--- Lib.swift
23+
24+
public struct Type1 {}
25+
public struct Type2 {}
26+
27+
//--- Client.swift
28+
29+
/// Simple public vs internal.
30+
public import Lib1 // expected-note {{imported 'public' here}}
31+
internal import Lib1 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
32+
33+
/// Simple public vs internal, inverted.
34+
internal import Lib2 // expected-warning {{module 'Lib2' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
35+
public import Lib2 // expected-note {{imported 'public' here}}
36+
37+
/// 3 different ones.
38+
public import Lib3 // expected-note 2 {{imported 'public' here}}
39+
internal import Lib3 // expected-warning {{module 'Lib3' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
40+
private import Lib3 // expected-warning {{module 'Lib3' is imported as 'public' from the same file; this 'private' access level will be ignored}}
41+
42+
/// private vs fileprivate, we don't really need this warning but it may point to unintended stylistic inconsistencies.
43+
fileprivate import Lib4 // expected-note {{imported 'fileprivate' here}}
44+
private import Lib4 // expected-warning {{module 'Lib4' is imported as 'fileprivate' from the same file; this 'private' access level will be ignored}}
45+
46+
// Don't complain about repeated imports. As far as this diagnostic
47+
// is concerned we may see this with scoped imports.
48+
internal import Lib5
49+
internal import Lib5
50+
internal import Lib5
51+
internal import Lib5
52+
53+
public func dummyAPI(t1: Lib1.Type1, t2: Lib2.Type1, t3: Lib3.Type1) {}
54+
55+
//--- Client_Swift5.swift
56+
57+
/// Simple public vs internal, imports defaults to public.
58+
import Lib1 // expected-note {{imported 'public' here}}
59+
// expected-error @-1 {{ambiguous implicit access level for import of 'Lib1'; it is imported as 'internal' elsewhere}}
60+
internal import Lib1 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
61+
// expected-note @-1 {{imported 'internal' here}}
62+
63+
// There's no warning about "will be ignored" for a matching implicit access level.
64+
public import Lib2
65+
// expected-note @-1 {{imported 'public' here}}
66+
import Lib2
67+
// expected-error @-1 {{ambiguous implicit access level for import of 'Lib2'; it is imported as 'public' elsewhere}}
68+
69+
public func dummyAPI(t: Lib1.Type1, t2: Lib2.Type1) {}
70+
71+
//--- Client_Swift6.swift
72+
73+
/// Simple public vs internal, imports default to internal.
74+
public import Lib1 // expected-note {{imported 'public' here}}
75+
import Lib1 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
76+
77+
// There's no warning about "will be ignored" for a matching implicit access level.
78+
import Lib2
79+
internal import Lib2
80+
81+
public func dummyAPI(t: Lib1.Type1) {}
82+
83+
//--- Client_Scoped.swift
84+
85+
/// Access level on scoped imports still import the whole module.
86+
public import struct Lib1.Type1 // expected-note {{imported 'public' here}}
87+
internal import struct Lib1.Type2 // expected-warning {{module 'Lib1' is imported as 'public' from the same file; this 'internal' access level will be ignored}}
88+
89+
public func dummyAPI(t: Lib1.Type1) {}
90+
91+
//--- Client_Clang.swift
92+
93+
public import ClangLib.Sub1 // expected-note {{imported 'public' here}}
94+
// expected-warning @-1 {{public import of 'Sub1' was not used in public declarations or inlinable code}}
95+
private import ClangLib.Sub1 // expected-warning {{module 'Sub1' is imported as 'public' from the same file; this 'private' access level will be ignored}}
96+
internal import ClangLib.Sub2
97+
98+
public func dummyAPI(t1: ClangType1, t2: ClangType2) {}
99+
100+
//--- module.modulemap
101+
102+
module ClangLib {
103+
header "ClangLib1.h"
104+
105+
explicit module Sub1 {
106+
header "ClangLib2.h"
107+
}
108+
109+
explicit module Sub2 {
110+
header "ClangLib3.h"
111+
}
112+
}
113+
114+
//--- ClangLib1.h
115+
struct ClangType1 {};
116+
//--- ClangLib2.h
117+
struct ClangType2 {};
118+
//--- ClangLib3.h

0 commit comments

Comments
 (0)