[MoveChecker] Distinguished scope end diagnostics.

There are several kinds of scopes at which it is required that an
address be initialized:
(1) the whole function -- for inout argument to the function
(2) the region a coroutine is active -- for an inout yielded by a
    coroutine into the function
(3) the region of a memory access -- for a `begin_access [modify]`.

The move checker enforces that they are initialized at that point by
adding instructions at which the field must be live to liveness.

Previously, all such scopes used the end of the function as the point at
which the memory had to have been reinitialized.  Here, the relevant end
of scope markers are used instead.

More importantly, here the diagnostic is made to vary--the diagnostic,
that is, that is issued in the face an address not being initialized at
the end of these different kind of scopes.

Author: nate-chandler

include/swift/AST/DiagnosticsSIL.def

@@ -806,6 +806,12 @@ ERROR(sil_movechecking_capture_consumed, none,
 ERROR(sil_movechecking_not_reinitialized_before_end_of_function, none,
       "missing reinitialization of %select{inout parameter|closure capture}1 '%0' "
       "after consume", (StringRef, bool))
+ERROR(sil_movechecking_not_reinitialized_before_end_of_coroutine, none,
+      "field '%0' was consumed but not reinitialized; "
+      "the field must be reinitialized during the access", (StringRef))
+ERROR(sil_movechecking_not_reinitialized_before_end_of_access, none,
+      "missing reinitialization of %select{inout parameter|closure capture}1 '%0' "
+      "after consume while accessing memory", (StringRef, bool))
 ERROR(sil_movechecking_value_consumed_in_a_loop, none,
       "'%0' consumed in a loop", (StringRef))
 ERROR(sil_movechecking_use_after_partial_consume, none,

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -368,15 +368,39 @@ static bool isReinitToInitConvertibleInst(SILInstruction *memInst) {
   }
 }
 
-/// Returns true if \p value a function argument from an inout argument or a
-/// value extracted from a closure captured box that we did not convert to an
-/// address.
+using ScopeRequiringFinalInit = DiagnosticEmitter::ScopeRequiringFinalInit;
+
+/// If \p markedAddr's operand must be initialized at the end of the scope it
+/// introduces, visit those scope ending ends.
+///
+/// Examples:
+/// (1) inout function argument.  Must be initialized at function exit.
+///
+///     sil [ossa] @f : $(inout MOV) -> ()
+///     entry(%addr : $*MOV):
+///     ...
+///       return %t : $() // %addr must be initialized here
+///
+/// (2) coroutine.  Must be initialized at end_apply/abort_apply.
+///
+///     (%addr, %token) = begin_apply ... -> @yields @inout MOV
+///     bbN:
+///       end_apply %token // %addr must be initialized here
+///     bbM:
+///       abort_apply %token // %addr must be initialized here
+///
+/// (3) modify access.  Must be initialized at end_access.
+///
+///     %addr = begin_access [modify] %location
 ///
-/// These are cases where we want to treat the end of the function as a liveness
-/// use to ensure that we reinitialize \p value before the end of the function
-/// if we consume \p value in the function body.
-static bool isInOutDefThatNeedsEndOfFunctionLiveness(
-    MarkUnresolvedNonCopyableValueInst *markedAddr) {
+///     end_access %addr // %addr must be initialized here
+///
+/// To enforce this requirement, function exiting instructions are treated as
+/// liveness uses of such addresses, ensuring that the address is initialized at
+/// that point.
+static bool visitScopeEndsRequiringInit(
+    MarkUnresolvedNonCopyableValueInst *markedAddr,
+    llvm::function_ref<void(SILInstruction *, ScopeRequiringFinalInit)> visit) {
   SILValue operand = markedAddr->getOperand();
 
   // TODO: This should really be a property of the marker instruction.
@@ -403,17 +427,32 @@ static bool isInOutDefThatNeedsEndOfFunctionLiveness(
     case SILArgumentConvention::Indirect_InoutAliasable:
     case SILArgumentConvention::Pack_Inout:
       LLVM_DEBUG(llvm::dbgs() << "Found inout arg: " << *fArg);
+      SmallVector<SILBasicBlock *, 8> exitBlocks;
+      markedAddr->getFunction()->findExitingBlocks(exitBlocks);
+      for (auto *block : exitBlocks) {
+        visit(block->getTerminator(), ScopeRequiringFinalInit::InoutArgument);
+      }
       return true;
     }
   }
   // Check for yields from a modify coroutine.
   if (auto bai =
           dyn_cast_or_null<BeginApplyInst>(operand->getDefiningInstruction())) {
+    for (auto *inst : bai->getTokenResult()->getUsers()) {
+      assert(isa<EndApplyInst>(inst) || isa<AbortApplyInst>(inst));
+      visit(inst, ScopeRequiringFinalInit::Coroutine);
+    }
     return true;
   }
   // Check for modify accesses.
   if (auto access = dyn_cast<BeginAccessInst>(operand)) {
-    return access->getAccessKind() == SILAccessKind::Modify;
+    if (access->getAccessKind() != SILAccessKind::Modify) {
+      return false;
+    }
+    for (auto *inst : access->getEndAccesses()) {
+      visit(inst, ScopeRequiringFinalInit::ModifyMemoryAccess);
+    }
+    return true;
   }
 
   return false;
@@ -534,12 +573,15 @@ struct UseState {
   /// The set of drop_deinits of this mark_unresolved_non_copyable_value
   llvm::SmallSetVector<SILInstruction *, 2> dropDeinitInsts;
 
-  /// A "inout terminator use" is an implicit liveness use of the entire value
-  /// placed on a terminator. We use this both so we add liveness for the
-  /// terminator user and so that we can use the set to quickly identify later
-  /// while emitting diagnostics that a liveness use is a terminator user and
-  /// emit a specific diagnostic message.
-  llvm::SmallSetVector<SILInstruction *, 2> implicitEndOfLifetimeLivenessUses;
+  /// Instructions indicating the end of a scope at which addr must be
+  /// initialized.
+  ///
+  /// Adding such instructions to liveness forces the value to be initialized at
+  /// them as required.
+  ///
+  /// See visitScopeEndsRequiringInit.
+  llvm::MapVector<SILInstruction *, ScopeRequiringFinalInit>
+      scopeEndsRequiringInit;
 
   /// We add debug_values to liveness late after we diagnose, but before we
   /// hoist destroys to ensure that we do not hoist destroys out of access
@@ -604,8 +646,13 @@ struct UseState {
   /// instruction.
   /// 2. In the case of a ref_element_addr or a global, this will contain the
   /// end_access.
-  bool isImplicitEndOfLifetimeLivenessUses(SILInstruction *inst) const {
-    return implicitEndOfLifetimeLivenessUses.count(inst);
+  llvm::Optional<ScopeRequiringFinalInit>
+  isImplicitEndOfLifetimeLivenessUses(SILInstruction *inst) const {
+    auto iter = scopeEndsRequiringInit.find(inst);
+    if (iter == scopeEndsRequiringInit.end()) {
+      return llvm::None;
+    }
+    return {iter->second};
   }
 
   /// Returns true if the given instruction is within the same block as a reinit
@@ -647,7 +694,7 @@ struct UseState {
     reinitInsts.clear();
     reinitToValueMultiMap.reset();
     dropDeinitInsts.clear();
-    implicitEndOfLifetimeLivenessUses.clear();
+    scopeEndsRequiringInit.clear();
     debugValue = nullptr;
   }
 
@@ -686,8 +733,8 @@ struct UseState {
       llvm::dbgs() << *inst;
     }
     llvm::dbgs() << "Implicit End Of Lifetime Liveness Users:\n";
-    for (auto *inst : implicitEndOfLifetimeLivenessUses) {
-      llvm::dbgs() << *inst;
+    for (auto pair : scopeEndsRequiringInit) {
+      llvm::dbgs() << pair.first;
     }
     llvm::dbgs() << "Debug Value User:\n";
     if (debugValue) {
@@ -724,25 +771,20 @@ struct UseState {
   initializeLiveness(FieldSensitiveMultiDefPrunedLiveRange &prunedLiveness);
 
   void initializeImplicitEndOfLifetimeLivenessUses() {
-    if (isInOutDefThatNeedsEndOfFunctionLiveness(address)) {
-      SmallVector<SILBasicBlock *, 8> exitBlocks;
-      address->getFunction()->findExitingBlocks(exitBlocks);
-      for (auto *block : exitBlocks) {
-        LLVM_DEBUG(llvm::dbgs() << "    Adding term as liveness user: "
-                                << *block->getTerminator());
-        implicitEndOfLifetimeLivenessUses.insert(block->getTerminator());
-      }
-      return;
-    }
-
+    visitScopeEndsRequiringInit(address, [&](auto *inst, auto kind) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "    Adding scope end as liveness user: " << *inst);
+      scopeEndsRequiringInit[inst] = kind;
+    });
     if (address->getCheckKind() == MarkUnresolvedNonCopyableValueInst::
                                        CheckKind::AssignableButNotConsumable) {
       if (auto *bai = dyn_cast<BeginAccessInst>(address->getOperand())) {
         for (auto *eai : bai->getEndAccesses()) {
           LLVM_DEBUG(llvm::dbgs() << "    Adding end_access as implicit end of "
                                      "lifetime liveness user: "
                                   << *eai);
-          implicitEndOfLifetimeLivenessUses.insert(eai);
+          scopeEndsRequiringInit[eai] =
+              ScopeRequiringFinalInit::ModifyMemoryAccess;
         }
       }
     }
@@ -804,7 +846,7 @@ struct UseState {
     // An "inout terminator use" is an implicit liveness use of the entire
     // value. This is because we need to ensure that our inout value is
     // reinitialized along exit paths.
-    if (implicitEndOfLifetimeLivenessUses.count(inst))
+    if (scopeEndsRequiringInit.count(inst))
       return true;
 
     return false;
@@ -1191,10 +1233,10 @@ void UseState::initializeLiveness(
   // ref_element_addr or global_addr, add a liveness use of the entire value on
   // the implicit end lifetime instruction. For inout this is terminators for
   // ref_element_addr, global_addr it is the end_access instruction.
-  for (auto *inst : implicitEndOfLifetimeLivenessUses) {
-    liveness.updateForUse(inst, TypeTreeLeafTypeRange(address),
+  for (auto pair : scopeEndsRequiringInit) {
+    liveness.updateForUse(pair.first, TypeTreeLeafTypeRange(address),
                           false /*lifetime ending*/);
-    LLVM_DEBUG(llvm::dbgs() << "Added liveness for inoutTermUser: " << *inst;
+    LLVM_DEBUG(llvm::dbgs() << "Added liveness for scope end: " << pair.first;
                liveness.print(llvm::dbgs()));
   }
 

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -669,7 +669,7 @@ void DiagnosticEmitter::emitAddressExclusivityHazardDiagnostic(
 void DiagnosticEmitter::emitAddressDiagnostic(
     MarkUnresolvedNonCopyableValueInst *markedValue,
     SILInstruction *lastLiveUser, SILInstruction *violatingUser,
-    bool isUseConsuming, bool isInOutEndOfFunction) {
+    bool isUseConsuming, llvm::Optional<ScopeRequiringFinalInit> scopeKind) {
   if (!useWithDiagnostic.insert(violatingUser).second)
     return;
   registerDiagnosticEmitted(markedValue);
@@ -695,12 +695,24 @@ void DiagnosticEmitter::emitAddressDiagnostic(
     return;
   }
 
-  if (isInOutEndOfFunction) {
-    diagnose(
-        astContext, markedValue,
-        diag::
-            sil_movechecking_not_reinitialized_before_end_of_function,
-        varName, isClosureCapture(markedValue));
+  if (scopeKind.has_value()) {
+    switch (scopeKind.value()) {
+    case ScopeRequiringFinalInit::InoutArgument:
+      diagnose(astContext, markedValue,
+               diag::sil_movechecking_not_reinitialized_before_end_of_function,
+               varName, isClosureCapture(markedValue));
+      break;
+    case ScopeRequiringFinalInit::Coroutine:
+      diagnose(astContext, markedValue,
+               diag::sil_movechecking_not_reinitialized_before_end_of_coroutine,
+               varName);
+      break;
+    case ScopeRequiringFinalInit::ModifyMemoryAccess:
+      diagnose(astContext, markedValue,
+               diag::sil_movechecking_not_reinitialized_before_end_of_access,
+               varName, isClosureCapture(markedValue));
+      break;
+    }
     diagnose(astContext, violatingUser,
              diag::sil_movechecking_consuming_use_here);
     return;

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.h

@@ -144,10 +144,29 @@ class DiagnosticEmitter {
     return valuesWithDiagnostics.count(markedValue);
   }
 
-  void emitAddressDiagnostic(MarkUnresolvedNonCopyableValueInst *markedValue,
-                             SILInstruction *lastLiveUse,
-                             SILInstruction *violatingUse, bool isUseConsuming,
-                             bool isInOutEndOfFunction = false);
+  /// The kind of scope at the end of which an address must be initialized.
+  enum class ScopeRequiringFinalInit {
+    /// The scope for an inout argument.
+    ///
+    /// The whole function.
+    InoutArgument,
+    /// The scope for an address yielded by a coroutine.
+    ///
+    /// It begins at the begin_apply and ends at all corresponding
+    /// end_apply/abort_apply instructions.
+    Coroutine,
+    /// The scope for an address done through an access scope marker.
+    ///
+    /// It begins at the begin_access and ends at all corresponding end_access
+    /// instructions.
+    ModifyMemoryAccess,
+  };
+
+  void emitAddressDiagnostic(
+      MarkUnresolvedNonCopyableValueInst *markedValue,
+      SILInstruction *lastLiveUse, SILInstruction *violatingUse,
+      bool isUseConsuming,
+      llvm::Optional<ScopeRequiringFinalInit> scopeKind = llvm::None);
   void emitInOutEndOfFunctionDiagnostic(
       MarkUnresolvedNonCopyableValueInst *markedValue,
       SILInstruction *violatingUse);

test/SILOptimizer/moveonly_addresschecker_diagnostics_resilient.swift

@@ -0,0 +1,92 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend                                      \
+// RUN:     %t/Library.swift                                        \
+// RUN:     -emit-module                                            \
+// RUN:     -enable-library-evolution                               \
+// RUN:     -enable-experimental-feature MoveOnlyPartialConsumption \
+// RUN:     -module-name Library                                    \
+// RUN:     -emit-module-path %t/Library.swiftmodule
+
+// RUN: %target-swift-frontend                                      \
+// RUN:     %t/Client.swift                                         \
+// RUN:     -emit-sil -verify                                       \
+// RUN:     -debug-diagnostic-names                                 \
+// RUN:     -sil-verify-all                                         \
+// RUN:     -enable-experimental-feature MoveOnlyPartialConsumption \
+// RUN:     -I %t
+
+
+//--- Library.swift
+
+public struct Ur : ~Copyable {
+  deinit {}
+}
+public func consume(_ ur: consuming Ur) {}
+public func borrow(_ ur: borrowing Ur) {}
+
+@frozen
+public struct AggFrozen : ~Copyable {
+  public var field1: Ur
+  public var field2: Ur
+}
+public func consume(_ a: consuming AggFrozen) {}
+public func borrow(_ ur: borrowing AggFrozen) {}
+
+@frozen
+public struct AggFrozenDeiniting : ~Copyable {
+  public var field1: Ur
+  public var field2: Ur
+  deinit {}
+}
+public func consume(_ a: consuming AggFrozenDeiniting) {}
+public func borrow(_ ur: borrowing AggFrozenDeiniting) {}
+
+public struct AggResilient : ~Copyable {
+  public var field1: Ur
+  public var field2: Ur
+}
+public func consume(_ a: consuming AggResilient) {}
+public func borrow(_ ur: borrowing AggResilient) {}
+
+public struct AggResilientDeiniting : ~Copyable {
+  public var field1: Ur
+  public var field2: Ur
+  deinit {}
+}
+public func consume(_ a: consuming AggResilientDeiniting) {}
+public func borrow(_ ur: borrowing AggResilientDeiniting) {}
+
+//--- Client.swift
+
+import Library
+
+struct AggLocalDeiniting : ~Copyable {
+  var field1: Ur
+  var field2: Ur
+  deinit {}
+}
+
+struct AggLocal : ~Copyable {
+  var field1: Ur
+  var field2: Ur
+}
+
+func consumeField1_AggFrozen(_ a: consuming AggFrozen) {
+  consume(a.field1)
+}
+
+func consumeField1_AggFrozenDeiniting(_ a: consuming AggFrozenDeiniting) {
+  consume(a.field1) // expected-error{{cannot partially consume 'a' when it has a deinitializer}}
+}
+
+func consumeField1_AggResilient(_ a: consuming AggResilient) {
+  consume(a.field1) // expected-error{{field 'a.field1' was consumed but not reinitialized; the field must be reinitialized during the access}}
+                    // expected-note@-1{{consumed here}}
+}
+
+func consumeField1_AggResilientDeiniting(_ a: consuming AggResilientDeiniting) {
+  consume(a.field1) // expected-error{{field 'a.field1' was consumed but not reinitialized; the field must be reinitialized during the access}}
+                    // expected-note@-1{{consumed here}}
+}