AST: Plug a hole in access control checking

A protocol extension of a private protocol can define internal
or public members. We should not be able to find these members
from another file or module if an internal or public type
conforms to the protocol.

Fixes <rdar://problem/21380336>.

Author: slavapestov

include/swift/AST/Decl.h

@@ -2265,6 +2265,18 @@ class ValueDecl : public Decl {
     return result;
   }
 
+  /// If this declaration is a member of a protocol extension, return the
+  /// minimum of the given access level and the protocol's access level.
+  ///
+  /// Otherwise, return the given access level unmodified.
+  ///
+  /// This is used when checking name lookup visibility. Protocol extension
+  /// members can be found when performing name lookup on a concrete type;
+  /// if the concrete type is visible from the lookup context but the
+  /// protocol is not, we do not want the protocol extension members to be
+  /// visible.
+  AccessLevel adjustAccessLevelForProtocolExtension(AccessLevel access) const;
+
   /// Determine whether this Decl has either Private or FilePrivate access.
   bool isOutermostPrivateOrFilePrivateScope() const;
 
@@ -2325,7 +2337,14 @@ class ValueDecl : public Decl {
   /// 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;
+  ///
+  /// If \p forConformance is true, we ignore the visibility of the protocol
+  /// when evaluating protocol extension members. This language rule allows a
+  /// protocol extension of a private protocol to provide default
+  /// implementations for the requirements of a public protocol, even when
+  /// the default implementations are not visible to name lookup.
+  bool isAccessibleFrom(const DeclContext *DC,
+                        bool forConformance = false) const;
 
   /// Retrieve the "interface" type of this value, which uses
   /// GenericTypeParamType if the declaration is generic. For a generic
@@ -4449,7 +4468,11 @@ class AbstractStorageDecl : public ValueDecl {
   /// context.
   ///
   /// If \p DC is null, returns true only if the setter is public.
-  bool isSetterAccessibleFrom(const DeclContext *DC) const;
+  ///
+  /// See \c isAccessibleFrom for a discussion of the \p forConformance
+  /// parameter.
+  bool isSetterAccessibleFrom(const DeclContext *DC,
+                              bool forConformance=false) const;
 
   /// Determine how this storage declaration should actually be accessed.
   AccessStrategy getAccessStrategy(AccessSemantics semantics,

lib/AST/NameLookup.cpp

@@ -1507,12 +1507,36 @@ void ClassDecl::recordObjCMethod(AbstractFunctionDecl *method) {
   vec.push_back(method);
 }
 
-static bool checkAccess(const DeclContext *useDC, const DeclContext *sourceDC,
-                        AccessLevel access) {
+AccessLevel
+ValueDecl::adjustAccessLevelForProtocolExtension(AccessLevel access) const {
+  if (auto *ext = dyn_cast<ExtensionDecl>(getDeclContext())) {
+    if (auto *protocol = ext->getAsProtocolOrProtocolExtensionContext()) {
+      // Note: it gets worse. The standard library has public methods
+      // in protocol extensions of a @usableFromInline internal protocol,
+      // 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);
+    }
+  }
+
+  return access;
+}
+
+static bool checkAccess(const DeclContext *useDC, const ValueDecl *VD,
+                        AccessLevel access, bool forConformance) {
   if (!useDC)
     return access >= AccessLevel::Public;
 
-  assert(sourceDC && "ValueDecl being accessed must have a valid DeclContext");
+  auto *sourceDC = VD->getDeclContext();
+
+  if (!forConformance)
+    access = VD->adjustAccessLevelForProtocolExtension(access);
+
   switch (access) {
   case AccessLevel::Private:
     return (useDC == sourceDC ||
@@ -1536,11 +1560,14 @@ static bool checkAccess(const DeclContext *useDC, const DeclContext *sourceDC,
   llvm_unreachable("bad access level");
 }
 
-bool ValueDecl::isAccessibleFrom(const DeclContext *DC) const {
-  return checkAccess(DC, getDeclContext(), getFormalAccess());
+bool ValueDecl::isAccessibleFrom(const DeclContext *DC,
+                                 bool forConformance) const {
+  auto access = getFormalAccess();
+  return checkAccess(DC, this, access, forConformance);
 }
 
-bool AbstractStorageDecl::isSetterAccessibleFrom(const DeclContext *DC) const {
+bool AbstractStorageDecl::isSetterAccessibleFrom(const DeclContext *DC,
+                                                 bool forConformance) const {
   assert(isSettable(DC));
 
   // If a stored property does not have a setter, it is still settable from the
@@ -1552,7 +1579,8 @@ bool AbstractStorageDecl::isSetterAccessibleFrom(const DeclContext *DC) const {
   if (isa<ParamDecl>(this))
     return true;
 
-  return checkAccess(DC, getDeclContext(), getSetterFormalAccess());
+  auto access = getSetterFormalAccess();
+  return checkAccess(DC, this, access, forConformance);
 }
 
 Type AbstractStorageDecl::getStorageInterfaceType() const {

lib/Sema/CSDiag.cpp

@@ -1546,8 +1546,15 @@ diagnoseUnviableLookupResults(MemberLookupResult &result, Type baseObjTy,
     case MemberLookupResult::UR_Inaccessible: {
       auto decl = result.UnviableCandidates[0].first.getDecl();
       // FIXME: What if the unviable candidates have different levels of access?
+      //
+      // If we found an inaccessible member of a protocol extension, it might
+      // be declared 'public'. This can only happen if the protocol is not
+      // visible to us, but the conforming type is. In this case, we need to
+      // clamp the formal access for diagnostics purposes to the formal access
+      // of the protocol itself.
       diagnose(nameLoc, diag::candidate_inaccessible, decl->getBaseName(),
-               decl->getFormalAccess());
+               decl->adjustAccessLevelForProtocolExtension(
+                 decl->getFormalAccess()));
       for (auto cand : result.UnviableCandidates)
         diagnose(cand.first.getDecl(), diag::decl_declared_here, memberName);
 

lib/Sema/TypeCheckNameLookup.cpp

@@ -54,7 +54,10 @@ namespace {
                         NameLookupOptions options,
                         bool isMemberLookup)
       : TC(tc), Result(result), DC(dc), Options(options),
-        IsMemberLookup(isMemberLookup) { }
+        IsMemberLookup(isMemberLookup) {
+      if (!TC.Context.LangOpts.EnableAccessControl)
+        Options |= NameLookupFlags::IgnoreAccessControl;
+    }
 
     ~LookupResultBuilder() {
       // If any of the results have a base, we need to remove
@@ -184,6 +187,16 @@ namespace {
           .second;
       } else if (found->isProtocolRequirement()) {
         witness = concrete->getWitnessDecl(found, &TC);
+
+        // It is possible that a requirement is visible to us, but
+        // not the witness. In this case, just return the requirement;
+        // we will perform virtual dispatch on the concrete type.
+        if (witness &&
+            !Options.contains(NameLookupFlags::IgnoreAccessControl) &&
+            !witness->isAccessibleFrom(DC)) {
+          addResult(found);
+          return;
+        }
       }
 
       // FIXME: the "isa<ProtocolDecl>()" check will be wrong for

lib/Sema/TypeCheckProtocol.cpp

@@ -1051,14 +1051,21 @@ bool WitnessChecker::checkWitnessAccess(AccessScope &requiredAccessScope,
                                         bool *isSetter) {
   *isSetter = false;
 
+  // Compute the intersection of the conforming type's access scope
+  // and the protocol's access scope.
   auto scopeIntersection =
     requiredAccessScope.intersectWith(Proto->getFormalAccessScope(DC));
   assert(scopeIntersection.hasValue());
 
   requiredAccessScope = *scopeIntersection;
 
   AccessScope actualScopeToCheck = requiredAccessScope;
-  if (!witness->isAccessibleFrom(actualScopeToCheck.getDeclContext())) {
+
+  // Setting the 'forConformance' flag means that we admit witnesses in
+  // protocol extensions that we can see, but are not necessarily as
+  // visible as the conforming type and protocol.
+  if (!witness->isAccessibleFrom(actualScopeToCheck.getDeclContext(),
+                                 /*forConformance=*/true)) {
     // Special case: if we have `@testable import` of the witness's module,
     // allow the witness to match if it would have matched for just this file.
     // That is, if '@testable' allows us to see the witness here, it should
@@ -1081,7 +1088,10 @@ bool WitnessChecker::checkWitnessAccess(AccessScope &requiredAccessScope,
     *isSetter = true;
 
     auto ASD = cast<AbstractStorageDecl>(witness);
-    if (!ASD->isSetterAccessibleFrom(actualScopeToCheck.getDeclContext()))
+
+    // See above about the forConformance flag.
+    if (!ASD->isSetterAccessibleFrom(actualScopeToCheck.getDeclContext(),
+                                     /*forConformance=*/true))
       return true;
   }
 

test/SILGen/Inputs/witness_accessibility_other.swift

@@ -0,0 +1,20 @@
+public protocol P {
+  func publicRequirement()
+}
+
+@usableFromInline
+protocol Q : P {
+  func internalRequirement()
+}
+
+fileprivate protocol R : Q {}
+
+extension R {
+  public func publicRequirement() {}
+}
+
+extension Q {
+  public func internalRequirement() {}
+}
+
+public struct S : R {}

test/SILGen/testable-multifile-other.swift

@@ -12,6 +12,8 @@
 // RUN: %target-swift-frontend -emit-ir -enable-sil-ownership -I %t -primary-file %s %S/testable-multifile.swift -module-name main -o /dev/null
 // RUN: %target-swift-frontend -emit-ir -enable-sil-ownership -I %t -O -primary-file %s %S/testable-multifile.swift -module-name main -o /dev/null
 
+@testable import TestableMultifileHelper
+
 func use<F: Fooable>(_ f: F) { f.foo() }
 func test(internalFoo: FooImpl, publicFoo: PublicFooImpl) {
   use(internalFoo)

test/SILGen/witness_accessibility_multi.swift

@@ -0,0 +1,25 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module-path %t/witness_accessibility_other.swiftmodule %S/Inputs/witness_accessibility_other.swift
+// RUN: %target-swift-frontend -I %t -emit-silgen -enable-sil-ownership %s | %FileCheck %s
+// RUN: %target-swift-frontend -I %t -emit-sil -enable-sil-ownership %s
+
+import witness_accessibility_other
+
+// We can see the conformance S : P, but we cannot see the witness for
+// P.publicRequirement() because it is defined in an extension of a
+// private protocol R to which S also conforms.
+//
+// So make sure the witness is invoked via virtual dispatch even with
+// a concrete base type.
+
+// CHECK-LABEL: sil @$S27witness_accessibility_multi22callsPublicRequirement1sy0a1_B6_other1SV_tF : $@convention(thin) (S) -> ()
+public func callsPublicRequirement(s: S) {
+
+  // CHECK: witness_method $S, #P.publicRequirement!1 : <Self where Self : P> (Self) -> () -> () : $@convention(witness_method: P) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0) -> ()
+  s.publicRequirement()
+
+  // CHECK: function_ref @$S27witness_accessibility_other1QPAAE19internalRequirementyyF : $@convention(method) <τ_0_0 where τ_0_0 : Q> (@in_guaranteed τ_0_0) -> ()
+  s.internalRequirement()
+
+  // CHECK: return
+}

test/SILOptimizer/Inputs/opaque_conformance.swift

@@ -0,0 +1,13 @@
+public protocol PublicProtocol {
+  func publicRequirement()
+}
+
+private protocol PrivateProtocol : PublicProtocol {}
+
+public struct Conformer : PrivateProtocol {}
+
+extension PrivateProtocol {
+  // This implementation is opaque to callers, since we can
+  // only find it via a private protocol.
+  public func publicRequirement() {}
+}

test/SILOptimizer/devirt_opaque_witness.swift

@@ -0,0 +1,29 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -emit-module-path %t/opaque_conformance.swiftmodule -primary-file %S/Inputs/opaque_conformance.swift
+// RUN: %target-swift-frontend -O -emit-sil -primary-file %s -I %t | %FileCheck %s
+
+import opaque_conformance
+
+public func callsPublicRequirement(_ c: Conformer) {
+  c.publicRequirement()
+}
+
+// CHECK-LABEL: sil @$S21devirt_opaque_witness22callsPublicRequirementyy0B12_conformance9ConformerVF : $@convention(thin) (Conformer) -> () {
+// CHECK:    bb0(%0 : $Conformer):
+// CHECK:      [[BOX:%.*]] = alloc_stack $Conformer
+// CHECK-NEXT: store %0 to [[BOX]] : $*Conformer
+// CHECK:      [[FN:%.*]] = function_ref @$S18opaque_conformance9ConformerVAA14PublicProtocolA2aDP17publicRequirementyyFTW : $@convention(witness_method: PublicProtocol) (@in_guaranteed Conformer) -> ()
+// CHECK-NEXT: [[RESULT:%.*]] = apply [[FN]]([[BOX]]) : $@convention(witness_method: PublicProtocol) (@in_guaranteed Conformer) -> ()
+// CHECK-NEXT: dealloc_stack [[BOX]] : $*Conformer
+// CHECK-NEXT: [[RESULT:%.*]] = tuple ()
+// CHECK-NEXT: return [[RESULT]] : $()
+
+// Note that [transparent] here doesn't mean anything since there's no body.
+// The important thing is that the witness_method call got devirtualized,
+// and the thunk has public linkage and no serialized body (since the body
+// references a private symbol of the module opaque_conformance).
+// CHECK-LABEL: sil [transparent] [thunk] @$S18opaque_conformance9ConformerVAA14PublicProtocolA2aDP17publicRequirementyyFTW : $@convention(witness_method: PublicProtocol) (@in_guaranteed Conformer) -> ()
+
+// CHECK-LABEL: sil_witness_table public_external Conformer: PublicProtocol module opaque_conformance {
+// CHECK-NEXT:    method #PublicProtocol.publicRequirement!1: <Self where Self : PublicProtocol> (Self) -> () -> () : @$S18opaque_conformance9ConformerVAA14PublicProtocolA2aDP17publicRequirementyyFTW
+// CHECK-NEXT: }

test/Sema/Inputs/accessibility_multi_other.swift

@@ -7,3 +7,15 @@ struct Members {
     set {}
   }
 }
+
+struct PrivateConformance : PrivateProtocol {}
+
+private protocol PrivateProtocol {}
+
+extension PrivateProtocol {
+  public func publicExtensionMember() {}
+  // expected-note@-1 {{'publicExtensionMember' declared here}}
+
+  internal func internalExtensionMember() {}
+  // expected-note@-1 {{'internalExtensionMember' declared here}}
+}

test/Sema/Inputs/accessibility_multi_other_module.swift

@@ -0,0 +1,19 @@
+public struct PrivateConformance : PrivateProtocol {}
+
+private protocol PrivateProtocol {}
+
+extension PrivateProtocol {
+  public func publicExtensionMember() {}
+
+  internal func internalExtensionMember() {}
+}
+
+public struct InternalConformance : InternalProtocol {}
+
+internal protocol InternalProtocol {}
+
+extension InternalProtocol {
+  public func publicExtensionMember() {}
+
+  internal func internalExtensionMember() {}
+}

test/Sema/accessibility_multi.swift

@@ -21,3 +21,11 @@ func testSubscript(_ instance: Members) {
   instance[] = 42 // expected-error {{cannot assign through subscript: subscript setter is inaccessible}}
   reset(&instance[]) // expected-error {{cannot pass immutable value as inout argument: subscript setter is inaccessible}}
 }
+
+func testPrivateConformance(_ instance: PrivateConformance) {
+  instance.publicExtensionMember()
+  // expected-error@-1 {{'publicExtensionMember' is inaccessible due to 'fileprivate' protection level}}
+
+  instance.internalExtensionMember()
+  // expected-error@-1 {{'internalExtensionMember' is inaccessible due to 'fileprivate' protection level}}
+}

test/Sema/accessibility_multi_module.swift

@@ -0,0 +1,21 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -primary-file %S/Inputs/accessibility_multi_other_module.swift -emit-module-path %t/accessibility_multi_other_module.swiftmodule
+// RUN: %target-swift-frontend -typecheck -I %t -primary-file %s -verify -verify-ignore-unknown
+
+import accessibility_multi_other_module
+
+func testPrivateConformance(_ instance: PrivateConformance) {
+  instance.publicExtensionMember()
+  // expected-error@-1 {{'publicExtensionMember' is inaccessible due to 'fileprivate' protection level}}
+
+  instance.internalExtensionMember()
+  // expected-error@-1 {{'internalExtensionMember' is inaccessible due to 'fileprivate' protection level}}
+}
+
+func testInternalConformance(_ instance: InternalConformance) {
+  instance.publicExtensionMember()
+  // expected-error@-1 {{'publicExtensionMember' is inaccessible due to 'internal' protection level}}
+
+  instance.internalExtensionMember()
+  // expected-error@-1 {{'internalExtensionMember' is inaccessible due to 'internal' protection level}}
+}