Merge pull request #68031 from gottesmm/pr-6ddb1c42655a8548ef91d3b239d11cb084ff7b6d

[consume-operator] Emit a better error message when failing to consume globals or escaping captures.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -840,6 +840,9 @@ ERROR(sil_movechecking_bug_exclusivity_violation, none,
 ERROR(sil_movekillscopyablevalue_move_applied_to_unsupported_move, none,
       "'consume' applied to value that the compiler does not support. This is a compiler bug. Please file a bug with a small example of the bug",
       ())
+ERROR(sil_movekillscopyable_move_applied_to_nonlocal_memory, none,
+      "'consume' cannot be applied to %select{globals|escaping captures}0",
+      (unsigned))
 
 
 // Implicit inout-to-UnsafeRawPointer conversions

lib/SILOptimizer/Mandatory/ConsumeOperatorCopyableAddressesChecker.cpp

@@ -2387,6 +2387,70 @@ bool ConsumeOperatorCopyableAddressesChecker::check(SILValue address) {
   return result;
 }
 
+//===----------------------------------------------------------------------===//
+//                     MARK: Unsupported Use Case Errors
+//===----------------------------------------------------------------------===//
+
+namespace {
+
+struct UnsupportedUseCaseDiagnosticEmitter {
+  MarkUnresolvedMoveAddrInst *mai;
+
+  ~UnsupportedUseCaseDiagnosticEmitter() {
+    assert(!mai && "Didn't call cleanup!\n");
+  }
+
+  bool cleanup() && {
+    // Now that we have emitted the error, replace the move_addr with a
+    // copy_addr so that future passes never see it. We mark it as a
+    // copy_addr [init].
+    SILBuilderWithScope builder(mai);
+    builder.createCopyAddr(mai->getLoc(), mai->getSrc(), mai->getDest(),
+                           IsNotTake, IsInitialization);
+    mai->eraseFromParent();
+    mai = nullptr;
+    return true;
+  }
+
+  ASTContext &getASTContext() const { return mai->getModule().getASTContext(); }
+
+  void emitUnsupportedUseCaseError() const {
+    auto diag =
+        diag::sil_movekillscopyablevalue_move_applied_to_unsupported_move;
+    diagnose(getASTContext(), mai->getLoc().getSourceLoc(), diag);
+  }
+
+  /// Try to pattern match if we were trying to move a global. In such a case,
+  /// emit a better error.
+  bool tryEmitCannotConsumeNonLocalMemoryError() const {
+    auto src = stripAccessMarkers(mai->getSrc());
+
+    if (auto *gai = dyn_cast<GlobalAddrInst>(src)) {
+      auto diag = diag::sil_movekillscopyable_move_applied_to_nonlocal_memory;
+      diagnose(getASTContext(), mai->getLoc().getSourceLoc(), diag, 0);
+      return true;
+    }
+
+    // If we have a project_box, then we must have an escaping capture. It is
+    // the only case that allocbox to stack doesn't handle today.
+    if (isa<ProjectBoxInst>(src)) {
+      auto diag = diag::sil_movekillscopyable_move_applied_to_nonlocal_memory;
+      diagnose(getASTContext(), mai->getLoc().getSourceLoc(), diag, 1);
+      return true;
+    }
+
+    return false;
+  }
+
+  void emit() const {
+    if (tryEmitCannotConsumeNonLocalMemoryError())
+      return;
+    emitUnsupportedUseCaseError();
+  }
+};
+
+} // namespace
+
 //===----------------------------------------------------------------------===//
 //                            Top Level Entrypoint
 //===----------------------------------------------------------------------===//
@@ -2490,17 +2554,9 @@ class ConsumeOperatorCopyableAddressesCheckerPass
         ++ii;
 
         if (auto *mai = dyn_cast<MarkUnresolvedMoveAddrInst>(inst)) {
-          auto diag =
-              diag::sil_movekillscopyablevalue_move_applied_to_unsupported_move;
-          diagnose(astContext, mai->getLoc().getSourceLoc(), diag);
-
-          // Now that we have emitted the error, replace the move_addr with a
-          // copy_addr so that future passes never see it. We mark it as a
-          // copy_addr [init].
-          SILBuilderWithScope builder(mai);
-          builder.createCopyAddr(mai->getLoc(), mai->getSrc(), mai->getDest(),
-                                 IsNotTake, IsInitialization);
-          mai->eraseFromParent();
+          UnsupportedUseCaseDiagnosticEmitter emitter{mai};
+          emitter.emit();
+          std::move(emitter).cleanup();
           lateMadeChange = true;
         }
       }

lib/SILOptimizer/Mandatory/ConsumeOperatorCopyableValuesChecker.cpp

@@ -507,7 +507,7 @@ bool ConsumeOperatorCopyableValuesChecker::check() {
 }
 
 //===----------------------------------------------------------------------===//
-//                        Unsupported Use Case Errors
+//                     MARK: Unsupported Use Case Errors
 //===----------------------------------------------------------------------===//
 
 static void emitUnsupportedUseCaseError(MoveValueInst *mvi) {
@@ -517,6 +517,25 @@ static void emitUnsupportedUseCaseError(MoveValueInst *mvi) {
   mvi->setAllowsDiagnostics(false);
 }
 
+/// Try to pattern match if we were trying to move a global. In such a case,
+/// emit a better error.
+static bool tryEmitCannotConsumeNonLocalMemoryError(MoveValueInst *mvi) {
+  auto *li = dyn_cast<LoadInst>(mvi->getOperand());
+  if (!li)
+    return false;
+
+  auto &astContext = mvi->getModule().getASTContext();
+  if (auto *gai =
+          dyn_cast<GlobalAddrInst>(stripAccessMarkers(li->getOperand()))) {
+    auto diag = diag::sil_movekillscopyable_move_applied_to_nonlocal_memory;
+    diagnose(astContext, mvi->getLoc().getSourceLoc(), diag, 0);
+    mvi->setAllowsDiagnostics(false);
+    return true;
+  }
+
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 //                            Top Level Entrypoint
 //===----------------------------------------------------------------------===//
@@ -579,6 +598,10 @@ class ConsumeOperatorCopyableValuesCheckerPass : public SILFunctionTransform {
               continue;
             }
 
+            // Try to emit a better error if we try to consume a global.
+            if (tryEmitCannotConsumeNonLocalMemoryError(mvi))
+              continue;
+
             if (!DisableUnhandledMoveDiagnostic)
               emitUnsupportedUseCaseError(mvi);
           }

test/SILOptimizer/consume_operator_kills_copyable_addressonly_vars.swift

@@ -588,7 +588,7 @@ public func nonEscapingpartialApplyTest<T : P>(_ x: __owned T) {
 public func partialApplyTest<T : P>(_ x: __owned T) -> () -> () {
     var x2 = x
     x2 = x
-    let _ = consume x2 // expected-error {{'consume' applied to value that the compiler does not support}}
+    let _ = consume x2 // expected-error {{'consume' cannot be applied to escaping captures}}
     let f = {
         print(x2)
     }

test/SILOptimizer/consume_operator_kills_copyable_values.swift

@@ -260,11 +260,11 @@ let myLetGlobal = Klass()
 var myVarGlobal = Klass()
 
 public func performMoveOnVarGlobalError() {
-    let _ = consume myVarGlobal // expected-error {{'consume' applied to value that the compiler does not support}}
+    let _ = consume myVarGlobal // expected-error {{'consume' cannot be applied to globals}}
 }
 
 public func performMoveOnLetGlobalError() {
-    let _ = consume myVarGlobal // expected-error {{'consume' applied to value that the compiler does not support}}
+    let _ = consume myVarGlobal // expected-error {{'consume' cannot be applied to globals}}
 }
 
 public func multipleVarsWithSubsequentBorrows() -> Bool {

test/SILOptimizer/consume_operator_nonlocalmemory.swift

@@ -0,0 +1,51 @@
+// RUN: %target-swift-frontend -verify %s -emit-sil -o /dev/null
+
+// This file tests that we emit good errors for global lets/vars,
+
+////////////////////////
+// MARK: Declarations //
+////////////////////////
+
+class Klass {
+  var s1 = "123"
+  let s2 = "123"
+}
+struct LoadableStruct { var k = Klass() }
+
+struct AddressOnlyStruct<T> { var t: T? = nil }
+
+////////////////////////
+// MARK: Global Tests //
+////////////////////////
+
+let globalLoadableLet = LoadableStruct()
+let _ = consume globalLoadableLet // expected-error {{'consume' cannot be applied to globals}}
+
+let globalAddressOnlyLet = AddressOnlyStruct<Any>()
+let _ = consume globalAddressOnlyLet // expected-error {{'consume' cannot be applied to globals}}
+
+var globalLoadableVar = LoadableStruct()
+let _ = consume globalLoadableVar // expected-error {{'consume' cannot be applied to globals}}
+
+var globalAddressOnlyVar = AddressOnlyStruct<Any>()
+let _ = consume globalAddressOnlyVar // expected-error {{'consume' cannot be applied to globals}}
+
+////////////////////////////
+// MARK: Mutable Captures //
+////////////////////////////
+
+func accessMutableCapture() -> (() -> ()) {
+  func useKlassInOut(_ x: inout Klass) {}
+  func useAddressOnlyStructInOut<T>(_ x: inout AddressOnlyStruct<T>) {}
+
+  var x = Klass()
+  var x2 = AddressOnlyStruct<Any>()
+  var f: () -> () = {}
+  f = {
+    useKlassInOut(&x)
+    useAddressOnlyStructInOut(&x2)
+  }
+  let _ = consume x // expected-error {{'consume' cannot be applied to escaping captures}}
+  let _ = consume x2 // expected-error {{'consume' cannot be applied to escaping captures}}
+  return f
+}