[SR-13088] Fix false positive downcast unrelated of types that cannot be statically known (swiftlang#32592)

LucianoPAlmeida · web-flow · commit 28fb66cbaf70 · 2020-07-02T22:06:29.000-03:00
* [TypeCheckConstraints] Adjusting cases where checked casts that cannot be determined statically were producing misleading warnings

* [tests] Adding regression tests for SR-13088

* [TypeCheckConstraints] Adjusting comment and adding an extra test case for SR13035

* [TypeCheckConstraints] Fixing typos in comments

* [AST] Moving implementation of isCollection from ConstraintSystem to AST TypeBase

* [TypeCheckConstraints] Adjusting logic to verify specific conformance to stdlib collection type before emit an downcast warning

* [TypeCheckConstraints] Creating new CheckedCastContextKind::CollectionElement to be able to verify special cases within typeCheckCheckedCast for collection elements

* [TypeCheckConstraints] Adjusting logic around generic substitution to check both subtype and supertype

* [Sema] Adding isKnownStdlibCollectionType and replacing all usages contraint system method

* [TypeChecker] Reverting fixes around array element types

* [TypeChecker] Abstract logic of check for conditional requirements on TypeChecker::couldDynamicallyConformToProtocol

* [TypeChecker] Ajdustinc can conformDynamically conform and adjust review comments

* [TypeChecker] Ajusting comments and fixing typos

* [TypeChecker] Adjusting existential and archetype logic to check inside couldDynamicConform

* [TypeChecker] Adjusting minor and adding existential check into couldDynamically conform.

* [TypeChecker] Adjusting comments
diff --git a/include/swift/AST/Types.h b/include/swift/AST/Types.h
@@ -785,6 +785,10 @@ class alignas(1 << TypeAlignInBits) TypeBase {
   
   /// Check if this is a nominal type defined at the top level of the Swift module
   bool isStdlibType();
+  
+  /// Check if this is either an Array, Set or Dictionary collection type defined
+  /// at the top level of the Swift module
+  bool isKnownStdlibCollectionType();
 
   /// If this is a class type or a bound generic class type, returns the
   /// (possibly generic) class.
diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp
@@ -760,6 +760,16 @@ bool TypeBase::isStdlibType() {
   return false;
 }
 
+bool TypeBase::isKnownStdlibCollectionType() {
+  if (auto *structType = getAs<BoundGenericStructType>()) {
+    auto &ctx = getASTContext();
+    auto *decl = structType->getDecl();
+    return decl == ctx.getArrayDecl() || decl == ctx.getDictionaryDecl() ||
+           decl == ctx.getSetDecl();
+  }
+  return false;
+}
+
 /// Remove argument labels from the function type.
 Type TypeBase::removeArgumentLabels(unsigned numArgumentLabels) {
   // If there is nothing to remove, don't.
diff --git a/lib/Sema/CSBindings.cpp b/lib/Sema/CSBindings.cpp
@@ -1035,7 +1035,7 @@ bool TypeVarBindingProducer::computeNext() {
     auto srcLocator = binding.getLocator();
     if (srcLocator &&
         srcLocator->isLastElement<LocatorPathElt::ApplyArgToParam>() &&
-        !type->hasTypeVariable() && CS.isCollectionType(type)) {
+        !type->hasTypeVariable() && type->isKnownStdlibCollectionType()) {
       // If the type binding comes from the argument conversion, let's
       // instead of binding collection types directly, try to bind
       // using temporary type variables substituted for element
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -3376,7 +3376,7 @@ bool MissingMemberFailure::diagnoseInLiteralCollectionContext() const {
 
   auto parentType = getType(parentExpr);
 
-  if (!isCollectionType(parentType) && !parentType->is<TupleType>())
+  if (!parentType->isKnownStdlibCollectionType() && !parentType->is<TupleType>())
     return false;
 
   if (isa<TupleExpr>(parentExpr)) {
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
@@ -208,11 +208,6 @@ class FailureDiagnostic {
       llvm::function_ref<void(GenericTypeParamType *, Type)> substitution =
           [](GenericTypeParamType *, Type) {});
 
-  bool isCollectionType(Type type) const {
-    auto &cs = getConstraintSystem();
-    return cs.isCollectionType(type);
-  }
-
   bool isArrayType(Type type) const {
     auto &cs = getConstraintSystem();
     return bool(cs.isArrayType(type));
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -3708,7 +3708,7 @@ bool ConstraintSystem::repairFailures(
     // func foo<T>(_: [T]) {}
     // foo(1) // expected '[Int]', got 'Int'
     // ```
-    if (isCollectionType(rhs)) {
+    if (rhs->isKnownStdlibCollectionType()) {
       std::function<Type(Type)> getArrayOrSetType = [&](Type type) -> Type {
         if (auto eltTy = isArrayType(type))
           return getArrayOrSetType(*eltTy);
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -878,18 +878,6 @@ Optional<Type> ConstraintSystem::isSetType(Type type) {
   return None;
 }
 
-bool ConstraintSystem::isCollectionType(Type type) {
-  if (auto *structType = type->getAs<BoundGenericStructType>()) {
-    auto &ctx = type->getASTContext();
-    auto *decl = structType->getDecl();
-    if (decl == ctx.getArrayDecl() || decl == ctx.getDictionaryDecl() ||
-        decl == ctx.getSetDecl())
-      return true;
-  }
-
-  return false;
-}
-
 bool ConstraintSystem::isAnyHashableType(Type type) {
   if (auto st = type->getAs<StructType>()) {
     auto &ctx = type->getASTContext();
diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h
@@ -3348,9 +3348,6 @@ class ConstraintSystem {
   /// element type of the set.
   static Optional<Type> isSetType(Type t);
 
-  /// Determine if the type in question is one of the known collection types.
-  static bool isCollectionType(Type t);
-
   /// Determine if the type in question is AnyHashable.
   static bool isAnyHashableType(Type t);
 
diff --git a/lib/Sema/TypeCheckConstraints.cpp b/lib/Sema/TypeCheckConstraints.cpp
@@ -3357,7 +3357,14 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType,
         const auto &fromElt = fromTuple->getElement(i);
         const auto &toElt = toTuple->getElement(i);
 
-        if (fromElt.getName() != toElt.getName())
+        // We should only perform name validation if both element have a label,
+        // because unlabeled tuple elements can be converted to labeled ones
+        // e.g.
+        // 
+        // let tup: (Any, Any) = (1, 1)
+        // _ = tup as! (a: Int, Int)
+        if ((!fromElt.getName().empty() && !toElt.getName().empty()) &&
+            fromElt.getName() != toElt.getName())
           return failed();
 
         auto result = checkElementCast(fromElt.getType(), toElt.getType(),
@@ -3527,8 +3534,9 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType,
         (toType->isAnyObject() || fromType->isAnyObject()))
       return CheckedCastKind::ValueCast;
 
-    // A cast from an existential type to a concrete type does not succeed. For
-    // example:
+    // If we have a cast from an existential type to a concrete type that we
+    // statically know doesn't conform to the protocol, mark the cast as always
+    // failing. For example:
     //
     // struct S {}
     // enum FooError: Error { case bar }
@@ -3541,19 +3549,10 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType,
     //   }
     // }
     //
-    // Note: we relax the restriction if the type we're casting to is a
-    // non-final class because it's possible that we might have a subclass
-    // that conforms to the protocol.
-    if (fromExistential && !toExistential) {
-      if (auto NTD = toType->getAnyNominal()) {
-        if (!toType->is<ClassType>() || NTD->isFinal()) {
-          auto protocolDecl =
-              dyn_cast_or_null<ProtocolDecl>(fromType->getAnyNominal());
-          if (protocolDecl &&
-              !conformsToProtocol(toType, protocolDecl, dc)) {
-            return failed();
-          }
-        }
+    if (auto *protocolDecl =
+          dyn_cast_or_null<ProtocolDecl>(fromType->getAnyNominal())) {
+      if (!couldDynamicallyConformToProtocol(toType, protocolDecl, dc)) {
+        return failed();
       }
     }
 
@@ -3616,10 +3615,12 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType,
     return CheckedCastKind::ValueCast;
   }
 
-  // If the destination type can be a supertype of the source type, we are
-  // performing what looks like an upcast except it rebinds generic
-  // parameters.
-  if (fromType->isBindableTo(toType))
+  // We perform an upcast while rebinding generic parameters if it's possible
+  // to substitute the generic arguments of the source type with the generic
+  // archetypes of the destination type. Or, if it's possible to substitute
+  // the generic arguments of the destination type with the generic archetypes
+  // of the source type, we perform a downcast instead.
+  if (toType->isBindableTo(fromType) || fromType->isBindableTo(toType))
     return CheckedCastKind::ValueCast;
   
   // Objective-C metaclasses are subclasses of NSObject in the ObjC runtime,
@@ -3636,8 +3637,8 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType,
     }
   }
 
-  // We can conditionally cast from NSError to an Error-conforming
-  // type.  This is handled in the runtime, so it doesn't need a special cast
+  // We can conditionally cast from NSError to an Error-conforming type.
+  // This is handled in the runtime, so it doesn't need a special cast
   // kind.
   if (Context.LangOpts.EnableObjCInterop) {
     auto nsObject = Context.getNSObjectType();
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -4527,6 +4527,40 @@ TypeChecker::conformsToProtocol(Type T, ProtocolDecl *Proto, DeclContext *DC,
   return lookupResult;
 }
 
+bool
+TypeChecker::couldDynamicallyConformToProtocol(Type type, ProtocolDecl *Proto,
+                                               DeclContext *DC) {
+  // An existential may have a concrete underlying type with protocol conformances
+  // we cannot know statically.
+  if (type->isExistentialType())
+    return true;
+  
+  // A generic archetype may have protocol conformances we cannot know
+  // statically.
+  if (type->is<ArchetypeType>())
+    return true;
+  
+  // A non-final class might have a subclass that conforms to the protocol.
+  if (auto *classDecl = type->getClassOrBoundGenericClass()) {
+    if (!classDecl->isFinal())
+      return true;
+  }
+
+  ModuleDecl *M = DC->getParentModule();
+  // For standard library collection types such as Array, Set or Dictionary
+  // which have custom casting machinery implemented in situations like:
+  //
+  //  func encodable(_ value: Encodable) {
+  //    _ = value as! [String : Encodable]
+  //  }
+  // we are skipping checking conditional requirements using lookupConformance,
+  // as an intermediate collection cast can dynamically change if the conditions
+  // are met or not.
+  if (type->isKnownStdlibCollectionType())
+    return !M->lookupConformance(type, Proto).isInvalid();
+  return !conformsToProtocol(type, Proto, DC).isInvalid();
+}
+
 /// Exposes TypeChecker functionality for querying protocol conformance.
 /// Returns a valid ProtocolConformanceRef only if all conditional
 /// requirements are successfully resolved.
diff --git a/lib/Sema/TypeChecker.h b/lib/Sema/TypeChecker.h
@@ -884,6 +884,17 @@ ProtocolConformanceRef conformsToProtocol(Type T, ProtocolDecl *Proto,
                                           DeclContext *DC,
                                           SourceLoc ComplainLoc = SourceLoc());
 
+/// This is similar to \c conformsToProtocol, but returns \c true for cases where
+/// the type \p T could be dynamically cast to \p Proto protocol, such as a non-final
+/// class where a subclass conforms to \p Proto.
+///
+/// \param DC The context in which to check conformance. This affects, for
+/// example, extension visibility.
+///
+///
+/// \returns True if \p T conforms to the protocol \p Proto, false otherwise.
+bool couldDynamicallyConformToProtocol(Type T, ProtocolDecl *Proto,
+                                       DeclContext *DC);
 /// Completely check the given conformance.
 void checkConformance(NormalProtocolConformance *conformance);
 
diff --git a/test/Constraints/casts.swift b/test/Constraints/casts.swift
@@ -235,8 +235,8 @@ func test_tuple_casts_no_warn() {
   _ = arr as! [(Foo, Foo, Foo)] // expected-warning {{cast from '[(Any, Any)]' to unrelated type '[(Foo, Foo, Foo)]' always fails}}
   _ = tup as! (Foo, Foo, Foo) // expected-warning {{cast from '(Any, Any)' to unrelated type '(Foo, Foo, Foo)' always fails}}
 
-  _ = arr as! [(a: Foo, Foo)] // expected-warning {{cast from '[(Any, Any)]' to unrelated type '[(a: Foo, Foo)]' always fails}}
-  _ = tup as! (a: Foo, Foo) // expected-warning {{cast from '(Any, Any)' to unrelated type '(a: Foo, Foo)' always fails}}
+  _ = arr as! [(a: Foo, Foo)] // Ok
+  _ = tup as! (a: Foo, Foo) // Ok
 }
 
 infix operator ^^^
@@ -335,3 +335,71 @@ func test_compatibility_coercions(_ arr: [Int], _ optArr: [Int]?, _ dict: [Strin
   // The array can also be inferred to be [Any].
   _ = ([] ?? []) as Array // expected-warning {{left side of nil coalescing operator '??' has non-optional type '[Any]', so the right side is never used}}
 }
+
+// SR-13088
+protocol JSON { }
+protocol JSONLeaf: JSON {}
+extension Int: JSONLeaf { }
+extension Array: JSON where Element: JSON { }
+
+protocol SR13035Error: Error {}
+class ChildError: SR13035Error {}
+
+protocol AnyC {
+  func foo()
+}
+
+protocol AnyEvent {}
+
+protocol A {
+  associatedtype C: AnyC
+}
+
+protocol EventA: A {
+  associatedtype Event
+}
+
+typealias Container<Namespace>
+  = (event: Namespace.Event, c: Namespace.C) where Namespace: EventA
+
+enum ConcreteA: EventA {
+  struct C: AnyC {
+    func foo() {}
+  }
+
+  enum Event: AnyEvent {
+    case test
+  }
+}
+
+func tests_SR13088_false_positive_always_fail_casts() {
+  // SR-13081
+  let x: JSON = [4] // [4]
+  _ = x as? [Any] // Ok
+
+  // SR-13035
+  func SR13035<SomeError: SR13035Error>(_ child: Result<String, ChildError>, _: Result<String, SomeError>) {
+    let _ = child as? Result<String, SomeError> // Ok
+  }
+
+  func SR13035_1<SomeError: SR13035Error, Child: ChildError>(_ child: Result<String, Child>, parent: Result<String, SomeError>) {
+    _ = child as? Result<String, SomeError> // Ok
+    _ = parent as? Result<String, Child> // OK
+  }
+
+  // SR-11434 and SR-12321
+  func encodable(_ value: Encodable) {
+    _ = value as! [String : Encodable] // Ok
+    _ = value as? [String: Encodable] // Ok
+  }   
+
+  // SR-13025
+  func coordinate(_ event: AnyEvent, from c: AnyC) {
+    switch (event, c) {
+    case let container as Container<ConcreteA>: // OK
+      container.c.foo()
+    default:
+      break
+    }
+  }
+}
