[CS] Preserve compatibility for collection coercions

Previously we could allow some invalid coercions to
sneak past Sema. In most cases these would either
cause crashes later down the pipeline or
miscompiles. However, for coercions between
collections, we emitted somewhat reasonable code
that performed a force cast.

This commit aims to preserve compatibility with
those collection coercions that previously
compiled, and emits a warning telling the user to
use either 'as?' or 'as!' instead.

Author: hamishknight

include/swift/AST/DiagnosticsSema.def

@@ -1057,6 +1057,10 @@ ERROR(missing_unwrap_optional_try,none,
 ERROR(missing_forced_downcast,none,
       "%0 is not convertible to %1; "
       "did you mean to use 'as!' to force downcast?", (Type, Type))
+WARNING(coercion_may_fail_warning,none,
+        "coercion from %0 to %1 may fail; use 'as?' or 'as!' instead",
+        (Type, Type))
+
 ERROR(missing_explicit_conversion,none,
       "%0 is not implicitly convertible to %1; "
       "did you mean to use 'as' to explicitly convert?", (Type, Type))

lib/Sema/CSDiagnostics.cpp

@@ -6184,3 +6184,11 @@ bool MissingQuialifierInMemberRefFailure::diagnoseAsError() {
   emitDiagnostic(choice, diag::decl_declared_here, choice->getFullName());
   return true;
 }
+
+bool CoercionAsForceCastFailure::diagnoseAsError() {
+  auto *coercion = cast<CoerceExpr>(getRawAnchor());
+  emitDiagnostic(coercion->getLoc(), diag::coercion_may_fail_warning,
+                 getFromType(), getToType())
+      .highlight(coercion->getSourceRange());
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -1977,6 +1977,16 @@ class MissingQuialifierInMemberRefFailure final : public FailureDiagnostic {
   bool diagnoseAsError();
 };
 
+/// Emits a warning about an attempt to use the 'as' operator as the 'as!'
+/// operator.
+class CoercionAsForceCastFailure final : public ContextualFailure {
+public:
+  CoercionAsForceCastFailure(const Solution &solution, Type fromType,
+                             Type toType, ConstraintLocator *locator)
+      : ContextualFailure(solution, fromType, toType, locator) {}
+
+  bool diagnoseAsError() override;
+};
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -1276,3 +1276,17 @@ AddQualifierToAccessTopLevelName::create(ConstraintSystem &cs,
                                          ConstraintLocator *locator) {
   return new (cs.getAllocator()) AddQualifierToAccessTopLevelName(cs, locator);
 }
+
+bool AllowCoercionToForceCast::diagnose(const Solution &solution,
+                                        bool asNote) const {
+  CoercionAsForceCastFailure failure(solution, getFromType(), getToType(),
+                                     getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowCoercionToForceCast *
+AllowCoercionToForceCast::create(ConstraintSystem &cs, Type fromType,
+                                 Type toType, ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      AllowCoercionToForceCast(cs, fromType, toType, locator);
+}

lib/Sema/CSFix.h

@@ -248,6 +248,9 @@ enum class FixKind : uint8_t {
   /// Member shadows a top-level name, such a name could only be accessed by
   /// prefixing it with a module name.
   AddQualifierToAccessTopLevelName,
+
+  /// A warning fix that allows a coercion to perform a force-cast.
+  AllowCoercionToForceCast,
 };
 
 class ConstraintFix {
@@ -1728,6 +1731,34 @@ class AllowNonClassTypeToConvertToAnyObject final : public ContextualMismatch {
   create(ConstraintSystem &cs, Type type, ConstraintLocator *locator);
 };
 
+/// A warning fix to maintain compatibility with the following:
+///
+/// \code
+/// func foo(_ arr: [Any]?) {
+///  _ = (arr ?? []) as [NSObject]
+/// }
+/// \endcode
+///
+/// which performs a force-cast of the array's elements, despite being spelled
+/// as a coercion.
+class AllowCoercionToForceCast final : public ContextualMismatch {
+  AllowCoercionToForceCast(ConstraintSystem &cs, Type fromType, Type toType,
+                           ConstraintLocator *locator)
+      : ContextualMismatch(cs, FixKind::AllowCoercionToForceCast, fromType,
+                           toType, locator, /*warning*/ true) {}
+
+public:
+  std::string getName() const {
+    return "allow coercion to be treated as a force-cast";
+  }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const;
+
+  static AllowCoercionToForceCast *create(ConstraintSystem &cs, Type fromType,
+                                          Type toType,
+                                          ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -7212,6 +7212,7 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
     return { type, count };
   };
 
+  const auto rawType1 = type1;
   type1 = getFixedTypeRecursive(type1, flags, /*wantRValue=*/true);
   type2 = getFixedTypeRecursive(type2, flags, /*wantRValue=*/true);
 
@@ -7342,13 +7343,41 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
     }
   }
 
+  // In a previous version of Swift, we could accidently drop the coercion
+  // constraint in certain cases. In most cases this led to either miscompiles
+  // or crashes later down the pipeline, but for coercions between collections
+  // we generated somewhat reasonable code that performed a force cast. To
+  // maintain compatibility with that behavior, allow the coercion between
+  // two collections, but add a warning fix telling the user to use as! or as?
+  // instead.
+  //
+  // We only need to perform this compatibility logic if the LHS type is a
+  // (potentially optional) type variable, as only such a constraint could have
+  // been previously been left unsolved.
+  //
+  // FIXME: Once we get a new language version, change this condition to only
+  // preserve compatibility for Swift 5.x mode.
+  auto canUseCompatFix =
+      rawType1->lookThroughAllOptionalTypes()->isTypeVariableOrMember();
+
+  auto makeCollectionResult = [&](SolutionKind result) -> SolutionKind {
+    // If we encountered an error and can use the compatibility fix, do so.
+    if (canUseCompatFix && result == SolutionKind::Error) {
+      auto *loc = getConstraintLocator(locator);
+      auto *fix = AllowCoercionToForceCast::create(*this, type1, type2, loc);
+      return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
+    }
+    return result;
+  };
+
   // Bridging the elements of an array.
   if (auto fromElement = isArrayType(unwrappedFromType)) {
     if (auto toElement = isArrayType(unwrappedToType)) {
       countOptionalInjections();
-      return simplifyBridgingConstraint(
+      auto result = simplifyBridgingConstraint(
           *fromElement, *toElement, subflags,
           locator.withPathElement(LocatorPathElt::GenericArgument(0)));
+      return makeCollectionResult(result);
     }
   }
 
@@ -7358,11 +7387,13 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
       addExplicitConversionConstraint(fromKeyValue->first, toKeyValue->first,
                                       ForgetChoice,
                                       locator.withPathElement(
-                                        LocatorPathElt::GenericArgument(0)));
+                                        LocatorPathElt::GenericArgument(0)),
+                                      /*warnOnFailure*/ canUseCompatFix);
       addExplicitConversionConstraint(fromKeyValue->second, toKeyValue->second,
                                       ForgetChoice,
                                       locator.withPathElement(
-                                        LocatorPathElt::GenericArgument(1)));
+                                        LocatorPathElt::GenericArgument(1)),
+                                      /*warnOnFailure*/ canUseCompatFix);
       countOptionalInjections();
       return SolutionKind::Solved;
     }
@@ -7372,9 +7403,10 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
   if (auto fromElement = isSetType(unwrappedFromType)) {
     if (auto toElement = isSetType(unwrappedToType)) {
       countOptionalInjections();
-      return simplifyBridgingConstraint(
+      auto result = simplifyBridgingConstraint(
           *fromElement, *toElement, subflags,
           locator.withPathElement(LocatorPathElt::GenericArgument(0)));
+      return makeCollectionResult(result);
     }
   }
 
@@ -9332,7 +9364,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::ExplicitlyConstructRawRepresentable:
   case FixKind::SpecifyBaseTypeForContextualMember:
   case FixKind::CoerceToCheckedCast:
-  case FixKind::SpecifyObjectLiteralTypeImport: {
+  case FixKind::SpecifyObjectLiteralTypeImport:
+  case FixKind::AllowCoercionToForceCast: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }
 
@@ -9796,7 +9829,7 @@ void ConstraintSystem::addFixConstraint(ConstraintFix *fix, ConstraintKind kind,
 
 void ConstraintSystem::addExplicitConversionConstraint(
     Type fromType, Type toType, RememberChoice_t rememberChoice,
-    ConstraintLocatorBuilder locator) {
+    ConstraintLocatorBuilder locator, bool warnOnFailure) {
   SmallVector<Constraint *, 3> constraints;
 
   auto locatorPtr = getConstraintLocator(locator);
@@ -9814,12 +9847,23 @@ void ConstraintSystem::addExplicitConversionConstraint(
                      fromType, toType, locatorPtr);
   constraints.push_back(bridgingConstraint);
 
+  // If we're allowed to emit a warning on failure, add an additional constraint
+  // to the disjunction that just records a warning fix and succeeds.
+  if (warnOnFailure) {
+    auto *fix =
+        AllowCoercionToForceCast::create(*this, fromType, toType, locatorPtr);
+    constraints.push_back(
+        Constraint::createFixed(*this, ConstraintKind::BridgingConversion, fix,
+                                fromType, toType, locatorPtr));
+  }
+
   addDisjunctionConstraint(constraints, locator, rememberChoice);
 }
 
 ConstraintSystem::SolutionKind
 ConstraintSystem::simplifyConstraint(const Constraint &constraint) {
-  switch (constraint.getKind()) {
+  auto matchKind = constraint.getKind();
+  switch (matchKind) {
   case ConstraintKind::Bind:
   case ConstraintKind::Equal:
   case ConstraintKind::BindParam:
@@ -9830,7 +9874,6 @@ ConstraintSystem::simplifyConstraint(const Constraint &constraint) {
   case ConstraintKind::OperatorArgumentConversion:
   case ConstraintKind::OpaqueUnderlyingType: {
     // Relational constraints.
-    auto matchKind = constraint.getKind();
 
     // If there is a fix associated with this constraint, apply it.
     if (auto fix = constraint.getFix()) {
@@ -9854,6 +9897,13 @@ ConstraintSystem::simplifyConstraint(const Constraint &constraint) {
   }
 
   case ConstraintKind::BridgingConversion:
+    // If there is a fix associated with this constraint, apply it.
+    if (auto fix = constraint.getFix()) {
+      return simplifyFixConstraint(fix, constraint.getFirstType(),
+                                   constraint.getSecondType(), matchKind, None,
+                                   constraint.getLocator());
+    }
+
     return simplifyBridgingConstraint(constraint.getFirstType(),
                                       constraint.getSecondType(),
                                       None, constraint.getLocator());

lib/Sema/ConstraintSystem.h

@@ -2861,9 +2861,12 @@ class ConstraintSystem {
   /// \param rememberChoice Whether the conversion disjunction should record its
   /// choice.
   /// \param locator The locator.
+  /// \param warnOnFailure Whether a warning fix should be used for recovery if
+  /// the conversion fails. This is used to preserve compatibility.
   void addExplicitConversionConstraint(Type fromType, Type toType,
                                        RememberChoice_t rememberChoice,
-                                       ConstraintLocatorBuilder locator);
+                                       ConstraintLocatorBuilder locator,
+                                       bool warnOnFailure = false);
 
   /// Add a disjunction constraint.
   void

test/Constraints/bridging.swift

@@ -373,7 +373,7 @@ func bridgeTupleToAnyObject() {
 
 // Array defaulting and bridging type checking error per rdar://problem/54274245
 func rdar54274245(_ arr: [Any]?) {
-  _ = (arr ?? []) as [NSObject] // expected-error {{type of expression is ambiguous without more context}}
+  _ = (arr ?? []) as [NSObject] // expected-warning {{coercion from '[Any]' to '[NSObject]' may fail; use 'as?' or 'as!' instead}}
 }
 
 // rdar://problem/60501780 - failed to infer NSString as a value type of a dictionary

test/Constraints/casts.swift

@@ -263,3 +263,28 @@ func test_coercions_with_overloaded_operator(str: String, optStr: String?, veryO
   _ = (veryOptString ?? "" ?? "") as [String] // expected-error {{cannot convert value of type 'String???' to type '[String]' in coercion}}
   _ = (veryOptString ?? "" ?? "" ?? "" ?? "") as [String] // expected-error {{cannot convert value of type 'String???' to type '[String]' in coercion}}
 }
+
+func id<T>(_ x: T) -> T { x }
+
+func test_compatibility_coercions(_ arr: [Int], _ optArr: [Int]?) {
+  // Don't fix the simple case where no type variable is introduced, that was
+  // always disallowed.
+  _ = arr as [String] // expected-error {{cannot convert value of type '[Int]' to type '[String]' in coercion}}
+  // expected-note@-1 {{arguments to generic parameter 'Element' ('Int' and 'String') are expected to be equal}}
+
+  // Apply the compatibility logic when a type variable is introduced. It's
+  // unfortunate that this means we'll temporarily accept code we didn't before,
+  // but it at least means we shouldn't break compatibility with anything.
+  _ = id(arr) as [String] // expected-warning {{coercion from '[Int]' to '[String]' may fail; use 'as?' or 'as!' instead}}
+
+  _ = (arr ?? []) as [String] // expected-warning {{coercion from '[Int]' to '[String]' may fail; use 'as?' or 'as!' instead}}
+  // expected-warning@-1 {{left side of nil coalescing operator '??' has non-optional type '[Int]', so the right side is never used}}
+  _ = (arr ?? [] ?? []) as [String] // expected-warning {{coercion from '[Int]' to '[String]' may fail; use 'as?' or 'as!' instead}}
+  // expected-warning@-1 2{{left side of nil coalescing operator '??' has non-optional type '[Int]', so the right side is never used}}
+  _ = (optArr ?? []) as [String] // expected-warning {{coercion from '[Int]' to '[String]' may fail; use 'as?' or 'as!' instead}}
+
+  // In this case the array literal can be inferred to be [String], so totally
+  // valid.
+  _ = ([] ?? []) as [String] // expected-warning {{left side of nil coalescing operator '??' has non-optional type '[String]', so the right side is never used}}
+  _ = (([] as Optional) ?? []) as [String]
+}

test/SILGen/collection_downcast.swift

@@ -239,3 +239,10 @@ func testSetDowncastBridgedConditional(_ dict: Set<NSObject>)
   // CHECK-NOT: destroy_value [[SET]]
   return dict as? Set<BridgedSwift>
 }
+
+// CHECK-LABEL: sil hidden [ossa] @$s19collection_downcast35testDeprecatedAnyToNSObjectCoercionyySayypGSgF
+func testDeprecatedAnyToNSObjectCoercion(_ arr: [Any]?) {
+  // CHECK: [[CAST_FN:%.+]] = function_ref @$ss15_arrayForceCastySayq_GSayxGr0_lF : $@convention(thin) <τ_0_0, τ_0_1> (@guaranteed Array<τ_0_0>) -> @owned Array<τ_0_1>
+  // CHECK: apply [[CAST_FN]]<Any, NSObject>({{%.+}}) : $@convention(thin) <τ_0_0, τ_0_1> (@guaranteed Array<τ_0_0>) -> @owned Array<τ_0_1> // user: %20
+  _ = (arr ?? []) as [NSObject]
+}