SE-0025: Allow public members inside internal types. (#3404)

jrose-apple · web-flow · commit 306eddab2680 · 2016-07-11T14:28:23.000-07:00
(and any other member with higher access control than its enclosing type) There's no effect, but it is now considered legal and the compiler will no longer warn about it. This allows an API author to prototype their API with proper access levels and still limit the top-level type. If the new getEffectiveAccess computation turns out to be expensive, we can cache the result. Note that the compiler will still warn when putting a public member inside an extension explicitly marked internal, because the extended type could be public and then including a public member would be valid. It is also still an error to put a public member inside a constrained extension of an internal type, though I think this one is safe to relax later. Progress on SE-0025 ('private' and 'fileprivate')
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -5942,24 +5942,6 @@ inline bool ValueDecl::isSettable(const DeclContext *UseDC,
     return false;
 }
 
-namespace impl {
-  bool isInternalDeclEffectivelyPublic(const ValueDecl *VD);
-}
-
-inline Accessibility ValueDecl::getEffectiveAccess() const {
-  switch (getFormalAccess()) {
-  case Accessibility::Public:
-    return Accessibility::Public;
-  case Accessibility::Internal:
-    if (impl::isInternalDeclEffectivelyPublic(this)) {
-      return Accessibility::Public;
-    }
-    return Accessibility::Internal;
-  case Accessibility::Private:
-    return Accessibility::Private;
-  }
-}
-
 inline Optional<VarDecl *>
 NominalTypeDecl::ToStoredProperty::operator()(Decl *decl) const {
   if (auto var = dyn_cast<VarDecl>(decl)) {
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -945,10 +945,6 @@ ERROR(access_control_setter_more,none,
       "%select{variable|property|subscript}1 cannot have "
       "%select{PRIVATE|an internal|a public}2 setter",
       (Accessibility, unsigned, Accessibility))
-WARNING(access_control_member_more,none,
-    "declaring %select{PRIVATE|an internal|a public}0 %1 for "
-    "%select{a private|an internal|PUBLIC}2 %3",
-    (Accessibility, DescriptiveDeclKind, Accessibility, DescriptiveDeclKind))
 WARNING(access_control_ext_member_more,none,
     "declaring %select{PRIVATE|an internal|a public}0 %1 in "
     "%select{a private|an internal|PUBLIC}2 extension",
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -44,23 +44,6 @@
 
 using namespace swift;
 
-bool impl::isInternalDeclEffectivelyPublic(const ValueDecl *VD) {
-  assert(VD->getFormalAccess() == Accessibility::Internal);
-
-  if (VD->getAttrs().hasAttribute<VersionedAttr>())
-    return true;
-
-  if (auto *fn = dyn_cast<FuncDecl>(VD))
-    if (auto *ASD = fn->getAccessorStorageDecl())
-      if (ASD->getAttrs().hasAttribute<VersionedAttr>())
-        return true;
-
-  if (VD->getModuleContext()->isTestingEnabled())
-    return true;
-
-  return false;
-}
-
 clang::SourceLocation ClangNode::getLocation() const {
   if (auto D = getAsDecl())
     return D->getLocation();
@@ -1810,6 +1793,64 @@ SourceLoc ValueDecl::getAttributeInsertionLoc(bool forModifier) const {
   return resultLoc.isValid() ? resultLoc : getStartLoc();
 }
 
+/// Returns true if \p VD needs to be treated as publicly-accessible at the SIL,
+/// LLVM, and machine levels.
+///
+/// The most common causes of this are -enable-testing and versioned non-public
+/// declarations.
+static bool isInternalDeclEffectivelyPublic(const ValueDecl *VD) {
+  assert(VD->getFormalAccess() == Accessibility::Internal);
+
+  if (VD->getAttrs().hasAttribute<VersionedAttr>())
+    return true;
+
+  if (auto *fn = dyn_cast<FuncDecl>(VD))
+    if (auto *ASD = fn->getAccessorStorageDecl())
+      if (ASD->getAttrs().hasAttribute<VersionedAttr>())
+        return true;
+
+  if (VD->getModuleContext()->isTestingEnabled())
+    return true;
+
+  return false;
+}
+
+Accessibility ValueDecl::getEffectiveAccess() const {
+  Accessibility effectiveAccess = getFormalAccess();
+
+  // Handle @testable.
+  switch (effectiveAccess) {
+  case Accessibility::Public:
+    break;
+  case Accessibility::Internal:
+    if (isInternalDeclEffectivelyPublic(this))
+      effectiveAccess = Accessibility::Public;
+    break;
+  case Accessibility::Private:
+    break;
+  }
+
+  if (auto enclosingNominal = dyn_cast<NominalTypeDecl>(getDeclContext())) {
+    effectiveAccess = std::min(effectiveAccess,
+                               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());
+      }
+    }
+
+  } else if (getDeclContext()->isLocalContext()) {
+    effectiveAccess = Accessibility::Private;
+  }
+
+  return effectiveAccess;
+}
+
 
 Type TypeDecl::getDeclaredType() const {
   if (auto TAD = dyn_cast<TypeAliasDecl>(this)) {
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -683,7 +683,6 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
   void visitRequiredAttr(RequiredAttr *attr);
   void visitRethrowsAttr(RethrowsAttr *attr);
 
-  bool visitAbstractAccessibilityAttr(AbstractAccessibilityAttr *attr);
   void visitAccessibilityAttr(AccessibilityAttr *attr);
   void visitSetterAccessibilityAttr(SetterAccessibilityAttr *attr);
 
@@ -1267,25 +1266,6 @@ void AttributeChecker::visitRethrowsAttr(RethrowsAttr *attr) {
   attr->setInvalid();
 }
 
-bool AttributeChecker::visitAbstractAccessibilityAttr(
-    AbstractAccessibilityAttr *attr) {
-  DeclContext *dc = D->getDeclContext();
-  if (auto nominal = dc->getAsNominalTypeOrNominalTypeExtensionContext()) {
-    Accessibility typeAccess = nominal->getFormalAccess();
-    if (attr->getAccess() > typeAccess) {
-      auto diag = TC.diagnose(attr->getLocation(),
-                              diag::access_control_member_more,
-                              attr->getAccess(),
-                              D->getDescriptiveKind(),
-                              typeAccess,
-                              nominal->getDescriptiveKind());
-      swift::fixItAccessibility(diag, cast<ValueDecl>(D), typeAccess);
-      return true;
-    }
-  }
-  return false;
-}
-
 void AttributeChecker::visitAccessibilityAttr(AccessibilityAttr *attr) {
   if (auto extension = dyn_cast<ExtensionDecl>(D)) {
     Type extendedTy = extension->getExtendedType();
@@ -1327,8 +1307,6 @@ void AttributeChecker::visitAccessibilityAttr(AccessibilityAttr *attr) {
       return;
     }
   }
-
-  visitAbstractAccessibilityAttr(attr);
 }
 
 void
@@ -1352,8 +1330,6 @@ AttributeChecker::visitSetterAccessibilityAttr(SetterAccessibilityAttr *attr) {
     attr->setInvalid();
     return;
   }
-
-  visitAbstractAccessibilityAttr(attr);
 }
 
 /// Check that the @_specialize type list has the correct number of entries.
diff --git a/test/SILGen/accessibility_warnings.swift b/test/SILGen/accessibility_warnings.swift
@@ -1,73 +1,98 @@
 // RUN: %target-parse-verify-swift
-// RUN: %target-swift-frontend -emit-silgen -o /dev/null %s
+// RUN: %target-swift-frontend -emit-silgen %s | FileCheck %s
 
 // This file tests that the AST produced after fixing accessibility warnings
 // is valid according to SILGen and the verifiers.
 
 public struct PublicStruct {
+  // CHECK-DAG: sil{{( \[.+\])*}} @_TFV22accessibility_warnings12PublicStructg9publicVarSi
   public var publicVar = 0
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings12PublicStructCfT_S0_
 }
 
 internal struct InternalStruct {
-  public var publicVar = 0 // expected-warning {{declaring a public var for an internal struct}} {{3-9=internal}}
+  public var publicVar = 0
+
+  public private(set) var publicVarPrivateSet = 0
 
-  public private(set) var publicVarPrivateSet = 0 // expected-warning {{declaring a public var for an internal struct}} {{3-9=internal}}
+  public public(set) var publicVarPublicSet = 0
 
-  public public(set) var publicVarPublicSet = 0 // expected-warning {{declaring a public var for an internal struct}} {{3-9=internal}} {{10-22=}}
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings14InternalStructg16publicVarGetOnlySi
+  public var publicVarGetOnly: Int { return 0 }
 
-  public var publicVarGetOnly: Int { return 0 } // expected-warning {{declaring a public var for an internal struct}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings14InternalStructg15publicVarGetSetSi
+  public var publicVarGetSet: Int { get { return 0 } set {} }
 
-  public var publicVarGetSet: Int { get { return 0 } set {} } // expected-warning {{declaring a public var for an internal struct}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings14InternalStructCfT_S0_
 }
 
 private struct PrivateStruct {
-  public var publicVar = 0 // expected-warning {{declaring a public var for a private struct}} {{3-9=private}}
+  public var publicVar = 0
+  // CHECK-DAG: sil private @_TFV22accessibility_warningsP33_5D2F2E026754A901C0FF90C404896D0213PrivateStructCfT_S0_
 }
 
 
 extension PublicStruct {
+  // CHECK-DAG: sil @_TFV22accessibility_warnings12PublicStructCfT1xSi_S0_
   public init(x: Int) { self.init() }
 
+  // CHECK-DAG: sil @_TFV22accessibility_warnings12PublicStructg18publicVarExtensionSi
   public var publicVarExtension: Int { get { return 0 } set {} }
 }
 
 extension InternalStruct {
-  public init(x: Int) { self.init() } // expected-warning {{declaring a public initializer for an internal struct}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings14InternalStructCfT1xSi_S0_
+  public init(x: Int) { self.init() }
 
-  public var publicVarExtension: Int { get { return 0 } set {} } // expected-warning {{declaring a public var for an internal struct}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings14InternalStructg18publicVarExtensionSi
+  public var publicVarExtension: Int { get { return 0 } set {} }
 }
 
 extension PrivateStruct {
-  public init(x: Int) { self.init() } // expected-warning {{declaring a public initializer for a private struct}} {{3-9=private}}
+  // CHECK-DAG: sil private @_TFV22accessibility_warningsP33_5D2F2E026754A901C0FF90C404896D0213PrivateStructCfT1xSi_S0_
+  public init(x: Int) { self.init() }
 
-  public var publicVarExtension: Int { get { return 0 } set {} } // expected-warning {{declaring a public var for a private struct}} {{3-9=private}}
+  // CHECK-DAG: sil private @_TFV22accessibility_warningsP33_5D2F2E026754A901C0FF90C404896D0213PrivateStructg18publicVarExtensionSi
+  public var publicVarExtension: Int { get { return 0 } set {} }
 }
 
 public extension PublicStruct {
+  // CHECK-DAG: sil @_TFV22accessibility_warnings12PublicStruct15extMemberPublicfT_T_
   public func extMemberPublic() {}
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings12PublicStructP33_5D2F2E026754A901C0FF90C404896D0213extImplPublicfT_T_
   private func extImplPublic() {}
 }
 internal extension PublicStruct {
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings12PublicStruct17extMemberInternalfT_T_
   public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings12PublicStructP33_5D2F2E026754A901C0FF90C404896D0215extImplInternalfT_T_
   private func extImplInternal() {}
 }
 private extension PublicStruct {
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings12PublicStructP33_5D2F2E026754A901C0FF90C404896D0216extMemberPrivatefT_T_
   public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=private}}
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings12PublicStructP33_5D2F2E026754A901C0FF90C404896D0214extImplPrivatefT_T_
   private func extImplPrivate() {}
 }
 
 internal extension InternalStruct {
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings14InternalStruct17extMemberInternalfT_T_
   public func extMemberInternal() {} // expected-warning {{declaring a public instance method in an internal extension}} {{3-9=internal}}
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings14InternalStructP33_5D2F2E026754A901C0FF90C404896D0215extImplInternalfT_T_
   private func extImplInternal() {}
 }
 private extension InternalStruct {
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings14InternalStructP33_5D2F2E026754A901C0FF90C404896D0216extMemberPrivatefT_T_
   public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=private}}
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings14InternalStructP33_5D2F2E026754A901C0FF90C404896D0214extImplPrivatefT_T_
   private func extImplPrivate() {}
 }
 
 
 private extension PrivateStruct {
+  // CHECK-DAG: sil private @_TFV22accessibility_warningsP33_5D2F2E026754A901C0FF90C404896D0213PrivateStruct16extMemberPrivatefT_T_
   public func extMemberPrivate() {} // expected-warning {{declaring a public instance method in a private extension}} {{3-9=private}}
+  // CHECK-DAG: sil private @_TFV22accessibility_warningsP33_5D2F2E026754A901C0FF90C404896D0213PrivateStruct14extImplPrivatefT_T_
   private func extImplPrivate() {}
 }
 
@@ -77,7 +102,10 @@ public protocol PublicReadOnlyOperations {
 }
 
 internal struct PrivateSettersForReadOnlyInternal : PublicReadOnlyOperations {
-  public private(set) var size = 0 // expected-warning {{declaring a public var for an internal struct}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden{{( \[.+\])*}} @_TFV22accessibility_warnings33PrivateSettersForReadOnlyInternalg4sizeSi
+  public private(set) var size = 0
+  // CHECK-DAG: sil hidden @_TFV22accessibility_warnings33PrivateSettersForReadOnlyInternalg9subscriptFSiSi
+  // CHECK-DAG: sil private @_TFV22accessibility_warnings33PrivateSettersForReadOnlyInternals9subscriptFSiSi
   internal private(set) subscript (_: Int) -> Int { // no-warning
     get { return 42 }
     set {}
@@ -86,22 +114,32 @@ internal struct PrivateSettersForReadOnlyInternal : PublicReadOnlyOperations {
 
 
 public class PublicClass {
+  // CHECK-DAG: sil{{( \[.+\])*}} @_TFC22accessibility_warnings11PublicClassg9publicVarSi
   public var publicVar = 0
+  // CHECK-DAG: sil hidden @_TFC22accessibility_warnings11PublicClasscfT_S0_
 }
 
 internal class InternalClass {
-  public var publicVar = 0 // expected-warning {{declaring a public var for an internal class}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden{{( \[.+\])*}} @_TFC22accessibility_warnings13InternalClassg9publicVarSi
+  public var publicVar = 0
 
-  public private(set) var publicVarPrivateSet = 0 // expected-warning {{declaring a public var for an internal class}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden [transparent] @_TFC22accessibility_warnings13InternalClassg19publicVarPrivateSetSi
+  public private(set) var publicVarPrivateSet = 0
 
-  public public(set) var publicVarPublicSet = 0 // expected-warning {{declaring a public var for an internal class}} {{3-9=internal}}
+  public public(set) var publicVarPublicSet = 0
 
-  public var publicVarGetOnly: Int { return 0 } // expected-warning {{declaring a public var for an internal class}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden @_TFC22accessibility_warnings13InternalClassg16publicVarGetOnlySi
+  public var publicVarGetOnly: Int { return 0 }
 
-  public var publicVarGetSet: Int { get { return 0 } set {} } // expected-warning {{declaring a public var for an internal class}} {{3-9=internal}}
+  // CHECK-DAG: sil hidden @_TFC22accessibility_warnings13InternalClassg15publicVarGetSetSi
+  public var publicVarGetSet: Int { get { return 0 } set {} }
+
+  // CHECK-DAG: sil hidden @_TFC22accessibility_warnings13InternalClasscfT_S0_
 }
 
 private class PrivateClass {
-  public var publicVar = 0 // expected-warning {{declaring a public var for a private class}} {{3-9=private}}
+  // CHECK-DAG: sil private{{( \[.+\])*}} @_TFC22accessibility_warningsP33_5D2F2E026754A901C0FF90C404896D0212PrivateClassg9publicVarSi
+  public var publicVar = 0
+  // CHECK-DAG: sil private @_TFC22accessibility_warningsP33_5D2F2E026754A901C0FF90C404896D0212PrivateClasscfT_S0_
 }
 
diff --git a/test/Sema/accessibility.swift b/test/Sema/accessibility.swift
diff --git a/test/attr/accessibility.swift b/test/attr/accessibility.swift
