-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Index] Record the access level of declarations in the index #85175
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.
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
Conversation
|
@swift-ci Please smoke test |
fa1ba18 to
4ebe576
Compare
|
@swift-ci Please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Function parameters are marked as fileprivate. I think we could make an argument that they should be marked as local instead of fileprivate even though they are indexed without index-locals enabled.
- Accessors such as willSet are marked as fileprivate. Since you can’t call them from anywhere else, I think this makes as much sense as anything. At least it matches the compiler’s effective access level.
My preference here would be to not include any access level for them.
Also CC @keith as IIRC you were thinking about adding access levels sometime in the past.
lib/Index/IndexSymbol.cpp
Outdated
| if (isLocalSymbol(D)) { | ||
| info.Properties |= SymbolProperty::Local; | ||
| } else if (auto *VD = dyn_cast<ValueDecl>(D)) { | ||
| switch (VD->getEffectiveAccess()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Access level was added to the index request (not the index itself) in #31942 and there was a long discussion there about whether it should be effective access or getFormalAccessScope, with the conclusion being getFormalAccessScope.
One notable case there was:
private enum E {
case a, b, c
}
extension E {
private func f() { print("Hello") }
}
Where apparently getEffectiveAccess gives fileprivate for f (which it isn't).
This will allow us to inspect the updated symbol info to decided whether we should report access levels for a declaration.
9d22688 to
da32274
Compare
|
Great calls with the reference to the index request reporting access levels. I merged the two implementations, while at it also realized that we didn’t report the access level for macros in the old implementation. One thing that crossed my mind while working on this: We only report access levels for definitions but both definitions and references are annotated as |
rdar://163256878
da32274 to
aa9c295
Compare
|
@swift-ci Please smoke test |
|
Thanks for the cc! I was definitely interested in this case, unfortunately because of the limited slack plan I can't scroll back to our discussion on my specific examples for why. But IIRC I wanted to use this for various lint rules about ACLs. I could see this helping with tools for automatically making imports private when possible, automatically downgrading ACLs, etc. So i'm very excited to see something like this land! cc @jpsim who might be interested as well |
Depends on swiftlang/llvm-project#11716
While updating the test cases, I noticed two slight oddities, for which I am happy with the current behavior, but I’m happy to be convinced by a different opinion.
fileprivate. I think we could make an argument that they should be marked aslocalinstead offileprivateeven though they are indexed without index-locals enabled.willSetare marked asfileprivate. Since you can’t call them from anywhere else, I think this makes as much sense as anything. At least it matches the compiler’s effective access level.rdar://163256878