[Gardening] SE-0169 PR Feedback

CHANGELOG.md

@@ -20,6 +20,9 @@ CHANGELOG
 
 Swift 4.0
 ---------
+* [SE-0169](https://github.com/apple/swift-evolution/blob/master/proposals/0169-improve-interaction-between-private-declarations-and-extensions.md):
+  In Swift 4 mode, private scope is shared between the declaration and all 
+  extensions of a specific type inside the same file.
 
 * [SE-0138](https://github.com/apple/swift-evolution/blob/master/proposals/0138-unsaferawbufferpointer.md#amendment-to-normalize-the-slice-type):
 

docs/AccessControl.rst

@@ -26,7 +26,8 @@ project--while not accidentally revealing entities to clients of a framework
 module.
 
 .. warning:: This document has not yet been updated for SE-0117, which adds the
-  "open" level of access.
+  "open" level of access, or SE-0169 which shares the private scope between
+  extensions and the declaration inside the same file.
 
 
 .. contents:: :local:

include/swift/AST/AccessScope.h

@@ -23,18 +23,14 @@ namespace swift {
 /// a particular declaration can be accessed.
 class AccessScope {
   /// The declaration context (if not public) along with a bit saying
-  /// whether this scope is private (or not).
+  /// whether this scope is private (or not). This bit is used to distinguish
+  /// file-level private and fileprivate.
   llvm::PointerIntPair<const DeclContext *, 1, bool> Value;
 public:
   AccessScope(const DeclContext *DC, bool isPrivate = false);
 
   static AccessScope getPublic() { return AccessScope(nullptr); }
 
-  /// Check if private access is allowed. This is a lexical scope check in Swift
-  /// 3 mode. In Swift 4 mode, declarations and extensions of the same type will
-  /// also allow access.
-  static bool allowsPrivateAccess(const DeclContext *useDC, const DeclContext *sourceDC);
-
   /// Returns nullptr if access scope is public.
   const DeclContext *getDeclContext() const { return Value.getPointer(); }
 
@@ -49,16 +45,17 @@ class AccessScope {
   bool isFileScope() const;
 
   /// Returns true if this is a child scope of the specified other access scope.
-  ///
-  /// \see DeclContext::isChildContextOf
   bool isChildOf(AccessScope AS) const {
     if (!isPublic() && !AS.isPublic())
-      return allowsPrivateAccess(getDeclContext(), AS.getDeclContext());
+      return getDeclContext()->isAccessibleChildOf(AS.getDeclContext());
     if (isPublic() && AS.isPublic())
       return false;
     return AS.isPublic();
   }
 
+  /// Check if the decl context is equal to or an accessible child of the DC.
+  bool isAccessibleFrom(const DeclContext *DC) const;
+
   /// Returns the associated access level for diagnostic purposes.
   Accessibility accessibilityForDiagnostics() const;
 

include/swift/AST/Decl.h

@@ -2154,38 +2154,23 @@ class ValueDecl : public Decl {
     return TypeAndAccess.getInt().hasValue();
   }
 
-  /// \see getFormalAccess
-  Accessibility getFormalAccessImpl(const DeclContext *useDC) const;
-
   /// Returns the access level specified explicitly by the user, or provided by
   /// default according to language rules.
   ///
-  /// This is the access used when calculating if access control is being used
-  /// consistently. If \p useDC is provided (the location where the value is
-  /// being used), features that affect formal access such as \c \@testable are
-  /// taken into account.
+  /// If \p useDC is provided (the location where the value is being used), 
+  /// features that affect formal access such as \c \@testable are taken into 
+  /// account.
   ///
   /// \sa getFormalAccessScope
-  Accessibility getFormalAccess(const DeclContext *useDC = nullptr) const {
-    assert(hasAccessibility() && "accessibility not computed yet");
-    Accessibility result = TypeAndAccess.getInt().getValue();
-    if (useDC && (result == Accessibility::Internal ||
-                  result == Accessibility::Public))
-      return getFormalAccessImpl(useDC);
-    return result;
-  }
+  /// \sa isAccessibleFrom
+  Accessibility getFormalAccess(const DeclContext *useDC = nullptr) const;
 
-  /// Returns the outermost DeclContext from which this declaration can be
-  /// accessed, or null if the declaration is public.
+  /// Returns the AccessScope from which this declaration can be accessed.
   ///
-  /// This is used when calculating if access control is being used
-  /// consistently. If \p useDC is provided (the location where the value is
-  /// being used), features that affect formal access such as \c \@testable are
+  /// If \p useDC is provided (the location where the value is being used), 
+  /// features that affect formal access such as \c \@testable are
   /// taken into account.
   ///
-  /// \invariant
-  /// <code>value.isAccessibleFrom(value.getFormalAccessScope())</code>
-  ///
   /// \sa getFormalAccess
   /// \sa isAccessibleFrom
   AccessScope
@@ -2211,14 +2196,6 @@ class ValueDecl : public Decl {
 
   /// Returns true if this declaration is accessible from the given context.
   ///
-  /// A private declaration is accessible from any DeclContext within the same
-  /// source file.
-  ///
-  /// An internal declaration is accessible from any DeclContext within the same
-  /// module.
-  ///
-  /// A public declaration is accessible everywhere.
-  ///
   /// If \p DC is null, returns true only if this declaration is public.
   bool isAccessibleFrom(const DeclContext *DC) const;
 
@@ -4131,6 +4108,18 @@ class AbstractStorageDecl : public ValueDecl {
 
   void overwriteSetterAccessibility(Accessibility accessLevel);
 
+  /// Returns the access level for the setter specified explicitly by the user,
+  /// or provided by default according to language rules.
+  ///
+  /// If \p useDC is provided (the location where the value is being used),
+  /// features that affect formal access such as \c \@testable are taken into
+  /// account.
+  ///
+  /// \sa getFormalAccessScope
+  /// \sa isAccessibleFrom
+  /// \sa isSetterAccessibleFrom
+  Accessibility getFormalSetterAccess(const DeclContext *DC) const;
+
   /// \brief Retrieve the materializeForSet function, if this
   /// declaration has one.
   FuncDecl *getMaterializeForSetFunc() const {

include/swift/AST/DeclContext.h

@@ -19,6 +19,7 @@
 #ifndef SWIFT_DECLCONTEXT_H
 #define SWIFT_DECLCONTEXT_H
 
+#include "swift/AST/AttrKind.h"
 #include "swift/AST/Identifier.h"
 #include "swift/AST/LookupKinds.h"
 #include "swift/AST/ResilienceExpansion.h"
@@ -35,6 +36,7 @@ namespace llvm {
 
 namespace swift {
   class AbstractFunctionDecl;
+  class AccessScope;
   class GenericEnvironment;
   class ASTContext;
   class ASTWalker;
@@ -374,6 +376,27 @@ class alignas(1 << DeclContextAlignInBits) DeclContext {
     return false;
   }
 
+  /// Return true if this context is an accessible child of the specified
+  /// \p Other decl context. This will perform an isChildContextOf check, and
+  /// when not in Swift 3 mode, the private scope is extended to include
+  /// declarations and extensions in the same file.
+  ///
+  /// \sa isChildContextOf
+  bool isAccessibleChildOf(const DeclContext *Other) const;
+
+  /// Returns the AccessScope from which the first declaration can be accessed.
+  ///
+  /// If \p useDC is provided (the location where the value is being used),
+  /// features that affect formal access such as \c \@testable are
+  /// taken into account.
+  ///
+  /// \p accessibility specifies the starting access level.
+  /// \p useNominalTypeAccessibility specifies if the nominal type associated
+  /// with extensions should alter the access scope.
+  AccessScope getAccessScope(const DeclContext *useDC,
+                             Accessibility accessibility,
+                             bool useNominalTypeAccessibility = true) const;
+
   /// Returns the module context that contains this context.
   ModuleDecl *getParentModule() const;
 

lib/AST/Decl.cpp

@@ -1952,57 +1952,31 @@ Accessibility ValueDecl::getEffectiveAccess() const {
   return effectiveAccess;
 }
 
-Accessibility ValueDecl::getFormalAccessImpl(const DeclContext *useDC) const {
-  assert((getFormalAccess() == Accessibility::Internal ||
-          getFormalAccess() == Accessibility::Public) &&
-         "should be able to fast-path non-internal cases");
-  assert(useDC && "should fast-path non-scoped cases");
-  if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
-    if (useSF->hasTestableImport(getModuleContext()))
-      return getTestableAccess(this);
-  return getFormalAccess();
+Accessibility ValueDecl::getFormalAccess(const DeclContext *useDC) const {
+  assert(hasAccessibility() && "accessibility not computed yet");
+  Accessibility result = TypeAndAccess.getInt().getValue();
+  if (useDC && (result == Accessibility::Internal ||
+                result == Accessibility::Public))
+    if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
+      if (useSF->hasTestableImport(getModuleContext()))
+        return getTestableAccess(this);
+  return result;
 }
 
-AccessScope ValueDecl::getFormalAccessScope(const DeclContext *useDC) const {
-  const DeclContext *result = getDeclContext();
-  Accessibility access = getFormalAccess(useDC);
-
-  while (!result->isModuleScopeContext()) {
-    if (result->isLocalContext() || access == Accessibility::Private)
-      return AccessScope(result, true);
-
-    if (auto enclosingNominal = dyn_cast<NominalTypeDecl>(result)) {
-      access = std::min(access, enclosingNominal->getFormalAccess(useDC));
-
-    } else if (auto enclosingExt = dyn_cast<ExtensionDecl>(result)) {
-      // Just check the base type. If it's a constrained extension, Sema should
-      // have already enforced access more strictly.
-      if (auto extendedTy = enclosingExt->getExtendedType()) {
-        if (auto nominal = extendedTy->getAnyNominal()) {
-          access = std::min(access, nominal->getFormalAccess(useDC));
-        }
-      }
-
-    } else {
-      llvm_unreachable("unknown DeclContext kind");
-    }
-
-    result = result->getParent();
-  }
-
-  switch (access) {
-  case Accessibility::Private:
-  case Accessibility::FilePrivate:
-    assert(result->isModuleScopeContext());
-    return AccessScope(result, access == Accessibility::Private);
-  case Accessibility::Internal:
-    return AccessScope(result->getParentModule());
-  case Accessibility::Public:
-  case Accessibility::Open:
-    return AccessScope::getPublic();
-  }
+Accessibility AbstractStorageDecl::getFormalSetterAccess(const DeclContext *useDC) const {
+  assert(hasAccessibility());
+  assert(GetSetInfo.getInt().hasValue());
+  Accessibility result = GetSetInfo.getInt().getValue();
+  if (useDC && (result == Accessibility::Internal ||
+                result == Accessibility::Public))
+    if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
+      if (useSF->hasTestableImport(getModuleContext()))
+        return getTestableAccess(this);
+  return result;
+}
 
-  llvm_unreachable("unknown accessibility level");
+AccessScope ValueDecl::getFormalAccessScope(const DeclContext *useDC) const {
+  return getDeclContext()->getAccessScope(useDC, getFormalAccess(useDC));
 }
 
 Type TypeDecl::getDeclaredInterfaceType() const {