Merge pull request #6798 from jckarter/nsnumber-conditional-casting-3.1

Sema: NSValue-to-value-type casts are failable and should be checked.

CHANGELOG.md

@@ -20,6 +20,11 @@ CHANGELOG
 Swift 3.1
 ---------
 
+* Bridging conversions from `NSNumber` or `NSValue` back to Swift number or
+  struct types using the `as` operator will now raise a warning, because these
+  conversions are type-checked at runtime. Dynamic casting with `as!` or `as?`
+  should be used going forward.
+
 * The `withoutActuallyEscaping` function from [SE-0103][] has been implemented.
   To pass off a non-escaping closure to an API that formally takes an
   `@escaping` closure, but which is used in a way that will not in fact 

include/swift/AST/DiagnosticsSema.def

@@ -812,6 +812,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(missing_forced_downcast_swift3_compat_warning,none,
+        "bridging %0 to %1 may fail at runtime and will become a checked "
+        "cast in Swift 4; did you mean to use 'as!' to force downcast?",
+        (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))

include/swift/AST/Expr.h

@@ -62,10 +62,12 @@ enum class ExprKind : uint8_t {
 #include "swift/AST/ExprNodes.def"
 };
 
-/// Discriminates the different kinds of checked cast supported.
+/// Discriminates certain kinds of checked cast that have specialized diagnostic
+/// and/or code generation peephole behavior.
 ///
-/// This enumeration should not exist. Only the collection downcast kinds are
-/// currently significant. Please don't add new kinds.
+/// This enumeration should not have any semantic effect on the behavior of a
+/// well-typed program, since the runtime can perform all casts that are
+/// statically accepted.
 enum class CheckedCastKind : unsigned {
   /// The kind has not been determined yet.
   Unresolved,
@@ -75,18 +77,25 @@ enum class CheckedCastKind : unsigned {
 
   /// The requested cast is an implicit conversion, so this is a coercion.
   Coercion = First_Resolved,
-  /// A non-value-changing checked cast.
+  /// A checked cast with no known specific behavior.
   ValueCast,
   // A downcast from an array type to another array type.
   ArrayDowncast,
   // A downcast from a dictionary type to another dictionary type.
   DictionaryDowncast,
   // A downcast from a set type to another set type.
   SetDowncast,
-  /// A bridging cast.
-  BridgingCast,
+  /// A bridging conversion that always succeeds.
+  BridgingCoercion,
+  /// A bridging conversion that may fail, because there are multiple Swift
+  /// value types that bridge to the same Cocoa object type.
+  ///
+  /// This kind is only used for Swift 3 compatibility diagnostics and is
+  /// treated the same as 'BridgingCoercion' otherwise. In Swift 4 or later,
+  /// any conversions with this kind show up as ValueCasts.
+  Swift3BridgingDowncast,
 
-  Last_CheckedCastKind = BridgingCast,
+  Last_CheckedCastKind = Swift3BridgingDowncast,
 };
 
 enum class AccessSemantics : unsigned char {

include/swift/AST/KnownFoundationEntities.def

@@ -32,6 +32,7 @@ FOUNDATION_ENTITY(NSSet)
 FOUNDATION_ENTITY(NSString)
 FOUNDATION_ENTITY(NSUInteger)
 FOUNDATION_ENTITY(NSURL)
+FOUNDATION_ENTITY(NSValue)
 FOUNDATION_ENTITY(NSZone)
 
 #undef FOUNDATION_ENTITY

lib/AST/ASTPrinter.cpp

@@ -4111,8 +4111,10 @@ StringRef swift::getCheckedCastKindName(CheckedCastKind kind) {
     return "dictionary_downcast";
   case CheckedCastKind::SetDowncast:
     return "set_downcast";
-  case CheckedCastKind::BridgingCast:
-    return "bridging_cast";
+  case CheckedCastKind::BridgingCoercion:
+    return "bridging_coercion";
+  case CheckedCastKind::Swift3BridgingDowncast:
+    return "bridging_downcast";
   }
   llvm_unreachable("bad checked cast name");
 }

lib/AST/Pattern.cpp

@@ -298,7 +298,7 @@ bool Pattern::isRefutablePattern() const {
     // If this is an always matching 'is' pattern, then it isn't refutable.
     if (auto *is = dyn_cast<IsPattern>(Node))
       if (is->getCastKind() == CheckedCastKind::Coercion ||
-          is->getCastKind() == CheckedCastKind::BridgingCast)
+          is->getCastKind() == CheckedCastKind::BridgingCoercion)
         return;
 
     // If this is an ExprPattern that isn't resolved yet, do some simple

lib/Sema/CSApply.cpp

@@ -2943,7 +2943,7 @@ namespace {
         break;
 
       case CheckedCastKind::Coercion:
-      case CheckedCastKind::BridgingCast:
+      case CheckedCastKind::BridgingCoercion:
         // Check is trivially true.
         tc.diagnose(expr->getLoc(), diag::isa_is_always_true, "is");
         expr->setCastKind(castKind);
@@ -2958,6 +2958,7 @@ namespace {
         }
         expr->setCastKind(castKind);
         break;
+      case CheckedCastKind::Swift3BridgingDowncast:
       case CheckedCastKind::ArrayDowncast:
       case CheckedCastKind::DictionaryDowncast:
       case CheckedCastKind::SetDowncast:
@@ -3263,6 +3264,20 @@ namespace {
 
       Expr *sub = expr->getSubExpr();
       Type toInstanceType = toType->lookThroughAllAnyOptionalTypes();
+
+      // Warn about NSNumber and NSValue bridging coercions we accepted in
+      // Swift 3 but which can fail at runtime.
+      if (tc.Context.LangOpts.isSwiftVersion3()
+          && tc.typeCheckCheckedCast(sub->getType(), toInstanceType,
+                                     CheckedCastContextKind::None,
+                                     dc, SourceLoc(), sub, SourceRange())
+               == CheckedCastKind::Swift3BridgingDowncast) {
+        tc.diagnose(expr->getLoc(),
+                    diag::missing_forced_downcast_swift3_compat_warning,
+                    sub->getType(), toInstanceType)
+          .fixItReplace(expr->getAsLoc(), "as!");
+      }
+
       sub = buildObjCBridgeExpr(sub, toInstanceType, locator);
       if (!sub) return nullptr;
       expr->setSubExpr(sub);
@@ -3296,7 +3311,7 @@ namespace {
       case CheckedCastKind::Unresolved:
         return nullptr;
       case CheckedCastKind::Coercion:
-      case CheckedCastKind::BridgingCast: {
+      case CheckedCastKind::BridgingCoercion: {
         if (SuppressDiagnostics)
           return nullptr;
 
@@ -3322,6 +3337,7 @@ namespace {
       }
 
       // Valid casts.
+      case CheckedCastKind::Swift3BridgingDowncast:
       case CheckedCastKind::ArrayDowncast:
       case CheckedCastKind::DictionaryDowncast:
       case CheckedCastKind::SetDowncast:
@@ -3366,7 +3382,7 @@ namespace {
         break;
 
       case CheckedCastKind::Coercion:
-      case CheckedCastKind::BridgingCast: {
+      case CheckedCastKind::BridgingCoercion: {
         if (SuppressDiagnostics)
           return nullptr;
 
@@ -3394,6 +3410,7 @@ namespace {
       }
 
       // Valid casts.
+      case CheckedCastKind::Swift3BridgingDowncast:
       case CheckedCastKind::ArrayDowncast:
       case CheckedCastKind::DictionaryDowncast:
       case CheckedCastKind::SetDowncast:
@@ -6974,17 +6991,35 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
                         fromType, toType, CheckedCastContextKind::None, DC,
                         coerceExpr->getLoc(), subExpr,
                         coerceExpr->getCastTypeLoc().getSourceRange());
-
+      
       switch (castKind) {
       // Invalid cast.
       case CheckedCastKind::Unresolved:
         // Fix didn't work, let diagnoseFailureForExpr handle this.
         return false;
       case CheckedCastKind::Coercion:
-      case CheckedCastKind::BridgingCast:
+      case CheckedCastKind::BridgingCoercion:
         llvm_unreachable("Coercions handled in other disjunction branch");
 
       // Valid casts.
+      case CheckedCastKind::Swift3BridgingDowncast: {
+        // Swift 3 accepted coercions from NSNumber and NSValue to Swift
+        // value types, even though there are multiple Swift types that
+        // bridge to those classes, and the bridging operation back into Swift
+        // is type-checked. For compatibility, downgrade to a warning.
+        assert(TC.Context.LangOpts.isSwiftVersion3()
+               && "should only appear in Swift 3 compat mode");
+
+        TC.diagnose(coerceExpr->getLoc(),
+                    diag::missing_forced_downcast_swift3_compat_warning,
+                    fromType, toType)
+          .highlight(coerceExpr->getSourceRange())
+          .fixItReplace(coerceExpr->getLoc(), "as!");
+
+        // This is just a warning, so allow the expression to type-check.
+        return false;
+      }
+
       case CheckedCastKind::ArrayDowncast:
       case CheckedCastKind::DictionaryDowncast:
       case CheckedCastKind::SetDowncast:

lib/Sema/CSDiag.cpp

@@ -3888,7 +3888,7 @@ addTypeCoerceFixit(InFlightDiagnostic &diag, ConstraintSystem *CS,
     llvm::raw_svector_ostream OS(buffer);
     toType->print(OS);
     bool canUseAs = Kind == CheckedCastKind::Coercion ||
-      Kind == CheckedCastKind::BridgingCast;
+      Kind == CheckedCastKind::BridgingCoercion;
     diag.fixItInsert(Lexer::getLocForEndOfToken(CS->DC->getASTContext().SourceMgr,
                                                 expr->getEndLoc()),
                      (llvm::Twine(canUseAs ? " as " : " as! ") +

lib/Sema/CSSimplify.cpp

@@ -2393,15 +2393,15 @@ ConstraintSystem::simplifyCheckedCastConstraint(
   };
 
   do {
-    // Dig out the fixed type to which this type refers.
+    // Dig out the fixed type this type refers to.
     fromType = getFixedTypeRecursive(fromType, flags, /*wantRValue=*/true);
 
     // If we hit a type variable without a fixed type, we can't
     // solve this yet.
     if (fromType->isTypeVariableOrMember())
       return formUnsolved();
 
-    // Dig out the fixed type to which this type refers.
+    // Dig out the fixed type this type refers to.
     toType = getFixedTypeRecursive(toType, flags, /*wantRValue=*/true);
 
     // If we hit a type variable without a fixed type, we can't
@@ -2491,7 +2491,8 @@ ConstraintSystem::simplifyCheckedCastConstraint(
   }
 
   case CheckedCastKind::Coercion:
-  case CheckedCastKind::BridgingCast:
+  case CheckedCastKind::BridgingCoercion:
+  case CheckedCastKind::Swift3BridgingDowncast:
   case CheckedCastKind::Unresolved:
     llvm_unreachable("Not a valid result");
   }
@@ -3376,6 +3377,15 @@ ConstraintSystem::simplifyBridgingConstraint(Type type1,
     Type bridgedValueType;
     if (auto objcClass = TC.Context.getBridgedToObjC(DC, unwrappedToType,
                                                      &bridgedValueType)) {
+      // Bridging NSNumber to NSValue is one-way, since there are multiple Swift
+      // value types that bridge to those object types. It requires a checked
+      // cast to get back.
+      // We accepted these coercions in Swift 3 mode, so we have to live with
+      // them (but give a warning) in that language mode.
+      if (!TC.Context.LangOpts.isSwiftVersion3()
+          && TC.isObjCClassWithMultipleSwiftBridgedTypes(objcClass, DC))
+        return SolutionKind::Error;
+
       // If the bridged value type is generic, the generic arguments
       // must either match or be bridged.
       // FIXME: This should be an associated type of the protocol.