Move-only-check the yielded result from read coroutines when they're noncopyable.

Mark the result of starting a read coroutine to be checked by the move-only checker, and then
update the pattern matching in the move checker itself so that it recognizes code patterns
involving yielding from and receiving yields from read coroutines. Teach move only diagnostics
to get the property name for an access through a read coroutine from the referenced declaration.

Author: jckarter

lib/SILGen/SILGenApply.cpp

@@ -4822,17 +4822,28 @@ namespace {
 /// Cleanup to end a coroutine application.
 class EndCoroutineApply : public Cleanup {
   SILValue ApplyToken;
+  std::vector<BeginBorrowInst *> BorrowedMoveOnlyValues;
 public:
   EndCoroutineApply(SILValue applyToken) : ApplyToken(applyToken) {}
 
+  void setBorrowedMoveOnlyValues(ArrayRef<BeginBorrowInst *> values) {
+    BorrowedMoveOnlyValues.insert(BorrowedMoveOnlyValues.end(),
+                                  values.begin(), values.end());
+  }
+
   void emit(SILGenFunction &SGF, CleanupLocation l, ForUnwind_t forUnwind) override {
+    for (auto i = BorrowedMoveOnlyValues.rbegin(), e = BorrowedMoveOnlyValues.rend();
+         i != e; ++i) {
+      SGF.B.createEndBorrow(l, *i);
+      SGF.B.createDestroyValue(l, (*i)->getOperand());
+    }
     if (forUnwind) {
       SGF.B.createAbortApply(l, ApplyToken);
     } else {
       SGF.B.createEndApply(l, ApplyToken);
     }
   }
-
+  
   void dump(SILGenFunction &SGF) const override {
 #ifndef NDEBUG
     llvm::errs() << "EndCoroutineApply "
@@ -4897,23 +4908,56 @@ SILGenFunction::emitBeginApply(SILLocation loc, ManagedValue fn,
   auto yieldInfos = substFnType->getYields();
   assert(yieldValues.size() == yieldInfos.size());
   bool useLoweredAddresses = silConv.useLoweredAddresses();
+  SmallVector<BeginBorrowInst *, 2> borrowedMoveOnlyValues;
   for (auto i : indices(yieldValues)) {
     auto value = yieldValues[i];
     auto info = yieldInfos[i];
     if (info.isIndirectInOut()) {
+      if (value->getType().isMoveOnly()) {
+        value = B.createMarkUnresolvedNonCopyableValueInst(loc, value,
+         MarkUnresolvedNonCopyableValueInst::CheckKind::AssignableButNotConsumable);
+      }
       yields.push_back(ManagedValue::forLValue(value));
     } else if (info.isConsumed()) {
+      if (value->getType().isMoveOnly()) {
+        value = B.createMarkUnresolvedNonCopyableValueInst(loc, value,
+         MarkUnresolvedNonCopyableValueInst::CheckKind::ConsumableAndAssignable);
+      }
       !useLoweredAddresses && value->getType().isTrivial(getFunction())
           ? yields.push_back(ManagedValue::forRValueWithoutOwnership(value))
           : yields.push_back(emitManagedRValueWithCleanup(value));
     } else if (info.isGuaranteed()) {
-      !useLoweredAddresses && value->getType().isTrivial(getFunction())
-          ? yields.push_back(ManagedValue::forRValueWithoutOwnership(value))
-          : yields.push_back(ManagedValue::forBorrowedRValue(value));
+      if (value->getType().isMoveOnly()) {
+        if (!useLoweredAddresses
+            || !value->getType().isAddressOnly(F)) {
+          // The move checker uses the lifetime of the "copy" for borrow checking.
+          value = B.createCopyValue(loc, value);
+          value = B.createMarkUnresolvedNonCopyableValueInst(loc, value,
+            MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
+          auto borrow = B.createBeginBorrow(loc, value);
+          yields.push_back(ManagedValue::forBorrowedRValue(borrow));
+          borrowedMoveOnlyValues.push_back(borrow);
+        } else {
+          value = B.createMarkUnresolvedNonCopyableValueInst(loc, value,
+            MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
+          yields.push_back(ManagedValue::forRValueWithoutOwnership(value));
+        }
+      } else {
+        !useLoweredAddresses && value->getType().isTrivial(getFunction())
+            ? yields.push_back(ManagedValue::forRValueWithoutOwnership(value))
+            : yields.push_back(ManagedValue::forBorrowedRValue(value));
+      }
     } else {
+      assert(!value->getType().isMoveOnly()
+             && "move-only types shouldn't be trivial");
       yields.push_back(ManagedValue::forRValueWithoutOwnership(value));
     }
   }
+  
+  if (!borrowedMoveOnlyValues.empty()) {
+    auto &endApply = static_cast<EndCoroutineApply &>(Cleanups.getCleanup(endApplyHandle));
+    endApply.setBorrowedMoveOnlyValues(borrowedMoveOnlyValues);
+  }
 
   return endApplyHandle;
 }

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -950,6 +950,9 @@ void UseState::initializeLiveness(
                              reinitInstAndValue.second);
     }
   }
+  
+  // FIXME: Whether the initial use is an initialization ought to be entirely
+  // derivable from the CheckKind of the mark instruction.
 
   // Then check if our markedValue is from an argument that is in,
   // in_guaranteed, inout, or inout_aliasable, consider the marked address to be
@@ -1048,6 +1051,12 @@ void UseState::initializeLiveness(
     recordInitUse(address, address, liveness.getTopLevelSpan());
     liveness.initializeDef(address, liveness.getTopLevelSpan());
   }
+  
+  if (auto *bai = dyn_cast_or_null<BeginApplyInst>(
+        stripAccessMarkers(address->getOperand())->getDefiningInstruction())) {
+    recordInitUse(address, address, liveness.getTopLevelSpan());
+    liveness.initializeDef(address, liveness.getTopLevelSpan());
+  }
 
   // Now that we have finished initialization of defs, change our multi-maps
   // from their array form to their map form.

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -113,6 +113,21 @@ static void getVariableNameForValue(SILValue value2,
       break;
     }
 
+    if (auto bai = dyn_cast_or_null<BeginApplyInst>(searchValue->getDefiningInstruction())) {
+      // Use the name of the property being accessed if we can get to it.
+      if (isa<FunctionRefBaseInst>(bai->getCallee())
+          || isa<MethodInst>(bai->getCallee())) {
+        variableNamePath.push_back(bai->getCallee()->getDefiningInstruction());
+        // Try to name the base of the property if this is a method.
+        if (bai->getSubstCalleeType()->hasSelfParam()) {
+          searchValue = bai->getSelfArgument();
+          continue;
+        } else {
+          break;
+        }
+      }
+    }
+
     // If we do not do an exact match, see if we can find a debug_var inst. If
     // we do, we always break since we have a root value.
     if (auto *use = getAnyDebugUse(searchValue)) {
@@ -132,12 +147,25 @@ static void getVariableNameForValue(SILValue value2,
       searchValue = cast<SingleValueInstruction>(searchValue)->getOperand(0);
       continue;
     }
-
+    
     // If we do not pattern match successfully, just set resulting string to
     // unknown and return early.
     resultingString += "unknown";
     return;
   }
+  
+  auto nameFromDecl = [&](Decl *d) -> StringRef {
+    if (d) {
+      if (auto accessor = dyn_cast<AccessorDecl>(d)) {
+        return accessor->getStorage()->getBaseName().userFacingName();
+      }
+      if (auto vd = dyn_cast<ValueDecl>(d)) {
+        return vd->getBaseName().userFacingName();
+      }
+    }
+    
+    return "<unknown decl>";
+  };
 
   // Walk backwards, constructing our string.
   while (true) {
@@ -148,6 +176,16 @@ static void getVariableNameForValue(SILValue value2,
         resultingString += i.getName();
       } else if (auto i = VarDeclCarryingInst(inst)) {
         resultingString += i.getName();
+      } else if (auto f = dyn_cast<FunctionRefBaseInst>(inst)) {
+        if (auto dc = f->getInitiallyReferencedFunction()->getDeclContext()) {
+          resultingString += nameFromDecl(dc->getAsDecl());
+        } else {
+          resultingString += "<unknown decl>";
+        }
+      } else if (auto m = dyn_cast<MethodInst>(inst)) {
+        resultingString += nameFromDecl(m->getMember().getDecl());
+      } else {
+        resultingString += "<unknown decl>";      
       }
     } else {
       auto value = next.get<SILValue>();

lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.cpp

@@ -481,6 +481,11 @@ void MoveOnlyObjectCheckerPImpl::check(DominanceInfo *domTree,
             i = copyToMoveOnly;
           }
 
+          // TODO: Instead of pattern matching specific code generation patterns,
+          // we should be able to eliminate any `copy_value` whose canonical
+          // lifetime fits within the borrow scope of the borrowed value being
+          // copied from.
+
           // Handle:
           //
           // bb0(%0 : @guaranteed $Type):
@@ -521,6 +526,24 @@ void MoveOnlyObjectCheckerPImpl::check(DominanceInfo *domTree,
               }
             }
           }
+          
+          // Handle:
+          //   (%yield, ..., %handle) = begin_apply
+          //   %copy = copy_value %yield
+          //   %mark = mark_unresolved_noncopyable_value [no_consume_or_assign] %copy
+          if (auto bai = dyn_cast_or_null<BeginApplyInst>(i->getOperand(0)->getDefiningInstruction())) {
+            if (i->getOperand(0)->getOwnershipKind() == OwnershipKind::Guaranteed) {
+              for (auto *use : markedInst->getConsumingUses()) {
+                destroys.push_back(cast<DestroyValueInst>(use->getUser()));
+              }
+              while (!destroys.empty())
+                destroys.pop_back_val()->eraseFromParent();
+              markedInst->replaceAllUsesWith(i->getOperand(0));
+              markedInst->eraseFromParent();
+              cvi->eraseFromParent();
+              continue;
+            }
+          }
         }
       }
     }

test/SILOptimizer/moveonly_read_coroutine.swift

@@ -0,0 +1,74 @@
+// RUN: %target-swift-frontend -DADDRESS_ONLY -emit-sil -verify %s
+// RUN: %target-swift-frontend -DLOADABLE -emit-sil -verify %s
+// RUN: %target-swift-frontend -DTRIVIAL -emit-sil -verify %s
+// RUN: %target-swift-frontend -DEMPTY -emit-sil -verify %s
+
+class X {}
+
+struct NC: ~Copyable {
+#if EMPTY
+#elseif TRIVIAL
+  var x: Int = 0
+#elseif LOADABLE
+  var x: X = X()
+#elseif ADDRESS_ONLY
+  var x: Any = X()
+#else
+#error("pick a mode")
+#endif
+  deinit { print("destroy") }
+}
+
+struct S {
+  var data: NC {
+    _read { yield NC() }
+  }
+}
+
+struct SNC: ~Copyable {
+  private var _data: NC = NC()
+
+  var data: NC {
+    _read { yield _data }
+  }
+}
+
+class C {
+  private var _data: NC = NC()
+
+  var data: NC {
+    _read { yield _data }
+  }
+}
+
+protocol P {
+  @_borrowed
+  var data: NC { get }
+}
+
+func borrow(_ nc: borrowing NC) {}
+func take(_ nc: consuming NC) {}
+
+func test(c: C) {
+  borrow(c.data)
+  take(c.data) // expected-error{{'c.data' is borrowed and cannot be consumed}} expected-note{{consumed here}}
+}
+func test(s: S) {
+  borrow(s.data)
+  take(s.data) // expected-error{{'s.data' is borrowed and cannot be consumed}} expected-note{{consumed here}}
+}
+func test(snc: borrowing SNC) {
+  borrow(snc.data)
+  take(snc.data) // expected-error{{'snc.data' is borrowed and cannot be consumed}} expected-note{{consumed here}}
+}
+
+// FIXME: get the actual variable name instead of 'unknown' in these cases.
+func test<T: P>(t: T) {
+  borrow(t.data)
+  take(t.data) // expected-error{{'unknown.data' is borrowed and cannot be consumed}} expected-note{{consumed here}}
+}
+func test(p: P) {
+  borrow(p.data)
+  take(p.data) // expected-error{{'unknown.data' is borrowed and cannot be consumed}} expected-note{{consumed here}}
+}
+