SILGen: Treat Optional x! force unwrapping as a forwarding operation.

Like `?` or property access, `x!` can be borrowing, consuming, or mutated
through depending on the use site and the ownership of the base value.
Alter SILGen to emit `x!` as a borrowing operation when the result is only
used as a borrow. Fix the move-only checker not to treat the unreachable
branch as a dead path for values and try to destroy the value unnecessarily
and possibly out-of-order with cleanups on the value. Fixes rdar://127459955.

Author: jckarter

lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp

@@ -1137,9 +1137,18 @@ void FieldSensitivePrunedLiveRange<LivenessWithDefs>::computeBoundary(
         for (SILBasicBlock *succBB : block->getSuccessors()) {
           if (FieldSensitivePrunedLiveBlocks::isDead(
                 getBlockLiveness(succBB, index))) {
-            PRUNED_LIVENESS_LOG(llvm::dbgs() << "Marking succBB as boundary edge: bb"
-                                    << succBB->getDebugID() << '\n');
-            boundary.getBoundaryEdgeBits(succBB).set(index);
+            // If the basic block ends in unreachable, don't consider it a
+            // boundary.
+            // TODO: Should also do this if the block's successors all always
+            // end in unreachable too.
+            if (isa<UnreachableInst>(succBB->getTerminator())) {
+              PRUNED_LIVENESS_LOG(llvm::dbgs() << "succBB ends in unreachable, skipping as boundary edge: bb"
+                                      << succBB->getDebugID() << '\n');
+            } else {
+              PRUNED_LIVENESS_LOG(llvm::dbgs() << "Marking succBB as boundary edge: bb"
+                                      << succBB->getDebugID() << '\n');
+              boundary.getBoundaryEdgeBits(succBB).set(index);
+            }
           }
         }
         asImpl().findBoundariesInBlock(block, index, /*isLiveOut*/ true,

lib/SILGen/SILGenApply.cpp

@@ -3223,41 +3223,54 @@ static StorageRefResult findStorageReferenceExprForBorrow(Expr *e) {
 
 Expr *SILGenFunction::findStorageReferenceExprForMoveOnly(Expr *argExpr,
                                            StorageReferenceOperationKind kind) {
+  ForceValueExpr *forceUnwrap = nullptr;
+  // Check for a force unwrap. This might show up inside or outside of the
+  // load.
+  if (auto *fu = dyn_cast<ForceValueExpr>(argExpr)) {
+    forceUnwrap = fu;
+    argExpr = fu->getSubExpr();
+  }
+  
   // If there's a load around the outer part of this arg expr, look past it.
   bool sawLoad = false;
   if (auto *li = dyn_cast<LoadExpr>(argExpr)) {
     argExpr = li->getSubExpr();
     sawLoad = true;
   }
 
+  // Check again for a force unwrap before the load.
+  if (auto *fu = dyn_cast<ForceValueExpr>(argExpr)) {
+    forceUnwrap = fu;
+    argExpr = fu->getSubExpr();
+  }
+
   // If we're consuming instead, then the load _must_ have been there.
   if (kind == StorageReferenceOperationKind::Consume && !sawLoad)
     return nullptr;
 
-  // If we did not see a load and our argExpr is a
-  // declref_expr, return nullptr. We have an object not something that will be
-  // in memory. This can happen with classes or with values captured by a
-  // closure.
-  //
-  // NOTE: If we see a member_ref_expr from a decl_ref_expr, we still process it
-  // since the declref_expr could be from a class.
+  // TODO: This section should be removed eventually. Decl refs should not be
+  // handled different from other storage. Removing it breaks some things
+  // currently.
   if (!sawLoad) {
     if (auto *declRef = dyn_cast<DeclRefExpr>(argExpr)) {
       assert(!declRef->getType()->is<LValueType>() &&
              "Shouldn't ever have an lvalue type here!");
-      
-      // Proceed if the storage references a global or static let.
-      // TODO: We should treat any storage reference as a borrow, it seems, but
-      // that currently disrupts what the move checker expects. It would also
-      // be valuable to borrow copyable global lets, but this is a targeted
-      // fix to allow noncopyable globals to work properly.
-      bool isGlobal = false;
-      if (auto vd = dyn_cast<VarDecl>(declRef->getDecl())) {
-        isGlobal = vd->isGlobalStorage();
-      }
-      
-      if (!isGlobal) {
-        return nullptr;
+             
+      // Proceed if the storage reference is a force unwrap.
+      if (!forceUnwrap) {
+        // Proceed if the storage references a global or static let.
+        // TODO: We should treat any storage reference as a borrow, it seems, but
+        // that currently disrupts what the move checker expects. It would also
+        // be valuable to borrow copyable global lets, but this is a targeted
+        // fix to allow noncopyable globals to work properly.
+        bool isGlobal = false;
+        if (auto vd = dyn_cast<VarDecl>(declRef->getDecl())) {
+          isGlobal = vd->isGlobalStorage();
+        }
+        
+        if (!isGlobal) {
+          return nullptr;
+        }
       }
     }
   }
@@ -3302,6 +3315,10 @@ Expr *SILGenFunction::findStorageReferenceExprForMoveOnly(Expr *argExpr,
   }
   if (!isMoveOnly)
     return nullptr;
+    
+  if (forceUnwrap) {
+    return forceUnwrap;
+  }
 
   return result.getTransitiveRoot();
 }

lib/SILGen/SILGenConvert.cpp

@@ -199,16 +199,21 @@ SILGenFunction::emitPreconditionOptionalHasValue(SILLocation loc,
   auto someDecl = getASTContext().getOptionalSomeDecl();
   auto noneDecl = getASTContext().getOptionalNoneDecl();
 
-  // If we have an object, make sure the object is at +1. All switch_enum of
-  // objects is done at +1.
   bool isAddress = optional.getType().isAddress();
+  bool isBorrow = !optional.isPlusOneOrTrivial(*this);
   SwitchEnumInst *switchEnum = nullptr;
   if (isAddress) {
     // We forward in the creation routine for
     // unchecked_take_enum_data_addr. switch_enum_addr is a +0 operation.
     B.createSwitchEnumAddr(loc, optional.getValue(),
                            /*defaultDest*/ nullptr,
                            {{someDecl, contBB}, {noneDecl, failBB}});
+  } else if (isBorrow) {
+    hadCleanup = false;
+    hadLValue = false;
+    switchEnum = B.createSwitchEnum(loc, optional.getValue(),
+                                    /*defaultDest*/ nullptr,
+                                    {{someDecl, contBB}, {noneDecl, failBB}});
   } else {
     optional = optional.ensurePlusOne(*this, loc);
     hadCleanup = true;

lib/SILGen/SILGenLValue.cpp

@@ -948,6 +948,9 @@ namespace {
                          ManagedValue base) && override {
       // Assert that the optional value is present and return the projected out
       // payload.
+      if (isConsumeAccess(getTypeData().getAccessKind())) {
+        base = base.ensurePlusOne(SGF, loc);
+      }
       return SGF.emitPreconditionOptionalHasValue(loc, base, isImplicitUnwrap);
     }
 
@@ -4355,6 +4358,16 @@ getOptionalObjectTypeData(SILGenFunction &SGF, SGFAccessKind accessKind,
 LValue SILGenLValue::visitForceValueExpr(ForceValueExpr *e,
                                          SGFAccessKind accessKind,
                                          LValueOptions options) {
+  // Since Sema doesn't reason about borrows, a borrowed force expr
+  // might end up type checked with the load inside of the force.
+  auto subExpr = e->getSubExpr();
+  if (auto load = dyn_cast<LoadExpr>(subExpr)) {
+    assert((isBorrowAccess(accessKind) || isConsumeAccess(accessKind))
+           && "should only see a (force_value (load)) lvalue as part of a "
+              "borrow or consume");
+    subExpr = load->getSubExpr();
+  }
+                                         
   // Like BindOptional, this is a read even if we only write to the result.
   // (But it's unnecessary to use a force this way!)
   LValue lv = visitRec(e->getSubExpr(),

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -1713,9 +1713,6 @@ struct CopiedLoadBorrowEliminationVisitor
             useWorklist.push_back(use);
         }
 
-        // If we have a switch_enum, we always need to convert it to a load
-        // [copy] since we need to destructure through it.
-        shouldConvertToLoadCopy |= isa<SwitchEnumInst>(nextUse->getUser());
         continue;
       }
       case OperandOwnership::Borrow:

test/SILGen/vtable_thunks.swift

@@ -145,18 +145,16 @@ class H: G {
 // CHECK-LABEL: sil private [thunk] [ossa] @$s13vtable_thunks1DC3iuo{{[_0-9a-zA-Z]*}}FTV
 // CHECK: bb0([[X:%.*]] : @guaranteed $B, [[Y:%.*]] : @guaranteed $Optional<B>, [[Z:%.*]] : @guaranteed $B, [[W:%.*]] : @guaranteed $D):
 // CHECK:   [[WRAP_X:%.*]] = enum $Optional<B>, #Optional.some!enumelt, [[X]] : $B
-// CHECK:   [[Y_COPY:%.*]] = copy_value [[Y]]
-// CHECK:   switch_enum [[Y_COPY]] : $Optional<B>, case #Optional.some!enumelt: [[SOME_BB:bb[0-9]+]], case #Optional.none!enumelt: [[NONE_BB:bb[0-9]+]]
+// CHECK:   switch_enum [[Y]] : $Optional<B>, case #Optional.some!enumelt: [[SOME_BB:bb[0-9]+]], case #Optional.none!enumelt: [[NONE_BB:bb[0-9]+]]
 
 // CHECK: [[NONE_BB]]:
 // CHECK:   [[DIAGNOSE_UNREACHABLE_FUNC:%.*]] = function_ref @$ss30_diagnoseUnexpectedNilOptional{{.*}}
 // CHECK:   apply [[DIAGNOSE_UNREACHABLE_FUNC]]
 // CHECK:   unreachable
 
-// CHECK: [[SOME_BB]]([[UNWRAP_Y:%.*]] : @owned $B):
-// CHECK:   [[BORROWED_UNWRAP_Y:%.*]] = begin_borrow [[UNWRAP_Y]]
+// CHECK: [[SOME_BB]]([[UNWRAP_Y:%.*]] : @guaranteed $B):
 // CHECK:   [[THUNK_FUNC:%.*]] = function_ref @$s13vtable_thunks1DC3iuo{{.*}}
-// CHECK:   [[RES:%.*]] = apply [[THUNK_FUNC]]([[WRAP_X]], [[BORROWED_UNWRAP_Y]], [[Z]], [[W]])
+// CHECK:   [[RES:%.*]] = apply [[THUNK_FUNC]]([[WRAP_X]], [[UNWRAP_Y]], [[Z]], [[W]])
 // CHECK:   [[WRAP_RES:%.*]] = enum $Optional<B>, {{.*}} [[RES]]
 // CHECK:   return [[WRAP_RES]]
 

test/SILOptimizer/discard_checking.swift

@@ -16,15 +16,15 @@ func globalThrowingFn() throws {}
 struct Basics: ~Copyable {
   consuming func test1(_ b: Bool) {
     guard b else {
-      fatalError("bah!") // expected-error {{must consume 'self' before exiting method that discards self}}
+      return // expected-error {{must consume 'self' before exiting method that discards self}}
     }
     discard self // expected-note {{discarded self here}}
   }
 
   consuming func test1_fixed(_ b: Bool) {
     guard b else {
       _ = consume self
-      fatalError("bah!")
+      return
     }
     discard self
   }
@@ -33,7 +33,7 @@ struct Basics: ~Copyable {
     repeat {
       switch c {
       case .red:
-        fatalError("bah!")
+        return
       case .blue:
         throw E.someError
       case .green:
@@ -49,7 +49,7 @@ struct Basics: ~Copyable {
       switch c {
       case .red:
         discard self
-        fatalError("bah!")
+        return
       case .blue:
         discard self
         throw E.someError
@@ -145,7 +145,7 @@ struct Basics: ~Copyable {
     if case .red = c {
       discard self // expected-note {{discarded self here}}
     }
-    fatalError("oh no") // expected-error {{must consume 'self' before exiting method that discards self}}
+    return // expected-error {{must consume 'self' before exiting method that discards self}}
   }
 
   consuming func test7_fixed(_ c: Color) throws {
@@ -154,17 +154,17 @@ struct Basics: ~Copyable {
       return
     }
     _ = consume self
-    fatalError("oh no")
+    return
   }
 
   consuming func test8(_ c: Color) throws {
     if case .red = c {
       discard self // expected-note {{discarded self here}}
     }
     if case .blue = c {
-      fatalError("hi") // expected-error {{must consume 'self' before exiting method that discards self}}
+      return
     }
-  }
+  } // expected-error {{must consume 'self' before exiting method that discards self}}
 
   consuming func test8_stillMissingAConsume1(_ c: Color) throws {
     if case .red = c {
@@ -173,7 +173,7 @@ struct Basics: ~Copyable {
     }
     if case .blue = c {
       _ = consume self
-      fatalError("hi")
+      return
     }
   } // expected-error {{must consume 'self' before exiting method that discards self}}
 
@@ -183,7 +183,7 @@ struct Basics: ~Copyable {
       return
     }
     if case .blue = c {
-      fatalError("hi") // expected-error {{must consume 'self' before exiting method that discards self}}
+      return // expected-error {{must consume 'self' before exiting method that discards self}}
     }
     _ = consume self
   }
@@ -195,7 +195,7 @@ struct Basics: ~Copyable {
     }
     if case .blue = c {
       _ = consume self
-      fatalError("hi")
+      return
     }
     _ = consume self
   }
@@ -407,7 +407,7 @@ struct Basics: ~Copyable {
     case 2:
       return // expected-error {{must consume 'self' before exiting method that discards self}}
     case 3:
-      fatalError("no") // expected-error {{must consume 'self' before exiting method that discards self}}
+      return // expected-error {{must consume 'self' before exiting method that discards self}}
     case 4:
       globalConsumingFn(self)
     default:
@@ -568,7 +568,7 @@ struct Money: ~Copyable {
 
   consuming func spend(_ charge: Int) throws -> Money {
     guard charge > 0 else {
-      fatalError("can't charge a negative amount!") // expected-error {{must consume 'self' before exiting method that discards self}}
+      return Money(balance: balance) // expected-error {{must consume 'self' before exiting method that discards self}}
     }
 
     if balance < charge  {