ConstraintSystem: clarify consuming conversions

There are a number of implicit conversions in Swift, such as to Optional
and to an existential, which are now possible for noncopyable types.

But all type casts are consuming operations for noncopyable types. So
it's confusing when a function that takes a borrowed argument of
optional type appears to be consuming:

```
func f(_ x: borrowing NC?) { ... }

let x = NC()
f(x)
f(x) // error!
```

So, rather than for people to write `x as T?` around all implicit
conversions, require them to write `consume x` around expressions
that will consume some lvalue. Since that makes it much more clear what
the consequences will be.

Expressions like `f(g())`, where you're passing an rvalue to the callee,
are not confusing. And those are exactly the expressions you're not
allowed to write `consume` for, anyway.

fixes rdar://127450418

Author: kavon

include/swift/AST/DiagnosticsSema.def

@@ -7848,6 +7848,10 @@ ERROR(noncopyable_objc_enum, none,
       "noncopyable enums cannot be marked '@objc'", ())
 ERROR(moveOnly_not_allowed_here,none,
       "'@_moveOnly' attribute is only valid on structs or enums", ())
+WARNING(consume_expression_needed_for_cast,none,
+      "implicit conversion to %0 is consuming", (Type))
+NOTE(add_consume_to_silence_warning,none,
+     "add 'consume' to silence this warning", ())
 ERROR(consume_expression_not_passed_lvalue,none,
       "'consume' can only be applied to a local binding ('let', 'var', or parameter)", ())
 ERROR(consume_expression_partial_copyable,none,

include/swift/Sema/ConstraintSystem.h

@@ -6582,6 +6582,13 @@ TypeVariableType *TypeVariableType::getNew(const ASTContext &C, unsigned ID,
 /// underlying forced downcast expression.
 ForcedCheckedCastExpr *findForcedDowncast(ASTContext &ctx, Expr *expr);
 
+/// Assuming the expression appears in a consuming context,
+/// if it does not already have an explicit `consume`,
+/// can I add `consume` around this expression?
+///
+/// \param module represents the module in which the expr appears
+bool canAddExplicitConsume(ModuleDecl *module, Expr *expr);
+
 // Count the number of overload sets present
 // in the expression and all of the children.
 class OverloadSetCounter : public ASTWalker {

lib/Sema/CSApply.cpp

@@ -355,6 +355,45 @@ static bool buildObjCKeyPathString(KeyPathExpr *E,
   return true;
 }
 
+/// Since a cast to an optional will consume a noncopyable type, check to see
+/// if injecting the value into an optional here will potentially be confusing.
+static bool willHaveConfusingConsumption(Type type,
+                                         ConstraintLocatorBuilder locator,
+                                         ConstraintSystem &cs) {
+  assert(type);
+  if (!type->isNoncopyable())
+    return false; /// If it's a copyable type, there's no confusion.
+
+  auto loc = cs.getConstraintLocator(locator);
+  if (!loc)
+    return true;
+
+  auto path = loc->getPath();
+  if (path.empty())
+    return true;
+
+  switch (loc->getPath().back().getKind()) {
+  case ConstraintLocator::FunctionResult:
+  case ConstraintLocator::ClosureResult:
+  case ConstraintLocator::ClosureBody:
+  case ConstraintLocator::ContextualType:
+  case ConstraintLocator::CoercionOperand:
+    return false; // These last-uses won't be confused for borrowing.
+
+  case ConstraintLocator::ApplyArgToParam: {
+    auto argLoc = loc->castLastElementTo<LocatorPathElt::ApplyArgToParam>();
+    auto paramFlags = argLoc.getParameterFlags();
+    if (paramFlags.getOwnershipSpecifier() == ParamSpecifier::Consuming)
+      return false; // Parameter already declares 'consuming'.
+
+    return true;
+  }
+
+  default:
+    return true;
+  }
+}
+
 namespace {
 
   /// Rewrites an expression by applying the solution of a constraint
@@ -3241,10 +3280,12 @@ namespace {
     }
 
   private:
-    /// A list of "suspicious" optional injections that come from
-    /// forced downcasts.
+    /// A list of "suspicious" optional injections.
     SmallVector<InjectIntoOptionalExpr *, 4> SuspiciousOptionalInjections;
 
+    /// A list of implicit coercions of noncopyable types.
+    SmallVector<Expr *, 4> ConsumingCoercions;
+
     /// Create a member reference to the given constructor.
     Expr *applyCtorRefExpr(Expr *expr, Expr *base, SourceLoc dotLoc,
                            DeclNameLoc nameLoc, bool implicit,
@@ -4466,9 +4507,9 @@ namespace {
       if (choice == 0) {
         // Convert the subexpression.
         Expr *sub = expr->getSubExpr();
-
-        sub = solution.coerceToType(sub, expr->getCastType(),
-                                    cs.getConstraintLocator(sub));
+        auto subLoc =
+            cs.getConstraintLocator(sub, ConstraintLocator::CoercionOperand);
+        sub = solution.coerceToType(sub, expr->getCastType(), subLoc);
         if (!sub)
           return nullptr;
 
@@ -5516,41 +5557,69 @@ namespace {
       assert(OpenedExistentials.empty());
 
       auto &ctx = cs.getASTContext();
+      auto *module = dc->getParentModule();
 
       // Look at all of the suspicious optional injections
       for (auto injection : SuspiciousOptionalInjections) {
-        auto *cast = findForcedDowncast(ctx, injection->getSubExpr());
-        if (!cast)
-          continue;
-
-        if (isa<ParenExpr>(injection->getSubExpr()))
-          continue;
+        if (auto *cast = findForcedDowncast(ctx, injection->getSubExpr())) {
+          if (!isa<ParenExpr>(injection->getSubExpr())) {
+            ctx.Diags.diagnose(
+                injection->getLoc(), diag::inject_forced_downcast,
+                cs.getType(injection->getSubExpr())->getRValueType());
+            auto exclaimLoc = cast->getExclaimLoc();
+            ctx.Diags
+                .diagnose(exclaimLoc, diag::forced_to_conditional_downcast,
+                          cs.getType(injection)->getOptionalObjectType())
+                .fixItReplace(exclaimLoc, "?");
+            ctx.Diags
+                .diagnose(cast->getStartLoc(),
+                          diag::silence_inject_forced_downcast)
+                .fixItInsert(cast->getStartLoc(), "(")
+                .fixItInsertAfter(cast->getEndLoc(), ")");
+          }
+        }
+      }
 
-        ctx.Diags.diagnose(
-            injection->getLoc(), diag::inject_forced_downcast,
-            cs.getType(injection->getSubExpr())->getRValueType());
-        auto exclaimLoc = cast->getExclaimLoc();
+      // Diagnose the implicit coercions of noncopyable values that happen in
+      // a context where it isn't "obviously" consuming already.
+      for (auto *coercion : ConsumingCoercions) {
+        assert(coercion->isImplicit());
         ctx.Diags
-            .diagnose(exclaimLoc, diag::forced_to_conditional_downcast,
-                      cs.getType(injection)->getOptionalObjectType())
-            .fixItReplace(exclaimLoc, "?");
+            .diagnose(coercion->getLoc(),
+                      diag::consume_expression_needed_for_cast,
+                      cs.getType(coercion));
         ctx.Diags
-            .diagnose(cast->getStartLoc(), diag::silence_inject_forced_downcast)
-            .fixItInsert(cast->getStartLoc(), "(")
-            .fixItInsertAfter(cast->getEndLoc(), ")");
+            .diagnose(coercion->getLoc(),
+                      diag::add_consume_to_silence_warning)
+            .fixItInsert(coercion->getStartLoc(), "consume ");
       }
     }
 
     /// Diagnose an optional injection that is probably not what the
-    /// user wanted, because it comes from a forced downcast.
-    void diagnoseOptionalInjection(InjectIntoOptionalExpr *injection) {
+    /// user wanted, because it comes from a forced downcast, or from an
+    /// implicitly consumed noncopyable type.
+    void diagnoseOptionalInjection(InjectIntoOptionalExpr *injection,
+                                   ConstraintLocatorBuilder locator) {
       // Check whether we have a forced downcast.
-      auto *cast =
-          findForcedDowncast(cs.getASTContext(), injection->getSubExpr());
-      if (!cast)
-        return;
-      
-      SuspiciousOptionalInjections.push_back(injection);
+      if (findForcedDowncast(cs.getASTContext(), injection->getSubExpr()))
+        SuspiciousOptionalInjections.push_back(injection);
+
+      /// Check if it needs an explicit consume, due to this being a cast.
+      auto *module = dc->getParentModule();
+      auto origType = cs.getType(injection->getSubExpr());
+      if (willHaveConfusingConsumption(origType, locator, cs) &&
+          canAddExplicitConsume(module, injection->getSubExpr()))
+        ConsumingCoercions.push_back(injection);
+    }
+
+    void diagnoseExistentialErasureOf(Expr *fromExpr, Expr *toExpr,
+                                      ConstraintLocatorBuilder locator) {
+      auto *module = dc->getParentModule();
+      auto fromType = cs.getType(fromExpr);
+      if (willHaveConfusingConsumption(fromType, locator, cs) &&
+          canAddExplicitConsume(module, fromExpr)) {
+        ConsumingCoercions.push_back(toExpr);
+      }
     }
   };
 } // end anonymous namespace
@@ -5821,7 +5890,7 @@ Expr *ExprRewriter::coerceOptionalToOptional(Expr *expr, Type toType,
     while (diff--) {
       Type type = toOptionals[diff];
       expr = cs.cacheType(new (ctx) InjectIntoOptionalExpr(expr, type));
-      diagnoseOptionalInjection(cast<InjectIntoOptionalExpr>(expr));
+      diagnoseOptionalInjection(cast<InjectIntoOptionalExpr>(expr), locator);
     }
 
     return expr;
@@ -6950,8 +7019,11 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
       return coerceSuperclass(expr, toType);
 
     case ConversionRestrictionKind::Existential:
-    case ConversionRestrictionKind::MetatypeToExistentialMetatype:
-      return coerceExistential(expr, toType, locator);
+    case ConversionRestrictionKind::MetatypeToExistentialMetatype: {
+      auto coerced = coerceExistential(expr, toType, locator);
+      diagnoseExistentialErasureOf(expr, coerced, locator);
+      return coerced;
+    }
 
     case ConversionRestrictionKind::ClassMetatypeToAnyObject: {
       assert(ctx.LangOpts.EnableObjCInterop &&
@@ -6980,7 +7052,7 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
 
       auto *result =
           cs.cacheType(new (ctx) InjectIntoOptionalExpr(expr, toType));
-      diagnoseOptionalInjection(result);
+      diagnoseOptionalInjection(result, locator);
       return result;
     }
 
@@ -7674,7 +7746,7 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
     if (!expr) return nullptr;
 
     auto *result = cs.cacheType(new (ctx) InjectIntoOptionalExpr(expr, toType));
-    diagnoseOptionalInjection(result);
+    diagnoseOptionalInjection(result, locator);
     return result;
   }
 

lib/Sema/TypeCheckConstraints.cpp

@@ -2317,6 +2317,10 @@ static Expr *lookThroughBridgeFromObjCCall(ASTContext &ctx, Expr *expr) {
 /// underlying forced downcast expression.
 ForcedCheckedCastExpr *swift::findForcedDowncast(ASTContext &ctx, Expr *expr) {
   expr = expr->getSemanticsProvidingExpr();
+
+  // Look through a consume, just in case.
+  if (auto consume = dyn_cast<ConsumeExpr>(expr))
+    expr = consume->getSubExpr();
 
   // Simple case: forced checked cast.
   if (auto forced = dyn_cast<ForcedCheckedCastExpr>(expr)) {
@@ -2368,6 +2372,18 @@ ForcedCheckedCastExpr *swift::findForcedDowncast(ASTContext &ctx, Expr *expr) {
   return nullptr;
 }
 
+bool swift::canAddExplicitConsume(ModuleDecl *module, Expr *expr) {
+  expr = expr->getSemanticsProvidingExpr();
+
+  // Is it already wrapped in a `consume`?
+  if (isa<ConsumeExpr>(expr))
+    return false;
+
+  // Is this expression valid to wrap inside a `consume`?
+  auto diags = findSyntacticErrorForConsume(module, SourceLoc(), expr);
+  return diags.empty();
+}
+
 void ConstraintSystem::forEachExpr(
     Expr *expr, llvm::function_ref<Expr *(Expr *)> callback) {
   struct ChildWalker : ASTWalker {

test/Sema/moveonly_casts.swift

@@ -0,0 +1,126 @@
+// RUN: %target-typecheck-verify-swift
+
+struct NC: ~Copyable {}
+
+func testBorrowing(_ v: borrowing NC?) {}
+func testConsuming(_ v: consuming NC?) {}
+func testInout(_ v: inout NC?) {}
+
+class MethodSir {
+  func borrow(_ v: borrowing NC?) {}
+  func consume(_ v: consuming NC?) {}
+}
+
+func testExplicitCasts() {
+  let nc = NC()
+  _ = nc as NC?
+}
+
+func testCalls() {
+  let method = MethodSir()
+  let foo = NC()
+  testBorrowing(foo) // expected-warning {{implicit conversion to 'NC?' is consuming}}
+                     // expected-note@-1 {{add 'consume' to silence this warning}} {{17-17=consume }}
+  testBorrowing(consume foo)
+  testBorrowing(foo as NC?)
+
+  method.borrow(foo) // expected-warning {{implicit conversion to 'NC?' is consuming}}
+                     // expected-note@-1 {{add 'consume' to silence this warning}} {{17-17=consume }}
+  method.borrow(consume foo)
+
+  testConsuming(foo)
+  testConsuming(consume foo)
+
+  var optNC: NC? = NC() // ConstraintLocator::ContextualType
+  testInout(&optNC)
+}
+
+func testReturn() -> NC? {
+    let foo = NC()
+    return foo // ConstraintLocator::ContextualType
+}
+
+func higherOrder(_ f: () -> NC?) -> NC? {
+  if let nc = f() {
+    nc // ConstraintLocator::ContextualType
+  } else {
+    nil
+  }
+}
+func callHigherOrder() {
+  let nc = NC()
+  let _ = higherOrder { nc } // ConstraintLocator::ClosureBody
+
+  let _ = higherOrder { return nc } // ConstraintLocator::ClosureBody
+}
+
+
+func delay(_ f: @autoclosure () -> NC?) -> NC? { f() }
+
+func testDelay() {
+  let nc = NC()
+  let _ = delay(nc) // expected-warning {{implicit conversion to 'NC?' is consuming}}
+                    // expected-note@-1 {{add 'consume' to silence this warning}} {{17-17=consume }}
+}
+
+struct PropCity {
+  var harmless1: NC? { NC() }
+  var harmless2: NC? {
+    get { return NC() }
+  }
+
+  subscript(_ i: Int) -> NC? { return NC() }
+
+  func chk(_ t: borrowing NC!) {}
+  func chkWithDefaultArg(_ oath: borrowing NC? = NC()) {}
+  func test(_ nc: consuming NC) {
+    chk(nc) // expected-warning {{implicit conversion to 'NC?' is consuming}}
+            // expected-note@-1 {{add 'consume' to silence this warning}} {{9-9=consume }}
+
+    chk(consume nc)
+
+    chkWithDefaultArg()
+    chkWithDefaultArg(nc) // expected-warning {{implicit conversion to 'NC?' is consuming}}
+                          // expected-note@-1 {{add 'consume' to silence this warning}} {{23-23=consume }}
+  }
+}
+
+protocol Veggie: ~Copyable {}
+struct Carrot: ~Copyable, Veggie {}
+
+func restockBorrow(_ x: borrowing any Veggie & ~Copyable) {}
+func restockConsume(_ x: consuming any Veggie & ~Copyable) {}
+
+func checkExistential() {
+  let carrot = Carrot()
+  restockBorrow(carrot) // expected-warning {{implicit conversion to 'any Veggie & ~Copyable' is consuming}}
+                        // expected-note@-1 {{add 'consume' to silence this warning}} {{17-17=consume }}
+  restockBorrow(consume carrot)
+
+  restockConsume(carrot)
+}
+
+func genericErasure<T: Veggie & ~Copyable>(_ veg: consuming T) {
+  restockBorrow(veg) // expected-warning {{implicit conversion to 'any Veggie & ~Copyable' is consuming}}
+                     // expected-note@-1 {{add 'consume' to silence this warning}} {{17-17=consume }}
+  restockBorrow(consume veg)
+  restockBorrow(veg as any Veggie & ~Copyable)
+  restockConsume(veg)
+
+  let _ = veg as any Veggie & ~Copyable
+}
+
+extension Veggie where Self: ~Copyable {
+  func inspect(_ b: borrowing any Veggie & ~Copyable) {}
+}
+extension Carrot {
+  consuming func check() {
+    inspect(self) // expected-warning {{implicit conversion to 'any Veggie & ~Copyable' is consuming}}
+                  // expected-note@-1 {{add 'consume' to silence this warning}} {{13-13=consume }}
+    inspect(consume self)
+    inspect(self as any Veggie & ~Copyable)
+
+    let _: any Veggie & ~Copyable = self
+  }
+}
+