Skip to content

Commit b060e5b

Browse files
committed
[move-only] Make sure we mark local function inout parameters with mark_must_check.
Originally, we were relying on capture info to determine if we needed to insert this mark_must_check. This ignored that the way that we are handling escaping/non-escaping is something that is approximated in the AST but actually determined at SIL level. With that in mind, rather than relying on the capture info here, just rely on us having an inout argument. The later SIL level checking for inout escapes is able to handle mark_must_check and knows how to turn off noncopyable errors in the closure where we detect the error to prevent us from emitting further errors due to the mark_must_check here. I discovered this while playing with the previous commit. rdar://112555589
1 parent 6675084 commit b060e5b

File tree

4 files changed

+52
-14
lines changed

4 files changed

+52
-14
lines changed

lib/SILGen/SILGenProlog.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1141,11 +1141,14 @@ static void emitCaptureArguments(SILGenFunction &SGF,
11411141
fArg->setClosureCapture(true);
11421142
arg = SILValue(fArg);
11431143

1144-
// If our capture is no escape and we have a noncopyable value, insert a
1145-
// consumable and assignable. If we have an escaping closure, we are going
1146-
// to emit an error later in SIL since it is illegal to capture an inout
1147-
// value in an escaping closure.
1148-
if (isInOut && ty.isPureMoveOnly() && capture.isNoEscape()) {
1144+
// If we have an inout noncopyable paramter, insert a consumable and
1145+
// assignable.
1146+
//
1147+
// NOTE: If we have an escaping closure, we are going to emit an error later
1148+
// in SIL since it is illegal to capture an inout value in an escaping
1149+
// closure. The later code knows how to handle that we have the
1150+
// mark_must_check here.
1151+
if (isInOut && ty.isPureMoveOnly()) {
11491152
arg = SGF.B.createMarkMustCheckInst(
11501153
Loc, arg, MarkMustCheckInst::CheckKind::ConsumableAndAssignable);
11511154
}

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2314,6 +2314,18 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
23142314
}
23152315
}
23162316

2317+
// If our partial apply takes this parameter as an inout parameter and it
2318+
// has the no move only diagnostics marker on it, do not emit an error
2319+
// either.
2320+
if (auto *f = pas->getCalleeFunction()) {
2321+
if (f->hasSemanticsAttr(semantics::NO_MOVEONLY_DIAGNOSTICS)) {
2322+
if (ApplySite(pas).getArgumentOperandConvention(*op).isInoutConvention()) {
2323+
diagnosticEmitter.emitEarlierPassEmittedDiagnostic(markedValue);
2324+
return false;
2325+
}
2326+
}
2327+
}
2328+
23172329
if (pas->isOnStack() ||
23182330
ApplySite(pas).getArgumentConvention(*op).isInoutConvention()) {
23192331
LLVM_DEBUG(llvm::dbgs() << "Found on stack partial apply or inout usage!\n");

test/SILGen/moveonly_escaping_closure.swift

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -614,7 +614,8 @@ func testConsumingEscapeClosureCaptureLet(_ f: consuming @escaping () -> ()) {
614614
// CHECK: } // end sil function '$s16moveonly_closure29testGlobalClosureCaptureInOutyyAA9SingleEltVzF'
615615

616616
// CHECK-LABEL: sil private [ossa] @$s16moveonly_closure29testGlobalClosureCaptureInOutyyAA9SingleEltVzFyycfU_ : $@convention(thin) (@inout_aliasable SingleElt) -> () {
617-
// CHECK: bb0([[PROJECT:%.*]] : @closureCapture
617+
// CHECK: bb0([[ARG:%.*]] : @closureCapture
618+
// CHECK: [[PROJECT:%.*]] = mark_must_check [consumable_and_assignable] [[ARG]]
618619
//
619620
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
620621
// CHECK: [[LOADED:%.*]] = load [copy] [[ACCESS]]
@@ -657,7 +658,8 @@ func testGlobalClosureCaptureInOut(_ x: inout SingleElt) {
657658
// CHECK: } // end sil function '$s16moveonly_closure31testLocalLetClosureCaptureInOutyyAA9SingleEltVzF'
658659
//
659660
// CHECK-LABEL: sil private [ossa] @$s16moveonly_closure31testLocalLetClosureCaptureInOutyyAA9SingleEltVzFyycfU_ : $@convention(thin) (@inout_aliasable SingleElt) -> () {
660-
// CHECK: bb0([[PROJECT:%.*]] : @closureCapture
661+
// CHECK: bb0([[ARG:%.*]] : @closureCapture
662+
// CHECK: [[PROJECT:%.*]] = mark_must_check [consumable_and_assignable] [[ARG]]
661663
//
662664
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
663665
// CHECK: [[LOADED:%.*]] = load [copy] [[ACCESS]]
@@ -704,7 +706,8 @@ func testLocalLetClosureCaptureInOut(_ x: inout SingleElt) {
704706
// CHECK: } // end sil function '$s16moveonly_closure31testLocalVarClosureCaptureInOutyyAA9SingleEltVzF'
705707
//
706708
// CHECK-LABEL: sil private [ossa] @$s16moveonly_closure31testLocalVarClosureCaptureInOutyyAA9SingleEltVzFyycfU_ : $@convention(thin) (@inout_aliasable SingleElt) -> () {
707-
// CHECK: bb0([[PROJECT:%.*]] : @closureCapture
709+
// CHECK: bb0([[ARG:%.*]] : @closureCapture
710+
// CHECK: [[PROJECT:%.*]] = mark_must_check [consumable_and_assignable] [[ARG]]
708711
//
709712
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
710713
// CHECK: [[LOADED:%.*]] = load [copy] [[ACCESS]]
@@ -752,7 +755,8 @@ func testLocalVarClosureCaptureInOut(_ x: inout SingleElt) {
752755
// CHECK: } // end sil function '$s16moveonly_closure026testInOutVarClosureCapturedE0yyyycz_AA9SingleEltVztF'
753756
//
754757
// CHECK-LABEL: sil private [ossa] @$s16moveonly_closure026testInOutVarClosureCapturedE0yyyycz_AA9SingleEltVztFyycfU_ : $@convention(thin) (@inout_aliasable SingleElt) -> () {
755-
// CHECK: bb0([[PROJECT:%.*]] : @closureCapture
758+
// CHECK: bb0([[ARG:%.*]] : @closureCapture
759+
// CHECK: [[PROJECT:%.*]] = mark_must_check [consumable_and_assignable] [[ARG]]
756760
//
757761
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
758762
// CHECK: [[LOADED:%.*]] = load [copy] [[ACCESS]]
@@ -807,7 +811,8 @@ func testInOutVarClosureCaptureInOut(_ f: inout () -> (), _ x: inout SingleElt)
807811
// CHECK: } // end sil function '$s16moveonly_closure38testConsumingEscapeClosureCaptureInOutyyyycn_AA9SingleEltVztF'
808812
//
809813
// CHECK-LABEL: sil private [ossa] @$s16moveonly_closure38testConsumingEscapeClosureCaptureInOutyyyycn_AA9SingleEltVztFyycfU_ : $@convention(thin) (@inout_aliasable SingleElt) -> () {
810-
// CHECK: bb0([[PROJECT:%.*]] : @closureCapture
814+
// CHECK: bb0([[ARG:%.*]] : @closureCapture
815+
// CHECK: [[PROJECT:%.*]] = mark_must_check [consumable_and_assignable] [[ARG]]
811816
//
812817
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[PROJECT]]
813818
// CHECK: [[LOADED:%.*]] = load [copy] [[ACCESS]]

test/SILOptimizer/moveonly_self_captures.swift

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ struct E2 : ~Copyable {
1010
var k = Klass()
1111
}
1212

13+
var g: () -> () = {}
1314
struct Test : ~Copyable {
1415
var e: E
1516
var e2: E2
@@ -28,12 +29,31 @@ struct Test : ~Copyable {
2829
e = E()
2930
e2 = E2()
3031
func capture() {
31-
let _ = self // expected-error {{copy of noncopyable typed value}}
32-
let _ = self.e2 // expected-error {{copy of noncopyable typed value}}
32+
let _ = self
33+
let _ = self.e2 // expected-error {{cannot partially consume 'self'}}
3334
}
3435
capture()
3536
}
3637

38+
init(y: ()) { // expected-error {{missing reinitialization of closure capture 'self' after consume}}
39+
e = E()
40+
e2 = E2()
41+
func capture() {
42+
let _ = self // expected-note {{consumed here}}
43+
}
44+
capture()
45+
}
46+
47+
init(z: ()) {
48+
e = E()
49+
e2 = E2()
50+
func capture() {
51+
let _ = self // expected-note {{captured here}}
52+
}
53+
capture()
54+
g = capture // expected-error {{escaping local function captures mutating 'self' parameter}}
55+
}
56+
3757
func captureByLocalFunction() {
3858
func capture() {
3959
let _ = self.e
@@ -85,5 +105,3 @@ struct Test : ~Copyable {
85105
}
86106
}
87107
}
88-
89-

0 commit comments

Comments
 (0)