Merge pull request #59833 from gottesmm/pr-816f80a06afa9ee661f077ea89193ef1cce5ece6

[no-implicit-copy] A few small fixes.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -729,6 +729,9 @@ ERROR(sil_moveonlychecker_value_consumed_more_than_once, none,
       "'%0' consumed more than once", (StringRef))
 NOTE(sil_moveonlychecker_consuming_use_here, none,
      "consuming use", ())
+ERROR(sil_moveonlychecker_not_understand_mark_move, none,
+      "Usage of @noImplicitCopy that the move checker does not know how to "
+      "check!", ())
 
 // move kills copyable values checker diagnostics
 ERROR(sil_movekillscopyablevalue_value_consumed_more_than_once, none,

include/swift/SIL/TypeLowering.h

@@ -169,12 +169,6 @@ enum IsInfiniteType_t : bool {
   IsInfiniteType = true,
 };
 
-/// Does this type contain a move only type that affects type lowering?
-enum IsMoveOnly_t : bool {
-  IsNotMoveOnly = false,
-  IsMoveOnly = true,
-};
-
 /// Extended type information used by SIL.
 class TypeLowering {
 public:
@@ -190,7 +184,6 @@ class TypeLowering {
       TypeExpansionSensitiveFlag = 1 << 4,
       InfiniteFlag               = 1 << 5,
       HasRawPointerFlag          = 1 << 6,
-      MoveOnlyFlag               = 1 << 7,
     };
     // clang-format on
 
@@ -205,15 +198,13 @@ class TypeLowering {
         IsAddressOnly_t isAddressOnly, IsResilient_t isResilient,
         IsTypeExpansionSensitive_t isTypeExpansionSensitive =
             IsNotTypeExpansionSensitive,
-        HasRawPointer_t hasRawPointer = DoesNotHaveRawPointer,
-        IsMoveOnly_t isMoveOnly = IsNotMoveOnly)
+        HasRawPointer_t hasRawPointer = DoesNotHaveRawPointer)
         : Flags((isTrivial ? 0U : NonTrivialFlag) |
                 (isFixedABI ? 0U : NonFixedABIFlag) |
                 (isAddressOnly ? AddressOnlyFlag : 0U) |
                 (isResilient ? ResilientFlag : 0U) |
                 (isTypeExpansionSensitive ? TypeExpansionSensitiveFlag : 0U) |
-                (hasRawPointer ? HasRawPointerFlag : 0U) |
-                (isMoveOnly ? MoveOnlyFlag : 0U)) {}
+                (hasRawPointer ? HasRawPointerFlag : 0U)) {}
 
     constexpr bool operator==(RecursiveProperties p) const {
       return Flags == p.Flags;
@@ -240,36 +231,6 @@ class TypeLowering {
       return {IsTrivial, IsFixedABI, IsNotAddressOnly, IsResilient};
     }
 
-    static constexpr RecursiveProperties forMoveOnlyReference() {
-      return {IsNotTrivial,
-              IsFixedABI,
-              IsNotAddressOnly,
-              IsNotResilient,
-              IsNotTypeExpansionSensitive,
-              DoesNotHaveRawPointer,
-              IsMoveOnly};
-    }
-
-    static constexpr RecursiveProperties forMoveOnlyOpaque() {
-      return {IsNotTrivial,
-              IsNotFixedABI,
-              IsAddressOnly,
-              IsNotResilient,
-              IsNotTypeExpansionSensitive,
-              DoesNotHaveRawPointer,
-              IsMoveOnly};
-    }
-
-    static constexpr RecursiveProperties forMoveOnlyResilient() {
-      return {IsTrivial,
-              IsFixedABI,
-              IsNotAddressOnly,
-              IsResilient,
-              IsNotTypeExpansionSensitive,
-              DoesNotHaveRawPointer,
-              IsMoveOnly};
-    }
-
     void addSubobject(RecursiveProperties other) {
       Flags |= other.Flags;
     }
@@ -296,9 +257,6 @@ class TypeLowering {
     IsInfiniteType_t isInfinite() const {
       return IsInfiniteType_t((Flags & InfiniteFlag) != 0);
     }
-    IsMoveOnly_t isMoveOnlyWrapped() const {
-      return IsMoveOnly_t((Flags & MoveOnlyFlag) != 0);
-    }
 
     void setNonTrivial() { Flags |= NonTrivialFlag; }
     void setNonFixedABI() { Flags |= NonFixedABIFlag; }
@@ -309,7 +267,6 @@ class TypeLowering {
               (isTypeExpansionSensitive ? TypeExpansionSensitiveFlag : 0);
     }
     void setInfinite() { Flags |= InfiniteFlag; }
-    void setMoveOnly() { Flags |= MoveOnlyFlag; }
   };
 
 private:

lib/SIL/IR/TypeLowering.cpp

@@ -244,24 +244,12 @@ namespace {
                                            RecursiveProperties::forReference());
     }
 
-    RecursiveProperties getMoveOnlyReferenceRecursiveProperties(
-        IsTypeExpansionSensitive_t isSensitive) {
-      return mergeIsTypeExpansionSensitive(isSensitive,
-                                           RecursiveProperties::forReference());
-    }
-
     RecursiveProperties
     getOpaqueRecursiveProperties(IsTypeExpansionSensitive_t isSensitive) {
       return mergeIsTypeExpansionSensitive(isSensitive,
                                            RecursiveProperties::forOpaque());
     }
 
-    RecursiveProperties getMoveOnlyOpaqueRecursiveProperties(
-        IsTypeExpansionSensitive_t isSensitive) {
-      return mergeIsTypeExpansionSensitive(
-          isSensitive, RecursiveProperties::forMoveOnlyOpaque());
-    }
-
 #define IMPL(TYPE, LOWERING)                                                 \
     RetTy visit##TYPE##Type(Can##TYPE##Type type, AbstractionPattern orig,   \
                             IsTypeExpansionSensitive_t isSensitive) {        \
@@ -704,12 +692,12 @@ namespace {
       if (lowering.isAddressOnly()) {
         return asImpl().handleMoveOnlyAddressOnly(
             type->getCanonicalType(),
-            getMoveOnlyOpaqueRecursiveProperties(isSensitive));
+            getOpaqueRecursiveProperties(isSensitive));
       }
 
       return asImpl().handleMoveOnlyReference(
           type->getCanonicalType(),
-          getMoveOnlyReferenceRecursiveProperties(isSensitive));
+          getReferenceRecursiveProperties(isSensitive));
     }
 
     RetTy handleAggregateByProperties(CanType type, RecursiveProperties props) {

lib/SILOptimizer/Mandatory/MoveOnlyChecker.cpp

@@ -476,17 +476,28 @@ struct MoveOnlyChecker {
   MoveOnlyChecker(SILFunction *fn, DeadEndBlocks *deBlocks)
       : fn(fn), copyOfBorrowedProjectionChecker(deBlocks) {}
 
+  /// Search through the current function for candidate mark_must_check
+  /// [noimplicitcopy]. If we find one that does not fit a pattern that we
+  /// understand, emit an error diagnostic telling the programmer that the move
+  /// checker did not know how to recognize this code pattern.
+  ///
+  /// \returns true if we deleted a mark_must_check inst that we didn't
+  /// recognize after emitting the diagnostic.
+  bool searchForCandidateMarkMustChecks();
+
   bool check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
              DominanceInfo *domTree);
 };
 
 } // namespace
 
-bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
-                            DominanceInfo *domTree) {
+bool MoveOnlyChecker::searchForCandidateMarkMustChecks() {
+  bool changed = false;
   for (auto &block : *fn) {
-    for (auto &ii : block) {
-      auto *mmci = dyn_cast<MarkMustCheckInst>(&ii);
+    for (auto ii = block.begin(), ie = block.end(); ii != ie;) {
+      auto *mmci = dyn_cast<MarkMustCheckInst>(&*ii);
+      ++ii;
+
       if (!mmci || !mmci->isNoImplicitCopy())
         continue;
 
@@ -496,6 +507,7 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
           if (auto *bbi = dyn_cast<BeginBorrowInst>(cvi->getOperand())) {
             if (bbi->isLexical()) {
               moveIntroducersToProcess.insert(mmci);
+              continue;
             }
           }
         }
@@ -505,11 +517,44 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
         if (auto *bbi = dyn_cast<BeginBorrowInst>(cvi->getOperand())) {
           if (bbi->isLexical()) {
             moveIntroducersToProcess.insert(mmci);
+            continue;
           }
         }
       }
+
+      // If we see a mark_must_check that is marked no implicit copy that we
+      // don't understand, emit a diagnostic to fail the compilation. This
+      // ensures that if someone marks something no implicit copy and we fail to
+      // check it, we fail the compilation.
+      //
+      // We then RAUW the mark_must_check once we have emitted the error since
+      // later passes expect that mark_must_check has been eliminated by
+      // us. Since we are failing already, this is ok to do.
+      diagnose(fn->getASTContext(), mmci->getLoc().getSourceLoc(),
+               diag::sil_moveonlychecker_not_understand_mark_move);
+      mmci->replaceAllUsesWith(mmci->getOperand());
+      mmci->eraseFromParent();
+      changed = true;
     }
   }
+  return changed;
+}
+
+bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
+                            DominanceInfo *domTree) {
+  bool changed = false;
+
+  // First search for candidates to process and emit diagnostics on any
+  // mark_must_check [noimplicitcopy] we didn't recognize.
+  changed |= searchForCandidateMarkMustChecks();
+
+  // If we didn't find any introducers to check, just return changed.
+  //
+  // NOTE: changed /can/ be true here if we had any mark_must_check
+  // [noimplicitcopy] that we didn't understand and emitting a diagnostic upon
+  // and then deleting.
+  if (moveIntroducersToProcess.empty())
+    return changed;
 
   auto callbacks =
       InstModCallbacks().onDelete([&](SILInstruction *instToDelete) {
@@ -518,7 +563,6 @@ bool MoveOnlyChecker::check(NonLocalAccessBlockAnalysis *accessBlockAnalysis,
         instToDelete->eraseFromParent();
       });
   InstructionDeleter deleter(std::move(callbacks));
-  bool changed = false;
 
   SmallVector<Operand *, 32> consumingUsesNeedingCopy;
   auto foundConsumingUseNeedingCopy = [&](Operand *use) {