[moveOnly] Emit an error diagnostic if a user applies _move to a value that the compiler doesn't know how to check (non-let, non-param today).

The error diagnostic tells the user that the compiler can't check the value. It
then instructs the user to make a feature request and provide the test case if
they think it is reasonable. I also provided an option to disable the diagnostic
to unblock people.

The reason why I think this is the right thing to do is we want people to know
that _move means they do not need to worry about the given binding being used
later in the program in some way without having to reason. For now I am doing
this by banning _move on non-lets, non-params. This is implemented by noting
that:

1. _move inserts move_value [allows_diagnostics].
2. The checker always removes [allows_diagnostics] after checking a _move.

Thus we know after we check, any move_value that is still marked with
[allows_diagnostic], it was a _move that we never used in any checking.

I added some test cases where this known triggers. I am either going to
implement some sort of support for performing _move on them or give a more
specific diagnostic. This is just an initial incremental step.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -740,6 +740,9 @@ NOTE(sil_movekillscopyablevalue_use_here, none,
      "use here", ())
 NOTE(sil_movekillscopyablevalue_value_consumed_in_loop, none,
      "cyclic move here. move will occur multiple times in the loop", ())
+ERROR(sil_movekillscopyablevalue_move_applied_to_unsupported_move, none,
+      "_move applied to value that the compiler does not know how to check. "
+      "Please file a bug or an enhancement request!", ())
 
 #define UNDEFINE_DIAGNOSTIC_MACROS
 #include "DefineDiagnosticMacros.h"

lib/SILOptimizer/Mandatory/MoveKillsCopyableValuesChecker.cpp

@@ -29,6 +29,9 @@
 
 using namespace swift;
 
+static llvm::cl::opt<bool>
+    DisableUnhandledMoveDiagnostic("sil-disable-unknown-move-diagnostic");
+
 //===----------------------------------------------------------------------===//
 //                            Diagnostic Utilities
 //===----------------------------------------------------------------------===//
@@ -383,6 +386,12 @@ bool MoveKillsCopyableValuesChecker::check() {
         // Only emit diagnostics if our move value allows us to.
         if (!mvi->getAllowDiagnostics())
           continue;
+
+        // Now that we know we may emit diagnostics for this, unset allows
+        // diagnostics so that we skip these when we search at the end for
+        // unvisited move_value [allows_diagnostics].
+        mvi->setAllowsDiagnostics(false);
+
         LLVM_DEBUG(llvm::dbgs() << "Move Value: " << *mvi);
         if (livenessInfo.liveness.isWithinBoundary(mvi)) {
           LLVM_DEBUG(llvm::dbgs() << "    WithinBoundary: Yes!\n");
@@ -412,6 +421,12 @@ namespace {
 class MoveKillsCopyableValuesCheckerPass : public SILFunctionTransform {
   void run() override {
     auto *fn = getFunction();
+    auto &astContext = fn->getASTContext();
+
+    // If we do not have experimental move only enabled, do not emit
+    // diagnostics.
+    if (!astContext.LangOpts.EnableExperimentalMoveOnly)
+      return;
 
     // Don't rerun diagnostics on deserialized functions.
     if (getFunction()->wasDeserializedCanonical())
@@ -425,6 +440,26 @@ class MoveKillsCopyableValuesCheckerPass : public SILFunctionTransform {
     if (MoveKillsCopyableValuesChecker(getFunction()).check()) {
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
     }
+
+    // Now search through our function one last time and any move_value
+    // [allows_diagnostics] that remain are ones that we did not know how to
+    // check so emit a diagnostic so the user doesn't assume that they have
+    // guarantees.
+    //
+    // TODO: Emit specific diagnostics here (e.x.: _move of global).
+    if (DisableUnhandledMoveDiagnostic)
+      return;
+    for (auto &block : *fn) {
+      for (auto &inst : block) {
+        if (auto *mvi = dyn_cast<MoveValueInst>(&inst)) {
+          if (mvi->getAllowDiagnostics()) {
+            auto diag = diag::
+                sil_movekillscopyablevalue_move_applied_to_unsupported_move;
+            diagnose(astContext, mvi->getLoc().getSourceLoc(), diag);
+          }
+        }
+      }
+    }
   }
 };
 

test/SILOptimizer/move_operator_kills_copyable_values.swift

@@ -229,9 +229,27 @@ public class KlassPair {
     var y = Klass()
 }
 
-// No error since we are not moving p itself, but are moving a copy of x that we
-// loaded from p and then destroying that.
+// TODO: Emit a better error here! We should state that we are applying _move to
+// a class field and that is illegal.
 public func performMoveOnOneEltOfKlassPair(_ p: KlassPair) {
-    let _ = _move(p.x)
+    let _ = _move(p.x) // expected-error {{_move applied to value that the compiler does not know how to check. Please file a bug or an enhancement request!}}
     nonConsumingUse(p.y)
 }
+
+let myLetGlobal = Klass()
+var myVarGlobal = Klass()
+
+public func performMoveOnVarGlobalError() {
+    let _ = _move(myVarGlobal) // expected-error {{_move applied to value that the compiler does not know how to check. Please file a bug or an enhancement request!}}
+}
+
+// TODO: Support vars
+public func performMoveOnVarError() {
+    var k = myVarGlobal
+    let _ = _move(k) // expected-error {{_move applied to value that the compiler does not know how to check. Please file a bug or an enhancement request!}}
+    k = myVarGlobal
+}
+
+public func performMoveOnLetGlobalError() {
+    let _ = _move(myVarGlobal) // expected-error {{_move applied to value that the compiler does not know how to check. Please file a bug or an enhancement request!}}
+}

test/stdlib/LifetimeManagement.swift

@@ -15,7 +15,8 @@ suite.test("copy") {
 
 suite.test("move") {
   let k = Klass()
-  expectTrue(k === _move(k))
+  let k2 = k
+  expectTrue(k2 === _move(k))
 }
 
 runAllTests()