Merge pull request swiftlang#34883 from LucianoPAlmeida/SR-13899-coerce-to-checked

[SR-13899] [Sema] Adjustments on coerce to checked cast diagnostics

Author: LucianoPAlmeida

include/swift/AST/DiagnosticsSema.def

@@ -1112,9 +1112,13 @@ ERROR(missing_unwrap_optional_try,none,
       "value of optional type %0 not unwrapped; did you mean to use 'try!' "
       "or chain with '?'?",
      (Type))
-ERROR(missing_forced_downcast,none,
-      "%0 is not convertible to %1; "
-      "did you mean to use 'as!' to force downcast?", (Type, Type))
+ERROR(cannot_coerce_to_type, none,
+      "%0 is not convertible to %1", (Type, Type))
+NOTE(missing_forced_downcast, none,
+     "did you mean to use 'as!' to force downcast?", ())
+NOTE(missing_optional_downcast, none,
+     "did you mean to use 'as?' to conditionally downcast?", ())
+     
 WARNING(coercion_may_fail_warning,none,
         "coercion from %0 to %1 may fail; use 'as?' or 'as!' instead",
         (Type, Type))

include/swift/Sema/CSFix.h

@@ -1728,20 +1728,25 @@ class UseRawValue final : public ConstraintFix {
                              Type expectedType, ConstraintLocator *locator);
 };
 
-/// Replace a coercion ('as') with a forced checked cast ('as!').
+/// Replace a coercion ('as') with runtime checked cast ('as!' or 'as?').
 class CoerceToCheckedCast final : public ContextualMismatch {
   CoerceToCheckedCast(ConstraintSystem &cs, Type fromType, Type toType,
-                      ConstraintLocator *locator)
+                      bool useConditionalCast, ConstraintLocator *locator)
       : ContextualMismatch(cs, FixKind::CoerceToCheckedCast, fromType, toType,
-                           locator) {}
+                           locator),
+        UseConditionalCast(useConditionalCast) {}
+  bool UseConditionalCast = false;
 
 public:
-  std::string getName() const override { return "as to as!"; }
+  std::string getName() const override {
+    return UseConditionalCast ? "as to as?" : "as to as!";
+  }
 
   bool diagnose(const Solution &solution, bool asNote = false) const override;
 
   static CoerceToCheckedCast *attempt(ConstraintSystem &cs, Type fromType,
-                                      Type toType, ConstraintLocator *locator);
+                                      Type toType, bool useConditionalCast,
+                                      ConstraintLocator *locator);
 };
 
 class RemoveInvalidCall final : public ConstraintFix {

lib/Sema/CSDiagnostics.cpp

@@ -964,20 +964,29 @@ bool NoEscapeFuncToTypeConversionFailure::diagnoseParameterUse() const {
   return true;
 }
 
-ASTNode MissingForcedDowncastFailure::getAnchor() const {
+ASTNode InvalidCoercionFailure::getAnchor() const {
   auto anchor = FailureDiagnostic::getAnchor();
   if (auto *assignExpr = getAsExpr<AssignExpr>(anchor))
     return assignExpr->getSrc();
   return anchor;
 }
 
-bool MissingForcedDowncastFailure::diagnoseAsError() {
+bool InvalidCoercionFailure::diagnoseAsError() {
   auto fromType = getFromType();
   auto toType = getToType();
 
-  emitDiagnostic(diag::missing_forced_downcast, fromType, toType)
-      .highlight(getSourceRange())
-      .fixItReplace(getLoc(), "as!");
+  emitDiagnostic(diag::cannot_coerce_to_type, fromType, toType);
+
+  if (UseConditionalCast) {
+    emitDiagnostic(diag::missing_optional_downcast)
+        .highlight(getSourceRange())
+        .fixItReplace(getLoc(), "as?");
+  } else {
+    emitDiagnostic(diag::missing_forced_downcast)
+        .highlight(getSourceRange())
+        .fixItReplace(getLoc(), "as!");
+  }
+
   return true;
 }
 
@@ -1051,10 +1060,19 @@ bool MissingExplicitConversionFailure::diagnoseAsError() {
   if (needsParensOutside)
     insertAfter += ")";
 
-  auto diagID =
-      useAs ? diag::missing_explicit_conversion : diag::missing_forced_downcast;
-  auto diag = emitDiagnostic(diagID, fromType, toType);
+  auto diagnose = [&]() {
+    if (useAs) {
+      return emitDiagnostic(diag::missing_explicit_conversion, fromType,
+                            toType);
+    } else {
+      // Emit error diagnostic.
+      emitDiagnostic(diag::cannot_coerce_to_type, fromType, toType);
+      // Emit and return note suggesting as! where the fix-it will be placed.
+      return emitDiagnostic(diag::missing_forced_downcast);
+    }
+  };
 
+  auto diag = diagnose();
   if (!insertBefore.empty()) {
     diag.fixItInsert(getSourceRange().Start, insertBefore);
   }

lib/Sema/CSDiagnostics.h

@@ -1939,12 +1939,15 @@ class ArgumentMismatchFailure : public ContextualFailure {
   bool diagnoseMisplacedMissingArgument() const;
 };
 
-/// Replace a coercion ('as') with a forced checked cast ('as!').
-class MissingForcedDowncastFailure final : public ContextualFailure {
+/// Replace a coercion ('as') with a runtime checked cast ('as!' or 'as?').
+class InvalidCoercionFailure final : public ContextualFailure {
+  bool UseConditionalCast;
+
 public:
-  MissingForcedDowncastFailure(const Solution &solution, Type fromType,
-                               Type toType, ConstraintLocator *locator)
-      : ContextualFailure(solution, fromType, toType, locator) {}
+  InvalidCoercionFailure(const Solution &solution, Type fromType, Type toType,
+                         bool useConditionalCast, ConstraintLocator *locator)
+      : ContextualFailure(solution, fromType, toType, locator),
+        UseConditionalCast(useConditionalCast) {}
 
   ASTNode getAnchor() const override;
 

lib/Sema/CSFix.cpp

@@ -131,13 +131,14 @@ TreatRValueAsLValue *TreatRValueAsLValue::create(ConstraintSystem &cs,
 
 bool CoerceToCheckedCast::diagnose(const Solution &solution,
                                    bool asNote) const {
-  MissingForcedDowncastFailure failure(solution, getFromType(), getToType(),
-                                       getLocator());
+  InvalidCoercionFailure failure(solution, getFromType(), getToType(),
+                                 UseConditionalCast, getLocator());
   return failure.diagnose(asNote);
 }
 
 CoerceToCheckedCast *CoerceToCheckedCast::attempt(ConstraintSystem &cs,
                                                   Type fromType, Type toType,
+                                                  bool useConditionalCast,
                                                   ConstraintLocator *locator) {
   // If any of the types has a type variable, don't add the fix.
   if (fromType->hasTypeVariable() || toType->hasTypeVariable())
@@ -159,7 +160,7 @@ CoerceToCheckedCast *CoerceToCheckedCast::attempt(ConstraintSystem &cs,
     return nullptr;
 
   return new (cs.getAllocator())
-      CoerceToCheckedCast(cs, fromType, toType, locator);
+      CoerceToCheckedCast(cs, fromType, toType, useConditionalCast, locator);
 }
 
 bool TreatArrayLiteralAsDictionary::diagnose(const Solution &solution,

lib/Sema/CSSimplify.cpp

@@ -3551,9 +3551,26 @@ bool ConstraintSystem::repairFailures(
                                   getConstraintLocator(coercion->getSubExpr())))
         return true;
 
-      // Repair a coercion ('as') with a forced checked cast ('as!').
-      if (auto *coerceToCheckCastFix = CoerceToCheckedCast::attempt(
-              *this, lhs, rhs, getConstraintLocator(locator))) {
+      // If the result type of the coercion has an value to optional conversion
+      // we can instead suggest the conditional downcast as it is safer in
+      // situations like conditional binding.
+      auto useConditionalCast = llvm::any_of(
+          ConstraintRestrictions,
+          [&](std::tuple<Type, Type, ConversionRestrictionKind> restriction) {
+            ConversionRestrictionKind restrictionKind;
+            Type type1, type2;
+            std::tie(type1, type2, restrictionKind) = restriction;
+
+            if (restrictionKind != ConversionRestrictionKind::ValueToOptional)
+              return false;
+
+            return rhs->isEqual(type1);
+          });
+
+      // Repair a coercion ('as') with a runtime checked cast ('as!' or 'as?').
+      if (auto *coerceToCheckCastFix =
+              CoerceToCheckedCast::attempt(*this, lhs, rhs, useConditionalCast,
+                                           getConstraintLocator(locator))) {
         conversionsOrFixes.push_back(coerceToCheckCastFix);
         return true;
       }
@@ -9560,8 +9577,8 @@ ConstraintSystem::simplifyRestrictedConstraintImpl(
         loc->isLastElement<LocatorPathElt::ApplyArgToParam>() ||
         loc->isForOptionalTry()) {
       if (restriction == ConversionRestrictionKind::Superclass) {
-        if (auto *fix =
-                CoerceToCheckedCast::attempt(*this, fromType, toType, loc))
+        if (auto *fix = CoerceToCheckedCast::attempt(
+                *this, fromType, toType, /*useConditionalCast*/ false, loc))
           return !recordFix(fix, impact);
       }
 

test/ClangImporter/Dispatch_test.swift

@@ -15,7 +15,8 @@ func test2(_ queue: DispatchQueue) {
 
   // Make sure the dispatch types are actually distinct types!
   let _ = queue as DispatchSource // expected-error {{cannot convert value of type 'DispatchQueue' to type 'DispatchSource' in coercion}}
-  let _ = base as DispatchSource // expected-error {{'NSObjectProtocol' is not convertible to 'DispatchSource'; did you mean to use 'as!' to force downcast?}}
+  let _ = base as DispatchSource // expected-error {{'NSObjectProtocol' is not convertible to 'DispatchSource'}}
+  // expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{16-18=as!}}
 }
 
 extension dispatch_queue_t {} // expected-error {{'dispatch_queue_t' is unavailable}}

test/ClangImporter/Security_test.swift

@@ -6,7 +6,8 @@ import Security
 
 _ = kSecClass as CFString
 _ = kSecClassGenericPassword as CFString
-_ = kSecClassGenericPassword as CFDictionary // expected-error {{'CFString?' is not convertible to 'CFDictionary'}} {{30-32=as!}}
+_ = kSecClassGenericPassword as CFDictionary // expected-error {{'CFString?' is not convertible to 'CFDictionary'}} 
+// expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{30-32=as!}}
 
 func testIntegration() {
   // Based on code in <rdar://problem/17162475>.

test/ClangImporter/objc_bridging_custom.swift

@@ -218,11 +218,16 @@ func testExplicitConversion(objc: APPManufacturerInfo<NSString>,
                             swift: ManufacturerInfo<NSString>) {
   // Bridging to Swift
   let _ = objc as ManufacturerInfo<NSString>
-  let _ = objc as ManufacturerInfo<NSNumber> // expected-error{{'APPManufacturerInfo<NSString>' is not convertible to 'ManufacturerInfo<NSNumber>'; did you mean to use 'as!' to force downcast?}}
-  let _ = objc as ManufacturerInfo<NSObject> // expected-error{{'APPManufacturerInfo<NSString>' is not convertible to 'ManufacturerInfo<NSObject>'; did you mean to use 'as!' to force downcast?}}
+  let _ = objc as ManufacturerInfo<NSNumber> // expected-error{{'APPManufacturerInfo<NSString>' is not convertible to 'ManufacturerInfo<NSNumber>'}}
+  // expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{16-18=as!}}
+  let _ = objc as ManufacturerInfo<NSObject> // expected-error{{'APPManufacturerInfo<NSString>' is not convertible to 'ManufacturerInfo<NSObject>'}}
+  // expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{16-18=as!}}
 
   // Bridging to Objective-C
   let _ = swift as APPManufacturerInfo<NSString>
-  let _ = swift as APPManufacturerInfo<NSNumber> // expected-error{{'ManufacturerInfo<NSString>' is not convertible to 'APPManufacturerInfo<NSNumber>'; did you mean to use 'as!' to force downcast?}}
-  let _ = swift as APPManufacturerInfo<NSObject> // expected-error{{'ManufacturerInfo<NSString>' is not convertible to 'APPManufacturerInfo<NSObject>'; did you mean to use 'as!' to force downcast?}}
+  let _ = swift as APPManufacturerInfo<NSNumber> // expected-error{{'ManufacturerInfo<NSString>' is not convertible to 'APPManufacturerInfo<NSNumber>'}}
+  // expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{17-19=as!}}
+  let _ = swift as APPManufacturerInfo<NSObject> // expected-error{{'ManufacturerInfo<NSString>' is not convertible to 'APPManufacturerInfo<NSObject>'}}
+  // expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{17-19=as!}}
+
 }

test/ClangImporter/objc_parse.swift

@@ -540,10 +540,12 @@ func testStrangeSelectors(obj: StrangeSelectors) {
 
 func testProtocolQualified(_ obj: CopyableNSObject, cell: CopyableSomeCell,
                            plainObj: NSObject, plainCell: SomeCell) {
-  _ = obj as NSObject // expected-error {{'CopyableNSObject' (aka 'NSCopying & NSObjectProtocol') is not convertible to 'NSObject'; did you mean to use 'as!' to force downcast?}} {{11-13=as!}}
+  _ = obj as NSObject // expected-error {{'CopyableNSObject' (aka 'NSCopying & NSObjectProtocol') is not convertible to 'NSObject'}} 
+  // expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{11-13=as!}}
   _ = obj as NSObjectProtocol
   _ = obj as NSCopying
-  _ = obj as SomeCell // expected-error {{'CopyableNSObject' (aka 'NSCopying & NSObjectProtocol') is not convertible to 'SomeCell'; did you mean to use 'as!' to force downcast?}} {{11-13=as!}}
+  _ = obj as SomeCell // expected-error {{'CopyableNSObject' (aka 'NSCopying & NSObjectProtocol') is not convertible to 'SomeCell'}}
+  // expected-note@-1 {{did you mean to use 'as!' to force downcast?}} {{11-13=as!}}
 
   _ = cell as NSObject
   _ = cell as NSObjectProtocol