Skip to content

Commit 6726d07

Browse files
committed
Associate PackageUnit with ModuleDecl
* Weakly reference ModuleDecl from PackageUnit * Add PackageUnit decl context getter and use it for a package AccessScope * Return module decl referenced by PackageUnit in getModuleScopeContext and getParentModule * Handle package acl in access scope checkers * Remove AccessLimitKind * Fix tests Resolves rdar://104987295, rdar://105187216
1 parent 302180f commit 6726d07

File tree

10 files changed

+199
-142
lines changed

10 files changed

+199
-142
lines changed

include/swift/AST/AccessScope.h

Lines changed: 67 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -20,40 +20,41 @@
2020

2121
namespace swift {
2222

23-
/// Used to provide the kind of scope limitation in AccessScope::Value
24-
enum class AccessLimitKind : uint8_t { None = 0, Private, Package };
25-
2623
/// The wrapper around the outermost DeclContext from which
2724
/// a particular declaration can be accessed.
2825
class AccessScope {
29-
/// The declaration context along with an enum indicating the level of
30-
/// scope limitation.
31-
/// If the declaration context is set, and the limit kind is Private, the
32-
/// access level is considered 'private'. Whether it's 'internal' or
33-
/// 'fileprivate' is determined by what the declaration context casts to. If
34-
/// the declaration context is null, and the limit kind is None, the access
35-
/// level is considered 'public'. If the limit kind is Private, the access
36-
/// level is considered SPI. If it's Package, the access level is considered
37-
/// 'package'. Below is a table showing the combinations.
26+
/// To validate access of a decl, access scope check for both decl site
27+
/// and use site needs to be done. The underlying mechanism uses a
28+
/// pointer-int pair to determine the scope, as shown in the table below.
3829
///
39-
/// AccessLimitKind DC == nullptr DC != nullptr
40-
/// ---------------------------------------------------------------------------
41-
/// None public fileprivate or internal (check DC to tell which)
42-
/// Private `@_spi` public private
43-
/// Package package (unused)
44-
45-
llvm::PointerIntPair<const DeclContext *, 2, AccessLimitKind> Value;
30+
/// AccessScope DeclContext bool AccessLevel
31+
/// --------------------------------------------
32+
/// Public nullptr false public or open
33+
/// Public nullptr true public `@_spi`
34+
/// Package PackageUnit (unused) package
35+
/// Module ModuleDecl false internal
36+
/// FileScope FileUnit false fileprivate
37+
/// Private FileUnit true private
38+
///
39+
/// For example, if a decl with `public` access level is referenced outside of
40+
/// its defining module, it will be maped to the <nullptr, false> pair during
41+
/// the access scope check. This pair is determined based on the decl's access
42+
/// level in \c getAccessScopeForFormalAccess and passed to
43+
/// \c checkAccessUsingAccessScope which checks decl context hierarchy
44+
/// and others.
45+
///
46+
/// Note that the use site always has a non-null DeclContext whether or not
47+
/// the use site itself is a public type.
48+
///
49+
/// \see AccessScope::getAccessScopeForFormalAccess
50+
/// \see AccessScope::checkAccessUsingAccessScope
51+
/// \see ASTHierarchy
52+
llvm::PointerIntPair<const DeclContext *, 1, bool> Value;
4653

4754
public:
48-
AccessScope(const DeclContext *DC,
49-
AccessLimitKind limitKind = AccessLimitKind::None);
55+
AccessScope(const DeclContext *DC, bool isPrivate = false);
5056

51-
static AccessScope getPublic() {
52-
return AccessScope(nullptr, AccessLimitKind::None);
53-
}
54-
static AccessScope getPackage() {
55-
return AccessScope(nullptr, AccessLimitKind::Package);
56-
}
57+
static AccessScope getPublic() { return AccessScope(nullptr, false); }
5758

5859
/// Check if private access is allowed. This is a lexical scope check in Swift
5960
/// 3 mode. In Swift 4 mode, declarations and extensions of the same type will
@@ -66,63 +67,58 @@ class AccessScope {
6667
bool operator==(AccessScope RHS) const { return Value == RHS.Value; }
6768
bool operator!=(AccessScope RHS) const { return !(*this == RHS); }
6869
bool hasEqualDeclContextWith(AccessScope RHS) const {
69-
if (isPublic())
70-
return RHS.isPublic();
71-
if (isPackage())
72-
return RHS.isPackage();
7370
return getDeclContext() == RHS.getDeclContext();
7471
}
7572

76-
bool isPublic() const {
77-
return !Value.getPointer() && Value.getInt() == AccessLimitKind::None;
78-
}
79-
bool isPrivate() const {
80-
return Value.getPointer() && Value.getInt() == AccessLimitKind::Private;
81-
}
73+
bool isPublic() const { return !Value.getPointer(); }
74+
bool isPrivate() const { return Value.getPointer() && Value.getInt(); }
8275
bool isFileScope() const;
8376
bool isInternal() const;
84-
bool isPackage() const {
85-
return !Value.getPointer() && Value.getInt() == AccessLimitKind::Package;
86-
}
87-
88-
/// Returns true if the context of this (use site) is more restrictive than
89-
/// the argument context (decl site). This function does _not_ check the
90-
/// restrictiveness of the access level between this and the argument. \see
91-
/// AccessScope::isInContext
92-
bool isChildOf(AccessScope AS) const {
93-
if (isInContext()) {
94-
if (AS.isInContext())
95-
return allowsPrivateAccess(getDeclContext(), AS.getDeclContext());
96-
else
97-
return AS.isPackage() || AS.isPublic();
98-
}
99-
if (isPackage())
100-
return AS.isPublic();
101-
// If this is public, it can't be less than access level of AS
102-
// so return false
103-
return false;
104-
}
77+
bool isPackage() const;
10578

106-
/// Result depends on whether it's called at a use site or a decl site:
79+
/// Checks if the DeclContext of this access scope (use site) is more
80+
/// restrictive than that of the argument (decl site) based on the DeclContext
81+
/// hierarchy (from most to least restrictive):
82+
/// decl/expr (e.g. ClassDecl) -> FileUnit -> ModuleDecl -> PackageUnit -> null
83+
///
84+
/// A few things to note:
85+
/// 1. If both have the same DeclContext, returns false as one is _not_ a
86+
/// child of the other.
87+
/// 2. This function does _not_ check the restrictiveness of the _access
88+
/// level_ between two decls.
89+
/// 3. The DeclContext of this (use site) is is always non-null even if it may
90+
/// have a `public` access level.
10791
///
10892
/// For example,
10993
///
11094
/// ```
111-
/// public func foo(_ arg: bar) {} // `bar` is a `package` decl in another
112-
/// module
113-
/// ```
95+
/// import OtherModule
11496
///
115-
/// The meaning of \c isInContext changes whether it's at the use site or the
116-
/// decl site.
97+
/// // `Foo` is a `public` struct defined in `OtherModule`
98+
/// public func myFunc(_ arg: OtherModule.Foo) {}
99+
/// ```
117100
///
118-
/// The use site of \c bar, i.e. \c foo, is "in context" (decl context is
119-
/// non-null), regardless of the access level of \c foo (\c public in this
120-
/// case).
101+
/// The use site of `Foo`is a function `myFunc`, and its DeclContext is
102+
/// non-null (e.g. FileUnit) even though the function decl itself is `public`.
103+
/// When `isChildOf` is called, the argument passed in is a pair <nullptr,
104+
/// false> created in \c getAccessScopeForFormalAccess based on the access
105+
/// level of `Foo`. Since FileUnit is a child of null in the DeclContext
106+
/// hierarchy described above, it returns true.
121107
///
122-
/// The decl site of \c bar is only "in context" if the access level of the
123-
/// decl is \c internal or more restrictive. The context at the decl site is\c
124-
/// FileUnit if the decl is \c fileprivate or \c private; \c ModuleDecl if \c
125-
/// internal, and null if \c package or \c public.
108+
/// \see AccessScope::getAccessScopeForFormalAccess
109+
/// \see ASTHierarchy
110+
bool isChildOf(AccessScope AS) const {
111+
if (isPackage()) // This needs to be checked first before isInContext
112+
return AS.isPublic();
113+
else if (isInContext()) {
114+
if (AS.isInContext())
115+
return allowsPrivateAccess(getDeclContext(), AS.getDeclContext());
116+
else
117+
return AS.isPublic();
118+
} else // It's public, so can't be a child of the argument scope
119+
return false;
120+
}
121+
126122
bool isInContext() const { return getDeclContext() != nullptr; }
127123

128124
/// Returns the associated access level for diagnostic purposes.

include/swift/AST/DeclContext.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -512,9 +512,11 @@ class alignas(1 << DeclContextAlignInBits) DeclContext
512512
return false;
513513
}
514514

515-
/// Returns the package context of the parent module.
515+
/// Returns the package unit of this context.
516+
/// \p lookupIfNotCurrent If the current decl context is not PackageUnit, look
517+
/// it up via parent module
516518
LLVM_READONLY
517-
PackageUnit *getParentModulePackage() const;
519+
PackageUnit *getPackageContext(bool lookupIfNotCurrent = false) const;
518520

519521
/// Returns the module context that contains this context.
520522
LLVM_READONLY

include/swift/AST/DiagnosticsSema.def

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1679,6 +1679,7 @@ ERROR(access_control_open_bad_decl,none,
16791679
WARNING(access_control_non_objc_open_member,none,
16801680
"non-'@objc' %0 in extensions cannot be overridden; use 'public' instead",
16811681
(DescriptiveDeclKind))
1682+
ERROR(access_control_requires_package_name, none, "decl has a package access level but no -package-name was passed", ())
16821683

16831684
ERROR(invalid_decl_attribute,none,
16841685
"'%0' attribute cannot be applied to this declaration", (DeclAttribute))

include/swift/AST/Module.h

Lines changed: 45 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -159,33 +159,48 @@ class OverlayFile;
159159
/// location.
160160
class ModuleSourceFileLocationMap;
161161

162-
/// Package unit used to allow grouping of modules by a package name.
163-
/// ModuleDecl can set PackageUnit as a parent in its DeclContext
164-
/// See \c ModuleDecl
162+
/// A unit that allows grouping of modules by a package name.
163+
/// It's set as a property in ModuleDecl, instead of as its parent decl context,
164+
/// since otherwise it will break the existing decl context lookups that assume
165+
/// ModuleDecl as the top level context. See \c ModuleDecl
165166
class PackageUnit: public DeclContext {
166-
167+
/// Identifies this package and used for the equality check
167168
Identifier PackageName;
168-
169-
PackageUnit(Identifier name);
169+
/// Weakly reference ModuleDecl that points to this package.
170+
/// Instead of having multiple ModuleDecls pointing to one PackageUnit, we
171+
/// create and set one PackageUnit per ModuleDecl, to make it easier to look
172+
/// up the module pointing to this package; this look up is needed in existing
173+
/// DeclContext functions, e.g. \c DeclContext::getModuleScopeContext,
174+
/// and \c DeclContext::getParentModule
175+
ModuleDecl &SourceModule;
176+
177+
PackageUnit(Identifier name, ModuleDecl &src)
178+
: DeclContext(DeclContextKind::Package, nullptr), PackageName(name),
179+
SourceModule(src) {}
170180

171181
public:
172-
static PackageUnit *
173-
create(Identifier name, ASTContext &ctx) {
174-
return new (ctx) PackageUnit(name);
182+
static PackageUnit *create(Identifier name, ModuleDecl &src,
183+
ASTContext &ctx) {
184+
return new (ctx) PackageUnit(name, src);
175185
}
176186

177187
static bool classof(const DeclContext *DC) {
178188
return DC->getContextKind() == DeclContextKind::Package;
179189
}
180190

181-
static bool classof(const PackageUnit *PU) {
182-
// FIXME: add a correct check
183-
return true;
184-
}
191+
static bool classof(const PackageUnit *PU) { return true; }
185192

186193
Identifier getName() const {
187194
return PackageName;
188195
}
196+
197+
ModuleDecl &getSourceModule() { return SourceModule; }
198+
199+
/// Equality check via package name instead of pointer comparison.
200+
/// Returns false if the name is empty.
201+
bool isSamePackageAs(PackageUnit *other) {
202+
return !(getName().empty()) && getName() == other->getName();
203+
}
189204
};
190205

191206
/// The minimum unit of compilation.
@@ -202,8 +217,10 @@ class ModuleDecl
202217
/// The ABI name of the module, if it differs from the module name.
203218
mutable Identifier ModuleABIName;
204219

205-
/// The name of the package this module belongs to
206-
mutable Identifier PackageName;
220+
/// A package this module belongs to. It's set as a property instead of a
221+
/// parent decl context; otherwise it will break the existing decl context
222+
/// lookups that assume ModuleDecl as the top level context.
223+
PackageUnit *Package;
207224

208225
/// Module name to use when referenced in clients module interfaces.
209226
mutable Identifier ExportAsName;
@@ -440,18 +457,21 @@ class ModuleDecl
440457
ModuleABIName = name;
441458
}
442459

443-
/// Get the package name of the module
444-
Identifier getPackageName() const { return PackageName; }
445-
446-
/// Set the name of the package this module belongs to
447-
void setPackageName(Identifier name) {
448-
PackageName = name;
449-
// TODO: uncomment when PackageUnit gets passed to constructor
450-
// PackageUnit *pkgUnit = PackageUnit::create(name, getASTContext());
451-
// DeclContext newContext = DeclContext(DeclContextKind::Module, pkgUnit);
452-
// setDeclContext(&newContext);
460+
/// Get the package name of this module
461+
/// FIXME: remove this and bump module version rdar://104723918
462+
Identifier getPackageName() const {
463+
if (auto pkg = getPackage())
464+
return pkg->getName();
465+
return Identifier();
453466
}
454467

468+
/// Get the package associated with this module
469+
PackageUnit *getPackage() const { return Package; }
470+
471+
/// Set the package this module is associated with
472+
/// FIXME: rename this with setPackage(name) rdar://104723918
473+
void setPackageName(Identifier name);
474+
455475
Identifier getExportAsName() const { return ExportAsName; }
456476

457477
void setExportAsName(Identifier name) {

lib/AST/ASTVerifier.cpp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -963,12 +963,6 @@ class Verifier : public ASTWalker {
963963
if (D->hasAccess()) {
964964
PrettyStackTraceDecl debugStack("verifying access", D);
965965
if (!D->getASTContext().isAccessControlDisabled()) {
966-
if (D->getFormalAccessScope().isPackage() &&
967-
D->getFormalAccess() < AccessLevel::Package) {
968-
Out << "non-package decl has no formal access scope\n";
969-
D->dump(Out);
970-
abort();
971-
}
972966
if (D->getFormalAccessScope().isPublic() &&
973967
D->getFormalAccess() < AccessLevel::Public) {
974968
Out << "non-public decl has no formal access scope\n";

lib/AST/Decl.cpp

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3773,6 +3773,10 @@ static AccessLevel getAdjustedFormalAccess(const ValueDecl *VD,
37733773
}
37743774

37753775
if (useDC) {
3776+
// If the use site decl context is PackageUnit, just return
3777+
// the access level that's passed in
3778+
if (auto usePkg = useDC->getPackageContext())
3779+
return access;
37763780
// Check whether we need to modify the access level based on
37773781
// @testable/@_private import attributes.
37783782
auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext());
@@ -3883,13 +3887,11 @@ getAccessScopeForFormalAccess(const ValueDecl *VD,
38833887
while (!resultDC->isModuleScopeContext()) {
38843888
if (isa<TopLevelCodeDecl>(resultDC)) {
38853889
return AccessScope(resultDC->getModuleScopeContext(),
3886-
access == AccessLevel::Private
3887-
? AccessLimitKind::Private
3888-
: AccessLimitKind::None);
3890+
access == AccessLevel::Private);
38893891
}
38903892

38913893
if (resultDC->isLocalContext() || access == AccessLevel::Private)
3892-
return AccessScope(resultDC, AccessLimitKind::Private);
3894+
return AccessScope(resultDC, /*private*/ true);
38933895

38943896
if (auto enclosingNominal = dyn_cast<GenericTypeDecl>(resultDC)) {
38953897
auto enclosingAccess =
@@ -3920,13 +3922,17 @@ getAccessScopeForFormalAccess(const ValueDecl *VD,
39203922
case AccessLevel::Private:
39213923
case AccessLevel::FilePrivate:
39223924
assert(resultDC->isModuleScopeContext());
3923-
return AccessScope(resultDC, access == AccessLevel::Private
3924-
? AccessLimitKind::Private
3925-
: AccessLimitKind::None);
3925+
return AccessScope(resultDC, access == AccessLevel::Private);
39263926
case AccessLevel::Internal:
39273927
return AccessScope(resultDC->getParentModule());
3928-
case AccessLevel::Package:
3929-
return AccessScope::getPackage();
3928+
case AccessLevel::Package: {
3929+
auto pkg = resultDC->getPackageContext(true);
3930+
if (!pkg) {
3931+
auto &d = VD->getASTContext().Diags;
3932+
d.diagnose(VD->getLoc(), diag::access_control_requires_package_name);
3933+
}
3934+
return AccessScope(pkg);
3935+
}
39303936
case AccessLevel::Public:
39313937
case AccessLevel::Open:
39323938
return AccessScope::getPublic();
@@ -3965,10 +3971,6 @@ static bool checkAccessUsingAccessScopes(const DeclContext *useDC,
39653971
// useDC is null only when caller wants to skip non-public type checks.
39663972
if (!useDC) return true;
39673973

3968-
// Check package access; accessing package decl should not be allowed if package names are different
3969-
if (accessScope.isPackage())
3970-
return VD->getDeclContext()->getParentModule()->getPackageName() == useDC->getParentModule()->getPackageName();
3971-
39723974
// Check SPI access
39733975
if (!VD->isSPI()) return true;
39743976
auto useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext());
@@ -4083,7 +4085,11 @@ static bool checkAccess(const DeclContext *useDC, const ValueDecl *VD,
40834085
auto *useSF = dyn_cast<SourceFile>(useFile);
40844086
return useSF && useSF->hasTestableOrPrivateImport(access, sourceModule);
40854087
}
4086-
case AccessLevel::Package:
4088+
case AccessLevel::Package: {
4089+
auto srcPkg = sourceDC->getPackageContext(true);
4090+
auto usePkg = useDC->getPackageContext(true);
4091+
return usePkg->isSamePackageAs(srcPkg);
4092+
}
40874093
case AccessLevel::Public:
40884094
case AccessLevel::Open:
40894095
return true;

0 commit comments

Comments
 (0)