[AST] Decl::is*AccessibleFrom methods should respect access control…

[xedin]

… flag

Otherwise integrated REPL and other tools like LLDB would produce
incorrect results or crash.

Resolves: rdar://problem/30933988

[jrose-apple]

This condition should be checked higher up, so that we don't even get to the point of asking checkAccess. Also, I'm not sure why LLDB would crash if something isn't accessible enough; it should just not type-check.

[xedin]

@jrose-apple It crashes because there are asserts in diagnostics which validate results of lookup. Also checking flag in is*AccessibleFrom methods directly might not be best because they all end up using checkAccess anyway, so it’s harder to make a mistake if it’s checked there.

[jrose-apple]

isAccessibleFrom isn't the right place to check either. Anything in Sema that's doing access control checks should check and not do the checks to begin with.

[xedin]

Arguably that would make things even worse because there are N callees and M possible invariants to check, done by callees (assuming that callees are even aware what they are, in this particular case it wasn't the case) makes it N * M checks required (both ifs + asserts).

I think it's perfectly reasonable for checkAccess to be aware of the flag and return true if access control is disabled instead of sprinkling checks all over Sema.

[slavapestov]

If we do decide to go with this approach, it might make some of the existing places that test the -disable-access-control flag redundant.

[xedin]

@slavapestov Indeed, there are a bunch of places where it's possible to remove that check which makes it a lot more consistent, because most methods in TypeChecker check that invariant in-place.

[xedin]

I'm preparing the patch, just need to run tests locally.

[xedin]

@slavapestov I've moved isAccessControlDisabled to ASTContext and and while refactoring it cleaned up unnecessary checks to it.

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 2177343a571853032aa6fe3ce4f6354818b037bb

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 2177343a571853032aa6fe3ce4f6354818b037bb

[xedin]

@swift-ci please test Linux platform

[slavapestov]

This looks like an improvement over the old way to me.

[jrose-apple]

Not all access checking goes through these entry points, and by doing this there's still a bunch of extra work being done to check accesses even though the result of the check is going to be ignored.

The existing principle is "AST just performs the query you ask it, Sema decides what it means".

[xedin]

By these do you mean is*AccessibleFrom? If so, check*AccessControl methods do check that flag internally at the moment, and what ever is left just needs to be refactored to do so.

The existing principle is "AST just performs the query you ask it, Sema decides what it means".

It seems to me that this principle is not strictly followed, because when I was refactoring use of EnableAccessControl I noticed that most of the check*AccessControl methods in AST do actually check that flag internally and return early.

[jrose-apple]

It seems to me that this principle is not strictly followed, because when I was refactoring use of EnableAccessControl I noticed that most of the check*AccessControl methods in AST do actually check that flag internally and return early.

Where are you seeing this? The only ones I see are in NameLookup.cpp and ModuleNameLookup.cpp, which, admittedly, violate my library-based rule, but which are higher-level concepts than the accessors on Decl.

[xedin]

The ones in lookup, in TypeCheckProtocol.cpp, TypeCheckAccess.cpp and in couple in WitnessChecker.

[jrose-apple]

Those are in Sema.

[xedin]

The most interesting use is in isUsableFromInlineRequired() which checks the flag internally instead of requiring it by its callers.

[xedin]

I have moved get{Setter}FormalAccess to checkAccess so is{Setter}AccessibleFrom no longer do any heavy weight work, I didn't realize that it did use request evaluator.

Since multiple methods in type-checker already use this approach and I elaborated on why I think it's better than asking callees to validate that invariant, I'm going to go ahead and merge this PR soon if there are no other significant reasons not to do this.

[jrose-apple]

Sorry, why is this better? checkAccess was abstracting over the setter/non-setter distinction. Passing in a flag makes that harder to understand and harder to keep separate.

Please do not merge this when I've brought up significant objections. You haven't demonstrated why this is better, and you haven't shown any cases where it actually causes errors in LLDB to do it the other way.

[xedin]

I've brought up significant objections

To be honest, I am still to see one expect "this is how I want it to be". I listed reasons why doing checks in callee makes this N * M check problem. I think one of the objections was about extra work which I addressed.

Sorry, why is this better? checkAccess was abstracting over the setter/non-setter distinction. Passing in a flag makes that harder to understand and harder to keep separate.

If you like it better I can add a parameter e.g. llvm::function_ref<AccessLevel()> getAccessLevel which gets passed to checkAccess. IMHO, again, is just a matter of personal preference.

You haven't demonstrated why this is better, and you haven't shown any cases where it actually causes errors in LLDB to do it the other way.

Minimum it simplifies a bunch of users of is{Setter}AccessibleFrom because they don't have to keep track of this invariant, note that EnableAccessControl was not and is not properly tested at all in constraint system code (arguably constraint system diagnostics shouldn't even worry about any other flags expect to typechecker/constraint system onces), as well as in CodeCompletion and, before my previous two patches, in override checking.

[xedin]

I made it so checkAccess accepts a callback to getAccessLevel instead of passing a forSetter flag.

[xedin]

We have talked offline today and agree that this a good thing to do, I'm also going to look at improving logic related to getFormalAccessScope which is used separately from is{Setter}AccessibleFrom.

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - ace1afb9eb1db546617feed5dab7b5536b9f7be0

[jrose-apple]

Additional notes for anyone following along: Pavel convinced me that (1) the conceptual purity line between AST and Sema wasn't buying us enough in practice, especially since we keep adding code without thinking about DisableAccessControl mode, and (2) the amount of work we could have saved is still something we could only save in DisableAccessControl mode, which means normal compilation is still doing it.

However, we also spotted another place to make this change: the getAdjustedFormalAccess helper inside Decl.cpp. As Pavel notes above, that covers most of the interesting cases outside of isAccessibleFrom, and it's also already a central point for handling @testable everywhere other than checkAccess.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - ace1afb9eb1db546617feed5dab7b5536b9f7be0