[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

@@ -1068,6 +1068,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

@@ -6199,3 +6199,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

@@ -1279,3 +1279,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 {
@@ -1737,6 +1740,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

@@ -7218,6 +7218,7 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
     return { type, count };
   };
 
+  const auto rawType1 = type1;
   type1 = getFixedTypeRecursive(type1, flags, /*wantRValue=*/true);
   type2 = getFixedTypeRecursive(type2, flags, /*wantRValue=*/true);
 
@@ -7258,6 +7259,85 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
     return SolutionKind::Solved;
   }
 
+  // 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();
+
+  // Unless we're allowing the collection compatibility fix, the source cannot
+  // be more optional than the destination.
+  if (!canUseCompatFix && numFromOptionals > numToOptionals)
+    return SolutionKind::Error;
+
+  auto makeCollectionResult = [&](SolutionKind result) -> SolutionKind {
+    // If we encountered an error and can use the compatibility fix, do so.
+    if (canUseCompatFix) {
+      if (numFromOptionals > numToOptionals || 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();
+      auto result = simplifyBridgingConstraint(
+          *fromElement, *toElement, subflags,
+          locator.withPathElement(LocatorPathElt::GenericArgument(0)));
+      return makeCollectionResult(result);
+    }
+  }
+
+  // Bridging the keys/values of a dictionary.
+  if (auto fromKeyValue = isDictionaryType(unwrappedFromType)) {
+    if (auto toKeyValue = isDictionaryType(unwrappedToType)) {
+      ConstraintFix *compatFix = nullptr;
+      if (canUseCompatFix) {
+        compatFix = AllowCoercionToForceCast::create(
+            *this, type1, type2, getConstraintLocator(locator));
+      }
+      addExplicitConversionConstraint(fromKeyValue->first, toKeyValue->first,
+                                      ForgetChoice,
+                                      locator.withPathElement(
+                                        LocatorPathElt::GenericArgument(0)),
+                                      compatFix);
+      addExplicitConversionConstraint(fromKeyValue->second, toKeyValue->second,
+                                      ForgetChoice,
+                                      locator.withPathElement(
+                                        LocatorPathElt::GenericArgument(1)),
+                                      compatFix);
+      countOptionalInjections();
+      return makeCollectionResult(SolutionKind::Solved);
+    }
+  }
+
+  // Bridging the elements of a set.
+  if (auto fromElement = isSetType(unwrappedFromType)) {
+    if (auto toElement = isSetType(unwrappedToType)) {
+      countOptionalInjections();
+      auto result = simplifyBridgingConstraint(
+          *fromElement, *toElement, subflags,
+          locator.withPathElement(LocatorPathElt::GenericArgument(0)));
+      return makeCollectionResult(result);
+    }
+  }
+
   // The source cannot be more optional than the destination, because bridging
   // conversions don't allow us to implicitly check for a value in the optional.
   if (numFromOptionals > numToOptionals) {
@@ -7348,42 +7428,6 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
     }
   }
 
-  // Bridging the elements of an array.
-  if (auto fromElement = isArrayType(unwrappedFromType)) {
-    if (auto toElement = isArrayType(unwrappedToType)) {
-      countOptionalInjections();
-      return simplifyBridgingConstraint(
-          *fromElement, *toElement, subflags,
-          locator.withPathElement(LocatorPathElt::GenericArgument(0)));
-    }
-  }
-
-  // Bridging the keys/values of a dictionary.
-  if (auto fromKeyValue = isDictionaryType(unwrappedFromType)) {
-    if (auto toKeyValue = isDictionaryType(unwrappedToType)) {
-      addExplicitConversionConstraint(fromKeyValue->first, toKeyValue->first,
-                                      ForgetChoice,
-                                      locator.withPathElement(
-                                        LocatorPathElt::GenericArgument(0)));
-      addExplicitConversionConstraint(fromKeyValue->second, toKeyValue->second,
-                                      ForgetChoice,
-                                      locator.withPathElement(
-                                        LocatorPathElt::GenericArgument(1)));
-      countOptionalInjections();
-      return SolutionKind::Solved;
-    }
-  }
-
-  // Bridging the elements of a set.
-  if (auto fromElement = isSetType(unwrappedFromType)) {
-    if (auto toElement = isSetType(unwrappedToType)) {
-      countOptionalInjections();
-      return simplifyBridgingConstraint(
-          *fromElement, *toElement, subflags,
-          locator.withPathElement(LocatorPathElt::GenericArgument(0)));
-    }
-  }
-
   return SolutionKind::Error;
 }
 
@@ -9331,7 +9375,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;
   }
 
@@ -9795,7 +9840,7 @@ void ConstraintSystem::addFixConstraint(ConstraintFix *fix, ConstraintKind kind,
 
 void ConstraintSystem::addExplicitConversionConstraint(
     Type fromType, Type toType, RememberChoice_t rememberChoice,
-    ConstraintLocatorBuilder locator) {
+    ConstraintLocatorBuilder locator, ConstraintFix *compatFix) {
   SmallVector<Constraint *, 3> constraints;
 
   auto locatorPtr = getConstraintLocator(locator);
@@ -9813,12 +9858,21 @@ void ConstraintSystem::addExplicitConversionConstraint(
                      fromType, toType, locatorPtr);
   constraints.push_back(bridgingConstraint);
 
+  // If we're allowed to use a compatibility fix that emits a warning on
+  // failure, add it to the disjunction so that it's recorded on failure.
+  if (compatFix) {
+    constraints.push_back(
+        Constraint::createFixed(*this, ConstraintKind::BridgingConversion,
+                                compatFix, 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:
@@ -9829,7 +9883,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()) {
@@ -9853,6 +9906,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 compatFix A compatibility fix that can be applied if the conversion
+  /// fails.
   void addExplicitConversionConstraint(Type fromType, Type toType,
                                        RememberChoice_t rememberChoice,
-                                       ConstraintLocatorBuilder locator);
+                                       ConstraintLocatorBuilder locator,
+                                       ConstraintFix *compatFix = nullptr);
 
   /// 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

@@ -275,3 +275,63 @@ func test_coercions_with_overloaded_operator(str: String, optStr: String?, veryO
   _ = ohno(ohno(ohno(str))) as String???
   _ = ohno(ohno(ohno(str))) as Int // expected-error {{cannot convert value of type 'String???' to type 'Int' in coercion}}
 }
+
+func id<T>(_ x: T) -> T { x }
+
+func test_compatibility_coercions(_ arr: [Int], _ optArr: [Int]?, _ dict: [String: Int], _ set: Set<Int>) {
+  // Successful coercions don't raise a warning.
+  _ = arr as [Any]?
+  _ = dict as [String: Int]?
+  _ = set as Set<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}}
+  _ = dict as [String: String] // expected-error {{cannot convert value of type '[String : Int]' to type '[String : String]' in coercion}}
+  // expected-note@-1 {{arguments to generic parameter 'Value' ('Int' and 'String') are expected to be equal}}
+  _ = dict as [String: String]? // expected-error {{cannot convert value of type '[String : Int]' to type '[String : String]?' in coercion}}
+  _ = (dict as [String: Int]?) as [String: Int] // expected-error {{value of optional type '[String : Int]?' must be unwrapped to a value of type '[String : Int]'}}
+  // expected-note@-1 {{coalesce using '??' to provide a default when the optional value contains 'nil'}}
+  // expected-note@-2 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}}
+  _ = set as Set<String>  // expected-error {{cannot convert value of type 'Set<Int>' to type 'Set<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}}
+
+  // Allow the coercion to increase optionality.
+  _ = (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 {{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 {{left side of nil coalescing operator '??' has non-optional type '[Int]', so the right side is never used}}
+  _ = (dict ?? [:]) as [String: String?]? // expected-warning {{coercion from '[String : Int]' to '[String : String?]?' may fail; use 'as?' or 'as!' instead}}
+  // expected-warning@-1 {{left side of nil coalescing operator '??' has non-optional type '[String : Int]', so the right side is never used}}
+  _ = (set ?? []) as Set<String>?? // expected-warning {{coercion from 'Set<Int>' to 'Set<String>??' may fail; use 'as?' or 'as!' instead}}
+  // expected-warning@-1 {{left side of nil coalescing operator '??' has non-optional type 'Set<Int>', so the right side is never used}}
+
+  // Allow the coercion to decrease optionality.
+  _ = ohno(ohno(ohno(arr))) as [String] // expected-warning {{coercion from '[Int]???' to '[String]' may fail; use 'as?' or 'as!' instead}}
+  _ = ohno(ohno(ohno(arr))) as [Int] // expected-warning {{coercion from '[Int]???' to '[Int]' may fail; use 'as?' or 'as!' instead}}
+  _ = ohno(ohno(ohno(Set<Int>()))) as Set<String> // expected-warning {{coercion from 'Set<Int>???' to 'Set<String>' may fail; use 'as?' or 'as!' instead}}
+  _ = ohno(ohno(ohno(["": ""]))) as [Int: String] // expected-warning {{coercion from '[String : String]???' to '[Int : String]' may fail; use 'as?' or 'as!' instead}}
+  _ = ohno(ohno(ohno(dict))) as [String: Int] // expected-warning {{coercion from '[String : Int]???' to '[String : Int]' 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]
+
+  // 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}}
+}