Limit ValueDecl::getFormalAccess and get rid of adjustAccessLevelForProtocolExtension #18048
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test compiler performance |
|
Build failed |
|
Build failed |
jrose-apple
force-pushed
the
finding-ways-to-scope
branch
from
8df9839 to
56c4cfe
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
Cool, that extra assertion doesn't hurt build times. |
jrose-apple
force-pushed
the
finding-ways-to-scope
branch
from
56c4cfe to
dca5d46
Compare
|
@swift-ci Please smoke test |
There was a problem hiding this comment.
Sorry for the delay, I wanted to review this carefully because it’s pretty subtle and obviously we’ve got it wrong before. Most of the comments are optional, except for my concern about the behavioral change with usable from inlinable members of extensions of public protocols.
lib/AST/Decl.cpp
Outdated
|
|
||
| AccessScope accessScope = | ||
| getAccessScopeForFormalAccess(VD, access, useDC, | ||
| treatUsableFromInlineAsPublic); |
There was a problem hiding this comment.
Note that the old behavior was that name lookup could find formally-public protocol members of extensions of usable from inline protocols. That is, we would adjust the internal protocol up to public if it was usable from inline, but not the extension member itself - it really has to be public.
This new check looks slightly different - if my understanding is correct you’re also applying the ‘usable from inline’ adjustment to the access level of the member itself. So this will make more things visible than were before.
I might be wrong but do you mind testing that a @usableFromInline member of an extension of a public protocol for example is not visible across a module boundary via name lookup?
There was a problem hiding this comment.
Tested and you are totally correct. That makes this a little more annoying to untangle, but it should be doable.
lib/AST/Decl.cpp
Outdated
| return checkAccessUsingAccessScopes(useDC, VD, access, forConformance); | ||
| } | ||
|
|
||
| switch (access) { |
There was a problem hiding this comment.
Maybe add a comment that the fast path relies on name lookup not visiting a type that is not visible to the caller, which is true for superclasses (you can’t have a public subclass of an internal class!) but not true for protocols (private conformances).
... but maybe we don’t need the fast path after all. If you think about it, computing an access scope isn’t that hard — in 90% of cases you’re just visiting a member of a type and then the type itself.
There was a problem hiding this comment.
It's at the call site, but it's probably worth including here too.
Access scope computation always walks the full DeclContext chain unless something is local, because it's recursive. It is something we could consider caching, though.
| if (hasStorage() && !isSettable(nullptr)) | ||
| return true; | ||
|
|
||
| if (isa<ParamDecl>(this)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| /// | ||
| /// \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, |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if (resultDC->isLocalContext() || access == AccessLevel::Private) | ||
| return AccessScope(resultDC, /*private*/true); | ||
|
|
||
| if (auto enclosingNominal = dyn_cast<NominalTypeDecl>(resultDC)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would expect all of that to be inlined anyway. I think it's fine.
| superclassCtor->getFormalAccessScope(/*useDC*/nullptr, | ||
| /*usableFromInlineAsPublic=*/true); | ||
|
|
||
| if (superclassInliningAccessScope.isPublic()) { |
There was a problem hiding this comment.
What if the superclass is public and has an inlinable initializer, and you define a private subclass? It looks like we will still inherit the attribute and probably diagnose it.
There was a problem hiding this comment.
I didn't change that part from what was there before, so…not a regression? :-) It doesn't seem to diagnose, but I can make sure we have a test case for it.
|
Thanks for all the comments! And very likely catching some real issues. I'll take a look today. |
...to push people towards getFormalAccessScope. The one use case that isn't covered by that is checking whether a declaration behaves as 'open' in the current file; I've added ValueDecl::hasOpenAccess to handle that specific case. No intended functionality change.
This gets adjustAccessLevelForProtocolExtension, a hack of sorts to begin with, out of ValueDecl's general API, and down to a helper for isAccessibleFrom and isSetterAccessibleFrom. (The only reason these two don't go through access scopes is as an optimization.)
This function (actually checkAccess) was relying on some implicit assumptions that aren't actually valid in all cases. When they're not, just fall back to a slower but more correct implementation; when they are, assert that the two implementations get the same answer. This allows us to get rid of adjustAccessLevelForProtocolExtension (see previous commit), though unfortunately not all of the associated hack. The diff is bigger than I'd like because it includes moving functions from NameLookup.cpp into Decl.cpp, but most of those didn't change. - checkAccess only changed in the one if branch for protocols - ValueDecl::isAccessibleFrom just added the assertion - AbstractStorageDecl::isSetterAccessibleFrom did not change No expected functionality change.
jrose-apple
force-pushed
the
finding-ways-to-scope
branch
from
dca5d46 to
cd22c5d
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
| // '@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() && |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Uh, hm. I'll write a test for that, then (in a follow-up PR).
There was a problem hiding this comment.
Going to have to defer to Slava on most of this; I don't have enough of a mental model of formal access vs. access scopes, either in language semantics or complier implementation, to know whether this is ok. The bits I can understand look like simple refactors, so they're fine. But there's clearly more going on here!
Access scopes implement the correct language semantics, so make sure we're encouraging clients to use them. In the one case where we deliberately aren't doing so, make sure that that doesn't change semantics. Finally, in this push we can eliminate ValueDecl::adjustAccessLevelForProtocolExtension, though unfortunately not all of the underlying problems that originally made it necessary.
This net-added code but I promise it's a cleanup. Really.