Use Swift-3-style access checking to downgrade errors to warnings.

jrose-apple · jrose-apple · commit 9489f10ce1fe · 2017-01-13T14:34:50.000-08:00
...instead of just ignoring the errors in certain cases, in service of
source compatibility.

Swift 3.0 wasn't nearly as strict as checking access control for types
because it didn't look at the TypeRepr at all (except to highlight a
particular part of the type in diagnostics). It also looked through
typealiases in certain cases. Approximate this behavior by running the
access checking logic for Types (rather than TypeReprs), and downgrade
access violation errors to warnings when the checks disagree.

Part of rdar://problem/29855782.
diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp
@@ -1147,7 +1147,7 @@ class AccessScopeChecker {
         Cache(caches[File]),
         Context(File->getASTContext()) {}
 
-  bool visitDecl(ValueDecl *VD, bool isInParameter = false) {
+  bool visitDecl(ValueDecl *VD) {
     if (!VD || isa<GenericTypeParamDecl>(VD))
       return true;
 
@@ -1158,23 +1158,6 @@ class AccessScopeChecker {
         return true;
     }
 
-    // Simulation for Swift 3 bugs where typealiases got canonicalized away and
-    // thus a reference to a private typealias would not be diagnosed.
-    if (auto *TAD = dyn_cast<TypeAliasDecl>(VD)) {
-      if (Context.isSwiftVersion3()) {
-        // - If the typealias was a dependent type, Swift 3 resolved it down to
-        //   its underlying type.
-        if (VD->getInterfaceType()->hasTypeParameter())
-          return true;
-        // - If the typealias was a function type in parameter position, Swift 3
-        //   would rebuild the type to mark it non-escaping, losing the sugar.
-        if (isInParameter &&
-            TAD->getDeclaredInterfaceType()->getAs<AnyFunctionType>()) {
-          return true;
-        }
-      }
-    }
-
     auto cached = Cache.find(VD);
     if (cached != Cache.end()) {
       Scope = Scope->intersectWith(cached->second);
@@ -1192,72 +1175,39 @@ class AccessScopeChecker {
 };
 
 class TypeReprAccessScopeChecker : private ASTWalker, AccessScopeChecker {
-  SmallVector<const TypeRepr *, 4> ParamParents;
-
   TypeReprAccessScopeChecker(const DeclContext *useDC,
-                             decltype(TypeChecker::TypeAccessScopeCache) &caches,
-                             bool isParameter)
+                             decltype(TypeChecker::TypeAccessScopeCache) &caches)
       : AccessScopeChecker(useDC, caches) {
-    if (isParameter)
-      ParamParents.push_back(nullptr);
-  }
-
-  bool isParamParent(const TypeRepr *TR) const {
-    return !ParamParents.empty() && ParamParents.back() == TR;
   }
 
   bool walkToTypeReprPre(TypeRepr *TR) override {
-    if (auto CITR = dyn_cast<ComponentIdentTypeRepr>(TR)) {
-      // In Swift 3, components other than the last one were not properly
-      // checked for availability.
-      // FIXME: We should try to downgrade these errors to warnings, not just
-      // skip diagnosing them.
-      if (Context.LangOpts.isSwiftVersion3()) {
-        const TypeRepr *parent = Parent.getAsTypeRepr();
-        if (auto *compound = dyn_cast_or_null<CompoundIdentTypeRepr>(parent))
-          if (compound->Components.back() != CITR)
-            return true;
-      }
-
-      return visitDecl(CITR->getBoundDecl(),
-                       isParamParent(Parent.getAsTypeRepr()));
-    }
-
-    auto parentTR = Parent.getAsTypeRepr();
-    if (auto parentFn = dyn_cast_or_null<FunctionTypeRepr>(parentTR)) {
-      // The argument tuple for a function is a parent of parameter TRs.
-      if (parentFn->getArgsTypeRepr() == TR)
-        ParamParents.push_back(TR);
-    } else if (isa<CompoundIdentTypeRepr>(TR) && isParamParent(parentTR)) {
-      // Compound TRs that would have been parameter TRs contain parameter
-      // TRs.
-      ParamParents.push_back(TR);
-    }
-
+    if (auto CITR = dyn_cast<ComponentIdentTypeRepr>(TR))
+      return visitDecl(CITR->getBoundDecl());
     return true;
   }
 
   bool walkToTypeReprPost(TypeRepr *TR) override {
-    if (isParamParent(TR))
-      ParamParents.pop_back();
     return Scope.hasValue();
   }
 
 public:
   static Optional<AccessScope>
   getAccessScope(TypeRepr *TR, const DeclContext *useDC,
-                 decltype(TypeChecker::TypeAccessScopeCache) &caches,
-                 bool isParameter) {
-    TypeReprAccessScopeChecker checker(useDC, caches, isParameter);
+                 decltype(TypeChecker::TypeAccessScopeCache) &caches) {
+    TypeReprAccessScopeChecker checker(useDC, caches);
     TR->walk(checker);
     return checker.Scope;
   }
 };
 
 class TypeAccessScopeChecker : private TypeWalker, AccessScopeChecker {
+  bool CanonicalizeParentTypes;
+
   TypeAccessScopeChecker(const DeclContext *useDC,
-                         decltype(TypeChecker::TypeAccessScopeCache) &caches)
-      : AccessScopeChecker(useDC, caches) {}
+                         decltype(TypeChecker::TypeAccessScopeCache) &caches,
+                         bool canonicalizeParentTypes)
+      : AccessScopeChecker(useDC, caches),
+        CanonicalizeParentTypes(canonicalizeParentTypes) {}
 
   Action walkToTypePre(Type T) override {
     ValueDecl *VD;
@@ -1268,14 +1218,34 @@ class TypeAccessScopeChecker : private TypeWalker, AccessScopeChecker {
     else
       VD = nullptr;
 
-    return visitDecl(VD) ? Action::Continue : Action::Stop;
+    if (!visitDecl(VD))
+      return Action::Stop;
+
+    if (!CanonicalizeParentTypes)
+      return Action::Continue;
+
+    Type nominalParentTy;
+    if (auto nominalTy = dyn_cast<NominalType>(T.getPointer())) {
+      nominalParentTy = nominalTy->getParent();
+    } else if (auto genericTy = dyn_cast<BoundGenericType>(T.getPointer())) {
+      nominalParentTy = genericTy->getParent();
+      for (auto genericArg : genericTy->getGenericArgs())
+        genericArg.walk(*this);
+    } else {
+      return Action::Continue;
+    }
+
+    if (nominalParentTy)
+      nominalParentTy->getCanonicalType().walk(*this);
+    return Action::SkipChildren;
   }
 
 public:
   static Optional<AccessScope>
   getAccessScope(Type T, const DeclContext *useDC,
-                 decltype(TypeChecker::TypeAccessScopeCache) &caches) {
-    TypeAccessScopeChecker checker(useDC, caches);
+                 decltype(TypeChecker::TypeAccessScopeCache) &caches,
+                 bool canonicalizeParentTypes = false) {
+    TypeAccessScopeChecker checker(useDC, caches, canonicalizeParentTypes);
     T.walk(checker);
     return checker.Scope;
   }
@@ -1311,8 +1281,7 @@ void TypeChecker::computeDefaultAccessibility(ExtensionDecl *ED) {
       auto accessScope =
           TypeReprAccessScopeChecker::getAccessScope(TL.getTypeRepr(),
                                                      ED->getDeclContext(),
-                                                     TypeAccessScopeCache,
-                                                     /*isParameter*/false);
+                                                     TypeAccessScopeCache);
       // This is an error case and will be diagnosed elsewhere.
       if (!accessScope.hasValue())
         return Accessibility::Public;
@@ -1533,7 +1502,7 @@ using CheckTypeAccessCallback =
 /// is never null.
 static void checkTypeAccessibilityImpl(
     TypeChecker &TC, TypeLoc TL, AccessScope contextAccessScope,
-    const DeclContext *useDC, bool isParameter,
+    const DeclContext *useDC,
     llvm::function_ref<CheckTypeAccessCallback> diagnose) {
   if (!TC.getLangOpts().EnableAccessControl)
     return;
@@ -1551,8 +1520,7 @@ static void checkTypeAccessibilityImpl(
   auto typeAccessScope =
     (TL.getTypeRepr()
      ? TypeReprAccessScopeChecker::getAccessScope(TL.getTypeRepr(), useDC,
-                                                  TC.TypeAccessScopeCache,
-                                                  isParameter)
+                                                  TC.TypeAccessScopeCache)
      : TypeAccessScopeChecker::getAccessScope(TL.getType(), useDC,
                                               TC.TypeAccessScopeCache));
 
@@ -1562,17 +1530,48 @@ static void checkTypeAccessibilityImpl(
   if (!typeAccessScope.hasValue())
     return;
 
-  if (typeAccessScope->isPublic() ||
-      typeAccessScope->hasEqualDeclContextWith(contextAccessScope) ||
-      contextAccessScope.isChildOf(*typeAccessScope))
+  auto shouldComplainAboutAccessScope =
+      [contextAccessScope](AccessScope scope) -> bool {
+    if (scope.isPublic())
+      return false;
+    if (scope.hasEqualDeclContextWith(contextAccessScope))
+      return false;
+    if (contextAccessScope.isChildOf(scope))
+      return false;
+    return true;
+  };
+
+  if (!shouldComplainAboutAccessScope(typeAccessScope.getValue()))
     return;
 
+  // Swift 3.0 wasn't nearly as strict as checking types because it didn't
+  // look at the TypeRepr at all except to highlight a particular part of the
+  // type in diagnostics, and looked through typealiases in other cases.
+  // Approximate this behavior by running our non-TypeRepr-based check again
+  // and downgrading to a warning when the checks disagree.
+  auto downgradeToWarning = DowngradeToWarning::No;
+  if (TC.getLangOpts().isSwiftVersion3()) {
+    auto typeOnlyAccessScope =
+       TypeAccessScopeChecker::getAccessScope(TL.getType(), useDC,
+                                              TC.TypeAccessScopeCache,
+                                              /*canonicalizeParents*/true);
+    if (typeOnlyAccessScope.hasValue()) {
+      // If Swift 4 would have complained about a private type, but Swift 4
+      // would only diagnose an internal type, complain about the Swift 3
+      // offense first to avoid confusing users.
+      if (shouldComplainAboutAccessScope(typeOnlyAccessScope.getValue()))
+        typeAccessScope = typeOnlyAccessScope;
+      else
+        downgradeToWarning = DowngradeToWarning::Yes;
+    }
+  }
+
   const TypeRepr *complainRepr =
         TypeAccessScopeDiagnoser::findTypeWithScope(
             TL.getTypeRepr(),
             *typeAccessScope,
             useDC);
-  diagnose(*typeAccessScope, complainRepr, DowngradeToWarning::No);
+  diagnose(*typeAccessScope, complainRepr, downgradeToWarning);
 }
 
 /// Checks if the access scope of the type described by \p TL is valid for the
@@ -1587,17 +1586,15 @@ static void checkTypeAccessibility(
     TypeChecker &TC, TypeLoc TL, const ValueDecl *context,
     llvm::function_ref<CheckTypeAccessCallback> diagnose) {
   const DeclContext *DC = context->getDeclContext();
-  bool isParam = false;
   if (isa<ParamDecl>(context)) {
-    isParam = true;
     context = dyn_cast<AbstractFunctionDecl>(DC);
     if (!context)
       context = cast<SubscriptDecl>(DC);
     DC = context->getDeclContext();
   }
 
   AccessScope contextAccessScope = context->getFormalAccessScope();
-  checkTypeAccessibilityImpl(TC, TL, contextAccessScope, DC, isParam,
+  checkTypeAccessibilityImpl(TC, TL, contextAccessScope, DC,
                              [=](AccessScope requiredAccessScope,
                                  const TypeRepr *offendingTR,
                                  DowngradeToWarning downgradeToWarning) {
@@ -1654,7 +1651,6 @@ static void checkGenericParamAccessibility(TypeChecker &TC,
     assert(param->getInherited().size() == 1);
     checkTypeAccessibilityImpl(TC, param->getInherited().front(), accessScope,
                                owner->getDeclContext(),
-                               /*isParameter*/false,
                                [&](AccessScope typeAccessScope,
                                    const TypeRepr *thisComplainRepr,
                                    DowngradeToWarning thisDowngrade) {
@@ -1690,23 +1686,23 @@ static void checkGenericParamAccessibility(TypeChecker &TC,
     case RequirementReprKind::TypeConstraint:
       checkTypeAccessibilityImpl(TC, requirement.getSubjectLoc(),
                                  accessScope, owner->getDeclContext(),
-                                 /*isParameter*/false, callback);
+                                 callback);
       checkTypeAccessibilityImpl(TC, requirement.getConstraintLoc(),
                                  accessScope, owner->getDeclContext(),
-                                 /*isParameter*/false, callback);
+                                 callback);
       break;
     case RequirementReprKind::LayoutConstraint:
       checkTypeAccessibilityImpl(TC, requirement.getSubjectLoc(),
                                  accessScope, owner->getDeclContext(),
-                                 /*isParameter*/false, callback);
+                                 callback);
       break;
     case RequirementReprKind::SameType:
       checkTypeAccessibilityImpl(TC, requirement.getFirstTypeLoc(),
                                  accessScope, owner->getDeclContext(),
-                                 /*isParameter*/false, callback);
+                                 callback);
       checkTypeAccessibilityImpl(TC, requirement.getSecondTypeLoc(),
                                  accessScope, owner->getDeclContext(),
-                                 /*isParameter*/false, callback);
+                                 callback);
       break;
     }
   }
diff --git a/test/Compatibility/accessibility_compound.swift b/test/Compatibility/accessibility_compound.swift
@@ -7,34 +7,35 @@ public struct PublicStruct {
   internal struct Internal {}
 }
 
-private typealias PrivateAlias = PublicStruct // expected-note {{type declared here}}
+private typealias PrivateAlias = PublicStruct // expected-note * {{type declared here}}
 
-public let a: PrivateAlias.Inner?
+public let a0 = nil as PrivateAlias.Inner? // expected-warning {{constant should not be declared public because its type 'PrivateAlias.Inner?' uses a private type}}
+public let a: PrivateAlias.Inner? // expected-warning {{constant should not be declared public because its type uses a private type}}
 public let b: PrivateAlias.Internal? // expected-error {{constant cannot be declared public because its type uses an internal type}}
 public let c: Pair<PrivateAlias.Inner, PublicStruct.Internal>? // expected-error {{constant cannot be declared public because its type uses an internal type}}
 public let c2: Pair<PublicStruct.Internal, PrivateAlias.Inner>? // expected-error {{constant cannot be declared public because its type uses an internal type}}
 public let d: PrivateAlias? // expected-error {{constant cannot be declared public because its type uses a private type}}
 
 
 // rdar://problem/21408035
-private class PrivateBox<T> { // expected-note {{type declared here}}
+private class PrivateBox<T> { // expected-note * {{type declared here}}
   typealias ValueType = T
   typealias AlwaysFloat = Float
 }
 
-let boxUnboxInt: PrivateBox<Int>.ValueType = 0 // FIXME: This used to error in Swift 3.0.
+let boxUnboxInt: PrivateBox<Int>.ValueType = 0 // expected-warning {{constant should be declared private because its type uses a private type}}
 let boxFloat: PrivateBox<Int>.AlwaysFloat = 0 // expected-error {{constant must be declared private or fileprivate because its type uses a private type}}
 
 private protocol PrivateProto {
   associatedtype Inner
 }
 extension PublicStruct: PrivateProto {}
 
-private class SpecificBox<T: PrivateProto> { // expected-note {{type declared here}}
+private class SpecificBox<T: PrivateProto> { // expected-note * {{type declared here}}
   typealias InnerType = T.Inner
   typealias AlwaysFloat = Float
 }
 
-let specificBoxUnboxInt: SpecificBox<PublicStruct>.InnerType = .init() // FIXME: This used to error in Swift 3.0.
+let specificBoxUnboxInt: SpecificBox<PublicStruct>.InnerType = .init() // expected-warning {{constant should be declared private because its type uses a private type}}
 let specificBoxFloat: SpecificBox<PublicStruct>.AlwaysFloat = 0 // expected-error {{constant must be declared private or fileprivate because its type uses a private type}}
 
diff --git a/test/Compatibility/accessibility_typealias.swift b/test/Compatibility/accessibility_typealias.swift
@@ -11,14 +11,14 @@ struct S<T> : P {
 }
 
 public struct G<T> {
-  typealias A = S<T>
+  typealias A = S<T> // expected-note {{declared here}}
 
-  public func foo<U : P>(u: U) where U.Element == A.Element {}
+  public func foo<U : P>(u: U) where U.Element == A.Element {} // expected-warning {{instance method should not be declared public because its generic requirement uses an internal type}}
 }
 
 public final class ReplayableGenerator<S: Sequence> : IteratorProtocol {
-    typealias Sequence = S
-    public typealias Element = Sequence.Iterator.Element
+    typealias Sequence = S // expected-note {{declared here}}
+    public typealias Element = Sequence.Iterator.Element // expected-warning {{type alias should not be declared public because its underlying type uses an internal type}}
 
     public func next() -> Element? {
       return nil
@@ -29,9 +29,9 @@ struct Generic<T> {
   fileprivate typealias Dependent = T
 }
 
-var x: Generic<Int>.Dependent = 3
+var x: Generic<Int>.Dependent = 3 // expected-warning {{variable should be declared fileprivate because its type uses a fileprivate type}}
 
-func internalFuncWithFileprivateAlias() -> Generic<Int>.Dependent {
+func internalFuncWithFileprivateAlias() -> Generic<Int>.Dependent { // expected-warning {{function should be declared fileprivate because its result uses a fileprivate type}}
   return 3
 }
 
@@ -44,14 +44,14 @@ var y = privateFuncWithFileprivateAlias()
 
 private typealias FnType = (_ x: Int) -> Void // expected-note * {{type declared here}}
 
-public var fn1: (FnType) -> Void = { _ in }
-public var fn2: (_ x: FnType) -> Void = { _ in }
-public var fn3: (main.FnType) -> Void = { _ in }
-public var fn4: (_ x: main.FnType) -> Void = { _ in }
-public var nested1: (_ x: (FnType) -> Void) -> Void = { _ in }
-public var nested2: (_ x: (main.FnType) -> Void) -> Void = { _ in }
-public func test1(x: FnType) {}
-public func test2(x: main.FnType) {}
+public var fn1: (FnType) -> Void = { _ in } // expected-warning {{should not be declared public}}
+public var fn2: (_ x: FnType) -> Void = { _ in } // expected-warning {{should not be declared public}}
+public var fn3: (main.FnType) -> Void = { _ in } // expected-warning {{should not be declared public}}
+public var fn4: (_ x: main.FnType) -> Void = { _ in } // expected-warning {{should not be declared public}}
+public var nested1: (_ x: (FnType) -> Void) -> Void = { _ in } // expected-warning {{should not be declared public}}
+public var nested2: (_ x: (main.FnType) -> Void) -> Void = { _ in } // expected-warning {{should not be declared public}}
+public func test1(x: FnType) {} // expected-warning {{should not be declared public}}
+public func test2(x: main.FnType) {} // expected-warning {{should not be declared public}}
 
 
 public func reject1(x: FnType?) {} // expected-error {{cannot be declared public}}
@@ -68,8 +68,8 @@ public var escaping3: (@escaping main.FnType) -> Void = { _ in } // expected-err
 public var escaping4: (_ x: @escaping main.FnType) -> Void = { _ in } // expected-error {{cannot be declared public}}
 
 public struct SubscriptTest {
-  public subscript(test1 x: FnType) -> () { return }
-  public subscript(test2 x: main.FnType) -> () { return }
+  public subscript(test1 x: FnType) -> () { return } // expected-warning {{should not be declared public}}
+  public subscript(test2 x: main.FnType) -> () { return } // expected-warning {{should not be declared public}}
 
   public subscript(reject1 x: FnType?) -> () { return } // expected-error {{cannot be declared public}}
   public subscript(reject2 x: main.FnType?) -> () { return } // expected-error {{cannot be declared public}}
diff --git a/test/Sema/accessibility_compound.swift b/test/Sema/accessibility_compound.swift
