Merge pull request #65743 from kavon/5.9-forget-usage-fixes

[5.9🍒] `_forget` usage fixes to match SE-390

Author: kavon

include/swift/AST/DiagnosticsSIL.def

@@ -735,6 +735,18 @@ ERROR(noimplicitcopy_used_on_generic_or_existential, none,
       "@_noImplicitCopy can not be used on a generic or existential typed "
       "binding or a nominal type containing such typed things", ())
 
+// forget statement
+ERROR(forget_nontrivial_storage,none,
+      "can only 'forget' type %0 if it contains trivially-destroyed "
+      "stored properties at this time",
+      (Type))
+NOTE(forget_nontrivial_storage_note,none,
+      "type %0 cannot be trivially destroyed",
+      (Type))
+NOTE(forget_nontrivial_implicit_storage_note,none,
+     "type %0 implicitly contains %1 which cannot be trivially destroyed",
+     (Type, Type))
+
 // move only checker diagnostics
 ERROR(sil_moveonlychecker_owned_value_consumed_more_than_once, none,
       "'%0' consumed more than once", (StringRef))

include/swift/AST/DiagnosticsSema.def

@@ -4594,9 +4594,16 @@ ERROR(opaque_type_var_no_underlying_type,none,
       "property declares an opaque return type, but cannot infer the "
       "underlying type from its initializer expression", ())
 
+
+//------------------------------------------------------------------------------
+// MARK: Forget Statement
+//------------------------------------------------------------------------------
 ERROR(forget_wrong_context_decl,none,
       "'forget' statement cannot appear in %0",
       (DescriptiveDeclKind))
+ERROR(forget_no_deinit,none,
+      "'forget' has no effect for type %0 unless it has a deinitializer",
+      (Type))
 ERROR(forget_wrong_context_closure,none,
       "'forget' statement cannot appear in closure",
       ())

lib/SILGen/SILGenStmt.cpp

@@ -765,6 +765,36 @@ void StmtEmitter::visitForgetStmt(ForgetStmt *S) {
   auto *nominal = fn->getDeclContext()->getSelfNominalTypeDecl();
   assert(nominal);
 
+  // Check if the nominal's contents are trivial. This is a temporary
+  // restriction until we get forget implemented the way we want.
+  for (auto *varDecl : nominal->getStoredProperties()) {
+    assert(varDecl->hasStorage());
+    auto varType = varDecl->getType();
+    auto &varTypeLowering = SGF.getTypeLowering(varType);
+    if (!varTypeLowering.isTrivial()) {
+      diagnose(getASTContext(),
+               S->getStartLoc(),
+               diag::forget_nontrivial_storage,
+               nominal->getDeclaredInterfaceType());
+
+      // emit a note pointing out the problematic storage type
+      if (auto varLoc = varDecl->getLoc()) {
+        diagnose(getASTContext(),
+                 varLoc,
+                 diag::forget_nontrivial_storage_note,
+                 varType);
+      } else {
+        diagnose(getASTContext(),
+                 nominal->getLoc(),
+                 diag::forget_nontrivial_implicit_storage_note,
+                 nominal->getDeclaredInterfaceType(),
+                 varType);
+      }
+
+      break; // only one diagnostic is needed per forget
+    }
+  }
+
   SGF.emitMoveOnlyMemberDestruction(selfValue.forward(SGF), nominal, loc,
                                     nullptr);
 }

lib/Sema/TypeCheckStmt.cpp

@@ -1251,22 +1251,33 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
       // Save this for SILGen, since Stmt's don't know their decl context.
       FS->setInnermostMethodContext(fn);
 
-      if (fn->isStatic() || isa<DestructorDecl>(fn)) {
+      if (fn->isStatic() || isa<DestructorDecl>(fn)
+          || isa<ConstructorDecl>(fn)) {
         ctx.Diags.diagnose(FS->getForgetLoc(), diag::forget_wrong_context_decl,
                            fn->getDescriptiveKind());
         diagnosed = true;
       }
     }
 
-    // This member function/accessor/etc has to be within a noncopyable type.
+    // check the kind of type this forget statement appears within.
     if (!diagnosed) {
-      Type nominalType =
-          fn->getDeclContext()->getSelfNominalTypeDecl()->getDeclaredType();
+      auto *nominalDecl = fn->getDeclContext()->getSelfNominalTypeDecl();
+      Type nominalType = nominalDecl->getDeclaredType();
+
+      // must be noncopyable
       if (!nominalType->isPureMoveOnly()) {
         ctx.Diags.diagnose(FS->getForgetLoc(),
                            diag::forget_wrong_context_copyable,
                            fn->getDescriptiveKind());
         diagnosed = true;
+
+      // has to have a deinit or else it's pointless.
+      } else if (!nominalDecl->getValueTypeDestructor()) {
+        ctx.Diags.diagnose(FS->getForgetLoc(),
+                           diag::forget_no_deinit,
+                           nominalType)
+            .fixItRemove(FS->getSourceRange());
+        diagnosed = true;
       } else {
         // Set the contextual type for the sub-expression before we typecheck.
         contextualInfo = {nominalType, CTP_ForgetStmt};
@@ -1321,30 +1332,21 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
 
     // The 'self' parameter must be owned (aka "consuming").
     if (!diagnosed) {
-      bool isConsuming = false;
       if (auto *funcDecl = dyn_cast<FuncDecl>(fn)) {
         switch (funcDecl->getSelfAccessKind()) {
         case SelfAccessKind::LegacyConsuming:
         case SelfAccessKind::Consuming:
-          isConsuming = true;
           break;
 
         case SelfAccessKind::Borrowing:
         case SelfAccessKind::NonMutating:
         case SelfAccessKind::Mutating:
-          isConsuming = false;
+          ctx.Diags.diagnose(FS->getForgetLoc(),
+                             diag::forget_wrong_context_nonconsuming,
+                             fn->getDescriptiveKind());
+          diagnosed = true;
           break;
         }
-      } else if (isa<ConstructorDecl>(fn)) {
-        // constructors are implicitly "consuming" of the self instance.
-        isConsuming = true;
-      }
-
-      if (!isConsuming) {
-        ctx.Diags.diagnose(FS->getForgetLoc(),
-                           diag::forget_wrong_context_nonconsuming,
-                           fn->getDescriptiveKind());
-        diagnosed = true;
       }
     }
 

test/Interpreter/moveonly_forget.swift

@@ -14,6 +14,8 @@ struct FileDescriptor {
   var fd: Int
   static var nextFD: Int = 0
 
+  consuming func forget() { _forget self }
+
   init() {
     self.fd = FileDescriptor.nextFD
     FileDescriptor.nextFD += 1
@@ -22,7 +24,7 @@ struct FileDescriptor {
   init(doForget: Bool) throws {
     self.init()
     if doForget {
-      _forget self
+      forget()
       throw E.err
     }
   }
@@ -61,10 +63,12 @@ struct FileDescriptor {
   case some(FileDescriptor)
   case nothing
 
+  consuming func forget() { _forget self }
+
   init(reinit: Bool) {
     self = .some(FileDescriptor())
     if reinit {
-      _forget self
+      forget()
       self = .some(FileDescriptor())
     }
   }

test/SILGen/forget.swift

@@ -7,6 +7,8 @@ func invokedDeinit() {}
   case some(File)
   case none
 
+  deinit {}
+
   // NOTE: we can't pattern match on self since
   // that would consume it before we can forget self!
   var test: Int {
@@ -58,70 +60,54 @@ func invokedDeinit() {}
 }
 
 @_moveOnly struct PointerTree {
-  let left: Ptr
-  let file: File
-  let popularity: Int
-  var right: Ptr
-
-  init(doForget: Bool, file: __owned File, ptr: Ptr) throws {
-    self.file = file
-    self.left = ptr
-    self.popularity = 0
-    self.right = ptr
+  let left: UnsafePointer<UInt>?
+  let file: Int = 0
+  lazy var popularity: Int = 0
+  var right: Float = 0.0
 
+  consuming func tryDestroy(doForget: Bool) throws {
     if doForget {
       _forget self
     }
     throw E.err
   }
 
-// CHECK-LABEL: sil hidden [ossa] @$s4test11PointerTreeV8doForget4file3ptrACSb_AA4FileVnAA3PtrCtKcfC
+// CHECK-LABEL: sil hidden [ossa] @$s4test11PointerTreeV10tryDestroy8doForgetySb_tKF : $@convention(method) (Bool, @owned PointerTree) -> @error any Error {
 // CHECK:   bb0{{.*}}:
 // CHECK:     [[SELF_BOX:%.*]] = alloc_box ${ var PointerTree }, var, name "self"
-// CHECK:     [[MUI:%.*]] = mark_uninitialized [rootself] [[SELF_BOX]] : ${ var PointerTree }
-// CHECK:     [[SELF:%.*]] = begin_borrow [lexical] [[MUI]] : ${ var PointerTree }
-// CHECK:     [[SELF_PTR:%.*]] = project_box [[SELF]] : ${ var PointerTree }, 0
+// CHECK:     [[SELF_PTR:%.*]] = project_box [[SELF_BOX]] : ${ var PointerTree }, 0
 //            .. skip to the conditional test ..
-// CHECK:     [[SHOULD_THROW:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
-// CHECK:     cond_br [[SHOULD_THROW]], bb1, bb2
+// CHECK:     [[SHOULD_FORGET:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
+// CHECK:     cond_br [[SHOULD_FORGET]], bb1, bb2
 //
 // CHECK:   bb1:
 // CHECK:     [[ACCESS:%.*]] = begin_access [read] [unknown] [[SELF_PTR]] : $*PointerTree
 // CHECK:     [[MMC:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]] : $*PointerTree
 // CHECK:     [[COPIED_SELF:%.*]] = load [copy] [[MMC]] : $*PointerTree
 // CHECK:     end_access [[ACCESS]] : $*PointerTree
-// CHECK:     ([[LEFT:%.*]], [[FILE:%.*]], {{%.*}}, [[RIGHT:%.*]]) = destructure_struct [[COPIED_SELF]] : $PointerTree
-// CHECK:     destroy_value [[LEFT]] : $Ptr
-// CHECK:     destroy_value [[FILE]] : $File
-// CHECK:     destroy_value [[RIGHT]] : $Ptr
+// CHECK:     end_lifetime [[COPIED_SELF]]
 // CHECK:     br bb3
 //
 // CHECK:   bb2:
 // CHECK:     br bb3
 //
 // CHECK:   bb3:
-// CHECK:     end_borrow [[SELF]] : ${ var PointerTree }
-// CHECK:     destroy_value [[MUI]] : ${ var PointerTree }
+// CHECK:     destroy_value [[SELF_BOX]] : ${ var PointerTree }
 // CHECK:     throw
 // CHECK: } // end sil function
 
 // After the mandatory passes have run, check for correct deinitializations within the init.
 
-// CHECK-SIL-LABEL: sil hidden @$s4test11PointerTreeV8doForget4file3ptrACSb_AA4FileVnAA3PtrCtKcfC
-// CHECK-SIL:     [[SHOULD_THROW:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
-// CHECK-SIL:     cond_br [[SHOULD_THROW]], bb1, bb2
+// CHECK-SIL-LABEL: sil hidden @$s4test11PointerTreeV10tryDestroy8doForgetySb_tKF
+// CHECK-SIL:     [[SHOULD_FORGET:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
+// CHECK-SIL:     cond_br [[SHOULD_FORGET]], bb1, bb2
 //
 // CHECK-SIL:  bb1:
 // CHECK-SIL:     [[ACCESS:%.*]] = begin_access [modify] [static] {{.*}} : $*PointerTree
 // CHECK-SIL:     [[SELF_VAL:%.*]] = load [[ACCESS]] : $*PointerTree
 // CHECK-SIL:     end_access [[ACCESS]] : $*PointerTree
-// CHECK-SIL:     [[LEFT:%.*]] = struct_extract [[SELF_VAL]] : $PointerTree, #PointerTree.left
-// CHECK-SIL:     [[FILE:%.*]] = struct_extract [[SELF_VAL]] : $PointerTree, #PointerTree.file
-// CHECK-SIL:     [[RIGHT:%.*]] = struct_extract [[SELF_VAL]] : $PointerTree, #PointerTree.right
-// CHECK-SIL:     strong_release [[LEFT]] : $Ptr
-// CHECK-SIL:     [[FILE_DEINIT:%.*]] = function_ref @$s4test4FileVfD : $@convention(method) (@owned File) -> ()
-// CHECK-SIL:     apply [[FILE_DEINIT]]([[FILE]])
-// CHECK-SIL:     strong_release [[RIGHT]] : $Ptr
+// CHECK-SIL-NOT: struct_extract
+                  // no accesses to the fields are expected because the fields are no-op destroyed.
 // CHECK-SIL:     br bb3
 //
 // CHECK-SIL:  bb2:
@@ -148,33 +134,31 @@ final class Wallet {
   case empty
   case within(Wallet)
 
-  init(inWallet wallet: Wallet? = nil) {
-    self = .within(Wallet())
+  consuming func changeTicket(inWallet wallet: Wallet? = nil) {
     if let existingWallet = wallet {
       _forget self
       self = .within(existingWallet)
     }
   }
   // As of now, we allow reinitialization after forget. Not sure if this is intended.
-  // CHECK-LABEL: sil hidden [ossa] @$s4test6TicketO8inWalletAcA0D0CSg_tcfC
+  // CHECK-LABEL: sil hidden [ossa] @$s4test6TicketO06changeB08inWalletyAA0E0CSg_tF : $@convention(method) (@guaranteed Optional<Wallet>, @owned Ticket) -> () {
   // CHECK:    [[SELF_REF:%.*]] = project_box [[SELF_BOX:%.*]] : ${ var Ticket }, 0
-  // CHECK:    switch_enum {{.*}} : $Optional<Wallet>, case #Optional.some!enumelt: bb2, case #Optional.none!enumelt: bb1
-  // CHECK:  bb2({{%.*}} : @owned $Wallet):
-  // CHECK:    br bb3
+  // CHECK:    switch_enum {{.*}} : $Optional<Wallet>, case #Optional.some!enumelt: [[HAVE_WALLET_BB:bb.*]], case #Optional.none!enumelt: {{.*}}
+  //
   // >> now we begin the destruction sequence, which involves pattern matching on self to destroy its innards
-  // CHECK:  bb3:
+  // CHECK:  [[HAVE_WALLET_BB]]({{%.*}} : @owned $Wallet):
   // CHECK:    [[SELF_ACCESS:%.*]] = begin_access [read] [unknown] {{%.*}} : $*Ticket
   // CHECK:    [[SELF_MMC:%.*]] = mark_must_check [no_consume_or_assign] [[SELF_ACCESS]]
   // CHECK:    [[SELF_COPY:%.*]] = load [copy] [[SELF_MMC]] : $*Ticket
   // CHECK:    end_access [[SELF_ACCESS:%.*]] : $*Ticket
-  // CHECK:    switch_enum [[SELF_COPY]] : $Ticket, case #Ticket.empty!enumelt: bb4, case #Ticket.within!enumelt: bb5
-  // CHECK:  bb4:
-  // CHECK:    br bb6
-  // CHECK:  bb5([[PREV_SELF_WALLET:%.*]] : @owned $Wallet):
+  // CHECK:    switch_enum [[SELF_COPY]] : $Ticket, case #Ticket.empty!enumelt: [[TICKET_EMPTY:bb[0-9]+]], case #Ticket.within!enumelt: [[TICKET_WITHIN:bb[0-9]+]]
+  // CHECK:  [[TICKET_EMPTY]]:
+  // CHECK:    br [[JOIN_POINT:bb[0-9]+]]
+  // CHECK:  [[TICKET_WITHIN]]([[PREV_SELF_WALLET:%.*]] : @owned $Wallet):
   // CHECK:    destroy_value [[PREV_SELF_WALLET]] : $Wallet
-  // CHECK:    br bb6
+  // CHECK:    br [[JOIN_POINT]]
   // >> from here on we are reinitializing self.
-  // CHECK:  bb6:
+  // CHECK:  [[JOIN_POINT]]:
   // CHECK:    [[NEW_SELF_VAL:%.*]] = enum $Ticket, #Ticket.within!enumelt, {{.*}} : $Wallet
   // CHECK:    [[SELF_ACCESS2:%.*]] = begin_access [modify] [unknown] [[SELF_REF]] : $*Ticket
   // CHECK:    [[SELF_MMC2:%.*]] = mark_must_check [assignable_but_not_consumable] [[SELF_ACCESS2]] : $*Ticket

test/SILOptimizer/moveonly_forget.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend -sil-verify-all -verify -emit-sil %s
+// RUN: %target-swift-frontend -sil-verify-all -verify -emit-sil -enable-experimental-feature MoveOnlyEnumDeinits %s
 
 func posix_close(_ t: Int) {}
 
@@ -29,6 +29,8 @@ struct GoodFileDescriptor {
 struct BadFileDescriptor {
   let _fd: Int = 0
 
+  deinit {}
+
   var rawFileDescriptor: Int {
     __consuming get { // expected-error {{'self' consumed more than once}}
       _forget self    // expected-note {{consuming use here}}
@@ -64,10 +66,12 @@ final class Wallet {
   case pagedOut(GoodFileDescriptor)
   case within(Wallet)
 
+  consuming func forget() { _forget self }
+
   init(inWallet wallet: Wallet? = nil) { // expected-error {{'self' consumed more than once}}
     self = .within(Wallet())
     if let existingWallet = wallet {
-      _forget self
+      forget()
       self = .within(existingWallet)
     }
     forget(forever: true) // expected-note {{consuming use here}}
@@ -88,4 +92,6 @@ final class Wallet {
     }
   }
 
+  deinit {}
+
 }

test/Sema/Inputs/forget_module_adjacent.swift

@@ -1,6 +1,7 @@
 public extension Regular {
   __consuming func shutdownParty() {
-    _forget self // ok; same module
+    // FIXME: rdar://108933330 (cannot define struct deinit with -enable-library-evolution)
+//    _forget self // ok; same module
   }
 }
 

test/Sema/Inputs/forget_module_defining.swift

@@ -1,20 +1,23 @@
 @_moveOnly
 public struct Regular {
   private let sorry = 0
+  // FIXME: rdar://108933330 (cannot define struct deinit with -enable-library-evolution)
+//  deinit {}
 }
 
-@_moveOnly
-@frozen public struct Frozen {
-  private let lotfan = 0
-}
-
-
 public extension Regular {
   __consuming func endParty() {
-    _forget self
+    // FIXME: rdar://108933330 (cannot define struct deinit with -enable-library-evolution)
+//    _forget self
   }
 }
 
+@_moveOnly
+@frozen public struct Frozen {
+  private let lotfan = 0
+  deinit {}
+}
+
 public extension Frozen {
   __consuming func endParty() {
     _forget self