[Gardening] SE-0169 PR Feedback #9465
Conversation
include/swift/AST/Decl.h
Outdated
| return getFormalAccessImpl(useDC); | ||
| return result; | ||
| } | ||
| Accessibility getFormalAccess(const DeclContext *useDC = nullptr) const; |
There was a problem hiding this comment.
This split was deliberate, so that an already-computed access could be inlined into the caller. It might not be worth it, though.
There was a problem hiding this comment.
Yea, no real reason to change this. I'll roll it back.
include/swift/AST/AccessScope.h
Outdated
| @@ -34,7 +34,7 @@ class AccessScope { | |||
| /// Check if private access is allowed. This is a lexical scope check in Swift | |||
| @@ -44,13 +51,14 @@ extension Outer.Middle.Inner { | |||
| } | |||
| } | |||
|
|
|||
| // Padded to start on line 55 to make @LINE-55 work | |||
There was a problem hiding this comment.
You could avoid this by using the split version as the input to FileCheck (and running FileCheck twice for the WMO case).
include/swift/AST/AccessScope.h
Outdated
| @@ -52,12 +47,15 @@ class AccessScope { | |||
| /// Returns true if this is a child scope of the specified other access scope. | |||
| bool isChildOf(AccessScope AS) const { | |||
| if (!isPublic() && !AS.isPublic()) | |||
| return allowsAccess(getDeclContext(), AS.getDeclContext()); | |||
| return getDeclContext()->isAccessibleChildOf(AS.getDeclContext()); | |||
There was a problem hiding this comment.
This was a helper method because DeclContext really shouldn't know about access. Can you put it back?
There was a problem hiding this comment.
Sounds good -- Keeping things isolated was my original instinct, but I mis-interpreted your original feedback. I see what your saying now though!
|
Everything else about the other commits looks good. |
| return setterAccess >= Accessibility::Public; | ||
|
|
||
| auto scope = getDeclContext()->getAccessScope(DC, setterAccess, | ||
| /* useNominalTypeAccessibility */ false); |
There was a problem hiding this comment.
This is false for the scenario of extending public protocols with private subtypes. The test that caught the behavior is test/SILGen/witness_accessibility.swift.
In this test, an extension on a private protocol implements a default requirement for a parent public protocol. The current access scope lookup would restrict the visibility of the extension method R.publicRequirement to fileprivate since the type of R is fileprivate. But with this level of visibility struct S: R doesn't conform to protocol P.
Now with all that said, I'm realizing that you might just be referring to why is this false just in the case of the setter, and that's a good catch -- I don't think there's a need for that. I'll add a test to confirm.
There was a problem hiding this comment.
You're giving me more credit than I deserve; I was indeed asking about both cases.
For what it's worth, test/SILGen/witness_accessibility.swift is wrong—you can't (today) satisfy a public requirement with something that depends on a private protocol, and most times the compiler will reject it (SR-697). But fixing that will probably break code, so okay.
| if (!useDC) | ||
| bool ValueDecl::isAccessibleFrom(const DeclContext *DC) const { | ||
| auto access = getFormalAccess(DC); | ||
| if (!DC) |
There was a problem hiding this comment.
This seems backwards to me. The only use for the formal-access-as-seen-from-DC at this point is when DC is null.
There was a problem hiding this comment.
Yea, this confuses me as well, but this behavior is documented in the .h and a number of things break without it. I can dig in a bit more as to why.
There was a problem hiding this comment.
Sorry, I just meant you can skip the "as-seen-from-the-DC" part. A null DC represents access from anywhere.
|
@KingOfBrian Do you still wish to pursue this pull request? If so, could you please rebase it and address Jordan's review feedback? |
|
Alright, since this has gone a while without any new activity, I'm going to close it. Please feel free to reopen this or send us another pull request with any updated changes. |
This PR addresses some of the feedback from #9098:
checkAccessibilityto use AccessScopesThe last change of
checkAccessibilitywas a bit more of a change than I was hoping for. It turns out that the method for determining the AccessScope is a bit different than what checkAccessibility was doing. BasicallygetFormalAccessScoperestricts visibility by the access of the extension's type, andcheckAccessibilitydoes not want to do that when performing some protocol conformance checks. I added auseNominalTypeAccessibilityflag to make the AccessScope creation work in both cases. If this change turns out to be a bad idea, I'm glad to split the commit and keep any of the style changes (allowsAccess->isAccessibleChildOf) and revertcheckAccessibilityback to it's original form.