Merge pull request #84476 from kavon/manual-ownership/usability-fixes-1

Usability improvements for ManualOwnership (aka "explicit copies mode")

Author: kavon

include/swift/AST/DiagnosticsSIL.def

@@ -435,6 +435,12 @@ NOTE(performance_called_from,none,
       "called from here", ())
 ERROR(manualownership_copy,none,
       "explicit 'copy' required here", ())
+ERROR(manualownership_copy_happened,none,
+      "accessing %0 produces a copy of it; write 'copy' to acknowledge", (Identifier))
+ERROR(manualownership_copy_demanded,none,
+      "ownership of %0 is demanded and cannot not be consumed; write 'copy' to satisfy", (Identifier))
+ERROR(manualownership_copy_captured,none,
+      "ownership of %0 is demanded by a closure; write 'copy' in its capture list to satisfy", (Identifier))
 
 // 'transparent' diagnostics
 ERROR(circular_transparent,none,

lib/Frontend/CompilerInvocation.cpp

@@ -2949,6 +2949,17 @@ static bool ParseSILArgs(SILOptions &Opts, ArgList &Args,
                        A->getAsString(Args));
     }
   }
+  // Have ManualOwnership imply MandatoryCopyPropagation.
+  // Once that pass becomes enabled by default, we don't need this.
+  if (LangOpts.hasFeature(ManualOwnership)) {
+    specifiedCopyPropagationOption = CopyPropagationOption::Always;
+
+    if (auto *Flag = Args.getLastArg(OPT_copy_propagation_state_EQ)) {
+      Diags.diagnose(SourceLoc(), diag::error_invalid_arg_combination,
+                       Flag->getAsString(Args),
+                       "-enable-experimental-feature ManualOwnership");
+    }
+  }
   if (Args.hasArg(OPT_enable_copy_propagation)) {
     specifiedCopyPropagationOption = CopyPropagationOption::Always;
   }

lib/SILGen/SILGenExpr.cpp

@@ -7315,7 +7315,7 @@ RValue RValueEmitter::visitCopyExpr(CopyExpr *E, SGFContext C) {
     return RValue(SGF, {optTemp->getManagedAddress()}, subType.getASTType());
   }
 
-  if (subType.isLoadable(SGF.F)) {
+  if (subType.isLoadable(SGF.F) || !SGF.silConv.useLoweredAddresses()) {
     ManagedValue mv =
       SGF.emitRValue(subExpr, SGFContext::AllowImmediatePlusZero)
          .getAsSingleValue(SGF, subExpr);

lib/SILOptimizer/Mandatory/PerformanceDiagnostics.cpp

@@ -20,6 +20,7 @@
 #include "swift/SILOptimizer/Analysis/BasicCalleeAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
 #include "swift/SILOptimizer/Utils/BasicBlockOptUtils.h"
+#include "swift/SILOptimizer/Utils/VariableNameUtils.h"
 #include "llvm/Support/Debug.h"
 
 using namespace swift;
@@ -376,12 +377,16 @@ bool PerformanceDiagnostics::visitInst(SILInstruction *inst,
   LocWithParent loc(inst->getLoc().getSourceLoc(), parentLoc);
 
   if (perfConstr == PerformanceConstraints::ManualOwnership) {
-    if (impact == RuntimeEffect::RefCounting) {
+    if (impact & RuntimeEffect::RefCounting) {
       bool shouldDiagnose = false;
       switch (inst->getKind()) {
+      case SILInstructionKind::PartialApplyInst:
+      case SILInstructionKind::DestroyAddrInst:
+      case SILInstructionKind::DestroyValueInst:
+        break; // These modify reference counts, but aren't copies.
       case SILInstructionKind::ExplicitCopyAddrInst:
       case SILInstructionKind::ExplicitCopyValueInst:
-        break;
+        break; // Explicitly acknowledged copies are OK.
       case SILInstructionKind::LoadInst: {
         // FIXME: we don't have an `explicit_load` and transparent functions can
         //   end up bringing in a `load [copy]`. A better approach is needed to
@@ -418,19 +423,40 @@ bool PerformanceDiagnostics::visitInst(SILInstruction *inst,
         break;
       }
       if (shouldDiagnose) {
+        LLVM_DEBUG(llvm::dbgs()
+                   << "function " << inst->getFunction()->getName()
+                   << "\n has unexpected copying instruction: " << *inst);
+
+        // Try to come up with a useful diagnostic.
+        if (auto svi = dyn_cast<SingleValueInstruction>(inst)) {
+          if (auto name = VariableNameInferrer::inferName(svi)) {
+            // Simplistic check for whether this is a closure capture.
+            for (auto user : svi->getUsers()) {
+              if (isa<PartialApplyInst>(user)) {
+                LLVM_DEBUG(llvm::dbgs() << "captured by "<< *user);
+                diagnose(loc, diag::manualownership_copy_captured, *name);
+                return false;
+              }
+            }
+
+            // There's no hope of borrowing access if there's a consuming use.
+            for (auto op : svi->getUses()) {
+              if (op->getOperandOwnership() == OperandOwnership::ForwardingConsume) {
+                LLVM_DEBUG(llvm::dbgs() << "demanded by "<< *(op->getUser()));
+                diagnose(loc, diag::manualownership_copy_demanded, *name);
+                return false;
+              }
+            }
+
+            diagnose(loc, diag::manualownership_copy_happened, *name);
+            return false;
+          }
+        }
+
+        // Back-up diagnostic, when all-else fails.
         diagnose(loc, diag::manualownership_copy);
-        llvm::dbgs() << "function " << inst->getFunction()->getName();
-        llvm::dbgs() << "\n has unexpected copying instruction: ";
-        inst->dump();
         return false; // Don't bail-out early; diagnose more issues in the func.
       }
-    } else if (impact == RuntimeEffect::ExclusivityChecking) {
-      // TODO: diagnose only the nested exclusivity; perhaps as a warning
-      //   since we don't yet have reference bindings?
-
-      // diagnose(loc, diag::performance_arc);
-      // inst->dump();
-      // return true;
     }
     return false;
   }

test/SIL/manual_ownership.swift

@@ -1,6 +1,5 @@
 // RUN: %target-swift-frontend %s -emit-sil -verify \
-// RUN:   -enable-experimental-feature ManualOwnership \
-// RUN:   -enable-copy-propagation=always
+// RUN:   -enable-experimental-feature ManualOwnership
 
 // REQUIRES: swift_feature_ManualOwnership
 
@@ -34,6 +33,14 @@ public class Triangle {
   borrowing func borrowing() {}
 }
 
+// MARK: utilities
+
+func eat(_ t: consuming Triangle) {}
+func use(_ t: borrowing Triangle) {}
+
+func consume_generic<T>(_ t: consuming T) {}
+func borrow_generic<T>(_ t: borrowing T) {}
+
 /// MARK: return statements
 
 @_manualOwnership
@@ -44,7 +51,7 @@ public func basic_return1() -> Triangle {
 
 @_manualOwnership
 public func basic_return2(t: Triangle) -> Triangle {
-  return t // expected-error {{explicit 'copy' required here}}
+  return t // expected-error {{ownership of 't' is demanded}}
 }
 @_manualOwnership
 public func basic_return2_fixed(t: Triangle) -> Triangle {
@@ -68,8 +75,9 @@ func reassign_with_lets() -> Triangle {
 func renamed_return(_ cond: Bool, _ a: Triangle) -> Triangle {
   let b = a
   let c = b
-  if cond { return b } // expected-error {{explicit 'copy' required here}}
-  return c // expected-error {{explicit 'copy' required here}}
+  // FIXME: we say 'c' instead of 'b', because of the propagation.
+  if cond { return b } // expected-error {{ownership of 'c' is demanded}}
+  return c // expected-error {{ownership of 'c' is demanded}}
 }
 
 @_manualOwnership
@@ -101,26 +109,27 @@ func basic_methods_borrowing(_ t1: Triangle) {
 @_manualOwnership
 func basic_methods_consuming(_ t1: Triangle) {
   let t2 = Triangle()
-  t1.consuming() // expected-error {{explicit 'copy' required here}}
+  t1.consuming() // expected-error {{ownership of 't1' is demanded}}
   t2.consuming()
 }
 @_manualOwnership
 func basic_methods_consuming_fixed(_ t1: Triangle) {
   let t2 = Triangle()
-  var t3 = Triangle()
-  t3 = Triangle()
 
   (copy t1).consuming()
   (copy t2).consuming()  // FIXME: why is this not propagated?
-  (copy t3).consuming()
 }
 
-func consumingFunc(_ t0: consuming Triangle) {}
+@_manualOwnership
+@discardableResult
+func consumingFunc(_ t0: consuming Triangle) -> Bool { return false }
+
+@_manualOwnership
 func plainFunc(_ t0: Triangle) {}
 
 @_manualOwnership
 func basic_function_call(_ t1: Triangle) {
-  consumingFunc(t1) // expected-error {{explicit 'copy' required here}}
+  consumingFunc(t1) // expected-error {{ownership of 't1' is demanded}}
   consumingFunc(copy t1)
   plainFunc(t1)
 }
@@ -146,7 +155,7 @@ func basic_function_call(_ t1: Triangle) {
 @_manualOwnership
 public func basic_loop_trivial_values(_ t: Triangle, _ xs: [Triangle]) {
   var p: Pair = t.a
-  for x in xs { // expected-error {{explicit 'copy' required here}}
+  for x in xs { // expected-error {{ownership of 'xs' is demanded}}
     p = p.midpoint(x.a)
   }
   t.a = p
@@ -169,33 +178,125 @@ public func basic_loop_trivial_values_fixed(_ t: Triangle, _ xs: [Triangle]) {
 
 @_manualOwnership
 public func basic_loop_nontrivial_values(_ t: Triangle, _ xs: [Triangle]) {
-  var p: Pair = t.nontrivial.a // expected-error {{explicit 'copy' required here}}
-  for x in xs { // expected-error {{explicit 'copy' required here}}
-    p = p.midpoint(x.nontrivial.a) // expected-error {{explicit 'copy' required here}}
+  var p: Pair = t.nontrivial.a // expected-error {{accessing 't.nontrivial' produces a copy of it}}
+  for x in xs { // expected-error {{ownership of 'xs' is demanded}}
+    p = p.midpoint(x.nontrivial.a) // expected-error {{accessing 'x.nontrivial' produces a copy of it}}
   }
-  t.nontrivial.a = p // expected-error {{explicit 'copy' required here}}
+  t.nontrivial.a = p // expected-error {{accessing 't.nontrivial' produces a copy of it}}
 }
 
 // FIXME: there should be no copies required in the below, other than what's already written.
 @_manualOwnership
 public func basic_loop_nontrivial_values_fixed(_ t: Triangle, _ xs: [Triangle]) {
-  var p: Pair = (copy t.nontrivial).a  // expected-error {{explicit 'copy' required here}}
+  var p: Pair = (copy t.nontrivial).a  // expected-error {{accessing 't.nontrivial' produces a copy of it}}
   for x in copy xs {
-    p = p.midpoint((copy x.nontrivial).a) // expected-error {{explicit 'copy' required here}}
+    p = p.midpoint((copy x.nontrivial).a) // expected-error {{accessing 'x.nontrivial' produces a copy of it}}
   }
-  (copy t.nontrivial).a = p // expected-error {{explicit 'copy' required here}}
+  (copy t.nontrivial).a = p // expected-error {{accessing 't.nontrivial' produces a copy of it}}
 }
 
 
 /// MARK: Globals
 let ref_result = [5, 13, 29]
 
+// FIXME: if we had a borrow operator, we could allow people to elide these simple copies that
+//   are present to avoid exclusivity issues. We'd need to start generating read coroutines.
 @_manualOwnership
 func access_global_1() -> Int {
-  return ref_result[2] // expected-error {{explicit 'copy' required here}}
+  return ref_result[2] // expected-error {{accessing 'ref_result' produces a copy of it}}
 }
 @_manualOwnership
 func access_global_1_fixed() -> Int {
 return (copy ref_result)[2]
 }
 
+/// MARK: closures
+
+// FIXME: (1) Closure capture lists need to support the short-hand [copy t] and produce explicit copies.
+//            We also need a better error message for when this is missed for closure captures.
+//        (2) Escaping closures need to be recursively checked by the PerformanceDiagnostics.
+//            We might just need to widen the propagation of [manual_ownership]?
+//        (3) Autoclosures have no ability to annotate captures. Is that OK?
+
+@_manualOwnership
+func closure_basic(_ t: Triangle) -> () -> Triangle {
+  return { return t } // expected-error {{ownership of 't' is demanded by a closure}}
+}
+@_manualOwnership
+func closure_basic_fixed(_ t: Triangle) -> () -> Triangle {
+  return { [t = copy t] in return t }
+}
+
+@_manualOwnership
+func closure_copies_in_body(_ t: Triangle) -> () -> Triangle {
+  return { [t = copy t] in
+    eat(t) // FIXME: missing required copies
+    eat(t)
+    return t }
+}
+
+@_manualOwnership
+func closure_copies_in_body_noescape(_ t: Triangle) -> Triangle {
+  let f = { [t = copy t] in
+    eat(t)  // FIXME: missing required copies
+    eat(t)
+    return t
+  }
+  return f()
+}
+
+@_manualOwnership
+func simple_assert(_ f: @autoclosure () -> Bool) {
+  guard f() else { fatalError() }
+}
+@_manualOwnership
+func try_to_assert(_ n: Int, _ names: [String]) {
+  simple_assert(names.count == n)
+}
+
+@_manualOwnership
+func copy_in_autoclosure(_ t: Triangle) {
+  simple_assert(consumingFunc(t)) // FIXME: missing required copies
+}
+
+/// MARK: generics
+
+@_manualOwnership
+func return_generic<T>(_ t: T) -> T {
+  return t // expected-error {{explicit 'copy' required here}}
+}
+@_manualOwnership
+func return_generic_fixed<T>(_ t: T) -> T {
+  return copy t
+}
+
+@_manualOwnership
+func reassign_with_lets<T>(_ t: T) -> T {
+  let x = t // expected-error {{explicit 'copy' required here}}
+  let y = x // expected-error {{explicit 'copy' required here}}
+  let z = y // expected-error {{explicit 'copy' required here}}
+  return copy z
+}
+
+// FIXME: there's copy propagation has no effect on address-only types.
+@_manualOwnership
+func reassign_with_lets_fixed<T>(_ t: T) -> T {
+  let x = copy t
+  let y = copy x
+  let z = copy y
+  return copy z
+}
+
+@_manualOwnership
+func copy_generic<T>(_ t: T)  {
+  consume_generic(t) // expected-error {{explicit 'copy' required here}}
+  borrow_generic(t)
+  consume_generic(t) // expected-error {{explicit 'copy' required here}}
+}
+
+@_manualOwnership
+func copy_generic_fixed<T>(_ t: T)  {
+  consume_generic(copy t)
+  borrow_generic(t)
+  consume_generic(copy t)
+}

test/SILGen/opaque_values_silgen.swift

@@ -885,3 +885,14 @@ struct Twople<T> {
 func throwTypedValue(_ e: Err) throws(Err) { throw e }
 
 struct Err : Error {}
+
+// CHECK-LABEL: sil{{.*}} [ossa] @copy_expr_generic : {{.*}} {
+// CHECK:       bb0([[E:%[^,]+]] : @guaranteed $T
+// CHECK:         [[E_COPY:%[^,]+]] = explicit_copy_value [[E]]
+// CHECK:         apply {{.*}}<T>([[E_COPY]])
+// CHECK-LABEL: } // end sil function 'copy_expr_generic'
+@_silgen_name("copy_expr_generic")
+func copy_expr_generic<T>(_ t: T)  {
+  eat_generic(copy t)
+}
+func eat_generic<T>(_ t: consuming T) {}