Take away some of ValueDecl::getFormalAccess's capabilities

...to push people towards getFormalAccessScope. The one use case that
isn't covered by that is checking whether a declaration behaves as
'open' in the current file; I've added ValueDecl::hasOpenAccess to
handle that specific case.

No intended functionality change.

Author: jrose-apple

include/swift/AST/Decl.h

@@ -2347,19 +2347,14 @@ class ValueDecl : public Decl {
   /// 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 treatUsableFromInlineAsPublic is true, declarations marked with the
-  /// \c @usableFromInline attribute are treated as public. This is normally
-  /// false for name lookup and other source language concerns, but true when
-  /// computing the linkage of generated functions.
+  /// Most of the time this is not the interesting value to check; access is
+  /// limited by enclosing scopes per SE-0025. Use #getFormalAccessScope to
+  /// check if access control is being used consistently, and to take features
+  /// such as \c \@testable and \c \@usableFromInline into account.
   ///
   /// \sa getFormalAccessScope
-  AccessLevel getFormalAccess(const DeclContext *useDC = nullptr,
-                              bool treatUsableFromInlineAsPublic = false) const;
+  /// \sa hasOpenAccess
+  AccessLevel getFormalAccess() const;
 
   /// If this declaration is a member of a protocol extension, return the
   /// minimum of the given access level and the protocol's access level.
@@ -2373,7 +2368,8 @@ class ValueDecl : public Decl {
   /// visible.
   AccessLevel adjustAccessLevelForProtocolExtension(AccessLevel access) const;
 
-  /// Determine whether this Decl has either Private or FilePrivate access.
+  /// Determine whether this Decl has either Private or FilePrivate access,
+  /// and its DeclContext does not.
   bool isOutermostPrivateOrFilePrivateScope() const;
 
   /// Returns the outermost DeclContext from which this declaration can be
@@ -2395,6 +2391,7 @@ class ValueDecl : public Decl {
   ///
   /// \sa getFormalAccess
   /// \sa isAccessibleFrom
+  /// \sa hasOpenAccess
   AccessScope
   getFormalAccessScope(const DeclContext *useDC = nullptr,
                        bool treatUsableFromInlineAsPublic = false) const;
@@ -2448,6 +2445,14 @@ class ValueDecl : public Decl {
   bool isAccessibleFrom(const DeclContext *DC,
                         bool forConformance = false) const;
 
+  /// Returns whether this declaration should be treated as \c open from
+  /// \p useDC. This is very similar to #getFormalAccess, but takes
+  /// \c \@testable into account.
+  ///
+  /// This is mostly only useful when considering requirements on an override:
+  /// if the base declaration is \c open, the override might have to be too.
+  bool hasOpenAccess(const DeclContext *useDC) const;
+
   /// Retrieve the "interface" type of this value, which uses
   /// GenericTypeParamType if the declaration is generic. For a generic
   /// function, this will have a GenericFunctionType with a

lib/AST/Decl.cpp

@@ -2280,10 +2280,30 @@ static AccessLevel getTestableAccess(const ValueDecl *decl) {
   return AccessLevel::Public;
 }
 
+/// Like ValueDecl::getFormalAccess, but takes into account the parameters that
+/// ValueDecl::getFormalAccessScope is aware of.
+static AccessLevel
+getAdjustedFormalAccess(const ValueDecl *VD, const DeclContext *useDC,
+                        bool treatUsableFromInlineAsPublic) {
+  AccessLevel result = VD->getFormalAccess();
+  if (treatUsableFromInlineAsPublic &&
+      result == AccessLevel::Internal &&
+      VD->isUsableFromInline()) {
+    return AccessLevel::Public;
+  }
+  if (useDC && (result == AccessLevel::Internal ||
+                result == AccessLevel::Public)) {
+    if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
+      if (useSF->hasTestableImport(VD->getModuleContext()))
+        return getTestableAccess(VD);
+  }
+  return result;
+}
+
 AccessLevel ValueDecl::getEffectiveAccess() const {
   auto effectiveAccess =
-    getFormalAccess(/*useDC=*/nullptr,
-                    /*treatUsableFromInlineAsPublic=*/true);
+    getAdjustedFormalAccess(this, /*useDC=*/nullptr,
+                            /*treatUsableFromInlineAsPublic=*/true);
 
   // Handle @testable.
   switch (effectiveAccess) {
@@ -2335,38 +2355,36 @@ AccessLevel ValueDecl::getEffectiveAccess() const {
   return effectiveAccess;
 }
 
-AccessLevel ValueDecl::getFormalAccess(const DeclContext *useDC,
-                                       bool treatUsableFromInlineAsPublic) const {
+AccessLevel ValueDecl::getFormalAccess() const {
   ASTContext &ctx = getASTContext();
-  AccessLevel result = ctx.evaluator(AccessLevelRequest{const_cast<ValueDecl *>(this)});
-  if (treatUsableFromInlineAsPublic &&
-      result == AccessLevel::Internal &&
-      isUsableFromInline()) {
-    return AccessLevel::Public;
-  }
-  if (useDC && (result == AccessLevel::Internal ||
-                result == AccessLevel::Public)) {
-    if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
-      if (useSF->hasTestableImport(getModuleContext()))
-        return getTestableAccess(this);
-  }
-  return result;
+  return ctx.evaluator(AccessLevelRequest{const_cast<ValueDecl *>(this)});
+}
+
+bool ValueDecl::hasOpenAccess(const DeclContext *useDC) const {
+  assert(isa<ClassDecl>(this) || isa<ConstructorDecl>(this) ||
+         isPotentiallyOverridable());
+
+  AccessLevel access =
+      getAdjustedFormalAccess(this, useDC,
+                              /*treatUsableFromInlineAsPublic*/false);
+  return access == AccessLevel::Open;
 }
 
 AccessScope
 ValueDecl::getFormalAccessScope(const DeclContext *useDC,
                                 bool treatUsableFromInlineAsPublic) const {
   const DeclContext *result = getDeclContext();
-  AccessLevel access = getFormalAccess(useDC, treatUsableFromInlineAsPublic);
+  AccessLevel access = getAdjustedFormalAccess(this, useDC,
+                                               treatUsableFromInlineAsPublic);
 
   while (!result->isModuleScopeContext()) {
     if (result->isLocalContext() || access == AccessLevel::Private)
       return AccessScope(result, true);
 
     if (auto enclosingNominal = dyn_cast<NominalTypeDecl>(result)) {
       auto enclosingAccess =
-        enclosingNominal->getFormalAccess(useDC,
-                                          treatUsableFromInlineAsPublic);
+        getAdjustedFormalAccess(enclosingNominal, useDC,
+                                treatUsableFromInlineAsPublic);
       access = std::min(access, enclosingAccess);
 
     } else if (auto enclosingExt = dyn_cast<ExtensionDecl>(result)) {
@@ -2375,8 +2393,8 @@ ValueDecl::getFormalAccessScope(const DeclContext *useDC,
       if (auto extendedTy = enclosingExt->getExtendedType()) {
         if (auto nominal = extendedTy->getAnyNominal()) {
           auto nominalAccess =
-            nominal->getFormalAccess(useDC,
-                                     treatUsableFromInlineAsPublic);
+            getAdjustedFormalAccess(nominal, useDC,
+                                    treatUsableFromInlineAsPublic);
           access = std::min(access, nominalAccess);
         }
       }

lib/AST/NameLookup.cpp

@@ -1529,11 +1529,12 @@ ValueDecl::adjustAccessLevelForProtocolExtension(AccessLevel access) const {
       // and expects these extension methods to witness public protocol
       // requirements. Which works at the ABI level, so let's keep
       // supporting that here by passing 'isUsageFromInline'.
-      auto protoAccess = protocol->getFormalAccess(/*useDC=*/nullptr,
-                                                   /*isUsageFromInline=*/true);
-      if (protoAccess == AccessLevel::Private)
-        protoAccess = AccessLevel::FilePrivate;
-      access = std::min(access, protoAccess);
+      auto protoAccess =
+          protocol->getFormalAccessScope(/*useDC=*/nullptr,
+                                         /*isUsageFromInline=*/true);
+      // FIXME: The calling code should be written in terms of AccessScope,
+      // so that this can just use AccessScope::intersectWith.
+      access = std::min(access, protoAccess.requiredAccessForDiagnostics());
     }
   }
 

lib/Sema/CodeSynthesis.cpp

@@ -2341,10 +2341,12 @@ configureGenericDesignatedInitOverride(ASTContext &ctx,
   return std::make_tuple(genericSig, genericEnv, genericParams, subMap);
 }
 
-static void configureDesignatedInitAttributes(TypeChecker &tc,
-                                              ClassDecl *classDecl,
-                                              ConstructorDecl *ctor,
-                                              ConstructorDecl *superclassCtor) {
+static void
+configureInheritedDesignatedInitAttributes(TypeChecker &tc,
+                                           ClassDecl *classDecl,
+                                           ConstructorDecl *ctor,
+                                           ConstructorDecl *superclassCtor) {
+  assert(ctor->getDeclContext() == classDecl);
   auto &ctx = tc.Context;
 
   AccessLevel access = classDecl->getFormalAccess();
@@ -2353,24 +2355,18 @@ static void configureDesignatedInitAttributes(TypeChecker &tc,
 
   ctor->setAccess(access);
 
-  // Inherit the @inlinable attribute.
-  if (superclassCtor->getFormalAccess(/*useDC=*/nullptr,
-                                      /*treatUsableFromInlineAsPublic=*/true)
-      >= AccessLevel::Public) {
+  AccessScope superclassInliningAccessScope =
+      superclassCtor->getFormalAccessScope(/*useDC*/nullptr,
+                                           /*usableFromInlineAsPublic=*/true);
+
+  if (superclassInliningAccessScope.isPublic()) {
     if (superclassCtor->getAttrs().hasAttribute<InlinableAttr>()) {
+      // Inherit the @inlinable attribute.
       auto *clonedAttr = new (ctx) InlinableAttr(/*implicit=*/true);
       ctor->getAttrs().add(clonedAttr);
-    }
-  }
 
-  // Inherit the @usableFromInline attribute. We need better abstractions
-  // for dealing with @usableFromInline.
-  if (superclassCtor->getFormalAccess(/*useDC=*/nullptr,
-                                      /*treatUsableFromInlineAsPublic=*/true)
-        >= AccessLevel::Public) {
-    if (access == AccessLevel::Internal &&
-        !superclassCtor->isDynamic() &&
-        !ctor->getAttrs().hasAttribute<InlinableAttr>()) {
+    } else if (access == AccessLevel::Internal && !superclassCtor->isDynamic()){
+      // Inherit the @usableFromInline attribute.
       auto *clonedAttr = new (ctx) UsableFromInlineAttr(/*implicit=*/true);
       ctor->getAttrs().add(clonedAttr);
     }
@@ -2482,8 +2478,8 @@ swift::createDesignatedInitOverride(TypeChecker &tc,
   tc.configureInterfaceType(ctor, genericSig);
   ctor->setValidationToChecked();
 
-  configureDesignatedInitAttributes(tc, classDecl,
-                                    ctor, superclassCtor);
+  configureInheritedDesignatedInitAttributes(tc, classDecl, ctor,
+                                             superclassCtor);
 
   if (kind == DesignatedInitKind::Stub) {
     // Make this a stub implementation.

lib/Sema/TypeCheckAttr.cpp

@@ -1934,11 +1934,10 @@ void AttributeChecker::visitSpecializeAttr(SpecializeAttr *attr) {
 void AttributeChecker::visitFixedLayoutAttr(FixedLayoutAttr *attr) {
   auto *VD = cast<ValueDecl>(D);
 
-  auto access = VD->getFormalAccess(/*useDC=*/nullptr,
-                                    /*treatUsableFromInlineAsPublic=*/true);
-  if (access < AccessLevel::Public) {
+  if (VD->getFormalAccess() < AccessLevel::Public &&
+      !VD->getAttrs().hasAttribute<UsableFromInlineAttr>()) {
     diagnoseAndRemoveAttr(attr, diag::fixed_layout_attr_on_internal_type,
-                          VD->getFullName(), access);
+                          VD->getFullName(), VD->getFormalAccess());
   }
 }
 
@@ -2093,9 +2092,8 @@ void AttributeChecker::visitFrozenAttr(FrozenAttr *attr) {
     break;
   }
 
-  auto access = ED->getFormalAccess(/*useDC=*/nullptr,
-                                    /*treatUsableFromInlineAsPublic=*/true);
-  if (access < AccessLevel::Public) {
+  if (ED->getFormalAccess() < AccessLevel::Public &&
+      !ED->getAttrs().hasAttribute<UsableFromInlineAttr>()) {
     diagnoseAndRemoveAttr(attr, diag::enum_frozen_nonpublic, attr);
   }
 }

lib/Sema/TypeCheckDecl.cpp

@@ -2957,8 +2957,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
       // already complained about the class being inherently
       // un-subclassable.
       if (!isInvalidSuperclass &&
-          Super->getFormalAccess(CD->getDeclContext())
-            < AccessLevel::Open &&
+          !Super->hasOpenAccess(CD->getDeclContext()) &&
           Super->getModuleContext() != CD->getModuleContext()) {
         TC.diagnose(CD, diag::superclass_not_open, superclassTy);
         isInvalidSuperclass = true;

lib/Sema/TypeCheckDeclOverride.cpp

@@ -794,17 +794,16 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
   // defining module.  This is not required for constructors, which are
   // never really "overridden" in the intended sense here, because of
   // course derived classes will change how the class is initialized.
-  AccessLevel matchAccess = baseDecl->getFormalAccess(dc);
+  bool baseHasOpenAccess = baseDecl->hasOpenAccess(dc);
   if (!isAccessor &&
-      matchAccess < AccessLevel::Open &&
+      !baseHasOpenAccess &&
       baseDecl->getModuleContext() != decl->getModuleContext() &&
       !isa<ConstructorDecl>(decl)) {
     diags.diagnose(decl, diag::override_of_non_open,
                    decl->getDescriptiveKind());
 
-  } else if (matchAccess == AccessLevel::Open &&
-             classDecl->getFormalAccess(dc) ==
-               AccessLevel::Open &&
+  } else if (baseHasOpenAccess &&
+             classDecl->hasOpenAccess(dc) &&
              decl->getFormalAccess() != AccessLevel::Open &&
              !decl->isFinal()) {
     {
@@ -847,7 +846,7 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
     if (shouldDiagnose || shouldDiagnoseSetter) {
       bool overriddenForcesAccess =
         (requiredAccessScope->hasEqualDeclContextWith(matchAccessScope) &&
-         matchAccess != AccessLevel::Open);
+         !baseHasOpenAccess);
       AccessLevel requiredAccess =
         requiredAccessScope->requiredAccessForDiagnostics();
       {

test/SILGen/inlinable_attribute.swift

@@ -1,5 +1,4 @@
-
-// RUN: %target-swift-emit-silgen -module-name inlinable_attribute -enable-sil-ownership -emit-verbose-sil %s | %FileCheck %s
+// RUN: %target-swift-emit-silgen -module-name inlinable_attribute -enable-sil-ownership -emit-verbose-sil -warnings-as-errors %s | %FileCheck %s
 
 // CHECK-LABEL: sil [serialized] @$S19inlinable_attribute15fragileFunctionyyF : $@convention(thin) () -> ()
 @inlinable public func fragileFunction() {
@@ -79,23 +78,54 @@ public class Horse {
   _ = h.gallop
 }
 
+@_fixed_layout
+public class PublicBase {
+  @inlinable
+  public init(horse: Horse) {}
+}
+
 @usableFromInline
 @_fixed_layout
-class Base {
+class UFIBase {
   @usableFromInline
   @inlinable
   init(horse: Horse) {}
 }
 
-// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute7DerivedCfd : $@convention(method) (@guaranteed Derived) -> @owned Builtin.NativeObject
-// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute7DerivedCfD : $@convention(method) (@owned Derived) -> ()
+// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute017PublicDerivedFromC0Cfd : $@convention(method) (@guaranteed PublicDerivedFromPublic) -> @owned Builtin.NativeObject
+// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute017PublicDerivedFromC0CfD : $@convention(method) (@owned PublicDerivedFromPublic) -> ()
 
 // Make sure the synthesized delegating initializer is inlinable also
 
-// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute7DerivedC5horseAcA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned Derived) -> @owned Derived
+// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute017PublicDerivedFromC0C5horseAcA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned PublicDerivedFromPublic) -> @owned PublicDerivedFromPublic
+@_fixed_layout
+public class PublicDerivedFromPublic : PublicBase {
+  // Allow @inlinable deinits
+  @inlinable deinit {}
+}
+
+// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute20UFIDerivedFromPublicC5horseAcA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned UFIDerivedFromPublic) -> @owned UFIDerivedFromPublic
+@usableFromInline
+@_fixed_layout
+class UFIDerivedFromPublic : PublicBase {
+}
+
+// CHECK-LABEL: sil [serialized] @$S19inlinable_attribute17UFIDerivedFromUFIC5horseAcA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned UFIDerivedFromUFI) -> @owned UFIDerivedFromUFI
 @usableFromInline
 @_fixed_layout
-class Derived : Base {
+class UFIDerivedFromUFI : UFIBase {
   // Allow @inlinable deinits
   @inlinable deinit {}
 }
+
+// CHECK-LABEL: sil hidden @$S19inlinable_attribute25InternalDerivedFromPublicC5horseAcA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned InternalDerivedFromPublic) -> @owned InternalDerivedFromPublic
+class InternalDerivedFromPublic : PublicBase {}
+
+// CHECK-LABEL: sil hidden @$S19inlinable_attribute22InternalDerivedFromUFIC5horseAcA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned InternalDerivedFromUFI) -> @owned InternalDerivedFromUFI
+class InternalDerivedFromUFI : UFIBase {}
+
+// CHECK-LABEL: sil private @$S19inlinable_attribute24PrivateDerivedFromPublic{{.+}}LLC5horseAdA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned PrivateDerivedFromPublic) -> @owned PrivateDerivedFromPublic
+private class PrivateDerivedFromPublic : PublicBase {}
+
+// CHECK-LABEL: sil private @$S19inlinable_attribute21PrivateDerivedFromUFI{{.+}}LLC5horseAdA5HorseC_tcfc : $@convention(method) (@owned Horse, @owned PrivateDerivedFromUFI) -> @owned PrivateDerivedFromUFI
+private class PrivateDerivedFromUFI : UFIBase {}