Skip to content

Commit c260792

Browse files
kavonkavon
authored
Merge pull request #65690 from kavon/forget-usage-fixes
`_forget` usage fixes to match SE-390
2 parents ace328a + 578da49 commit c260792

File tree

13 files changed

+244
-78
lines changed

13 files changed

+244
-78
lines changed

‎include/swift/AST/DiagnosticsSIL.def

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,18 @@ ERROR(noimplicitcopy_used_on_generic_or_existential, none,
735735
"@_noImplicitCopy can not be used on a generic or existential typed "
736736
"binding or a nominal type containing such typed things", ())
737737

738+
// forget statement
739+
ERROR(forget_nontrivial_storage,none,
740+
"can only 'forget' type %0 if it contains trivially-destroyed "
741+
"stored properties at this time",
742+
(Type))
743+
NOTE(forget_nontrivial_storage_note,none,
744+
"type %0 cannot be trivially destroyed",
745+
(Type))
746+
NOTE(forget_nontrivial_implicit_storage_note,none,
747+
"type %0 implicitly contains %1 which cannot be trivially destroyed",
748+
(Type, Type))
749+
738750
// move only checker diagnostics
739751
ERROR(sil_moveonlychecker_owned_value_consumed_more_than_once, none,
740752
"'%0' consumed more than once", (StringRef))

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4599,9 +4599,16 @@ ERROR(opaque_type_var_no_underlying_type,none,
45994599
"property declares an opaque return type, but cannot infer the "
46004600
"underlying type from its initializer expression", ())
46014601

4602+
4603+
//------------------------------------------------------------------------------
4604+
// MARK: Forget Statement
4605+
//------------------------------------------------------------------------------
46024606
ERROR(forget_wrong_context_decl,none,
46034607
"'forget' statement cannot appear in %0",
46044608
(DescriptiveDeclKind))
4609+
ERROR(forget_no_deinit,none,
4610+
"'forget' has no effect for type %0 unless it has a deinitializer",
4611+
(Type))
46054612
ERROR(forget_wrong_context_closure,none,
46064613
"'forget' statement cannot appear in closure",
46074614
())

‎lib/SILGen/SILGenStmt.cpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,36 @@ void StmtEmitter::visitForgetStmt(ForgetStmt *S) {
765765
auto *nominal = fn->getDeclContext()->getSelfNominalTypeDecl();
766766
assert(nominal);
767767

768+
// Check if the nominal's contents are trivial. This is a temporary
769+
// restriction until we get forget implemented the way we want.
770+
for (auto *varDecl : nominal->getStoredProperties()) {
771+
assert(varDecl->hasStorage());
772+
auto varType = varDecl->getType();
773+
auto &varTypeLowering = SGF.getTypeLowering(varType);
774+
if (!varTypeLowering.isTrivial()) {
775+
diagnose(getASTContext(),
776+
S->getStartLoc(),
777+
diag::forget_nontrivial_storage,
778+
nominal->getDeclaredInterfaceType());
779+
780+
// emit a note pointing out the problematic storage type
781+
if (auto varLoc = varDecl->getLoc()) {
782+
diagnose(getASTContext(),
783+
varLoc,
784+
diag::forget_nontrivial_storage_note,
785+
varType);
786+
} else {
787+
diagnose(getASTContext(),
788+
nominal->getLoc(),
789+
diag::forget_nontrivial_implicit_storage_note,
790+
nominal->getDeclaredInterfaceType(),
791+
varType);
792+
}
793+
794+
break; // only one diagnostic is needed per forget
795+
}
796+
}
797+
768798
SGF.emitMoveOnlyMemberDestruction(selfValue.forward(SGF), nominal, loc,
769799
nullptr);
770800
}

‎lib/Sema/TypeCheckStmt.cpp

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1251,22 +1251,33 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
12511251
// Save this for SILGen, since Stmt's don't know their decl context.
12521252
FS->setInnermostMethodContext(fn);
12531253

1254-
if (fn->isStatic() || isa<DestructorDecl>(fn)) {
1254+
if (fn->isStatic() || isa<DestructorDecl>(fn)
1255+
|| isa<ConstructorDecl>(fn)) {
12551256
ctx.Diags.diagnose(FS->getForgetLoc(), diag::forget_wrong_context_decl,
12561257
fn->getDescriptiveKind());
12571258
diagnosed = true;
12581259
}
12591260
}
12601261

1261-
// This member function/accessor/etc has to be within a noncopyable type.
1262+
// check the kind of type this forget statement appears within.
12621263
if (!diagnosed) {
1263-
Type nominalType =
1264-
fn->getDeclContext()->getSelfNominalTypeDecl()->getDeclaredType();
1264+
auto *nominalDecl = fn->getDeclContext()->getSelfNominalTypeDecl();
1265+
Type nominalType = nominalDecl->getDeclaredType();
1266+
1267+
// must be noncopyable
12651268
if (!nominalType->isPureMoveOnly()) {
12661269
ctx.Diags.diagnose(FS->getForgetLoc(),
12671270
diag::forget_wrong_context_copyable,
12681271
fn->getDescriptiveKind());
12691272
diagnosed = true;
1273+
1274+
// has to have a deinit or else it's pointless.
1275+
} else if (!nominalDecl->getValueTypeDestructor()) {
1276+
ctx.Diags.diagnose(FS->getForgetLoc(),
1277+
diag::forget_no_deinit,
1278+
nominalType)
1279+
.fixItRemove(FS->getSourceRange());
1280+
diagnosed = true;
12701281
} else {
12711282
// Set the contextual type for the sub-expression before we typecheck.
12721283
contextualInfo = {nominalType, CTP_ForgetStmt};
@@ -1321,30 +1332,21 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
13211332

13221333
// The 'self' parameter must be owned (aka "consuming").
13231334
if (!diagnosed) {
1324-
bool isConsuming = false;
13251335
if (auto *funcDecl = dyn_cast<FuncDecl>(fn)) {
13261336
switch (funcDecl->getSelfAccessKind()) {
13271337
case SelfAccessKind::LegacyConsuming:
13281338
case SelfAccessKind::Consuming:
1329-
isConsuming = true;
13301339
break;
13311340

13321341
case SelfAccessKind::Borrowing:
13331342
case SelfAccessKind::NonMutating:
13341343
case SelfAccessKind::Mutating:
1335-
isConsuming = false;
1344+
ctx.Diags.diagnose(FS->getForgetLoc(),
1345+
diag::forget_wrong_context_nonconsuming,
1346+
fn->getDescriptiveKind());
1347+
diagnosed = true;
13361348
break;
13371349
}
1338-
} else if (isa<ConstructorDecl>(fn)) {
1339-
// constructors are implicitly "consuming" of the self instance.
1340-
isConsuming = true;
1341-
}
1342-
1343-
if (!isConsuming) {
1344-
ctx.Diags.diagnose(FS->getForgetLoc(),
1345-
diag::forget_wrong_context_nonconsuming,
1346-
fn->getDescriptiveKind());
1347-
diagnosed = true;
13481350
}
13491351
}
13501352

‎test/Interpreter/moveonly_forget.swift

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ struct FileDescriptor {
1414
var fd: Int
1515
static var nextFD: Int = 0
1616

17+
consuming func forget() { _forget self }
18+
1719
init() {
1820
self.fd = FileDescriptor.nextFD
1921
FileDescriptor.nextFD += 1
@@ -22,7 +24,7 @@ struct FileDescriptor {
2224
init(doForget: Bool) throws {
2325
self.init()
2426
if doForget {
25-
_forget self
27+
forget()
2628
throw E.err
2729
}
2830
}
@@ -61,10 +63,12 @@ struct FileDescriptor {
6163
case some(FileDescriptor)
6264
case nothing
6365

66+
consuming func forget() { _forget self }
67+
6468
init(reinit: Bool) {
6569
self = .some(FileDescriptor())
6670
if reinit {
67-
_forget self
71+
forget()
6872
self = .some(FileDescriptor())
6973
}
7074
}

‎test/SILGen/forget.swift

Lines changed: 29 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ func invokedDeinit() {}
77
case some(File)
88
case none
99

10+
deinit {}
11+
1012
// NOTE: we can't pattern match on self since
1113
// that would consume it before we can forget self!
1214
var test: Int {
@@ -60,71 +62,55 @@ func invokedDeinit() {}
6062
}
6163

6264
@_moveOnly struct PointerTree {
63-
let left: Ptr
64-
let file: File
65-
let popularity: Int
66-
var right: Ptr
67-
68-
init(doForget: Bool, file: __owned File, ptr: Ptr) throws {
69-
self.file = file
70-
self.left = ptr
71-
self.popularity = 0
72-
self.right = ptr
65+
let left: UnsafePointer<UInt>?
66+
let file: Int = 0
67+
lazy var popularity: Int = 0
68+
var right: Float = 0.0
7369

70+
consuming func tryDestroy(doForget: Bool) throws {
7471
if doForget {
7572
_forget self
7673
}
7774
throw E.err
7875
}
7976

80-
// CHECK-LABEL: sil hidden [ossa] @$s4test11PointerTreeV8doForget4file3ptrACSb_AA4FileVnAA3PtrCtKcfC
77+
// CHECK-LABEL: sil hidden [ossa] @$s4test11PointerTreeV10tryDestroy8doForgetySb_tKF : $@convention(method) (Bool, @owned PointerTree) -> @error any Error {
8178
// CHECK: bb0{{.*}}:
8279
// CHECK: [[SELF_BOX:%.*]] = alloc_box ${ var PointerTree }, var, name "self"
83-
// CHECK: [[MUI:%.*]] = mark_uninitialized [rootself] [[SELF_BOX]] : ${ var PointerTree }
84-
// CHECK: [[SELF:%.*]] = begin_borrow [lexical] [[MUI]] : ${ var PointerTree }
85-
// CHECK: [[SELF_PTR:%.*]] = project_box [[SELF]] : ${ var PointerTree }, 0
80+
// CHECK: [[SELF_PTR:%.*]] = project_box [[SELF_BOX]] : ${ var PointerTree }, 0
8681
// .. skip to the conditional test ..
87-
// CHECK: [[SHOULD_THROW:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
88-
// CHECK: cond_br [[SHOULD_THROW]], bb1, bb2
82+
// CHECK: [[SHOULD_FORGET:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
83+
// CHECK: cond_br [[SHOULD_FORGET]], bb1, bb2
8984
//
9085
// CHECK: bb1:
9186
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[SELF_PTR]] : $*PointerTree
9287
// CHECK: [[MMC:%.*]] = mark_must_check [no_consume_or_assign] [[ACCESS]] : $*PointerTree
9388
// CHECK: [[COPIED_SELF:%.*]] = load [copy] [[MMC]] : $*PointerTree
9489
// CHECK: end_access [[ACCESS]] : $*PointerTree
9590
// CHECK: [[DD:%.*]] = drop_deinit [[COPIED_SELF]]
96-
// CHECK: ([[LEFT:%.*]], [[FILE:%.*]], {{%.*}}, [[RIGHT:%.*]]) = destructure_struct [[DD]] : $PointerTree
97-
// CHECK: destroy_value [[LEFT]] : $Ptr
98-
// CHECK: destroy_value [[FILE]] : $File
99-
// CHECK: destroy_value [[RIGHT]] : $Ptr
91+
// CHECK: end_lifetime [[DD]]
10092
// CHECK: br bb3
10193
//
10294
// CHECK: bb2:
10395
// CHECK: br bb3
10496
//
10597
// CHECK: bb3:
106-
// CHECK: end_borrow [[SELF]] : ${ var PointerTree }
107-
// CHECK: destroy_value [[MUI]] : ${ var PointerTree }
98+
// CHECK: destroy_value [[SELF_BOX]] : ${ var PointerTree }
10899
// CHECK: throw
109100
// CHECK: } // end sil function
110101

111102
// After the mandatory passes have run, check for correct deinitializations within the init.
112103

113-
// CHECK-SIL-LABEL: sil hidden @$s4test11PointerTreeV8doForget4file3ptrACSb_AA4FileVnAA3PtrCtKcfC
114-
// CHECK-SIL: [[SHOULD_THROW:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
115-
// CHECK-SIL: cond_br [[SHOULD_THROW]], bb1, bb2
104+
// CHECK-SIL-LABEL: sil hidden @$s4test11PointerTreeV10tryDestroy8doForgetySb_tKF
105+
// CHECK-SIL: [[SHOULD_FORGET:%.*]] = struct_extract {{.*}} : $Bool, #Bool._value
106+
// CHECK-SIL: cond_br [[SHOULD_FORGET]], bb1, bb2
116107
//
117108
// CHECK-SIL: bb1:
118109
// CHECK-SIL: [[ACCESS:%.*]] = begin_access [modify] [static] {{.*}} : $*PointerTree
119110
// CHECK-SIL: [[SELF_VAL:%.*]] = load [[ACCESS]] : $*PointerTree
120111
// CHECK-SIL: end_access [[ACCESS]] : $*PointerTree
121-
// CHECK-SIL: [[LEFT:%.*]] = struct_extract [[SELF_VAL]] : $PointerTree, #PointerTree.left
122-
// CHECK-SIL: [[FILE:%.*]] = struct_extract [[SELF_VAL]] : $PointerTree, #PointerTree.file
123-
// CHECK-SIL: [[RIGHT:%.*]] = struct_extract [[SELF_VAL]] : $PointerTree, #PointerTree.right
124-
// CHECK-SIL: strong_release [[LEFT]] : $Ptr
125-
// CHECK-SIL: [[FILE_DEINIT:%.*]] = function_ref @$s4test4FileVfD : $@convention(method) (@owned File) -> ()
126-
// CHECK-SIL: apply [[FILE_DEINIT]]([[FILE]])
127-
// CHECK-SIL: strong_release [[RIGHT]] : $Ptr
112+
// CHECK-SIL-NOT: struct_extract
113+
// no accesses to the fields are expected because the fields are no-op destroyed.
128114
// CHECK-SIL: br bb3
129115
//
130116
// CHECK-SIL: bb2:
@@ -151,34 +137,32 @@ final class Wallet {
151137
case empty
152138
case within(Wallet)
153139

154-
init(inWallet wallet: Wallet? = nil) {
155-
self = .within(Wallet())
140+
consuming func changeTicket(inWallet wallet: Wallet? = nil) {
156141
if let existingWallet = wallet {
157142
_forget self
158143
self = .within(existingWallet)
159144
}
160145
}
161146
// As of now, we allow reinitialization after forget. Not sure if this is intended.
162-
// CHECK-LABEL: sil hidden [ossa] @$s4test6TicketO8inWalletAcA0D0CSg_tcfC
147+
// CHECK-LABEL: sil hidden [ossa] @$s4test6TicketO06changeB08inWalletyAA0E0CSg_tF : $@convention(method) (@guaranteed Optional<Wallet>, @owned Ticket) -> () {
163148
// CHECK: [[SELF_REF:%.*]] = project_box [[SELF_BOX:%.*]] : ${ var Ticket }, 0
164-
// CHECK: switch_enum {{.*}} : $Optional<Wallet>, case #Optional.some!enumelt: bb2, case #Optional.none!enumelt: bb1
165-
// CHECK: bb2({{%.*}} : @owned $Wallet):
166-
// CHECK: br bb3
149+
// CHECK: switch_enum {{.*}} : $Optional<Wallet>, case #Optional.some!enumelt: [[HAVE_WALLET_BB:bb.*]], case #Optional.none!enumelt: {{.*}}
150+
//
167151
// >> now we begin the destruction sequence, which involves pattern matching on self to destroy its innards
168-
// CHECK: bb3:
152+
// CHECK: [[HAVE_WALLET_BB]]({{%.*}} : @owned $Wallet):
169153
// CHECK: [[SELF_ACCESS:%.*]] = begin_access [read] [unknown] {{%.*}} : $*Ticket
170154
// CHECK: [[SELF_MMC:%.*]] = mark_must_check [no_consume_or_assign] [[SELF_ACCESS]]
171155
// CHECK: [[SELF_COPY:%.*]] = load [copy] [[SELF_MMC]] : $*Ticket
172156
// CHECK: end_access [[SELF_ACCESS:%.*]] : $*Ticket
173157
// CHECK: [[DD:%.*]] = drop_deinit [[SELF_COPY]] : $Ticket
174-
// CHECK: switch_enum [[DD]] : $Ticket, case #Ticket.empty!enumelt: bb4, case #Ticket.within!enumelt: bb5
175-
// CHECK: bb4:
176-
// CHECK: br bb6
177-
// CHECK: bb5([[PREV_SELF_WALLET:%.*]] : @owned $Wallet):
158+
// CHECK: switch_enum [[DD]] : $Ticket, case #Ticket.empty!enumelt: [[TICKET_EMPTY:bb[0-9]+]], case #Ticket.within!enumelt: [[TICKET_WITHIN:bb[0-9]+]]
159+
// CHECK: [[TICKET_EMPTY]]:
160+
// CHECK: br [[JOIN_POINT:bb[0-9]+]]
161+
// CHECK: [[TICKET_WITHIN]]([[PREV_SELF_WALLET:%.*]] : @owned $Wallet):
178162
// CHECK: destroy_value [[PREV_SELF_WALLET]] : $Wallet
179-
// CHECK: br bb6
163+
// CHECK: br [[JOIN_POINT]]
180164
// >> from here on we are reinitializing self.
181-
// CHECK: bb6:
165+
// CHECK: [[JOIN_POINT]]:
182166
// CHECK: [[NEW_SELF_VAL:%.*]] = enum $Ticket, #Ticket.within!enumelt, {{.*}} : $Wallet
183167
// CHECK: [[SELF_ACCESS2:%.*]] = begin_access [modify] [unknown] [[SELF_REF]] : $*Ticket
184168
// CHECK: [[SELF_MMC2:%.*]] = mark_must_check [assignable_but_not_consumable] [[SELF_ACCESS2]] : $*Ticket

‎test/SILOptimizer/moveonly_forget.swift

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %target-swift-frontend -sil-verify-all -verify -emit-sil %s
1+
// RUN: %target-swift-frontend -sil-verify-all -verify -emit-sil -enable-experimental-feature MoveOnlyEnumDeinits %s
22

33
func posix_close(_ t: Int) {}
44

@@ -29,6 +29,8 @@ struct GoodFileDescriptor {
2929
struct BadFileDescriptor {
3030
let _fd: Int = 0
3131

32+
deinit {}
33+
3234
var rawFileDescriptor: Int {
3335
__consuming get { // expected-error {{'self' consumed more than once}}
3436
_forget self // expected-note {{consuming use here}}
@@ -64,10 +66,12 @@ final class Wallet {
6466
case pagedOut(GoodFileDescriptor)
6567
case within(Wallet)
6668

69+
consuming func forget() { _forget self }
70+
6771
init(inWallet wallet: Wallet? = nil) { // expected-error {{'self' consumed more than once}}
6872
self = .within(Wallet())
6973
if let existingWallet = wallet {
70-
_forget self
74+
forget()
7175
self = .within(existingWallet)
7276
}
7377
forget(forever: true) // expected-note {{consuming use here}}
@@ -88,4 +92,6 @@ final class Wallet {
8892
}
8993
}
9094

95+
deinit {}
96+
9197
}

‎test/Sema/Inputs/forget_module_adjacent.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
public extension Regular {
22
__consuming func shutdownParty() {
3-
_forget self // ok; same module
3+
// FIXME: rdar://108933330 (cannot define struct deinit with -enable-library-evolution)
4+
// _forget self // ok; same module
45
}
56
}
67

‎test/Sema/Inputs/forget_module_defining.swift

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,23 @@
11
@_moveOnly
22
public struct Regular {
33
private let sorry = 0
4+
// FIXME: rdar://108933330 (cannot define struct deinit with -enable-library-evolution)
5+
// deinit {}
46
}
57

6-
@_moveOnly
7-
@frozen public struct Frozen {
8-
private let lotfan = 0
9-
}
10-
11-
128
public extension Regular {
139
__consuming func endParty() {
14-
_forget self
10+
// FIXME: rdar://108933330 (cannot define struct deinit with -enable-library-evolution)
11+
// _forget self
1512
}
1613
}
1714

15+
@_moveOnly
16+
@frozen public struct Frozen {
17+
private let lotfan = 0
18+
deinit {}
19+
}
20+
1821
public extension Frozen {
1922
__consuming func endParty() {
2023
_forget self

0 commit comments

Comments
 (0)