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

The -enable-testing flag makes ValueDecl::getEffectiveAccess()
say that internal declarations are public.

This would lead us to emit spurious diagnostics if a default
argument of an internal function referenced a private symbol,
for example, which is something we actually want to allow.

This is a second revision of the patch -- instead of changing
getEffectiveAccess() to take an extra parameter, this changes
getFormalAccessScope() instead.

Fixes <rdar://problem/32592973>.

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,7 +2203,8 @@ 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.

lib/AST/Decl.cpp

@@ -1359,7 +1359,8 @@ bool AbstractStorageDecl::hasFixedLayout() const {
 
   // Private and (unversioned) internal variables always have a
   // fixed layout.
-  if (getEffectiveAccess() < 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;
 
@@ -1953,21 +1954,17 @@ static Accessibility getTestableAccess(const ValueDecl *decl) {
 }
 
 Accessibility ValueDecl::getEffectiveAccess() const {
-  Accessibility effectiveAccess = getFormalAccess();
+  auto effectiveAccess = getFormalAccess(/*useDC=*/nullptr,
+                                         /*respectVersionedAttr=*/true);
 
   // Handle @testable.
   switch (effectiveAccess) {
-  case Accessibility::Public:
-    if (getModuleContext()->isTestingEnabled())
-      effectiveAccess = getTestableAccess(this);
-    break;
   case Accessibility::Open:
     break;
+  case Accessibility::Public:
   case Accessibility::Internal:
     if (getModuleContext()->isTestingEnabled())
       effectiveAccess = getTestableAccess(this);
-    else if (isVersionedInternalDecl(this))
-      effectiveAccess = Accessibility::Public;
     break;
   case Accessibility::FilePrivate:
     break;
@@ -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() < 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() < 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() >= Accessibility::Public)
+  if (D->getFormalAccessScope(/*useDC=*/nullptr,
+                              /*respectVersionedAttr=*/true).isPublic())
     return false;
 
   // Enum cases are handled as part of their containing enum.
@@ -110,7 +111,8 @@ bool TypeChecker::diagnoseInlineableDeclRef(SourceLoc loc,
 
   diagnose(loc, diag::resilience_decl_unavailable,
            D->getDescriptiveKind(), D->getFullName(),
-           D->getFormalAccess(), getFragileFunctionKind(DC));
+           D->getFormalAccessScope().accessibilityForDiagnostics(),
+           getFragileFunctionKind(DC));
   diagnose(D, diag::resilience_decl_declared_here,
            D->getDescriptiveKind(), D->getFullName());
   return true;

lib/Sema/TypeCheckAttr.cpp

@@ -1824,19 +1824,16 @@ void AttributeChecker::visitSpecializeAttr(SpecializeAttr *attr) {
                                         /*allowConcreteGenericParams=*/true);
 }
 
-static Accessibility getAccessForDiagnostics(const ValueDecl *D) {
-  return std::min(D->getFormalAccess(),
-                  D->getEffectiveAccess());
-}
-
 void AttributeChecker::visitFixedLayoutAttr(FixedLayoutAttr *attr) {
   auto *VD = cast<ValueDecl>(D);
 
-  if (VD->getEffectiveAccess() < 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(),
-                getAccessForDiagnostics(VD))
+                access)
         .fixItRemove(attr->getRangeWithAt());
     attr->setInvalid();
   }
@@ -1886,13 +1883,13 @@ void AttributeChecker::visitInlineableAttr(InlineableAttr *attr) {
 
   // @_inlineable can only be applied to public or @_versioned
   // declarations.
-  if (VD->getFormalAccess() < Accessibility::Internal ||
-      (!VD->getAttrs().hasAttribute<VersionedAttr>() &&
-       VD->getFormalAccess() < 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(),
-                getAccessForDiagnostics(VD))
+                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() == Accessibility::Public)
+      func->getFormalAccessScope(/*useDC=*/nullptr,
+                                 /*respectVersionedAttr=*/true).isPublic())
     expansion = ResilienceExpansion::Minimal;
 
   for (auto &param : *params) {

test/attr/attr_fixed_layout.swift

@@ -1,5 +1,7 @@
 // RUN: %target-swift-frontend -typecheck -verify -dump-ast -enable-resilience %s 2>&1 | %FileCheck --check-prefix=RESILIENCE-ON %s
+// RUN: %target-swift-frontend -typecheck -verify -dump-ast -enable-resilience -enable-testing %s 2>&1 | %FileCheck --check-prefix=RESILIENCE-ON %s
 // RUN: %target-swift-frontend -typecheck -verify -dump-ast %s 2>&1 | %FileCheck --check-prefix=RESILIENCE-OFF %s
+// RUN: %target-swift-frontend -typecheck -verify -dump-ast %s -enable-testing 2>&1 | %FileCheck --check-prefix=RESILIENCE-OFF %s
 
 //
 // Public types with @_fixed_layout are always fixed layout
@@ -54,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 {}

test/attr/attr_inlineable.swift

@@ -1,4 +1,5 @@
 // RUN: %target-typecheck-verify-swift -swift-version 4
+// RUN: %target-typecheck-verify-swift -swift-version 4 -enable-testing
 
 @_inlineable struct TestInlineableStruct {}
 // expected-error@-1 {{@_inlineable cannot be applied to this declaration}}

test/attr/attr_versioned.swift

@@ -1,4 +1,5 @@
 // RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -enable-testing
 
 @_versioned private func privateVersioned() {}
 // expected-error@-1 {{'@_versioned' attribute can only be applied to internal declarations, but 'privateVersioned' is private}}