[consume-operator] Change consume so that its result is not considered an lvalue.

gottesmm · gottesmm · commit 67fcb1c86e54 · 2023-05-17T22:42:42.000-07:00
Before this change it was possible to:

1. Call mutating methods on a consume result.
2. assign into a consume (e.x.: var x = ...; (consume x) = value.

From an implementation perspective, this involved just taking the logic I
already used for the CopyExpr and reusing it for ConsumeExpr with some small
tweaks.

rdar://109479440
diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h
@@ -2052,17 +2052,14 @@ class AwaitExpr final : public IdentityExpr {
 
 /// ConsumeExpr - A 'consume' surrounding an lvalue expression marking the
 /// lvalue as needing to be moved.
-///
-/// getSemanticsProvidingExpr() looks through this because it doesn't
-/// provide the value and only very specific clients care where the
-/// 'move' was written.
-class ConsumeExpr final : public IdentityExpr {
+class ConsumeExpr final : public Expr {
+  Expr *SubExpr;
   SourceLoc ConsumeLoc;
 
 public:
   ConsumeExpr(SourceLoc consumeLoc, Expr *sub, Type type = Type(),
               bool implicit = false)
-      : IdentityExpr(ExprKind::Consume, sub, type, implicit),
+      : Expr(ExprKind::Consume, implicit, type), SubExpr(sub),
         ConsumeLoc(consumeLoc) {}
 
   static ConsumeExpr *createImplicit(ASTContext &ctx, SourceLoc moveLoc,
@@ -2072,7 +2069,10 @@ class ConsumeExpr final : public IdentityExpr {
 
   SourceLoc getLoc() const { return ConsumeLoc; }
 
-  SourceLoc getStartLoc() const { return ConsumeLoc; }
+  Expr *getSubExpr() const { return SubExpr; }
+  void setSubExpr(Expr *E) { SubExpr = E; }
+
+  SourceLoc getStartLoc() const { return getLoc(); }
   SourceLoc getEndLoc() const { return getSubExpr()->getEndLoc(); }
 
   static bool classof(const Expr *e) {
diff --git a/include/swift/AST/ExprNodes.def b/include/swift/AST/ExprNodes.def
@@ -107,11 +107,11 @@ ABSTRACT_EXPR(Identity, Expr)
   EXPR(Paren, IdentityExpr)
   EXPR(DotSelf, IdentityExpr)
   EXPR(Await, IdentityExpr)
-  EXPR(Consume, IdentityExpr)
   EXPR(Borrow, IdentityExpr)
   EXPR(UnresolvedMemberChainResult, IdentityExpr)
   EXPR_RANGE(Identity, Paren, UnresolvedMemberChainResult)
 EXPR(Copy, Expr)
+EXPR(Consume, Expr)
 ABSTRACT_EXPR(AnyTry, Expr)
   EXPR(Try, AnyTryExpr)
   EXPR(ForceTry, AnyTryExpr)
diff --git a/lib/AST/ASTWalker.cpp b/lib/AST/ASTWalker.cpp
@@ -699,6 +699,14 @@ class Traversal : public ASTVisitor<Traversal, Expr*, Stmt*,
     return nullptr;
   }
 
+  Expr *visitConsumeExpr(ConsumeExpr *E) {
+    if (Expr *subExpr = doIt(E->getSubExpr())) {
+      E->setSubExpr(subExpr);
+      return E;
+    }
+    return nullptr;
+  }
+
   Expr *visitTupleExpr(TupleExpr *E) {
     for (unsigned i = 0, e = E->getNumElements(); i != e; ++i)
       if (E->getElement(i)) {
diff --git a/lib/SILGen/SILGenExpr.cpp b/lib/SILGen/SILGenExpr.cpp
@@ -6116,27 +6116,47 @@ RValue RValueEmitter::visitErrorExpr(ErrorExpr *E, SGFContext C) {
 }
 
 RValue RValueEmitter::visitConsumeExpr(ConsumeExpr *E, SGFContext C) {
-  auto *subExpr = cast<DeclRefExpr>(E->getSubExpr());
+  auto *subExpr = E->getSubExpr();
   auto subASTType = subExpr->getType()->getCanonicalType();
-
   auto subType = SGF.getLoweredType(subASTType);
 
+  if (auto *li = dyn_cast<LoadExpr>(subExpr)) {
+    FormalEvaluationScope writeback(SGF);
+    LValue lv =
+      SGF.emitLValue(li->getSubExpr(), SGFAccessKind::ReadWrite);
+    auto address = SGF.emitAddressOfLValue(subExpr, std::move(lv));
+    auto optTemp = SGF.emitTemporary(E, SGF.getTypeLowering(subType));
+    SILValue toAddr = optTemp->getAddressForInPlaceInitialization(SGF, E);
+    if (address.getType().isMoveOnly()) {
+      SGF.B.createCopyAddr(subExpr, address.getLValueAddress(), toAddr, IsNotTake,
+                           IsInitialization);
+    } else {
+      SGF.B.createMarkUnresolvedMoveAddr(subExpr, address.getLValueAddress(), toAddr);
+    }
+    optTemp->finishInitialization(SGF);
+
+    if (subType.isLoadable(SGF.F)) {
+      ManagedValue value = SGF.B.createLoadTake(E, optTemp->getManagedAddress());
+      if (value.getType().isTrivial(SGF.F))
+        return RValue(SGF, {value}, subType.getASTType());
+      return RValue(SGF, {value}, subType.getASTType());
+    }
+
+    return RValue(SGF, {optTemp->getManagedAddress()}, subType.getASTType());
+  }
+
   if (subType.isLoadable(SGF.F)) {
-    auto mv = SGF.emitRValue(subExpr).getAsSingleValue(SGF, subExpr);
+    ManagedValue mv = SGF.emitRValue(subExpr).getAsSingleValue(SGF, subExpr);
     if (mv.getType().isTrivial(SGF.F))
       return RValue(SGF, {mv}, subType.getASTType());
     mv = SGF.B.createMoveValue(E, mv);
-    auto *movedValue = cast<MoveValueInst>(mv.getValue());
-    movedValue->setAllowsDiagnostics(true /*set allows diagnostics*/);
+    // Set the flag so we check this.
+    cast<MoveValueInst>(mv.getValue())->setAllowsDiagnostics(true);
     return RValue(SGF, {mv}, subType.getASTType());
   }
 
   // If we aren't loadable, then create a temporary initialization and
-  // mark_unresolved_move into that if we have a copyable type if we have a move
-  // only, just add a copy_addr init.
-  //
-  // The reason why we do this is that we only use mark_unresolved_move_addr and
-  // the move operator checker for copyable values.
+  // explicit_copy_addr into that.
   std::unique_ptr<TemporaryInitialization> optTemp;
   optTemp = SGF.emitTemporary(E, SGF.getTypeLowering(subType));
   SILValue toAddr = optTemp->getAddressForInPlaceInitialization(SGF, E);
@@ -6163,7 +6183,6 @@ RValue RValueEmitter::visitCopyExpr(CopyExpr *E, SGFContext C) {
   auto subType = SGF.getLoweredType(subASTType);
 
   if (auto *li = dyn_cast<LoadExpr>(subExpr)) {
-
     FormalEvaluationScope writeback(SGF);
     LValue lv =
         SGF.emitLValue(li->getSubExpr(), SGFAccessKind::BorrowedAddressRead);
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -3546,6 +3546,27 @@ namespace {
       return expr;
     }
 
+    Expr *visitConsumeExpr(ConsumeExpr *expr) {
+      auto toType = simplifyType(cs.getType(expr));
+      cs.setType(expr, toType);
+
+      auto *subExpr = expr->getSubExpr();
+      auto type = simplifyType(cs.getType(subExpr));
+
+      // Let's load the value associated with this consume.
+      if (type->hasLValueType()) {
+        subExpr = coerceToType(subExpr, type->getRValueType(),
+                               cs.getConstraintLocator(subExpr));
+
+        if (!subExpr)
+          return nullptr;
+      }
+
+      expr->setSubExpr(subExpr);
+
+      return expr;
+    }
+
     Expr *visitAnyTryExpr(AnyTryExpr *expr) {
       auto *subExpr = expr->getSubExpr();
       auto type = simplifyType(cs.getType(subExpr));
diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp
@@ -1899,6 +1899,16 @@ namespace {
       return valueTy;
     }
 
+    Type visitConsumeExpr(ConsumeExpr *expr) {
+      auto valueTy = CS.createTypeVariable(CS.getConstraintLocator(expr),
+                                           TVO_PrefersSubtypeBinding |
+                                               TVO_CanBindToNoEscape);
+      CS.addConstraint(ConstraintKind::Equal, valueTy,
+                       CS.getType(expr->getSubExpr()),
+                       CS.getConstraintLocator(expr));
+      return valueTy;
+    }
+
     Type visitAnyTryExpr(AnyTryExpr *expr) {
       return CS.getType(expr->getSubExpr());
     }
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -422,7 +422,10 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
     }
 
     void checkConsumeExpr(ConsumeExpr *consumeExpr) {
-      if (!isa<DeclRefExpr>(consumeExpr->getSubExpr())) {
+      auto *subExpr = consumeExpr->getSubExpr();
+      if (auto *li = dyn_cast<LoadExpr>(subExpr))
+        subExpr = li->getSubExpr();
+      if (!isa<DeclRefExpr>(subExpr)) {
         Ctx.Diags.diagnose(consumeExpr->getLoc(),
                            diag::consume_expression_not_passed_lvalue);
       }
diff --git a/test/SILGen/consume_operator.swift b/test/SILGen/consume_operator.swift
@@ -0,0 +1,73 @@
+// RUN: %target-swift-emit-silgen -enable-copy-propagation=requested-passes-only -module-name consume %s -disable-objc-attr-requires-foundation-module | %FileCheck %s
+
+class Klass {}
+
+protocol P {
+  static var value: Self { get }
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s7consume15testLoadableLetyyF : $@convention(thin) () -> () {
+// CHECK: [[ORIG_VALUE:%.*]] = apply {{%.*}}({{%.*}}) : $@convention(method) (@thick Klass.Type) -> @owned Klass
+// CHECK: [[BORROWED_VALUE:%.*]] = begin_borrow [lexical] [[ORIG_VALUE]]
+// CHECK: [[COPY:%.*]] = copy_value [[BORROWED_VALUE:%.*]]
+// CHECK: move_value [allows_diagnostics] [[COPY]]
+// CHECK: } // end sil function '$s7consume15testLoadableLetyyF'
+func testLoadableLet() {
+  let k = Klass()
+  let _ = consume k
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s7consume15testLoadableVaryyF : $@convention(thin) () -> () {
+// CHECK: [[BOX:%.*]] = alloc_box $
+// CHECK: [[BORROW_BOX:%.*]] = begin_borrow [lexical] [[BOX]]
+// CHECK: [[PROJECT:%.*]] = project_box [[BORROW_BOX]]
+//
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [unknown] [[PROJECT]]
+// CHECK: assign {{%.*}} to [[ACCESS]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [unknown] [[PROJECT]]
+// CHECK: [[STACK:%.*]] = alloc_stack $Klass
+// CHECK: mark_unresolved_move_addr [[ACCESS]] to [[STACK]]
+// CHECK: [[VALUE:%.*]] = load [take] [[STACK]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: } // end sil function '$s7consume15testLoadableVaryyF'
+func testLoadableVar() {
+  var k = Klass()
+  k = Klass()
+  let _ = consume k
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s7consume18testAddressOnlyLetyyxmAA1PRzlF : $@convention(thin) <T where T : P> (@thick T.Type) -> () {
+// CHECK: [[BOX:%.*]] = alloc_stack [lexical] $T
+//
+// CHECK: [[STACK:%.*]] = alloc_stack $T
+// CHECK: mark_unresolved_move_addr [[BOX]] to [[STACK]]
+//
+// CHECK: } // end sil function '$s7consume18testAddressOnlyLetyyxmAA1PRzlF'
+func testAddressOnlyLet<T : P>(_ t: T.Type) {
+  let k = T.value
+  let _ = consume k
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s7consume18testAddressOnlyVaryyxmAA1PRzlF : $@convention(thin) <T where T : P> (@thick T.Type) -> () {
+// CHECK: [[BOX:%.*]] = alloc_box $
+// CHECK: [[BORROW_BOX:%.*]] = begin_borrow [lexical] [[BOX]]
+// CHECK: [[PROJECT:%.*]] = project_box [[BORROW_BOX]]
+//
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [unknown] [[PROJECT]]
+// CHECK: copy_addr [take] {{%.*}} to [[ACCESS]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: [[ACCESS:%.*]] = begin_access [modify] [unknown] [[PROJECT]]
+// CHECK: [[STACK:%.*]] = alloc_stack $
+// CHECK: mark_unresolved_move_addr [[ACCESS]] to [[STACK]]
+// CHECK: end_access [[ACCESS]]
+//
+// CHECK: } // end sil function '$s7consume18testAddressOnlyVaryyxmAA1PRzlF'
+func testAddressOnlyVar<T : P>(_ t: T.Type) {
+  var k = T.value
+  k = T.value
+  let _ = consume k
+}
diff --git a/test/Sema/move_expr.swift b/test/Sema/move_expr.swift
@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -disable-availability-checking
+// RUN: %target-typecheck-verify-swift -disable-availability-checking -enable-experimental-feature NoImplicitCopy
 
 class Klass {
     var k: Klass? = nil
@@ -63,3 +63,23 @@ func testVarClassAccessField() {
     t = Klass()
     let _ = consume t.k // expected-error {{'consume' can only be applied to lvalues}}
 }
+
+func testConsumeResultImmutable() {
+  class Klass {}
+
+  struct Test {
+    var k = Klass()
+    mutating func mutatingTest() {}
+    func borrowingTest() {}
+    consuming func consumingTest() {}
+  }
+
+  var t = Test()
+  t.mutatingTest()
+  consume t.borrowingTest() // expected-error {{'consume' can only be applied to lvalues}}
+  (consume t).borrowingTest()
+  (consume t).consumingTest()
+  (consume t).mutatingTest() // expected-error {{cannot use mutating member on immutable value of type 'Test'}}
+  (consume t) = Test() // expected-error {{cannot assign to immutable expression of type 'Test'}}
+  consume t = Test() // expected-error {{cannot assign to immutable expression of type 'Test'}}
+}

Original file line number	Diff line number	Diff line change
`@@ -422,7 +422,10 @@ static void diagSyntacticUseRestrictions(const Expr E, const DeclContext DC,`
`422`	`422`	`}`
`423`	`423`
`424`	`424`	`void checkConsumeExpr(ConsumeExpr *consumeExpr) {`
`425`		`- if (!isa<DeclRefExpr>(consumeExpr->getSubExpr())) {`
	`425`	`+ auto *subExpr = consumeExpr->getSubExpr();`
	`426`	`+ if (auto *li = dyn_cast<LoadExpr>(subExpr))`
	`427`	`+ subExpr = li->getSubExpr();`
	`428`	`+ if (!isa<DeclRefExpr>(subExpr)) {`
`426`	`429`	`Ctx.Diags.diagnose(consumeExpr->getLoc(),`
`427`	`430`	`diag::consume_expression_not_passed_lvalue);`
`428`	`431`	`}`