Merge pull request #66445 from jckarter/noncopyable-computed-property-address-only-base

SILGen: Don't copy a borrowed noncopyable address-only base of a computed property access.

Author: jckarter

lib/SILGen/SILGenApply.cpp

@@ -6455,7 +6455,12 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorAddressBaseArg() {
   // If the base is currently an address, we may have to copy it.
   if (shouldLoadBaseAddress()) {
     if (selfParam.isConsumed() ||
-        base.getType().isAddressOnly(SGF.F)) {
+        (base.getType().isAddressOnly(SGF.F)
+         // If a move-only base is borrowed, then we have to try our best to
+         // borrow it in-place without copying.
+         // TODO: Can we avoid copying a non-move-only value too in this
+         // circumstance?
+         && !base.getType().isMoveOnly())) {
       // The load can only be a take if the base is a +1 rvalue.
       auto shouldTake = IsTake_t(base.hasCleanup());
 
@@ -6464,6 +6469,11 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorAddressBaseArg() {
                                       SGFContext(), shouldTake);
       return ArgumentSource(loc, RValue(SGF, loc, baseFormalType, base));
     }
+    
+    // If the type is address-only, we can borrow the memory location as is.
+    if (base.getType().isAddressOnly(SGF.F)) {
+      return ArgumentSource(loc, RValue(SGF, loc, baseFormalType, base));
+    }
 
     // If we do not have a consumed base and need to perform a load, perform a
     // formal access load borrow.

lib/SILGen/SILGenLValue.cpp

@@ -1171,6 +1171,11 @@ namespace {
       assert(base
              && base.getType().isAddress()
              && "should have an address base to borrow from");
+      // If the base value is address-only then we can borrow from the
+      // address in-place.
+      if (!base.getType().isLoadable(SGF.F)) {
+        return base;
+      }
       auto result = SGF.B.createLoadBorrow(loc, base.getValue());
       return SGF.emitFormalEvaluationManagedBorrowedRValueWithCleanup(loc,
          base.getValue(), result);

test/ModuleInterface/moveonly_user.swift

@@ -5,17 +5,13 @@
 // RUN: %target-swift-frontend -emit-sil -sil-verify-all -I %t %s > /dev/null
 
 // >> now again with library evolution; we expect the same result.
-// FIXME: move checker doesn't like it when you specify library evolution
 // RUN: %target-swift-frontend -DSYNTHESIZE_ACCESSORS -enable-library-evolution -emit-module -o %t/Hello.swiftmodule %S/Inputs/moveonly_api.swift
 // RUN: %target-swift-frontend -emit-sil -sil-verify-all -I %t %s > /dev/null
 
 // FIXME: ideally this would also try executing the program rather than just generating SIL
 
 // FIXME: make this test work when we're not synthesizing the accessors
 
-// rdar://106164128
-// XFAIL: *
-
 import Hello
 
 func simpleTest() {

test/SILGen/moveonly_addressonly_computed_property.swift

@@ -0,0 +1,80 @@
+// RUN: %target-swift-emit-silgen %s | %FileCheck %s
+// RUN: %target-swift-frontend -emit-sil -verify %s
+
+// rdar://109161396
+
+public protocol P {}
+
+@_moveOnly
+public struct M {
+   private var x: P
+   var other: CInt { 0 }
+
+   var otherMoveOnly: M {
+       _read {
+           yield self
+       }
+   }
+
+   @_silgen_name("no")
+   init()
+}
+
+// CHECK-LABEL: sil [ossa] @${{.*}}4test3mut
+// CHECK: [[CHECK:%.*]] = mark_must_check [consumable_and_assignable] %0
+// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[CHECK]]
+// CHECK: [[RESULT:%.*]] = apply {{.*}}([[ACCESS]])
+// CHECK; end_access [[ACCESS]]
+// CHECK: return [[RESULT]]
+public func test(mut: inout M) -> CInt {
+  return mut.other
+}
+
+// CHECK-LABEL: sil [ossa] @${{.*}}4test6borrow
+// CHECK: [[CHECK:%.*]] = mark_must_check [no_consume_or_assign] %0
+// CHECK: [[RESULT:%.*]] = apply {{.*}}([[CHECK]])
+// CHECK: return [[RESULT]]
+public func test(borrow: borrowing M) -> CInt {
+  return borrow.other
+}
+
+// CHECK-LABEL: sil [ossa] @${{.*}}4test7consume
+// CHECK: [[BOX:%.*]] = project_box
+// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[BOX]]
+// CHECK: [[CHECK:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]]
+// CHECK: [[RESULT:%.*]] = apply {{.*}}([[CHECK]])
+// CHECK; end_access [[ACCESS]]
+// CHECK: return [[RESULT]]
+public func test(consume: consuming M) -> CInt {
+  return consume.other
+}
+
+// CHECK-LABEL: sil [ossa] @${{.*}}4test3own
+// CHECK: [[CHECK:%.*]] = mark_must_check [consumable_and_assignable] %0
+// CHECK: [[RESULT:%.*]] = apply {{.*}}([[CHECK]])
+// CHECK: return [[RESULT]]
+public func test(own: __owned M) -> CInt {
+  return own.other
+}
+
+func use(_: CInt, andMutate _: inout M) {}
+func use(_: CInt, andConsume _: consuming M) {}
+func borrow(_: borrowing M, andMutate _: inout M) {}
+func borrow(_: borrowing M, andConsume _: consuming M) {}
+
+public func testNoInterferenceGet(mut: inout M, extra: consuming M) {
+    // This should not cause exclusivity interference, since the result of
+    // the getter can have an independent lifetime from the borrow.
+    use(mut.other, andMutate: &mut)
+    use(mut.other, andConsume: mut)
+    mut = extra
+}
+
+public func testInterferenceRead(mut: inout M, extra: consuming M) {
+    // This should cause exclusivity interference, since in order to borrow
+    // the yielded result from the `_read`, we need to keep the borrow of
+    // the base going.
+    borrow(mut.otherMoveOnly, andMutate: &mut) // expected-error{{}} expected-note{{}}
+    borrow(mut.otherMoveOnly, andConsume: mut) // expected-error{{}} expected-note{{}}
+    mut = extra
+}