Merge pull request swiftlang#10168 from slavapestov/default-arguments-versus-effective-access-v2

Sema: Targeted fix for bad interaction between resilience checks and -enable-testing [v2]

Author: slavapestov

include/swift/AST/Decl.h

@@ -2162,6 +2162,8 @@ class ValueDecl : public Decl {
   /// \see getFormalAccess
   Accessibility getFormalAccessImpl(const DeclContext *useDC) const;
 
+  bool isVersionedInternalDecl() const;
+
   /// Returns the access level specified explicitly by the user, or provided by
   /// default according to language rules.
   ///
@@ -2171,9 +2173,16 @@ class ValueDecl : public Decl {
   /// taken into account.
   ///
   /// \sa getFormalAccessScope
-  Accessibility getFormalAccess(const DeclContext *useDC = nullptr) const {
+  Accessibility getFormalAccess(const DeclContext *useDC = nullptr,
+                                bool respectVersionedAttr = false) const {
     assert(hasAccessibility() && "accessibility not computed yet");
     Accessibility result = TypeAndAccess.getInt().getValue();
+    if (respectVersionedAttr &&
+        result == Accessibility::Internal &&
+        isVersionedInternalDecl()) {
+      assert(!useDC);
+      return Accessibility::Public;
+    }
     if (useDC && (result == Accessibility::Internal ||
                   result == Accessibility::Public))
       return getFormalAccessImpl(useDC);
@@ -2194,17 +2203,15 @@ class ValueDecl : public Decl {
   /// \sa getFormalAccess
   /// \sa isAccessibleFrom
   AccessScope
-  getFormalAccessScope(const DeclContext *useDC = nullptr) const;
+  getFormalAccessScope(const DeclContext *useDC = nullptr,
+                       bool respectVersionedAttr = false) const;
 
   /// Returns the access level that actually controls how a declaration should
   /// be emitted and may be used.
   ///
   /// This is the access used when making optimization and code generation
   /// decisions. It should not be used at the AST or semantic level.
-  ///
-  /// If \p forLinkage is false, we ignore -enable-testing; only @_versioned
-  /// can increase visibility.
-  Accessibility getEffectiveAccess(bool forLinkage = true) const;
+  Accessibility getEffectiveAccess() const;
 
   void setAccessibility(Accessibility access) {
     assert(!hasAccessibility() && "accessibility already set");

lib/AST/Decl.cpp

@@ -1359,7 +1359,8 @@ bool AbstractStorageDecl::hasFixedLayout() const {
 
   // Private and (unversioned) internal variables always have a
   // fixed layout.
-  if (getEffectiveAccess(/*forLinkage=*/false) < Accessibility::Public)
+  if (!getFormalAccessScope(/*useDC=*/nullptr,
+                            /*respectVersionedAttr=*/true).isPublic())
     return true;
 
   // Check for an explicit @_fixed_layout attribute.
@@ -1915,18 +1916,18 @@ SourceLoc ValueDecl::getAttributeInsertionLoc(bool forModifier) const {
 
 /// Returns true if \p VD needs to be treated as publicly-accessible
 /// at the SIL, LLVM, and machine levels due to being versioned.
-static bool isVersionedInternalDecl(const ValueDecl *VD) {
-  assert(VD->getFormalAccess() == Accessibility::Internal);
+bool ValueDecl::isVersionedInternalDecl() const {
+  assert(getFormalAccess() == Accessibility::Internal);
 
-  if (VD->getAttrs().hasAttribute<VersionedAttr>())
+  if (getAttrs().hasAttribute<VersionedAttr>())
     return true;
 
-  if (auto *FD = dyn_cast<FuncDecl>(VD))
+  if (auto *FD = dyn_cast<FuncDecl>(this))
     if (auto *ASD = FD->getAccessorStorageDecl())
       if (ASD->getAttrs().hasAttribute<VersionedAttr>())
         return true;
 
-  if (auto *EED = dyn_cast<EnumElementDecl>(VD))
+  if (auto *EED = dyn_cast<EnumElementDecl>(this))
     if (EED->getParentEnum()->getAttrs().hasAttribute<VersionedAttr>())
       return true;
 
@@ -1952,22 +1953,18 @@ static Accessibility getTestableAccess(const ValueDecl *decl) {
   return Accessibility::Public;
 }
 
-Accessibility ValueDecl::getEffectiveAccess(bool forLinkage) const {
-  Accessibility effectiveAccess = getFormalAccess();
+Accessibility ValueDecl::getEffectiveAccess() const {
+  auto effectiveAccess = getFormalAccess(/*useDC=*/nullptr,
+                                         /*respectVersionedAttr=*/true);
 
   // Handle @testable.
   switch (effectiveAccess) {
-  case Accessibility::Public:
-    if (forLinkage && getModuleContext()->isTestingEnabled())
-      effectiveAccess = getTestableAccess(this);
-    break;
   case Accessibility::Open:
     break;
+  case Accessibility::Public:
   case Accessibility::Internal:
-    if (forLinkage && getModuleContext()->isTestingEnabled())
+    if (getModuleContext()->isTestingEnabled())
       effectiveAccess = getTestableAccess(this);
-    else if (isVersionedInternalDecl(this))
-      effectiveAccess = Accessibility::Public;
     break;
   case Accessibility::FilePrivate:
     break;
@@ -1978,15 +1975,15 @@ Accessibility ValueDecl::getEffectiveAccess(bool forLinkage) const {
 
   if (auto enclosingNominal = dyn_cast<NominalTypeDecl>(getDeclContext())) {
     effectiveAccess = std::min(effectiveAccess,
-                               enclosingNominal->getEffectiveAccess(forLinkage));
+                               enclosingNominal->getEffectiveAccess());
 
   } else if (auto enclosingExt = dyn_cast<ExtensionDecl>(getDeclContext())) {
     // 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()) {
         effectiveAccess = std::min(effectiveAccess,
-                                   nominal->getEffectiveAccess(forLinkage));
+                                   nominal->getEffectiveAccess());
       }
     }
 
@@ -2008,23 +2005,28 @@ Accessibility ValueDecl::getFormalAccessImpl(const DeclContext *useDC) const {
   return getFormalAccess();
 }
 
-AccessScope ValueDecl::getFormalAccessScope(const DeclContext *useDC) const {
+AccessScope ValueDecl::getFormalAccessScope(const DeclContext *useDC,
+                                            bool respectVersionedAttr) const {
   const DeclContext *result = getDeclContext();
-  Accessibility access = getFormalAccess(useDC);
+  Accessibility access = getFormalAccess(useDC, respectVersionedAttr);
 
   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));
+      access = std::min(access,
+                        enclosingNominal->getFormalAccess(useDC,
+                                                          respectVersionedAttr));
 
     } 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));
+          access = std::min(access,
+                            nominal->getFormalAccess(useDC,
+                                                     respectVersionedAttr));
         }
       }
 
@@ -2117,7 +2119,8 @@ int TypeDecl::compare(const TypeDecl *type1, const TypeDecl *type2) {
 bool NominalTypeDecl::hasFixedLayout() const {
   // Private and (unversioned) internal types always have a
   // fixed layout.
-  if (getEffectiveAccess(/*forLinkage=*/false) < Accessibility::Public)
+  if (!getFormalAccessScope(/*useDC=*/nullptr,
+                            /*respectVersionedAttr=*/true).isPublic())
     return true;
 
   // Check for an explicit @_fixed_layout attribute.

lib/AST/DeclContext.cpp

@@ -550,7 +550,8 @@ ResilienceExpansion DeclContext::getResilienceExpansion() const {
 
       // If the function is not externally visible, we will not be serializing
       // its body.
-      if (AFD->getEffectiveAccess(/*forLinkage=*/false) < Accessibility::Public)
+      if (!AFD->getFormalAccessScope(/*useDC=*/nullptr,
+                                     /*respectVersionedAttr=*/true).isPublic())
         break;
 
       // Bodies of public transparent and always-inline functions are

lib/Sema/ResilienceDiagnostics.cpp

@@ -91,7 +91,8 @@ bool TypeChecker::diagnoseInlineableDeclRef(SourceLoc loc,
     return false;
 
   // Public declarations are OK.
-  if (D->getEffectiveAccess(/*forLinkage=*/false) >= Accessibility::Public)
+  if (D->getFormalAccessScope(/*useDC=*/nullptr,
+                              /*respectVersionedAttr=*/true).isPublic())
     return false;
 
   // Enum cases are handled as part of their containing enum.

lib/Sema/TypeCheckAttr.cpp

@@ -1827,11 +1827,13 @@ void AttributeChecker::visitSpecializeAttr(SpecializeAttr *attr) {
 void AttributeChecker::visitFixedLayoutAttr(FixedLayoutAttr *attr) {
   auto *VD = cast<ValueDecl>(D);
 
-  if (VD->getEffectiveAccess(/*forLinkage=*/false) < Accessibility::Public) {
+  auto access = VD->getFormalAccess(/*useDC=*/nullptr,
+                                    /*respectVersionedAttr=*/true);
+  if (access < Accessibility::Public) {
     TC.diagnose(attr->getLocation(),
                 diag::fixed_layout_attr_on_internal_type,
                 VD->getBaseName(),
-                VD->getFormalAccessScope().accessibilityForDiagnostics())
+                access)
         .fixItRemove(attr->getRangeWithAt());
     attr->setInvalid();
   }
@@ -1881,11 +1883,13 @@ void AttributeChecker::visitInlineableAttr(InlineableAttr *attr) {
 
   // @_inlineable can only be applied to public or @_versioned
   // declarations.
-  if (VD->getEffectiveAccess(/*forLinkage=*/false) < Accessibility::Public) {
+  auto access = VD->getFormalAccess(/*useDC=*/nullptr,
+                                    /*respectVersionedAttr=*/true);
+  if (access < Accessibility::Public) {
     TC.diagnose(attr->getLocation(),
                 diag::inlineable_decl_not_public,
                 VD->getBaseName(),
-                VD->getFormalAccessScope().accessibilityForDiagnostics())
+                access)
         .fixItRemove(attr->getRangeWithAt());
     attr->setInvalid();
     return;

lib/Sema/TypeCheckStmt.cpp

@@ -1321,7 +1321,8 @@ static void checkDefaultArguments(TypeChecker &tc,
   // caller.
   auto expansion = func->getResilienceExpansion();
   if (!tc.Context.isSwiftVersion3() &&
-      func->getEffectiveAccess(/*forLinkage=*/false) == Accessibility::Public)
+      func->getFormalAccessScope(/*useDC=*/nullptr,
+                                 /*respectVersionedAttr=*/true).isPublic())
     expansion = ResilienceExpansion::Minimal;
 
   for (auto &param : *params) {

test/attr/attr_fixed_layout.swift

@@ -56,7 +56,6 @@ struct Rectangle {
 // expected-error@-1 {{'@_fixed_layout' attribute can only be applied to '@_versioned' or public declarations, but 'InternalStruct' is internal}}
 
   @_fixed_layout public struct NestedStruct {}
-  // expected-error@-1 {{'@_fixed_layout' attribute can only be applied to '@_versioned' or public declarations, but 'NestedStruct' is internal}}
 }
 
 @_fixed_layout fileprivate struct FileprivateStruct {}