[SR-2209] Add real AccessScope type. #4579

aleksgapp · 2016-08-31T22:59:13Z

This pull request adds real AccessScope type that wraps DeclContext along with the bit, saying whether a top-level DeclContext should be treated as private rather than fileprivate. This changes fixes the issue when diagnostics occasionally claims that declaration is fileprivate even though the user has written private.

Now diagnostics claims correct messages, but there is one moment that I'd like to clarify.

Currently this is a valid code:

private protocol PrivateProto {
  func privateReq()
}
public struct PublicStruct: PrivateProto {
  fileprivate func privateReq() {}
}

And we have tests for error message and fix-it:

private/public/internal/fileprivate struct Struct: PrivateProto {
  private func privateReq() {} // expected-error {{... must be declared fileprivate because ... in fileprivate protocol 'PrivateProto'}} {{3-10=fileprivate}}
}

I find this error message a bit confusing, it would be clearer to see this error message next to the protocol declaration:

private protocol PrivateProto { // {{ minimum accessibility for the top-level declaration is fileprivate }} {{3-10=fileprivate}}
  func privateReq()
}

Is this a valid point and should I start an evolution thread for this change?

Resolves SR-2209.

jrose-apple · 2016-09-01T02:58:32Z

Thanks, Aleksey! I'll answer your question first and then add some review comments tomorrow morning.

The behavior of SE-0025 states that private at the top-level is equivalent to fileprivate, but it's still allowed. The intent here is that people will want to write private by default, and that fileprivate is rare enough that some projects won't ever actually write it at all. That means that once all the access scope stuff goes in, the diagnostic should say "must be declared fileprivate because…in private protocol 'PrivateProto'". Or perhaps we could even drop the word "private" at that point ("in protocol 'PrivateProto'"); the real requirement is that the method is callable from outside the type body.

jrose-apple · 2016-09-01T16:07:38Z

include/swift/AST/AccessScope.h

+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the AccessScope class.


This doesn't really add anything beyond the file header (which is also pretty sparse). If there's nothing more useful to say here, you can just remove it.

jrose-apple · 2016-09-01T16:43:16Z

Sorry to pile a ton of comments on—this overall looks good and is pretty much spot-on the approach I was trying to describe in the bug. Thank you for doing this!

aleksgapp · 2016-09-02T12:06:32Z

Thanks Jordan! I was expecting a lot of comments about second C++ code in my life.

jrose-apple · 2016-09-07T16:36:50Z

lib/AST/Module.cpp

-    const DeclContext *accessScope = nominal->getFormalAccessScope();
-    if (accessScope && !accessScope->isModuleContext())
+    auto DC = nominal->getFormalAccessScope().getDeclContext();
+    if (DC && !DC->isModuleContext())


I think this code is really saying accessScope.isPrivate() || accessScope.isFileScope(). That's a little more expressive than checking the associated DC.

jrose-apple · 2016-09-07T16:58:27Z

This is looking great. A few more comments, and questions on what you think is the best way to handle certain things.

aleksgapp · 2016-09-16T13:39:51Z

Done!

I've desided to stick with private bit, I might be wrong, but it doesn't seem that we gain any benefit from switching this extra bit to fileprivate.

Also adding extra logic for intersectWith when intersecting fileprivate with private on top-level doesn't really help. The biggest problem for diagnostics is to say that something is fileprivate when it is actually declared as private at the top-level in particular cases.

Accessibility accessLevel = accessScope.isFileScope() 
  ? Accessibility::FilePrivate 
  : accessScope.accessibilityForDiagnostics();

This condition appears in all places where we need to emit fileprivate for top-level private, it doesn't look very nice and I'd like to hear you opinion on this, what would be better:

Add extra parameter to existing accessibilityForDiagnostics:

Accessibility accessibilityForDiagnostics(bool topLevelPrivateAsFilePrivate = false) const {
  ...
  if (getDeclContext()->isModuleScopeContext()) {
    return isPrivate() && !topLevelPrivateAsFilePrivate 
      ? Accessibility::Private
      : Accessibility::FilePrivate;
  }
  ...
}

or add extra function for this purpose:

Accessibility requiredAccessibilityForDiagnostics() const {
  return isFileScope() ? Accessibility::FilePrivate : accessibilityForDiagnostics()
}

or leave it as is?

jrose-apple · 2016-11-03T22:19:34Z

Aleksey, I'm very sorry for leaving this behind for two months. It probably needs to be rebased at this point, especially given the pile of access changes that went in late in 3.0.1. Would you like to do that or should I?

For your most recent question, I think I like the separate function better.

I'll do another pass on the code once we get past the rebase step, but I'm pretty happy with it.

1. Add new AccessScope type that just wraps a plain DeclContext. 2. Propagate it into all uses of "ValueDecl::getFormalAccessScope". 3. Turn all operations that combine access scopes into methods on AccessScope. 4. Add the "private" flag to distinguish "private" from "fileprivate" scope for top-level DeclContext.

aleksgapp · 2016-11-07T12:01:54Z

@jrose-apple No worries, thanks for coming back 😃
I've been keeping eye on this and already resolved some conflicts before. Anyway, I've rebased it once again and made a separate function for the required accessibility.

jrose-apple

So much "explicit is better than implicit". :-) More comments:

jrose-apple · 2016-11-07T16:59:15Z

include/swift/AST/AccessScope.h

+public:
+  AccessScope(const DeclContext *DC, bool isPrivate = false)
+    : Value(DC, isPrivate) {
+        if (!DC || isa<ModuleDecl>(DC))


Style nitpick: I think our dominant indentation strategy here is to double-indent the member initializer line and then single-indent the body as usual.

jrose-apple · 2016-11-07T17:02:21Z

include/swift/AST/AccessScope.h

+  /// Returns true if this is a child scope of the specified other access scope.
+  ///
+  /// \see DeclContext::isChildContextOf
+  bool isChildOf(AccessScope AS) const {


I believe DeclContext::isChildContextOf returns false if the two are the same, which implies that public->isChildOf(public) should also return false here.

jrose-apple · 2016-11-07T17:12:39Z

include/swift/AST/Decl.h

@@ -32,6 +32,7 @@
 #include "llvm/Support/TrailingObjects.h"

 namespace swift {
+  class AccessScope;


Since we're returning by value, a forward-declaration isn't good enough. The header is cheap enough to just include, I think.

(Unless there's a circularity issue that I've forgotten, in which case we should figure out a better way to fix it.)

I'm afraid that's my fix for the circularity, I'll see what can be done.

jrose-apple · 2016-11-07T17:13:40Z

include/swift/AST/DiagnosticsSema.def

      "|cannot be declared %select{PRIVATE|fileprivate|internal|public}3}2 "
      "because its type uses "
      "%select{a private|a fileprivate|an internal|PUBLIC}4 type",
-      (bool, bool, bool, Accessibility, Accessibility))
+      (bool, bool, bool, Accessibility, Accessibility, bool))


Just seeing this now: please put the new flag directly after the access level it affects (for all of the modified diagnostics). The jump from argument 3 to argument 5 confused me for a bit.

jrose-apple · 2016-11-07T17:17:07Z

lib/Sema/TypeCheckDecl.cpp

  Action walkToTypePre(Type ty) override {
    // Assume failure until we post-visit this node.
    // This will be correct as long as we don't ever have self-referential
    // Types.
    auto cached = Cache.find(ty);
    if (cached != Cache.end()) {
-      auto opaqueCached = reinterpret_cast<intptr_t>(cached->second);
-      RawScopeStack.back() = intersectAccess(RawScopeStack.back(),opaqueCached);
+      auto last = &RawScopeStack.back();


Nitpick: I think we'd prefer auto &last = RawScopeStack.back() or even AccessScope &last = RawScopeStack.back(), because it won't be reassigned. (Same below.)

jrose-apple · 2016-11-07T17:24:23Z

lib/Sema/TypeCheckDecl.cpp

                                   const TypeRepr *thisComplainRepr) {
-      if (!minAccessScope ||
-          typeAccessScope->isChildContextOf(minAccessScope) ||
+      if (typeAccessScope.isChildOf(minAccessScope) ||
          (!complainRepr && typeAccessScope == minAccessScope)) {


I think this one's supposed to be checking for the same DC rather than identical scopes. ("If we can warn about either one, we should pick the one with a TypeRepr.")

jrose-apple · 2016-11-07T17:30:33Z

lib/Sema/TypeCheckProtocol.cpp

                          ValueDecl *requirement,
                          ValueDecl *witness,
                          bool *isSetter) {
  *isSetter = false;

-  const DeclContext *protoAccessScope = Proto->getFormalAccessScope(DC);
+  requiredAccessScope =
+    *requiredAccessScope.intersectWith(Proto->getFormalAccessScope(DC));


Extra * here? How did this compile?

It is needed here, becauseintersectWith returns Optional.

Previously here was an assert for "non-intersecting scopes" case, now it will crash in the same scenario. Should I make an explicit assert?

Ah, right, my bad. (Somehow I misread this as equivalent to requiredAccessScope->intersectWith.) The explicit assert is probably a good idea just to make it a little faster to track down what happened if it ever breaks.

jrose-apple · 2016-11-07T17:44:18Z

lib/Sema/TypeCheckDecl.cpp

      bool isExplicit =
        SD->getAttrs().hasAttribute<AccessibilityAttr>() ||
        SD->getDeclContext()->getAsProtocolOrProtocolExtensionContext();
      auto diagID = diag::subscript_type_access;
      if (downgradeToWarning == DowngradeToWarning::Yes)
        diagID = diag::subscript_type_access_warn;
+      auto subscriptDeclAccess = isExplicit
+        ? SD->getFormalAccess()
+        : minAccessScope.requiredAccessibilityForDiagnostics();


This seems fishy to me, because it's doing something different than before. Can you explain why it's correct to get the required access instead of the currently-inferred access? (and above as well)

In order to solve this situation.

+subscript (a: PrivateStruct) -> Int { return 0 } // expected-error {{subscript must be declared fileprivate because its index uses a private type}} -subscript (a: PrivateStruct) -> Int { return 0 } // expected-error {{subscript must be declared fileprivate because its index uses a fileprivate type}} +subscript (a: Int) -> PrivateStruct { return PrivateStruct() } // expected-error {{subscript must be declared fileprivate because its element type uses a private type}} -subscript (a: Int) -> PrivateStruct { return PrivateStruct() } // expected-error {{subscript must be declared fileprivate because its element type uses a fileprivate type}}

If the accessibility declaration isn't explicit, minAccessScope was used as the required access scope for subscript declaration, which leads to this incorrect message - {{subscript must be declared private because its index uses a private type}}.

Doing this change I tweaked the parameters for the subscript_type_access diagnostics in such way, so it complained on the inferred accessibility level in explicit case, and suggest required in implicit case:
%select{must be declared %select{...}1 | cannot be declared %select{...}1}0 where 1 parameter depends on 0 parameter

Am I missing something here?

:-/ I guess I'm unhappy because the same diagnostic argument is being used for two different purposes now, which makes me think there should just be two separate diagnostics. We can fix that in a follow-up commit, though.

aleksgapp · 2016-11-09T20:24:37Z

Done!

jrose-apple · 2016-11-09T20:48:40Z

@swift-ci Please test

aleksgapp · 2016-11-10T10:54:37Z

Hmm, something is wrong with OSX test run on CI, can we try again?

jrose-apple · 2016-11-10T17:05:41Z

Ah, yeah, we saw this yesterday on another PR. Strange.

@swift-ci Please test macOS

swift-ci · 2016-11-10T17:07:05Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 3419925
Test requested by - @jrose-apple

jrose-apple · 2016-11-10T17:08:36Z

Okay, now there's a problem on master. Holding off for a bit longer…

jrose-apple · 2016-11-11T18:11:33Z

@swift-ci Please test

jrose-apple · 2016-11-11T19:16:26Z

Merged into master. Thank you, Aleksey!

Merge main 2022-05-20

jrose-apple self-assigned this Aug 31, 2016

jrose-apple reviewed Sep 1, 2016
View reviewed changes

aleksgapp force-pushed the sr-2209-access-scope branch 4 times, most recently from ee987fe to 49271d8 Compare September 7, 2016 09:31

jrose-apple reviewed Sep 7, 2016
View reviewed changes

aleksgapp force-pushed the sr-2209-access-scope branch from 49271d8 to f5a619a Compare September 16, 2016 09:22

aleksgapp force-pushed the sr-2209-access-scope branch from f5a619a to 493808f Compare September 22, 2016 13:55

aleksgapp force-pushed the sr-2209-access-scope branch 2 times, most recently from 7532046 to 1c26dcf Compare November 7, 2016 11:43

jrose-apple reviewed Nov 7, 2016

View reviewed changes

aleksgapp force-pushed the sr-2209-access-scope branch from 1c26dcf to 58dac9d Compare November 9, 2016 15:27

[SR-2209] Fix access scope diagnostics for top-level declarations.

3419925

aleksgapp force-pushed the sr-2209-access-scope branch from 58dac9d to 3419925 Compare November 9, 2016 20:18

jrose-apple merged commit a7b027d into swiftlang:master Nov 11, 2016

aleksgapp deleted the sr-2209-access-scope branch November 15, 2016 13:23

swift-ci mentioned this pull request Sep 1, 2016

[SR-2209] Make a real "AccessScope" type and use it in access checking #44816

Closed

kateinoigakukun added a commit that referenced this pull request Aug 31, 2022

Merge pull request #4579 from swiftwasm/katei/merge-main-2022-05-20

8773be3

Merge main 2022-05-20

[SR-2209] Add real AccessScope type. #4579

[SR-2209] Add real AccessScope type. #4579

Uh oh!

Conversation

aleksgapp commented Aug 31, 2016

Uh oh!

jrose-apple commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Sep 1, 2016

Uh oh!

aleksgapp commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Sep 7, 2016

Uh oh!

aleksgapp commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Nov 3, 2016

Uh oh!

aleksgapp commented Nov 7, 2016

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksgapp Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksgapp commented Nov 9, 2016

Uh oh!

jrose-apple commented Nov 9, 2016

Uh oh!

aleksgapp commented Nov 10, 2016

Uh oh!

jrose-apple commented Nov 10, 2016

Uh oh!

swift-ci commented Nov 10, 2016

Uh oh!

jrose-apple commented Nov 10, 2016

Uh oh!

jrose-apple commented Nov 11, 2016

Uh oh!

jrose-apple commented Nov 11, 2016

Uh oh!

Uh oh!

aleksgapp commented Sep 2, 2016 •

edited

Loading

aleksgapp commented Sep 16, 2016 •

edited

Loading

aleksgapp Nov 8, 2016 •

edited

Loading