[SourceKit][SR-12837] Add effective access level of references #31942

rockbruno · 2020-05-21T19:15:41Z

Since enum elements inherit their access level from their parent, the compiler wasn't indexing it:

This PR makes enum elements rob its parent's access attribute to allow SourceKit to index it as an attribute.

I think it makes sense to have this because enum elements do have access levels, it's just that you don't write them explicitly. The context for this PR is that the lack of this property causes a bug in a tool I wrote when I need to determine if an enum element is public or not.

(FYI, the same problem exists in extensions that have the access level defined in the extension itself, instead of its inner methods.)

rockbruno · 2020-05-21T19:17:13Z

lib/Index/Index.cpp

+     for (auto *accessAttr : ED->getAttrs().getAttributes<AccessControlAttr, true>()) {
+       EED->getAttrs().add((DeclAttribute *)accessAttr);
+     }
+  }


I initially thought this would be possible by changing SymbolInfo. Since that's not the case I don't believe this is the best place for this change, but I wasn't sure where else to put it.

xwu

This isn't correct. Effective visibility is not the same as the declared access level, and the two should not be mixed up.

Take the case of extensions: Both private extension T { func f() { ... } } and extension T { private func f() { ... } } are possible to write. The two examples mean different things, as the effective visibility of f is not the same. That is, private inside the scope is different from private outside the scope. It is critically important to make that distinction: after all, that is why private exists.

In Swift, it is explicitly permitted to write something like: internal struct S { public var v: Int }. The declared access modifier for S.v is public, but its effective visibility is internal. Tools that expect information on the effective visibility cannot rely on the declared access modifier; it must compute that information.

Similarly, in private enum E { case c; private static var d: Self { c } }, c and d do not have the same visibility. (Put another way, if it were possible to write out explicit access levels for enum cases, then the case in enum E { private case c } would also have different effective visibility than in private enum E { case c }.) We should not be pretending that an access modifier is declared where it is not, in order to facilitate tools making use of that information erroneously when they actually need to access the effective visibility.

It is not the fault of SourceKit that it represents this information accurately, as it currently does.

Since the compiler already computes effective visibility, I suppose it would be possible to expose whether a particular entity is part of the public API and/or ABI to SourceKit; this is probably what you'd want, if I understand the use case correctly.

rockbruno · 2020-05-22T09:20:16Z

I see, I understand the difference now. The effective level is really what I'm looking for. Is retrieving this possible and exposing it in SourceKit a good idea?

I mentioned public but I would basically like to be able to determine the access level of any decl, as currently I would have to ship a mini-Swift compiler to determine it based on these rules.

xwu · 2020-05-22T17:30:09Z

The effective visibility is not always expressible. For, example, there is no spelling for the visibility of x:

struct A {
  private struct B {
    var x: Int
  }
}

You are right that the rules are complicated. However, if you want to determine the effective visibility of a declaration, all the information is there in the SourceKit output, so there is no need for a compiler. You can build all of the necessary logic into your tooling.

nathawes

It's best to avoid mutating the AST in the indexing code paths as because we index while building as well (where we share the same AST with the rest of the compiler), it could in theory affect the compiler output.

For getting the access level though I think it's better to leave the 'attributes' field as is and instead add a separate 'key.effective_access' field to the index response if that's what your tool is really after (you can add new keys in UIDs.py), plus it seems generally useful. While it's true that you could compute the access level in your tool, it would definitely be non-trivial particularly for items in extensions which have a base type in a separate file.

I think you can just add a new UIdent field to 'EntityInfo' in LangSupport.h for the effective access level and populate it in the 'withEntityInfo' functions in SwiftIndexing.cpp. The IndexSymbol you get there has a reference to the decl (symbol.decl) and if it's a ValueDecl you can call getEffectiveAccess on it. You can check how the existing 'attributes' field is populated in withEntityInfo to see how to get a UIdent. You can either reuse the ones for the public/private/open attributes, or create new UIdents specifically for access levels rather than attributes. Then you just need to update SKIndexingConsumer::startSourceEntity in Requests.cpp to make use of the new field in EntityInfo by adding it to the response object.

Hope that helps!

rockbruno · 2020-05-23T12:54:37Z

Thanks guys! I was able to get it working with @nathawes 's approach. What do you think?
I haven't updated the tests because the diff will be very large, so I'm planning on making sure the approach is correct before doing so.

test/SourceKit/Indexing/index_effective_access_level.swift

xwu · 2020-05-23T16:06:08Z

test/SourceKit/Indexing/index_effective_access_level.swift

+}
+
+extension PrivateEn {
+    private func prFoo() {}


Likewise here. Did you intend to have two conflicting declarations?

xwu · 2020-05-23T16:08:54Z

test/SourceKit/Indexing/index_effective_access_level.swift

+
+extension PrivateEn {
+    private func prFoo() {}
+}


This file is missing salient test cases. Please include the scenarios I've enumerated earlier, where members have higher declared access levels than their containing type, and where members have private access levels that give them lower visibility than fileprivate (which cannot be expressed).

xwu · 2020-05-23T16:12:48Z

test/SourceKit/Indexing/index_effective_access_level.swift.response

+          key.attribute: source.decl.attribute.public
+        }
+      ],
+      key.effective_access_level: source.decl.attribute.public


This may be how it's stored internally, but it is inaccurate: extensions should have no effective visibility of their own because they don't "exist" independently of what they extend. Where a shorthand access level is used, it is only giving the default access of its members. We debated this at length back in the day and that is the model that the core team wanted exposed to users.

xwu · 2020-05-23T16:24:31Z

test/SourceKit/Indexing/index_effective_access_level.swift.response

+              key.attribute: source.decl.attribute.private
+            }
+          ],
+          key.effective_access_level: source.decl.attribute.fileprivate


This is incorrect: the effective visibility of extension PrivateEn { private func prFoo() {} } is less than fileprivate. You can prove this for yourself by trying to call that method (you would need to give your duplicated methods different names first) from a free function in the same file:

private enum E { case a, b, c } extension E { private func f() { print("Hello") } } E.a.f() // error: 'f' is inaccessible due to 'private' protection level

And this is what I've been saying earlier: there is no unambiguous way to distinguish this private scope from a totally different private scope. Without more radically designing your output, all you can get here is whether a declaration is effectively public, internal, fileprivate, or something less than that. I would not reuse source.decl.attribute.private to describe everything in the latter category, as it's misleading (it would make every private scope appear the same).

I am confused on how to proceed because that output came directly from getEffectiveAccess(). Do you mean that ideally the compiler should have an additional enum case for the latter category?

Right, so getEffectiveAccess() isn't giving you the right information here. That is also what it says in the documentation: "This is the access used when making optimization and code generation decisions. It should not be used at the AST or semantic level."

Ignore the part where I talk about how best to represent the correct result; the first issue is that you're getting an incorrect result here.

I think I understood that we should never use attribute.private to describe AccessLevel.private, but It's unclear to me the strategy to make the compiler stop returning fileprivate in this case (should it have returned private?)

EDIT: Nevermind, I posted this comment slightly after you posted yours 😄

Right, so getEffectiveAccess() isn't giving you the right information here. That is also what it says in the documentation: "This is the access used when making optimization and code generation decisions. It should not be used at the AST or semantic level."

Oh sorry, I missed that. Using this was probably a bad suggestion on my part... Basing this on getFormalAccessScope().isPublic/Private/FileScope/Internal() instead is probably a better fit and I think will give the correct result here. Just remember to check isFileScope() (to indicate fileprivate) before isPrivate(), because private things accessible at file scope are effectively fileprivate.

tools/SourceKit/lib/SwiftLang/SwiftIndexing.cpp

rockbruno · 2020-05-24T13:55:31Z

Thanks again guys! I think I understand the theory of this now. I changed it to getFormalAccessScope and it looks like the tests are correct now as I added more cases that check how scopes are reduced and kept. Since we can't use the private keyword here, I also ended up making new IDs and naming the latter less_than_fileprivate. Also removed the new key from extensions.

xwu

This is really, really nice!

xwu · 2020-05-24T15:35:43Z

test/SourceKit/Indexing/index_effective_access_level.swift

+    func b() {}
+    fileprivate func c() {}
+    private func d() {}
+}


These test cases are great. One nit and one suggestion:

Nit: Would you mind giving these a consistent naming scheme? It would just make it easier to review the result. Perhaps something like PrivateEnum, PrivateStruct, InternalEnum, InternalStruct, filePrivateFunc, publicFunc, etc. (The cases are fine as-is, since they don't have any access modifiers, but perhaps even they can be given unique letters.) That way, it'll be easier for a reader (i.e., me) to pick out when the results are unexpected. Also, it can be helpful to readers like me to avoid ad-hoc abbreviations like flPr.

Suggestion: Could you please add (a) some private extensions with private members and public extensions with members that don't have any access modifier; (b) some declarations other than cases and functions (initializers, subscripts, variables with different getters and setters); (c) protocol declarations, since the members can't have their own access modifiers; (d) open classes with a mix of public and open members; (e) an import statement and an @_exported import statement (in case SymbolKind::Module comes into play)? Should all work but just in case...

xwu · 2020-05-24T15:45:27Z

tools/SourceKit/lib/SwiftLang/SwiftLangSupport.cpp

+    return EffectiveAccess_Internal;
+  } else if (Scope.isFileScope()) {
+    return EffectiveAccess_FilePrivate;
+  } else {


To be defensive, I would still check else if (Scope.isPrivate()), then use else { llvm_unreachable("Unsupported access scope") }.

Nit: since open is technically an access level, but its scope would be public, what do you think of calling this feature just "effective access" or "effective access scope" instead of "effective access level"?

rockbruno · 2020-05-25T20:09:00Z

I've pushed changes to refactor that testing class, renamed it to effective_access and also updated the remaining test cases 👍
I tried to add as many cases as possible from your description, I just didn't add a public extension with members without accesses because that was already there (if I understood it correctly)

The output looks mostly correct, but I noticed that the protocol members are getting the new key in their definition inside the protocol itself. Is this behavior correct?

xwu · 2020-05-25T20:30:30Z

test/SourceKit/Indexing/index_effective_access_level.swift.response

+                  key.attribute: source.decl.attribute.public
+                }
+              ],
+              key.effective_access: source.decl.effective_access.public


...Thanks for writing a more thorough set of tests. This is helpful in revealing problems such as this:

The "effective access" of the setter shouldn't be 'public' if it's private(set), should it?

That's an issue with getFormalAccess(), isn't it? For the purposes of this feature, I really only care about the .decl.'s access (the getter, but in the "main output"). Do you agree in this case with leaving the explicit getters and setters outputs out of this feature?

Sure, that seems fine.

There's a separate getSetterFormalAccessScope() for this on AbstractStorageDecl. I think that should be exposed a separate 'effective_setter_access' field on the decl itself (rather than accessors), but can definitely be left as future work for when someone wants that info.

xwu · 2020-05-25T20:35:48Z

I noticed that the protocol members are getting the new key in their definition inside the protocol itself. Is this behavior correct?

I think it's correct. The reason you can't use explicit access modifiers is that all members are supposed to be as visible as the protocol itself. But the issue of visibility applies to them as they do members of concrete types, and it's not out of the questions that one day we might support requirements that are private (e.g., implementation details).

While we're being complete, let's make sure property wrappers are correctly supported (varying the access modifiers used on the wrapper and on the wrapped property).

rockbruno · 2020-05-26T16:24:19Z

I made the changes to remove the key from accessors and added some content with PropertyWrappers.

From the new output, the generic value from the property wrapper caught my eye:

        {
          key.kind: source.lang.swift.decl.generic_type_param,
          key.name: "T",
          key.usr: "s:28index_effective_access_level21PublicPropertyWrapperV1Txmfp",
          key.line: 53,
          key.column: 37,
          key.effective_access: source.decl.effective_access.internal
        },

This one is internal regardless of the wrapper's access value, but I don't know enough to see if that's correct or not... In any case definitions of generics might also fall in the list of things that aren't important for this feature

xwu · 2020-05-26T18:18:00Z

That's interesting. Yeah, I think for the purposes of this feature, it would probably be appropriate to suppress this result; not sure if it's correct or not off the top of my head. This brings up additional questions, though:

Inside a function, will local variables all be labeled as 'less than fileprivate'? Seems unnecessary.
What about loops and other control structures (if let, etc.)?
If you have a where clause on your extension for a generic type, will that bring up additional occurrences like this?

If this proves to be too pervasive, perhaps the approach should be a whitelist of situations where this information is included.

Regarding importing other modules: I was curious to see what import Module would look like after SourceKit is done with it; will it say that the import declaration has internal effective access? If so, is that actually helpful?

nathawes · 2020-05-26T19:25:11Z

That's interesting. Yeah, I think for the purposes of this feature, it would probably be appropriate to suppress this result; not sure if it's correct or not off the top of my head.

It looks like the compiler doesn't care about the accessibility of generic type parameters looking at the implementation (https://github.com/apple/swift/blob/master/lib/AST/AccessRequests.cpp#L72) so suppressing the field is probably the right move there.

This brings up additional questions, though:

Inside a function, will local variables all be labeled as 'less than fileprivate'? Seems unnecessary.

What about loops and other control structures (if let, etc.)?

It won't hurt to add a test case (if none of the existing tests already cover it), but the index request shouldn't be returning anything for local declarations. The one exception is parameters that don't have separate external and internal argument labels (i.e. foo(extenalAndInternalName: Int). Those are indexed so that the global rename refactoring can update the parameter references as well when renaming the containing function's argument labels.

If you have a where clause on your extension for a generic type, will that bring up additional occurrences like this?

Only references rather than declarations can appear in that position, so these shouldn't get the new field already, since the new field is being within a if (!isRef) block.

If this proves to be too pervasive, perhaps the approach should be a whitelist of situations where this information is included.

Regarding importing other modules: I was curious to see what import Module would look like after SourceKit is done with it; will it say that the import declaration has internal effective access? If so, is that actually helpful?

We don't index import declarations themselves at the moment (just the module reference within them), which doesn't get the new field because it's a reference. The new test shows that https://github.com/apple/swift/blob/90f7472fa2a5181752d802a36cde5c275d57dde4/test/SourceKit/Indexing/index_effective_access_level.swift.response#L3532.

nathawes · 2020-05-26T19:27:49Z

@swift-ci please test

swift-ci · 2020-05-26T19:30:30Z

Build failed
Swift Test OS X Platform
Git Sha - c68d3085df4a49a536f977589c934bb90058161b

rockbruno · 2020-05-26T19:30:31Z

I like the idea of whitelisting instead of blacklisting, it makes things easier for everyone. If someone would like additional support they can whitelist and work out the kinks. There's too many things to cover.

I pushed the changes to do this and added some missing cases (global variables, typealiases and associated types). I just pushed an additional commit too to show local properties not being indexed so this test trips if this gets changed.

swift-ci · 2020-05-26T19:30:45Z

Build failed
Swift Test Linux Platform
Git Sha - c68d3085df4a49a536f977589c934bb90058161b

nathawes · 2020-05-26T19:31:23Z

@swift-ci please test

swift-ci · 2020-05-26T19:33:57Z

Build failed
Swift Test OS X Platform
Git Sha - 03834769be7608b9df82309bc333ae342bcffc0e

swift-ci · 2020-05-26T19:34:16Z

Build failed
Swift Test Linux Platform
Git Sha - 03834769be7608b9df82309bc333ae342bcffc0e

swift-ci · 2020-05-26T23:02:53Z

Build failed
Swift Test Linux Platform
Git Sha - 05996f7019f4b32399754dfbfe04be93748e73a7

rockbruno · 2020-05-27T07:46:52Z

One of the tests was linux only. Fixed it
I am not sure what the backtrace test is, hoping it's just a flaky one

nathawes · 2020-05-27T16:06:33Z

@swift-ci please test

swift-ci · 2020-05-27T16:09:07Z

Build failed
Swift Test Linux Platform
Git Sha - 05996f7019f4b32399754dfbfe04be93748e73a7

swift-ci · 2020-05-27T16:10:00Z

Build failed
Swift Test OS X Platform
Git Sha - 05996f7019f4b32399754dfbfe04be93748e73a7

nathawes · 2020-05-27T22:31:57Z

@swift-ci please test Linux

swift-ci · 2020-05-28T00:40:10Z

Build failed
Swift Test Linux Platform
Git Sha - 0970ebe1428ddc7bb7ccd04dab3dc7ad68052d5c

nathawes · 2020-05-29T02:18:56Z

@swift-ci please smoke test Linux

rockbruno · 2020-05-29T17:02:11Z

Saw that they treated that test aed0d91, I've rebased it

nathawes · 2020-05-29T17:06:12Z

@swift-ci please test

swift-ci · 2020-05-29T17:09:06Z

Build failed
Swift Test Linux Platform
Git Sha - 0970ebe1428ddc7bb7ccd04dab3dc7ad68052d5c

swift-ci · 2020-05-29T17:09:07Z

Build failed
Swift Test OS X Platform
Git Sha - 0970ebe1428ddc7bb7ccd04dab3dc7ad68052d5c

nathawes · 2020-05-29T20:44:02Z

@swift-ci please test OS X Platform

nathawes · 2020-06-02T17:16:34Z

@xwu just wanted to check if you had a chance to look over the latest changes. Should I go ahead and merge this now?

xwu · 2020-06-02T22:09:24Z

@rockbruno Mind if I take a day or two to look it over?

xwu

Sorry for the delay, I've been preoccupied with some other stuff. I have to admit I haven't been able to slog through the test output this time round, but I like the revised approach. I just have a few small comments.

tools/SourceKit/lib/SwiftLang/SwiftIndexing.cpp

Review comments have been addressed

Co-authored-by: Xiaodi Wu <13952+xwu@users.noreply.github.com>

nathawes · 2020-06-09T15:44:30Z

@swift-ci please test

swift-ci · 2020-06-09T15:46:45Z

Build failed
Swift Test Linux Platform
Git Sha - b50813a

swift-ci · 2020-06-09T15:47:10Z

Build failed
Swift Test OS X Platform
Git Sha - b50813a

rockbruno commented May 21, 2020

View reviewed changes

theblixguy requested a review from nathawes May 21, 2020 19:20

xwu previously requested changes May 21, 2020

View reviewed changes

nathawes requested changes May 22, 2020

View reviewed changes

rockbruno force-pushed the allelementsmatter branch 2 times, most recently from 51f5bf3 to d740bd4 Compare May 23, 2020 12:53

rockbruno changed the title ~~[SourceKit][SR-12837] Add access level of enum elements as an attribute~~ [SourceKit][SR-12837] Add effective access level of references May 23, 2020

xwu reviewed May 23, 2020

View reviewed changes

nathawes reviewed May 23, 2020

View reviewed changes

tools/SourceKit/lib/SwiftLang/SwiftIndexing.cpp Outdated Show resolved Hide resolved

rockbruno force-pushed the allelementsmatter branch from d740bd4 to 3214290 Compare May 24, 2020 13:53

xwu reviewed May 24, 2020

View reviewed changes

xwu reviewed May 25, 2020

View reviewed changes

rockbruno force-pushed the allelementsmatter branch from 4cd511f to 90f7472 Compare May 26, 2020 16:21

rockbruno added 6 commits May 29, 2020 18:59

[SourceKit] Add Effective Scope to Index

2a48e19

Improve tests and generate the rest of the output

f000639

Remove accessors and add PW tests

9b05823

Whitelist instead of blacklist

51c0024

Add a test showing local content being ignored

6ebf0d5

Run linux only test

b50813a

rockbruno force-pushed the allelementsmatter branch from 0970ebe to b50813a Compare May 29, 2020 16:59

xwu reviewed Jun 8, 2020

View reviewed changes

tools/SourceKit/lib/SwiftLang/SwiftIndexing.cpp Outdated Show resolved Hide resolved

tools/SourceKit/lib/SwiftLang/SwiftIndexing.cpp Show resolved Hide resolved

Update tools/SourceKit/lib/SwiftLang/SwiftIndexing.cpp

346771f

Co-authored-by: Xiaodi Wu <13952+xwu@users.noreply.github.com>

nathawes merged commit 47554c1 into swiftlang:master Jun 15, 2020

swift-ci mentioned this pull request May 22, 2020

[SR-12836] Extensions with access modifiers don't have such accesses indexed #55282

Open

[SourceKit][SR-12837] Add effective access level of references #31942

[SourceKit][SR-12837] Add effective access level of references #31942

Conversation

rockbruno commented May 21, 2020

Choose a reason for hiding this comment

xwu left a comment • edited Loading

Choose a reason for hiding this comment

rockbruno commented May 22, 2020 • edited Loading

xwu commented May 22, 2020 • edited Loading

nathawes left a comment • edited Loading

Choose a reason for hiding this comment

rockbruno commented May 23, 2020

Choose a reason for hiding this comment

xwu May 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xwu May 23, 2020 • edited Loading

Choose a reason for hiding this comment

rockbruno May 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockbruno commented May 24, 2020 • edited Loading

xwu left a comment

Choose a reason for hiding this comment

xwu May 24, 2020 • edited Loading

Choose a reason for hiding this comment

xwu May 24, 2020 • edited Loading

Choose a reason for hiding this comment

rockbruno commented May 25, 2020

xwu May 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xwu commented May 25, 2020 • edited Loading

rockbruno commented May 26, 2020 • edited Loading

xwu commented May 26, 2020

nathawes commented May 26, 2020

nathawes commented May 26, 2020

swift-ci commented May 26, 2020

rockbruno commented May 26, 2020

swift-ci commented May 26, 2020

nathawes commented May 26, 2020

swift-ci commented May 26, 2020

swift-ci commented May 26, 2020

swift-ci commented May 26, 2020

rockbruno commented May 27, 2020 • edited Loading

nathawes commented May 27, 2020

swift-ci commented May 27, 2020

swift-ci commented May 27, 2020

nathawes commented May 27, 2020

swift-ci commented May 28, 2020

nathawes commented May 29, 2020

rockbruno commented May 29, 2020

nathawes commented May 29, 2020

swift-ci commented May 29, 2020

swift-ci commented May 29, 2020

nathawes commented May 29, 2020

nathawes commented Jun 2, 2020

xwu commented Jun 2, 2020

xwu left a comment

Choose a reason for hiding this comment

nathawes commented Jun 9, 2020

swift-ci commented Jun 9, 2020

swift-ci commented Jun 9, 2020

xwu left a comment •

edited

Loading

rockbruno commented May 22, 2020 •

edited

Loading

xwu commented May 22, 2020 •

edited

Loading

nathawes left a comment •

edited

Loading

xwu May 23, 2020 •

edited

Loading

xwu May 23, 2020 •

edited

Loading

rockbruno May 23, 2020 •

edited

Loading

rockbruno commented May 24, 2020 •

edited

Loading

xwu May 24, 2020 •

edited

Loading

xwu May 24, 2020 •

edited

Loading

xwu May 25, 2020 •

edited

Loading

xwu commented May 25, 2020 •

edited

Loading

rockbruno commented May 26, 2020 •

edited

Loading

rockbruno commented May 27, 2020 •

edited

Loading