[MoveChecker] Separate partial reinit from consume

nate-chandler · nate-chandler · commit 03f904af0c90 · 2024-01-29T18:29:40.000-08:00
diff --git a/include/swift/Basic/Features.def b/include/swift/Basic/Features.def
@@ -151,6 +151,7 @@ EXPERIMENTAL_FEATURE(OldOwnershipOperatorSpellings, true)
 EXPERIMENTAL_FEATURE(MoveOnlyEnumDeinits, true)
 EXPERIMENTAL_FEATURE(MoveOnlyTuples, true)
 EXPERIMENTAL_FEATURE(MoveOnlyPartialConsumption, true)
+EXPERIMENTAL_FEATURE(MoveOnlyPartialReinitialization, true)
 
 EXPERIMENTAL_FEATURE(OneWayClosureParameters, false)
 EXPERIMENTAL_FEATURE(LayoutPrespecialization, true)
diff --git a/lib/AST/ASTPrinter.cpp b/lib/AST/ASTPrinter.cpp
@@ -3631,6 +3631,10 @@ static bool usesFeatureMoveOnlyPartialConsumption(Decl *decl) {
   return false;
 }
 
+static bool usesFeatureMoveOnlyPartialReinitialization(Decl *decl) {
+  return false;
+}
+
 static bool usesFeatureNoncopyableGenerics(Decl *decl) {
   auto checkMarking = [](auto &marking) -> bool {
     switch (marking.getInverse().getKind()) {
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp
@@ -1654,8 +1654,8 @@ struct CopiedLoadBorrowEliminationVisitor
 
 /// Whether an error should be emitted in response to a partial consumption.
 static llvm::Optional<PartialMutationError>
-shouldEmitPartialMutationError(UseState &useState, SILInstruction *user,
-                               SILType useType,
+shouldEmitPartialMutationError(UseState &useState, PartialMutation::Kind kind,
+                               SILInstruction *user, SILType useType,
                                TypeTreeLeafTypeRange usedBits) {
   SILFunction *fn = useState.getFunction();
 
@@ -1672,18 +1672,19 @@ shouldEmitPartialMutationError(UseState &useState, SILInstruction *user,
   LLVM_DEBUG(llvm::dbgs() << "    Iter Type: " << iterType << '\n'
                           << "    Target Type: " << targetType << '\n');
 
-  if (!fn->getModule().getASTContext().LangOpts.hasFeature(
-          Feature::MoveOnlyPartialConsumption)) {
-    LLVM_DEBUG(llvm::dbgs() << "    MoveOnlyPartialConsumption disabled!\n");
-    // If the types equal, just bail early.
-    if (iterType == targetType) {
-      LLVM_DEBUG(llvm::dbgs() << "    IterType is TargetType! Exiting early "
-                                 "without emitting error!\n");
-      return {};
-    }
+  if (iterType == targetType) {
+    LLVM_DEBUG(llvm::dbgs() << "    IterType is TargetType! Exiting early "
+                               "without emitting error!\n");
+    return {};
+  }
 
+  auto feature = partialMutationFeature(kind);
+  if (!fn->getModule().getASTContext().LangOpts.hasFeature(feature)) {
+    LLVM_DEBUG(llvm::dbgs()
+               << "    " << getFeatureName(feature) << " disabled!\n");
+    // If the types equal, just bail early.
     // Emit the error.
-    return {PartialMutationError::featureDisabled(iterType)};
+    return {PartialMutationError::featureDisabled(iterType, kind)};
   }
 
   LLVM_DEBUG(llvm::dbgs() << "    MoveOnlyPartialConsumption enabled!\n");
@@ -1715,13 +1716,14 @@ shouldEmitPartialMutationError(UseState &useState, SILInstruction *user,
 
 static bool checkForPartialMutation(UseState &useState,
                                     DiagnosticEmitter &diagnosticEmitter,
+                                    PartialMutation::Kind kind,
                                     SILInstruction *user, SILType useType,
                                     TypeTreeLeafTypeRange usedBits,
                                     PartialMutation partialMutateKind) {
   // We walk down from our ancestor to our projection, emitting an error if
   // any of our types have a deinit.
   auto error =
-      shouldEmitPartialMutationError(useState, user, useType, usedBits);
+      shouldEmitPartialMutationError(useState, kind, user, useType, usedBits);
   if (!error)
     return false;
 
@@ -1762,8 +1764,9 @@ void PartialReinitChecker::performPartialReinitChecking(
             initToValues.first, index,
             [&](SILInstruction *consumingInst) -> bool {
               return !checkForPartialMutation(
-                  useState, diagnosticEmitter, initToValues.first,
-                  value->getType(), TypeTreeLeafTypeRange(index, index + 1),
+                  useState, diagnosticEmitter, PartialMutation::Kind::Reinit,
+                  initToValues.first, value->getType(),
+                  TypeTreeLeafTypeRange(index, index + 1),
                   PartialMutation::reinit(*consumingInst));
             });
 
@@ -1797,8 +1800,9 @@ void PartialReinitChecker::performPartialReinitChecking(
             reinitToValues.first, index,
             [&](SILInstruction *consumingInst) -> bool {
               return !checkForPartialMutation(
-                  useState, diagnosticEmitter, reinitToValues.first,
-                  value->getType(), TypeTreeLeafTypeRange(index, index + 1),
+                  useState, diagnosticEmitter, PartialMutation::Kind::Reinit,
+                  reinitToValues.first, value->getType(),
+                  TypeTreeLeafTypeRange(index, index + 1),
                   PartialMutation::reinit(*consumingInst));
             });
         if (emittedError)
@@ -2028,7 +2032,8 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
     // If we have a copy_addr, we are either going to have a take or a
     // copy... in either case, this copy_addr /is/ going to be a consuming
     // operation. Make sure to check if we semantically destructure.
-    checkForPartialMutation(useState, diagnosticEmitter, op->getUser(),
+    checkForPartialMutation(useState, diagnosticEmitter,
+                            PartialMutation::Kind::Consume, op->getUser(),
                             op->get()->getType(), *leafRange,
                             PartialMutation::consume());
 
@@ -2223,7 +2228,8 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
     } else {
       // Now that we know that we are going to perform a take, perform a
       // checkForDestructure.
-      checkForPartialMutation(useState, diagnosticEmitter, op->getUser(),
+      checkForPartialMutation(useState, diagnosticEmitter,
+                              PartialMutation::Kind::Consume, op->getUser(),
                               op->get()->getType(), *leafRange,
                               PartialMutation::consume());
 
@@ -2280,7 +2286,8 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
     // error.
     unsigned numDiagnostics =
         moveChecker.diagnosticEmitter.getDiagnosticCount();
-    checkForPartialMutation(useState, diagnosticEmitter, op->getUser(),
+    checkForPartialMutation(useState, diagnosticEmitter,
+                            PartialMutation::Kind::Consume, op->getUser(),
                             op->get()->getType(), *leafRange,
                             PartialMutation::consume());
     if (numDiagnostics != moveChecker.diagnosticEmitter.getDiagnosticCount()) {
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp
@@ -967,13 +967,11 @@ void DiagnosticEmitter::emitCannotPartiallyMutateError(
   if (!pathString.empty())
     varName.append(pathString);
 
-  bool hasPartialConsumption =
-      astContext.LangOpts.hasFeature(Feature::MoveOnlyPartialConsumption);
-  (void)hasPartialConsumption;
-
   switch (error) {
-  case PartialMutationError::Kind::FeatureDisabled:
-    assert(!hasPartialConsumption);
+  case PartialMutationError::Kind::FeatureDisabled: {
+    assert(!astContext.LangOpts.hasFeature(
+        partialMutationFeature(error.getKind())));
+
     switch (kind) {
     case PartialMutation::Kind::Consume:
       diagnose(astContext, user, diag::sil_movechecking_cannot_destructure,
@@ -988,8 +986,12 @@ void DiagnosticEmitter::emitCannotPartiallyMutateError(
     }
     registerDiagnosticEmitted(address);
     return;
+  }
   case PartialMutationError::Kind::HasDeinit: {
-    assert(hasPartialConsumption);
+    assert(
+        astContext.LangOpts.hasFeature(Feature::MoveOnlyPartialConsumption) ||
+        astContext.LangOpts.hasFeature(
+            Feature::MoveOnlyPartialReinitialization));
     switch (kind) {
     case PartialMutation::Kind::Consume:
       diagnose(astContext, user,
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyTypeUtils.h b/lib/SILOptimizer/Mandatory/MoveOnlyTypeUtils.h
@@ -20,6 +20,7 @@
 #ifndef SWIFT_SILOPTIMIZER_MANDATORY_MOVEONLYTYPEUTILS_H
 #define SWIFT_SILOPTIMIZER_MANDATORY_MOVEONLYTYPEUTILS_H
 
+#include "swift/Basic/Feature.h"
 #include "swift/Basic/TaggedUnion.h"
 #include "swift/SIL/FieldSensitivePrunedLiveness.h"
 #include "swift/SIL/SILBuilder.h"
@@ -127,6 +128,15 @@ class PartialMutation {
   }
 };
 
+inline Feature partialMutationFeature(PartialMutation::Kind kind) {
+  switch (kind) {
+  case PartialMutation::Kind::Consume:
+    return Feature::MoveOnlyPartialConsumption;
+  case PartialMutation::Kind::Reinit:
+    return Feature::MoveOnlyPartialReinitialization;
+  }
+}
+
 /// How a partial mutation (consume/reinit) is illegal for diagnostic emission.
 ///
 /// Emulates the following enum with associated value:
@@ -136,7 +146,9 @@ class PartialMutation {
 /// case hasDeinit(SILType, NominalTypeDecl)
 /// }
 class PartialMutationError {
-  struct FeatureDisabled {};
+  struct FeatureDisabled {
+    PartialMutation::Kind kind;
+  };
   struct HasDeinit {
     NominalTypeDecl &nominal;
   };
@@ -176,8 +188,9 @@ class PartialMutationError {
     return Kind::HasDeinit;
   }
 
-  static PartialMutationError featureDisabled(SILType type) {
-    return PartialMutationError(type, FeatureDisabled{});
+  static PartialMutationError featureDisabled(SILType type,
+                                              PartialMutation::Kind kind) {
+    return PartialMutationError(type, FeatureDisabled{kind});
   }
 
   static PartialMutationError hasDeinit(SILType type,
@@ -186,6 +199,7 @@ class PartialMutationError {
   }
 
   NominalTypeDecl &getNominal() { return kind.get<HasDeinit>().nominal; };
+  PartialMutation::Kind getKind() { return kind.get<FeatureDisabled>().kind; };
 };
 } // namespace siloptimizer
 } // namespace swift
diff --git a/test/SILOptimizer/moveonly_addresschecker_diagnostics.swift b/test/SILOptimizer/moveonly_addresschecker_diagnostics.swift
@@ -1,4 +1,4 @@
-// RUN: %target-swift-emit-sil -O -sil-verify-all -verify -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyClasses %s
+// RUN: %target-swift-emit-sil -O -sil-verify-all -verify -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature MoveOnlyPartialReinitialization -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyClasses %s
 
 //////////////////
 // Declarations //
diff --git a/test/SILOptimizer/moveonly_addresschecker_diagnostics_without_partial_reinit.swift b/test/SILOptimizer/moveonly_addresschecker_diagnostics_without_partial_reinit.swift
@@ -0,0 +1,45 @@
+// RUN: %target-swift-emit-sil -O -sil-verify-all -verify -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyClasses -enable-experimental-feature MoveOnlyPartialConsumption %s
+
+// Test diagnostics for partial-consumption without partial-reinitialization.
+
+struct Ur : ~Copyable {
+  deinit {}
+}
+
+struct Agg : ~Copyable {
+  var u1: Ur
+  var u2: Ur
+}
+
+func borrow(_ u1: borrowing Ur) {}
+func consume(_ u1: consuming Ur) {}
+func consume(_ u1: consuming Agg) {}
+func rewrite(_ u: inout Ur) {}
+
+// First consumes and then reinits each field.
+func reinit_1(agg: inout Agg, u1: consuming Ur, u2: consuming Ur) {
+  consume(agg.u1)
+  consume(agg.u2)
+  agg.u1 = u1 // expected-error{{cannot partially reinitialize 'agg' after it has been consumed; only full reinitialization is allowed}}
+              // expected-note@-3{{consumed here}}
+  agg.u2 = u2 // expected-error{{cannot partially reinitialize 'agg' after it has been consumed; only full reinitialization is allowed}}
+              // expected-note@-4{{consumed here}}
+}
+
+// Just replace fields of inout argument, no reinitialization.
+func not_reinit_1(agg: inout Agg, u1: consuming Ur, u2: consuming Ur) {
+  agg.u1 = u1
+  agg.u2 = u2
+}
+
+// Full reinitialization is legal.
+func not_reinit_2(agg: inout Agg, agg2: consuming Agg) {
+  consume(agg)
+  agg = agg2
+}
+
+// Passing fields as inout is allowed.
+func not_reinit_3(agg: inout Agg) {
+  rewrite(&agg.u1)
+  rewrite(&agg.u2)
+}
diff --git a/test/SILOptimizer/moveonly_generics_basic.swift b/test/SILOptimizer/moveonly_generics_basic.swift
@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend %s -sil-verify-all -verify -emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoncopyableGenerics
+// RUN: %target-swift-frontend %s -sil-verify-all -verify -emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature MoveOnlyPartialReinitialization -enable-experimental-feature NoncopyableGenerics
 
 // REQUIRES: asserts
 
diff --git a/test/SILOptimizer/moveonly_trivial_addresschecker_diagnostics.swift b/test/SILOptimizer/moveonly_trivial_addresschecker_diagnostics.swift
@@ -1,4 +1,4 @@
-// RUN: %target-swift-emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -sil-verify-all -verify %s
+// RUN: %target-swift-emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature MoveOnlyPartialReinitialization -sil-verify-all -verify %s
 
 //////////////////
 // Declarations //

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-// RUN: %target-swift-emit-sil -O -sil-verify-all -verify -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyClasses %s`
	`1`	`+// RUN: %target-swift-emit-sil -O -sil-verify-all -verify -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature MoveOnlyPartialReinitialization -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyClasses %s`
`2`	`2`
`3`	`3`	`//////////////////`
`4`	`4`	`// Declarations //`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-// RUN: %target-swift-frontend %s -sil-verify-all -verify -emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoncopyableGenerics`
	`1`	`+// RUN: %target-swift-frontend %s -sil-verify-all -verify -emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature MoveOnlyPartialReinitialization -enable-experimental-feature NoncopyableGenerics`
`2`	`2`
`3`	`3`	`// REQUIRES: asserts`
`4`	`4`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-// RUN: %target-swift-emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -sil-verify-all -verify %s`
	`1`	`+// RUN: %target-swift-emit-sil -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature MoveOnlyPartialReinitialization -sil-verify-all -verify %s`
`2`	`2`
`3`	`3`	`//////////////////`
`4`	`4`	`// Declarations //`