Skip to content

Limit ValueDecl::getFormalAccess and get rid of adjustAccessLevelForProtocolExtension #18048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 25, 2018
Merged

Limit ValueDecl::getFormalAccess and get rid of adjustAccessLevelForProtocolExtension #18048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca14d6c
Take away some of ValueDecl::getFormalAccess's capabilities
jrose-apple Jul 17, 2018
fef3a37
Use access scopes instead of adjustAccessLevelForProtocolExtension
jrose-apple Jul 18, 2018
cd22c5d
Use access scopes for the hard cases in ValueDecl::isAccessibleFrom
jrose-apple Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 17 additions & 24 deletions include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2347,33 +2347,17 @@ class ValueDecl : public Decl {
/// Returns the access level specified explicitly by the user, or provided by
/// default according to language rules.
///
/// This is the access used when calculating if access control is being used
/// consistently. If \p useDC is provided (the location where the value is
/// being used), features that affect formal access such as \c \@testable are
/// taken into account.
///
/// If \p treatUsableFromInlineAsPublic is true, declarations marked with the
/// \c @usableFromInline attribute are treated as public. This is normally
/// false for name lookup and other source language concerns, but true when
/// computing the linkage of generated functions.
/// Most of the time this is not the interesting value to check; access is
/// limited by enclosing scopes per SE-0025. Use #getFormalAccessScope to
/// check if access control is being used consistently, and to take features
/// such as \c \@testable and \c \@usableFromInline into account.
///
/// \sa getFormalAccessScope
AccessLevel getFormalAccess(const DeclContext *useDC = nullptr,
bool treatUsableFromInlineAsPublic = false) const;

/// If this declaration is a member of a protocol extension, return the
/// minimum of the given access level and the protocol's access level.
///
/// Otherwise, return the given access level unmodified.
///
/// This is used when checking name lookup visibility. Protocol extension
/// members can be found when performing name lookup on a concrete type;
/// if the concrete type is visible from the lookup context but the
/// protocol is not, we do not want the protocol extension members to be
/// visible.
AccessLevel adjustAccessLevelForProtocolExtension(AccessLevel access) const;
/// \sa hasOpenAccess
AccessLevel getFormalAccess() const;

/// Determine whether this Decl has either Private or FilePrivate access.
/// Determine whether this Decl has either Private or FilePrivate access,
/// and its DeclContext does not.
bool isOutermostPrivateOrFilePrivateScope() const;

/// Returns the outermost DeclContext from which this declaration can be
Expand All @@ -2395,6 +2379,7 @@ class ValueDecl : public Decl {
///
/// \sa getFormalAccess
/// \sa isAccessibleFrom
/// \sa hasOpenAccess
AccessScope
getFormalAccessScope(const DeclContext *useDC = nullptr,
bool treatUsableFromInlineAsPublic = false) const;
Expand Down Expand Up @@ -2448,6 +2433,14 @@ class ValueDecl : public Decl {
bool isAccessibleFrom(const DeclContext *DC,
bool forConformance = false) const;

/// Returns whether this declaration should be treated as \c open from
/// \p useDC. This is very similar to #getFormalAccess, but takes
/// \c \@testable into account.
///
/// This is mostly only useful when considering requirements on an override:
/// if the base declaration is \c open, the override might have to be too.
bool hasOpenAccess(const DeclContext *useDC) const;

/// Retrieve the "interface" type of this value, which uses
/// GenericTypeParamType if the declaration is generic. For a generic
/// function, this will have a GenericFunctionType with a
Expand Down
232 changes: 197 additions & 35 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2280,10 +2280,44 @@ static AccessLevel getTestableAccess(const ValueDecl *decl) {
return AccessLevel::Public;
}

/// Adjust \p access based on whether \p VD is \@usableFromInline or has been
/// testably imported from \p useDC.
///
/// \p access isn't always just `VD->getFormalAccess()` because this adjustment
/// may be for a write, in which case the setter's access might be used instead.
static AccessLevel getAdjustedFormalAccess(const ValueDecl *VD,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: split this into two functions, adjustAccessForUsableFomInline() and adjustAccessForTestableImport(). This might make the code clearer. Also the testable check is potentially expensive (you’re walking up DCs and then iterating over all imports) so it might be beneficial to only do it once, at the end of getFormalAccessScope().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I experimented with the split but I don't feel great about it: there are only a few call sites that pass a constant for one of the arguments, and if we ever add another kind of adjustment it will be harder to find them all. But it would save on repeatedly checking for testable imports…

I'm going to leave it out of this PR, anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that having 2 separate forms of adjustment (or more in the future) might make Slava's suggestion awkward; but can I add a vote here for a slightly more descriptive name than "adjusted"? It's not clear from the name that said adjustments are either comprehensive, desirable or idempotent. Maybe it'd be enough to say "getFullyAdjustedFormalAccess"? or "getCompletedFormalAccess" (and renaming "getFormalAccess" to "getIncompleteFormalAccess")? I'm not sure what a clearer term would be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of those are more descriptive either. I don't have a better name because it's really just "adjust the formal access based on the context provided by these unrelated parameters". It's at least a static helper and not relevant outside of implementing these APIs on ValueDecl.

AccessLevel access,
const DeclContext *useDC,
bool treatUsableFromInlineAsPublic) {
if (treatUsableFromInlineAsPublic &&
access == AccessLevel::Internal &&
VD->isUsableFromInline()) {
return AccessLevel::Public;
}

if (useDC && (access == AccessLevel::Internal ||
access == AccessLevel::Public)) {
if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
if (useSF->hasTestableImport(VD->getModuleContext()))
return getTestableAccess(VD);
}

return access;
}

/// Convenience overload that uses `VD->getFormalAccess()` as the access to
/// adjust.
static AccessLevel
getAdjustedFormalAccess(const ValueDecl *VD, const DeclContext *useDC,
bool treatUsableFromInlineAsPublic) {
return getAdjustedFormalAccess(VD, VD->getFormalAccess(), useDC,
treatUsableFromInlineAsPublic);
}

AccessLevel ValueDecl::getEffectiveAccess() const {
auto effectiveAccess =
getFormalAccess(/*useDC=*/nullptr,
/*treatUsableFromInlineAsPublic=*/true);
getAdjustedFormalAccess(this, /*useDC=*/nullptr,
/*treatUsableFromInlineAsPublic=*/true);

// Handle @testable.
switch (effectiveAccess) {
Expand Down Expand Up @@ -2335,48 +2369,54 @@ AccessLevel ValueDecl::getEffectiveAccess() const {
return effectiveAccess;
}

AccessLevel ValueDecl::getFormalAccess(const DeclContext *useDC,
bool treatUsableFromInlineAsPublic) const {
AccessLevel ValueDecl::getFormalAccess() const {
ASTContext &ctx = getASTContext();
AccessLevel result = ctx.evaluator(AccessLevelRequest{const_cast<ValueDecl *>(this)});
if (treatUsableFromInlineAsPublic &&
result == AccessLevel::Internal &&
isUsableFromInline()) {
return AccessLevel::Public;
}
if (useDC && (result == AccessLevel::Internal ||
result == AccessLevel::Public)) {
if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
if (useSF->hasTestableImport(getModuleContext()))
return getTestableAccess(this);
}
return result;
return ctx.evaluator(AccessLevelRequest{const_cast<ValueDecl *>(this)});
}

AccessScope
ValueDecl::getFormalAccessScope(const DeclContext *useDC,
bool treatUsableFromInlineAsPublic) const {
const DeclContext *result = getDeclContext();
AccessLevel access = getFormalAccess(useDC, treatUsableFromInlineAsPublic);
bool ValueDecl::hasOpenAccess(const DeclContext *useDC) const {
assert(isa<ClassDecl>(this) || isa<ConstructorDecl>(this) ||
isPotentiallyOverridable());

while (!result->isModuleScopeContext()) {
if (result->isLocalContext() || access == AccessLevel::Private)
return AccessScope(result, true);
AccessLevel access =
getAdjustedFormalAccess(this, useDC,
/*treatUsableFromInlineAsPublic*/false);
return access == AccessLevel::Open;
}

if (auto enclosingNominal = dyn_cast<NominalTypeDecl>(result)) {
/// Given the formal access level for using \p VD, compute the scope where
/// \p VD may be accessed, taking \@usableFromInline, \@testable imports,
/// and enclosing access levels into account.
///
/// \p access isn't always just `VD->getFormalAccess()` because this adjustment
/// may be for a write, in which case the setter's access might be used instead.
static AccessScope
getAccessScopeForFormalAccess(const ValueDecl *VD,
AccessLevel formalAccess,
const DeclContext *useDC,
bool treatUsableFromInlineAsPublic) {
AccessLevel access = getAdjustedFormalAccess(VD, formalAccess, useDC,
treatUsableFromInlineAsPublic);
const DeclContext *resultDC = VD->getDeclContext();

while (!resultDC->isModuleScopeContext()) {
if (resultDC->isLocalContext() || access == AccessLevel::Private)
return AccessScope(resultDC, /*private*/true);

if (auto enclosingNominal = dyn_cast<NominalTypeDecl>(resultDC)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: micro-optimize these checks by changing the while into an infinite loo and only switch over the DeclContext kind once. Check for access == Private first since its cheap.

Copy link
Contributor Author

@jrose-apple jrose-apple Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect all of that to be inlined anyway. I think it's fine.

auto enclosingAccess =
enclosingNominal->getFormalAccess(useDC,
treatUsableFromInlineAsPublic);
getAdjustedFormalAccess(enclosingNominal, useDC,
treatUsableFromInlineAsPublic);
access = std::min(access, enclosingAccess);

} else if (auto enclosingExt = dyn_cast<ExtensionDecl>(result)) {
} else if (auto enclosingExt = dyn_cast<ExtensionDecl>(resultDC)) {
// Just check the base type. If it's a constrained extension, Sema should
// have already enforced access more strictly.
if (auto extendedTy = enclosingExt->getExtendedType()) {
if (auto nominal = extendedTy->getAnyNominal()) {
auto nominalAccess =
nominal->getFormalAccess(useDC,
treatUsableFromInlineAsPublic);
getAdjustedFormalAccess(nominal, useDC,
treatUsableFromInlineAsPublic);
access = std::min(access, nominalAccess);
}
}
Expand All @@ -2385,16 +2425,16 @@ ValueDecl::getFormalAccessScope(const DeclContext *useDC,
llvm_unreachable("unknown DeclContext kind");
}

result = result->getParent();
resultDC = resultDC->getParent();
}

switch (access) {
case AccessLevel::Private:
case AccessLevel::FilePrivate:
assert(result->isModuleScopeContext());
return AccessScope(result, access == AccessLevel::Private);
assert(resultDC->isModuleScopeContext());
return AccessScope(resultDC, access == AccessLevel::Private);
case AccessLevel::Internal:
return AccessScope(result->getParentModule());
return AccessScope(resultDC->getParentModule());
case AccessLevel::Public:
case AccessLevel::Open:
return AccessScope::getPublic();
Expand All @@ -2403,6 +2443,128 @@ ValueDecl::getFormalAccessScope(const DeclContext *useDC,
llvm_unreachable("unknown access level");
}

AccessScope
ValueDecl::getFormalAccessScope(const DeclContext *useDC,
bool treatUsableFromInlineAsPublic) const {
return getAccessScopeForFormalAccess(this, getFormalAccess(), useDC,
treatUsableFromInlineAsPublic);
}

/// Checks if \p VD may be used from \p useDC, taking \@testable imports into
/// account.
///
/// Whenever the enclosing context of \p VD is usable from \p useDC, this
/// should compute the same result as checkAccess, below, but more slowly.
///
/// See ValueDecl::isAccessibleFrom for a description of \p forConformance.
static bool checkAccessUsingAccessScopes(const DeclContext *useDC,
const ValueDecl *VD,
AccessLevel access) {
AccessScope accessScope =
getAccessScopeForFormalAccess(VD, access, useDC,
/*treatUsableFromInlineAsPublic*/false);
return accessScope.getDeclContext() == useDC ||
AccessScope(useDC).isChildOf(accessScope);
}

/// Checks if \p VD may be used from \p useDC, taking \@testable imports into
/// account.
///
/// When \p access is the same as `VD->getFormalAccess()` and the enclosing
/// context of \p VD is usable from \p useDC, this ought to be the same as
/// getting the AccessScope for `VD` and checking if \p useDC is within it.
/// However, there's a source compatibility hack around protocol extensions
/// that makes it not quite the same.
///
/// See ValueDecl::isAccessibleFrom for a description of \p forConformance.
static bool checkAccess(const DeclContext *useDC, const ValueDecl *VD,
AccessLevel access, bool forConformance) {
auto *sourceDC = VD->getDeclContext();

if (!forConformance) {
if (auto *proto = sourceDC->getAsProtocolOrProtocolExtensionContext()) {
// FIXME: Swift 4.1 allowed accessing protocol extension methods that were
// marked 'public' if the protocol was '@_versioned' (now
// '@usableFromInline'). Which works at the ABI level, so let's keep
// supporting that here by explicitly checking for it.
if (access == AccessLevel::Public) {
assert(proto->getDeclContext()->isModuleScopeContext() &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We diagnose nested protocols in typeCheckDecl() but nothing prevents you from doing name lookup into them, and various practicalswift test cases exercise this (also see test/decl/nested/protocol.swift), so I think this still has to work (but it's OK if it doesn't return a correct result).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, hm. I'll write a test for that, then (in a follow-up PR).

"if we get nested protocols, this should not apply to them");
if (proto->getFormalAccess() == AccessLevel::Internal &&
proto->isUsableFromInline()) {
return true;
}
}

// Skip the fast path below and just compare access scopes.
return checkAccessUsingAccessScopes(useDC, VD, access);
}
}

// Fast path: assume that the client context already has access to our parent
// DeclContext, and only check what might be different about this declaration.
if (!useDC)
return access >= AccessLevel::Public;

switch (access) {
case AccessLevel::Private:
return (useDC == sourceDC ||
AccessScope::allowsPrivateAccess(useDC, sourceDC));
case AccessLevel::FilePrivate:
return useDC->getModuleScopeContext() == sourceDC->getModuleScopeContext();
case AccessLevel::Internal: {
const ModuleDecl *sourceModule = sourceDC->getParentModule();
const DeclContext *useFile = useDC->getModuleScopeContext();
if (useFile->getParentModule() == sourceModule)
return true;
if (auto *useSF = dyn_cast<SourceFile>(useFile))
if (useSF->hasTestableImport(sourceModule))
return true;
return false;
}
case AccessLevel::Public:
case AccessLevel::Open:
return true;
}
llvm_unreachable("bad access level");
}

bool ValueDecl::isAccessibleFrom(const DeclContext *useDC,
bool forConformance) const {
auto access = getFormalAccess();
bool result = checkAccess(useDC, this, access, forConformance);

// For everything outside of protocols and operators, we should get the same
// result using either implementation of checkAccess, because useDC must
// already have access to this declaration's DeclContext.
// FIXME: Arguably, we're doing the wrong thing for operators here too,
// because we're finding internal operators within private types. Fortunately
// we have a requirement that a member operator take the enclosing type as an
// argument, so it won't ever match.
assert(getDeclContext()->getAsProtocolOrProtocolExtensionContext() ||
isOperator() ||
result == checkAccessUsingAccessScopes(useDC, this, access));

return result;
}

bool AbstractStorageDecl::isSetterAccessibleFrom(const DeclContext *DC,
bool forConformance) const {
assert(isSettable(DC));

// If a stored property does not have a setter, it is still settable from the
// designated initializer constructor. In this case, don't check setter
// access; it is not set.
if (hasStorage() && !isSettable(nullptr))
return true;

if (isa<ParamDecl>(this))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn’t add this part, but I’m wondering if a fast path for ParamDecl makes sense in isAccessibleFrom() too. Actually, an even better fast path check might be this->getDeclContext()->isLocalContext() - if you find a definition that’s immediately in a local context, it must be via unqualified lookup, and it must be visible to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how much trouble this has caused us I'm hesitant to include any new fast paths unless I can figure out how to write assertions for them.

return true;

auto access = getSetterFormalAccess();
return checkAccess(DC, this, access, forConformance);
}

void ValueDecl::copyFormalAccessFrom(const ValueDecl *source,
bool sourceIsParentContext) {
if (!hasAccess()) {
Expand Down
Loading