Skip to content

Commit 16baee3

Browse files
xymusxymus
authored
Merge pull request #63948 from xymus/access-level-import-context
[Sema] Context validation checks for access-level on imports
2 parents 239ec62 + 2ddb859 commit 16baee3

11 files changed

+320
-11
lines changed

‎include/swift/AST/DiagnosticsSema.def‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,10 +1011,10 @@ ERROR(import_restriction_conflict,none,
10111011
"%select{implementation-only|SPI only|exported}2 ",
10121012
(Identifier, unsigned, unsigned))
10131013

1014-
WARNING(module_not_compiled_with_library_evolution,none,
1015-
"module %0 was not compiled with library evolution support; "
1016-
"using it means binary compatibility for %1 can't be guaranteed",
1017-
(Identifier, Identifier))
1014+
ERROR(module_not_compiled_with_library_evolution,none,
1015+
"module %0 was not compiled with library evolution support; "
1016+
"using it means binary compatibility for %1 can't be guaranteed",
1017+
(Identifier, Identifier))
10181018

10191019
ERROR(module_allowable_client_violation,none,
10201020
"module %0 doesn't allow importation from module %1",
@@ -2061,6 +2061,9 @@ ERROR(access_level_on_import_unsupported, none,
20612061
"only 'public', 'package', 'internal', 'fileprivate' and 'private' "
20622062
"are unsupported",
20632063
(DeclAttribute))
2064+
ERROR(access_level_conflict_with_exported,none,
2065+
"'%0' is incompatible with %1; it can only be applied to public imports",
2066+
(DeclAttribute, DeclAttribute))
20642067

20652068
// Opaque return types
20662069
ERROR(opaque_type_invalid_constraint,none,
@@ -3164,6 +3167,18 @@ ERROR(error_public_import_of_private_module,none,
31643167
"private module %0 is imported publicly from the public module %1",
31653168
(Identifier, Identifier))
31663169

3170+
ERROR(inconsistent_implicit_access_level_on_import,none,
3171+
"ambiguous implicit access level for import of %0; it is imported as "
3172+
"'%select{private|fileprivate|internal|package|public|%error}1' elsewhere",
3173+
(Identifier, AccessLevel))
3174+
NOTE(inconsistent_implicit_access_level_on_import_here,none,
3175+
"imported "
3176+
"'%select{private|fileprivate|internal|package|public|%error}0' here",
3177+
(AccessLevel))
3178+
FIXIT(inconsistent_implicit_access_level_on_import_fixit,
3179+
"%select{private|fileprivate|internal|package|public|%error}0 ",
3180+
(AccessLevel))
3181+
31673182
ERROR(implementation_only_decl_non_override,none,
31683183
"'@_implementationOnly' can only be used on overrides", ())
31693184
ERROR(implementation_only_override_changed_type,none,

‎include/swift/AST/TypeCheckRequests.h‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3367,6 +3367,27 @@ class CheckInconsistentSPIOnlyImportsRequest
33673367
bool isCached() const { return true; }
33683368
};
33693369

3370+
/// Report default imports if other imports of the same target from this
3371+
/// module have an explicitly defined access level. In such a case, all imports
3372+
/// of the target module need an explicit access level or it may be made public
3373+
/// by error. This applies only to pre-Swift 6 mode.
3374+
class CheckInconsistentAccessLevelOnImport
3375+
: public SimpleRequest<CheckInconsistentAccessLevelOnImport,
3376+
evaluator::SideEffect(SourceFile *),
3377+
RequestFlags::Cached> {
3378+
public:
3379+
using SimpleRequest::SimpleRequest;
3380+
3381+
private:
3382+
friend SimpleRequest;
3383+
3384+
evaluator::SideEffect evaluate(Evaluator &evaluator, SourceFile *mod) const;
3385+
3386+
public:
3387+
// Cached.
3388+
bool isCached() const { return true; }
3389+
};
3390+
33703391
/// Checks to see if any of the imports in a module use \c @_weakLinked
33713392
/// in one file and not in another.
33723393
///

‎include/swift/AST/TypeCheckerTypeIDZone.def‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ SWIFT_REQUEST(TypeChecker, CheckInconsistentImplementationOnlyImportsRequest,
3535
evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
3636
SWIFT_REQUEST(TypeChecker, CheckInconsistentSPIOnlyImportsRequest,
3737
evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
38+
SWIFT_REQUEST(TypeChecker, CheckInconsistentAccessLevelOnImport,
39+
evaluator::SideEffect(SourceFile *), Cached, NoLocationInfo)
3840
SWIFT_REQUEST(TypeChecker, CheckInconsistentWeakLinkedImportsRequest,
3941
evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
4042
SWIFT_REQUEST(TypeChecker, CheckRedeclarationRequest,

‎lib/Sema/ImportResolution.cpp‎

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -737,7 +737,8 @@ void UnboundImport::validateAllowableClient(ModuleDecl *importee,
737737

738738
void UnboundImport::validateResilience(NullablePtr<ModuleDecl> topLevelModule,
739739
SourceFile &SF) {
740-
if (import.options.contains(ImportFlags::ImplementationOnly))
740+
if (import.options.contains(ImportFlags::ImplementationOnly) ||
741+
import.accessLevel < AccessLevel::Public)
741742
return;
742743

743744
// Per getTopLevelModule(), we'll only get nullptr here for non-Swift modules,
@@ -760,12 +761,20 @@ void UnboundImport::validateResilience(NullablePtr<ModuleDecl> topLevelModule,
760761
topLevelModule.get()->isResilient())
761762
return;
762763

763-
ctx.Diags.diagnose(import.module.getModulePath().front().Loc,
764-
diag::module_not_compiled_with_library_evolution,
765-
topLevelModule.get()->getName(),
766-
SF.getParentModule()->getName());
767-
// FIXME: Once @_implementationOnly is a real feature, we should have a fix-it
768-
// to add it.
764+
auto inFlight = ctx.Diags.diagnose(import.module.getModulePath().front().Loc,
765+
diag::module_not_compiled_with_library_evolution,
766+
topLevelModule.get()->getName(),
767+
SF.getParentModule()->getName());
768+
769+
if (ctx.LangOpts.hasFeature(Feature::AccessLevelOnImport)) {
770+
SourceLoc attrLoc = import.accessLevelLoc;
771+
if (attrLoc.isValid())
772+
inFlight.fixItReplace(attrLoc, "internal");
773+
else
774+
inFlight.fixItInsert(import.importLoc, "internal ");
775+
} else {
776+
inFlight.limitBehavior(DiagnosticBehavior::Warning);
777+
}
769778
}
770779

771780
void UnboundImport::diagnoseInvalidAttr(DeclAttrKind attrKind,
@@ -917,6 +926,41 @@ CheckInconsistentSPIOnlyImportsRequest::evaluate(
917926
return {};
918927
}
919928

929+
evaluator::SideEffect
930+
CheckInconsistentAccessLevelOnImport::evaluate(
931+
Evaluator &evaluator, SourceFile *SF) const {
932+
933+
auto mod = SF->getParentModule();
934+
auto diagnose = [mod](const ImportDecl *implicitImport,
935+
const ImportDecl *otherImport) {
936+
auto otherAccessLevel = otherImport->getAccessLevel();
937+
938+
auto &diags = mod->getDiags();
939+
{
940+
InFlightDiagnostic error =
941+
diags.diagnose(implicitImport, diag::inconsistent_implicit_access_level_on_import,
942+
implicitImport->getModule()->getName(), otherAccessLevel);
943+
error.fixItInsert(implicitImport->getStartLoc(),
944+
diag::inconsistent_implicit_access_level_on_import_fixit,
945+
otherAccessLevel);
946+
}
947+
948+
SourceLoc accessLevelLoc = otherImport->getStartLoc();
949+
if (auto attr = otherImport->getAttrs().getAttribute<AccessControlAttr>())
950+
accessLevelLoc = attr->getLocation();
951+
diags.diagnose(accessLevelLoc,
952+
diag::inconsistent_implicit_access_level_on_import_here,
953+
otherAccessLevel);
954+
};
955+
956+
auto predicate = [](ImportDecl *decl) {
957+
return !decl->isAccessLevelImplicit();
958+
};
959+
960+
findInconsistentImportsAcrossModule(mod, predicate, diagnose);
961+
return {};
962+
}
963+
920964
evaluator::SideEffect
921965
CheckInconsistentWeakLinkedImportsRequest::evaluate(Evaluator &evaluator,
922966
ModuleDecl *mod) const {

‎lib/Sema/TypeCheckAttr.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -970,6 +970,14 @@ bool AttributeChecker::visitAbstractAccessControlAttr(
970970
attr);
971971
return true;
972972
}
973+
974+
if (attr->getAccess() != AccessLevel::Public) {
975+
if (auto exportedAttr = D->getAttrs().getAttribute<ExportedAttr>()) {
976+
diagnoseAndRemoveAttr(attr, diag::access_level_conflict_with_exported,
977+
exportedAttr, attr);
978+
return true;
979+
}
980+
}
973981
}
974982

975983
return false;

‎lib/Sema/TypeCheckDeclPrimary.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1959,6 +1959,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
19591959
if (target &&
19601960
!ID->getAttrs().hasAttribute<ImplementationOnlyAttr>() &&
19611961
!ID->getAttrs().hasAttribute<SPIOnlyAttr>() &&
1962+
ID->getAccessLevel() == AccessLevel::Public &&
19621963
target->getLibraryLevel() == LibraryLevel::SPI) {
19631964

19641965
auto &diags = ID->getASTContext().Diags;

‎lib/Sema/TypeChecker.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,13 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
313313
CheckInconsistentSPIOnlyImportsRequest{SF},
314314
{});
315315

316+
if (!Ctx.LangOpts.isSwiftVersionAtLeast(6)) {
317+
evaluateOrDefault(
318+
Ctx.evaluator,
319+
CheckInconsistentAccessLevelOnImport{SF},
320+
{});
321+
}
322+
316323
evaluateOrDefault(
317324
Ctx.evaluator,
318325
CheckInconsistentWeakLinkedImportsRequest{SF->getParentModule()}, {});

‎test/Sema/access-level-and-non-resilient-import.swift‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/// Check diagnostics on a resilient modules importing publicly a
2+
/// a non-resilient module.
3+
4+
// RUN: %empty-directory(%t)
5+
// RUN: split-file %s %t
6+
7+
// REQUIRES: asserts
8+
9+
/// Build the libraries.
10+
// RUN: %target-swift-frontend -emit-module %t/DefaultLib.swift -o %t
11+
// RUN: %target-swift-frontend -emit-module %t/PublicLib.swift -o %t
12+
// RUN: %target-swift-frontend -emit-module %t/PackageLib.swift -o %t
13+
// RUN: %target-swift-frontend -emit-module %t/InternalLib.swift -o %t
14+
// RUN: %target-swift-frontend -emit-module %t/FileprivateLib.swift -o %t
15+
// RUN: %target-swift-frontend -emit-module %t/PrivateLib.swift -o %t
16+
17+
/// A resilient client will error on public imports.
18+
// RUN: %target-swift-frontend -typecheck %t/Client_Swift5.swift -I %t \
19+
// RUN: -enable-library-evolution -swift-version 5 \
20+
// RUN: -enable-experimental-feature AccessLevelOnImport -verify
21+
// RUN: %target-swift-frontend -typecheck %t/Client_Swift6.swift -I %t \
22+
// RUN: -enable-library-evolution -swift-version 6 \
23+
// RUN: -enable-experimental-feature AccessLevelOnImport -verify
24+
25+
/// A non-resilient client doesn't complain.
26+
// RUN: %target-swift-frontend -typecheck %t/Client_Swift5.swift -I %t \
27+
// RUN: -swift-version 5 \
28+
// RUN: -enable-experimental-feature AccessLevelOnImport
29+
// RUN: %target-swift-frontend -typecheck %t/Client_Swift6.swift -I %t \
30+
// RUN: -swift-version 6 \
31+
// RUN: -enable-experimental-feature AccessLevelOnImport
32+
33+
//--- DefaultLib.swift
34+
//--- PublicLib.swift
35+
//--- PackageLib.swift
36+
//--- InternalLib.swift
37+
//--- FileprivateLib.swift
38+
//--- PrivateLib.swift
39+
40+
//--- Client_Swift5.swift
41+
42+
import DefaultLib // expected-error {{module 'DefaultLib' was not compiled with library evolution support; using it means binary compatibility for 'Client_Swift5' can't be guaranteed}} {{1-1=internal }}
43+
public import PublicLib // expected-error {{module 'PublicLib' was not compiled with library evolution support; using it means binary compatibility for 'Client_Swift5' can't be guaranteed}} {{1-7=internal}}
44+
package import PackageLib
45+
internal import InternalLib
46+
fileprivate import FileprivateLib
47+
private import PrivateLib
48+
49+
//--- Client_Swift6.swift
50+
51+
import DefaultLib
52+
public import PublicLib // expected-error {{module 'PublicLib' was not compiled with library evolution support; using it means binary compatibility for 'Client_Swift6' can't be guaranteed}} {{1-7=internal}}
53+
package import PackageLib
54+
internal import InternalLib
55+
fileprivate import FileprivateLib
56+
private import PrivateLib

‎test/Sema/access-level-import-inconsistencies.swift‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/// Import inconsistencies.
2+
///
3+
/// Swift 5 case, with slow adoption of access level on imports.
4+
/// Report default imports if any other import of the same module
5+
/// has an access level below public (the default).
6+
///
7+
/// Swift 6 case, with default imports as internal.
8+
/// Don't report any mismatch.
9+
10+
// RUN: %empty-directory(%t)
11+
// RUN: split-file %s %t
12+
13+
// REQUIRES: asserts
14+
15+
/// Build the library.
16+
// RUN: %target-swift-frontend -emit-module %t/Lib.swift -o %t
17+
18+
/// Check diagnostics.
19+
20+
//--- Lib.swift
21+
public struct LibType {}
22+
23+
// RUN: %target-swift-frontend -typecheck %t/OneFile_AllExplicit.swift -I %t \
24+
// RUN: -enable-experimental-feature AccessLevelOnImport -verify
25+
//--- OneFile_AllExplicit.swift
26+
public import Lib
27+
package import Lib
28+
internal import Lib
29+
fileprivate import Lib
30+
private import Lib
31+
32+
// RUN: %target-swift-frontend -typecheck %t/ManyFiles_AllExplicit_File?.swift -I %t \
33+
// RUN: -enable-experimental-feature AccessLevelOnImport -verify
34+
//--- ManyFiles_AllExplicit_FileA.swift
35+
public import Lib
36+
//--- ManyFiles_AllExplicit_FileB.swift
37+
package import Lib
38+
//--- ManyFiles_AllExplicit_FileC.swift
39+
internal import Lib
40+
//--- ManyFiles_AllExplicit_FileD.swift
41+
fileprivate import Lib
42+
//--- ManyFiles_AllExplicit_FileE.swift
43+
private import Lib
44+
45+
// RUN: %target-swift-frontend -typecheck %t/ManyFiles_ImplicitVsInternal_FileB.swift -I %t \
46+
// RUN: -enable-experimental-feature AccessLevelOnImport -verify \
47+
// RUN: -primary-file %t/ManyFiles_ImplicitVsInternal_FileA.swift
48+
//--- ManyFiles_ImplicitVsInternal_FileA.swift
49+
import Lib // expected-error {{ambiguous implicit access level for import of 'Lib'; it is imported as 'internal' elsewhere}}
50+
//--- ManyFiles_ImplicitVsInternal_FileB.swift
51+
internal import Lib // expected-note {{imported 'internal' here}}
52+
53+
// RUN: %target-swift-frontend -typecheck %t/ManyFiles_ImplicitVsPackage_FileB.swift -I %t \
54+
// RUN: -enable-experimental-feature AccessLevelOnImport -verify \
55+
// RUN: -primary-file %t/ManyFiles_ImplicitVsPackage_FileA.swift
56+
//--- ManyFiles_ImplicitVsPackage_FileA.swift
57+
import Lib // expected-error {{ambiguous implicit access level for import of 'Lib'; it is imported as 'package' elsewhere}}
58+
//--- ManyFiles_ImplicitVsPackage_FileB.swift
59+
package import Lib // expected-note {{imported 'package' here}} @:1
60+
61+
// RUN: %target-swift-frontend -typecheck %t/ManyFiles_AmbiguitySwift6_File?.swift -I %t \
62+
// RUN: -enable-experimental-feature AccessLevelOnImport -verify -swift-version 6
63+
//--- ManyFiles_AmbiguitySwift6_FileA.swift
64+
import Lib
65+
//--- ManyFiles_AmbiguitySwift6_FileB.swift
66+
internal import Lib

‎test/Sema/conflicting-import-restrictions.swift‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,3 +35,35 @@
3535

3636
//--- SPIOnly_IOI_Exported.swift
3737
@_spiOnly @_implementationOnly @_exported import Lib // expected-error {{module 'Lib' cannot be both exported and implementation-only}}
38+
39+
/// Access levels on imports
40+
// RUN: %target-swift-frontend -typecheck %t/Public_Exported.swift -I %t -verify \
41+
// RUN: -experimental-spi-only-imports -verify \
42+
// RUN: -enable-experimental-feature AccessLevelOnImport
43+
// RUN: %target-swift-frontend -typecheck %t/Package_Exported.swift -I %t -verify \
44+
// RUN: -experimental-spi-only-imports -verify \
45+
// RUN: -enable-experimental-feature AccessLevelOnImport
46+
// RUN: %target-swift-frontend -typecheck %t/Internal_Exported.swift -I %t -verify \
47+
// RUN: -experimental-spi-only-imports -verify \
48+
// RUN: -enable-experimental-feature AccessLevelOnImport
49+
// RUN: %target-swift-frontend -typecheck %t/Fileprivate_Exported.swift -I %t -verify \
50+
// RUN: -experimental-spi-only-imports -verify \
51+
// RUN: -enable-experimental-feature AccessLevelOnImport
52+
// RUN: %target-swift-frontend -typecheck %t/Private_Exported.swift -I %t -verify \
53+
// RUN: -experimental-spi-only-imports -verify \
54+
// RUN: -enable-experimental-feature AccessLevelOnImport
55+
56+
//--- Public_Exported.swift
57+
@_exported public import Lib
58+
59+
//--- Package_Exported.swift
60+
@_exported package import Lib // expected-error {{'@_exported' is incompatible with 'package'; it can only be applied to public imports}}
61+
62+
//--- Internal_Exported.swift
63+
@_exported internal import Lib // expected-error {{'@_exported' is incompatible with 'internal'; it can only be applied to public imports}}
64+
65+
//--- Fileprivate_Exported.swift
66+
@_exported fileprivate import Lib // expected-error {{'@_exported' is incompatible with 'fileprivate'; it can only be applied to public imports}}
67+
68+
//--- Private_Exported.swift
69+
@_exported private import Lib // expected-error {{'@_exported' is incompatible with 'private'; it can only be applied to public imports}}

0 commit comments

Comments
 (0)