Skip to content

Commit f30c3bd

Browse files
tkremenektkremenek
authored
Merge pull request #10448 from rjmccall/noescape-param-exclusivity-4.0
Implement the restriction on calls to non-escaping function parameter…
2 parents 4048c96 + e21d0ef commit f30c3bd

File tree

6 files changed

+217
-47
lines changed

6 files changed

+217
-47
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,18 @@ ERROR(incorrect_explicit_closure_result,none,
267267
"declared closure result %0 is incompatible with contextual type %1",
268268
(Type, Type))
269269

270+
// FIXME: make this an error when we've fixed the Dispatch problem.
271+
WARNING(err_noescape_param_call,none,
272+
"passing a %select{|closure which captures a }1non-escaping function "
273+
"parameter %0 to a call to a non-escaping function parameter can allow "
274+
"re-entrant modification of a variable",
275+
(DeclName, unsigned))
276+
WARNING(warn_noescape_param_call,none,
277+
"passing a %select{|closure which captures a }1non-escaping function "
278+
"parameter %0 to a call to a non-escaping function parameter can allow "
279+
"re-entrant modification of a variable and will be illegal in Swift 4",
280+
(DeclName, unsigned))
281+
270282
ERROR(cannot_call_function_value,none,
271283
"cannot invoke value of function type with argument list '%0'",
272284
(StringRef))

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 159 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
210210
}
211211

212212
// Verify noescape parameter uses.
213-
checkNoEscapeParameterUse(DRE, nullptr);
213+
checkNoEscapeParameterUse(DRE, nullptr, OperandKind::None);
214214

215215
// Verify warn_unqualified_access uses.
216216
checkUnqualifiedAccessUse(DRE);
@@ -239,7 +239,7 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
239239
if (auto MakeEsc = dyn_cast<MakeTemporarilyEscapableExpr>(E)) {
240240
if (auto DRE =
241241
dyn_cast<DeclRefExpr>(MakeEsc->getNonescapingClosureValue()))
242-
checkNoEscapeParameterUse(DRE, MakeEsc);
242+
checkNoEscapeParameterUse(DRE, MakeEsc, OperandKind::MakeEscapable);
243243
}
244244

245245
// Check function calls, looking through implicit conversions on the
@@ -263,33 +263,11 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
263263
while (auto ignoredBase = dyn_cast<DotSyntaxBaseIgnoredExpr>(Base))
264264
Base = ignoredBase->getRHS();
265265
if (auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
266-
checkNoEscapeParameterUse(DRE, Call);
266+
checkNoEscapeParameterUse(DRE, Call, OperandKind::Callee);
267267
checkForSuspiciousBitCasts(DRE, Call);
268268
}
269269

270-
auto *Arg = Call->getArg();
271-
272-
// The argument could be shuffled if it includes default arguments,
273-
// label differences, or other exciting things like that.
274-
if (auto *TSE = dyn_cast<TupleShuffleExpr>(Arg))
275-
Arg = TSE->getSubExpr();
276-
277-
// The argument is either a ParenExpr or TupleExpr.
278-
ArrayRef<Expr*> arguments;
279-
SmallVector<Expr *, 1> Scratch;
280-
if (auto *TE = dyn_cast<TupleExpr>(Arg))
281-
arguments = TE->getElements();
282-
else if (auto *PE = dyn_cast<ParenExpr>(Arg)) {
283-
Scratch.push_back(PE->getSubExpr());
284-
arguments = makeArrayRef(Scratch);
285-
}
286-
else {
287-
Scratch.push_back(Call->getArg());
288-
arguments = makeArrayRef(Scratch);
289-
}
290-
291-
// Check each argument.
292-
for (auto arg : arguments) {
270+
visitArguments(Call, [&](Expr *arg) {
293271
// InOutExpr's are allowed in argument lists directly.
294272
if (auto *IOE = dyn_cast<InOutExpr>(arg)) {
295273
if (isa<CallExpr>(Call))
@@ -307,18 +285,12 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
307285
AcceptableInOutExprs.insert(IOE);
308286
}
309287

310-
while (1) {
311-
if (auto conv = dyn_cast<ImplicitConversionExpr>(arg))
312-
arg = conv->getSubExpr();
313-
else if (auto *PE = dyn_cast<ParenExpr>(arg))
314-
arg = PE->getSubExpr();
315-
else
316-
break;
317-
}
288+
// Also give special treatment to noescape function arguments.
289+
arg = lookThroughArgument(arg);
318290

319291
if (auto *DRE = dyn_cast<DeclRefExpr>(arg))
320-
checkNoEscapeParameterUse(DRE, Call);
321-
}
292+
checkNoEscapeParameterUse(DRE, Call, OperandKind::Argument);
293+
});
322294
}
323295

324296
// If we have an assignment expression, scout ahead for acceptable _'s.
@@ -359,6 +331,38 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
359331
return { true, E };
360332
}
361333

334+
static void visitArguments(ApplyExpr *apply,
335+
llvm::function_ref<void(Expr*)> fn) {
336+
auto *arg = apply->getArg();
337+
338+
// The argument could be shuffled if it includes default arguments,
339+
// label differences, or other exciting things like that.
340+
if (auto *TSE = dyn_cast<TupleShuffleExpr>(arg))
341+
arg = TSE->getSubExpr();
342+
343+
// The argument is either a ParenExpr or TupleExpr.
344+
if (auto *TE = dyn_cast<TupleExpr>(arg)) {
345+
for (auto elt : TE->getElements())
346+
fn(elt);
347+
} else if (auto *PE = dyn_cast<ParenExpr>(arg)) {
348+
fn(PE->getSubExpr());
349+
} else {
350+
fn(arg);
351+
}
352+
}
353+
354+
static Expr *lookThroughArgument(Expr *arg) {
355+
while (1) {
356+
if (auto conv = dyn_cast<ImplicitConversionExpr>(arg))
357+
arg = conv->getSubExpr();
358+
else if (auto *PE = dyn_cast<ParenExpr>(arg))
359+
arg = PE->getSubExpr();
360+
else
361+
break;
362+
}
363+
return arg;
364+
}
365+
362366
Expr *walkToExprPost(Expr *E) override {
363367
checkInvalidPartialApplication(E);
364368
return E;
@@ -446,9 +450,118 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
446450
TC.diagnose(E->getStartLoc(), diag::value_of_module_type);
447451
}
448452

453+
class NoEscapeArgument {
454+
llvm::PointerIntPair<ParamDecl*, 1, bool> ParamAndIsCapture;
455+
public:
456+
NoEscapeArgument() {}
457+
NoEscapeArgument(ParamDecl *param, bool isCapture)
458+
: ParamAndIsCapture(param, isCapture) {
459+
assert(param);
460+
}
461+
462+
explicit operator bool() const {
463+
return ParamAndIsCapture.getPointer() != nullptr;
464+
}
465+
466+
ParamDecl *getDecl() const { return ParamAndIsCapture.getPointer(); }
467+
bool isDeclACapture() const { return ParamAndIsCapture.getInt(); }
468+
469+
static NoEscapeArgument find(TypeChecker &tc, ValueDecl *decl,
470+
bool isCapture) {
471+
if (auto param = dyn_cast<ParamDecl>(decl)) {
472+
if (auto fnType =
473+
param->getInterfaceType()->getAs<AnyFunctionType>()) {
474+
if (fnType->isNoEscape())
475+
return { param, isCapture };
476+
}
477+
return {};
478+
}
479+
480+
if (auto fn = dyn_cast<AbstractFunctionDecl>(decl)) {
481+
if (fn->getDeclContext()->isLocalContext()) {
482+
return findInCaptures(tc, fn);
483+
}
484+
return {};
485+
}
486+
487+
// FIXME: captures of computed local vars? Can these be non-escaping?
488+
return {};
489+
}
490+
491+
static NoEscapeArgument findInCaptures(TypeChecker &tc,
492+
AnyFunctionRef fn) {
493+
// Ensure we have accurate capture information for the function.
494+
tc.computeCaptures(fn);
495+
496+
for (const auto &capture : fn.getCaptureInfo().getCaptures()) {
497+
if (capture.isDynamicSelfMetadata()) continue;
498+
if (auto param = find(tc, capture.getDecl(), true))
499+
return param;
500+
}
501+
return {};
502+
}
503+
};
504+
505+
/// Enforce the exclusivity rule against calling a non-escaping
506+
/// function parameter with another non-escaping function parameter
507+
/// as an argument.
508+
void checkNoEscapeParameterCall(ApplyExpr *apply) {
509+
NoEscapeArgument noescapeArgument;
510+
Expr *problematicArg = nullptr;
511+
512+
visitArguments(apply, [&](Expr *arg) {
513+
// Just find the first problematic argument.
514+
if (noescapeArgument) return;
515+
516+
// Remember the expression which used the argument.
517+
problematicArg = arg;
518+
519+
// Look through the same set of nodes that we look through when
520+
// checking for no-escape functions.
521+
arg = lookThroughArgument(arg);
522+
523+
// If the argument isn't noescape, ignore it.
524+
auto fnType = arg->getType()->getAs<AnyFunctionType>();
525+
if (!fnType || !fnType->isNoEscape())
526+
return;
527+
528+
// Okay, it should be a closure or a decl ref.
529+
if (auto declRef = dyn_cast<DeclRefExpr>(arg)) {
530+
noescapeArgument =
531+
NoEscapeArgument::find(TC, declRef->getDecl(), false);
532+
} else if (auto closure = dyn_cast<AbstractClosureExpr>(arg)) {
533+
noescapeArgument =
534+
NoEscapeArgument::findInCaptures(TC, closure);
535+
} else {
536+
// This can happen with withoutActuallyEscaping.
537+
assert(isa<OpaqueValueExpr>(arg) &&
538+
"unexpected expression yielding noescape closure");
539+
}
540+
});
541+
542+
if (!noescapeArgument) return;
543+
544+
// In Swift 3, this is just a warning.
545+
TC.diagnose(apply->getLoc(),
546+
TC.Context.isSwiftVersion3()
547+
? diag::warn_noescape_param_call
548+
: diag::err_noescape_param_call,
549+
noescapeArgument.getDecl()->getName(),
550+
noescapeArgument.isDeclACapture())
551+
.highlight(problematicArg->getSourceRange());
552+
}
553+
554+
enum class OperandKind {
555+
None,
556+
Callee,
557+
Argument,
558+
MakeEscapable,
559+
};
560+
449561
/// The DRE argument is a reference to a noescape parameter. Verify that
450562
/// its uses are ok.
451-
void checkNoEscapeParameterUse(DeclRefExpr *DRE, Expr *ParentExpr=nullptr) {
563+
void checkNoEscapeParameterUse(DeclRefExpr *DRE, Expr *parent,
564+
OperandKind useKind) {
452565
// This only cares about declarations of noescape function type.
453566
auto AFT = DRE->getDecl()->getInterfaceType()->getAs<AnyFunctionType>();
454567
if (!AFT || !AFT->isNoEscape())
@@ -463,10 +576,15 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
463576
// either as the callee or as an argument (in which case, the typechecker
464577
// validates that the noescape bit didn't get stripped off), or as
465578
// a special case, in the binding of a withoutActuallyEscaping block.
466-
if (ParentExpr
467-
&& (isa<ApplyExpr>(ParentExpr) // param()
468-
|| isa<MakeTemporarilyEscapableExpr>(ParentExpr)))
469-
return;
579+
if (parent) {
580+
if (auto apply = dyn_cast<ApplyExpr>(parent)) {
581+
if (isa<ParamDecl>(DRE->getDecl()) && useKind == OperandKind::Callee)
582+
checkNoEscapeParameterCall(apply);
583+
return;
584+
} else if (isa<MakeTemporarilyEscapableExpr>(parent)) {
585+
return;
586+
}
587+
}
470588

471589
TC.diagnose(DRE->getStartLoc(), diag::invalid_noescape_use,
472590
DRE->getDecl()->getBaseName(),

‎test/SILOptimizer/access_enforcement_noescape.swift

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,6 @@ func doTwo(_: ()->(), _: ()->()) {}
2626
// Helper
2727
func doOneInout(_: ()->(), _: inout Int) {}
2828

29-
// FIXME: statically prohibit a call to a non-escaping closure
30-
// parameter using another non-escaping closure parameter as an argument.
31-
func reentrantNoescape(fn: (() -> ()) -> ()) {
32-
fn { fn {} }
33-
}
34-
3529
// Error: Cannot capture nonescaping closure.
3630
// func reentrantCapturedNoescape(fn: (() -> ()) -> ()) {
3731
// let c = { fn {} }

‎test/attr/attr_noescape.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,7 @@ func apply<T, U>(_ f: @noescape (T) -> U, g: @noescape (@noescape (T) -> U) -> U
304304
// expected-warning@-2{{@noescape is the default and is deprecated}} {{46-56=}}
305305
// expected-warning@-3{{@noescape is the default and is deprecated}} {{57-66=}}
306306
return g(f)
307+
// expected-warning@-1{{passing a non-escaping function parameter 'f' to a call to a non-escaping function parameter}}
307308
}
308309

309310
// <rdar://problem/19997577> @noescape cannot be applied to locals, leading to duplication of code

‎test/expr/postfix/call/noescape-param-exclusivity-swift3.swift

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// RUN: %target-typecheck-verify-swift -swift-version 3
2+
3+
func foo(fn: (() -> ()) -> ()) {
4+
fn { fn {} } // expected-warning {{passing a closure which captures a non-escaping function parameter 'fn' to a call to a non-escaping function parameter can allow re-entrant modification of a variable and will be illegal in Swift 4}}
5+
}

‎test/expr/postfix/call/noescape-param-exclusivity.swift

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// RUN: %target-typecheck-verify-swift -swift-version 4
2+
3+
// FIXME: make these errors
4+
5+
func test0(fn: (() -> ()) -> ()) {
6+
fn { fn {} } // expected-warning {{passing a closure which captures a non-escaping function parameter 'fn' to a call to a non-escaping function parameter can allow re-entrant modification of a variable}}
7+
}
8+
9+
func test1(fn: (() -> ()) -> ()) { // expected-note {{parameter 'fn' is implicitly non-escaping}}
10+
// TODO: infer that this function is noescape from its captures
11+
func foo() {
12+
fn { fn {} } // expected-warning {{can allow re-entrant modification}}
13+
// expected-error@-1 {{declaration closing over non-escaping parameter 'fn' may allow it to escape}}
14+
}
15+
}
16+
17+
func test2(x: inout Int, fn: (() -> ()) -> ()) {
18+
func foo(myfn: () -> ()) {
19+
x += 1
20+
myfn()
21+
}
22+
23+
// Make sure we only complain about calls to noescape parameters.
24+
foo { fn {} }
25+
}
26+
27+
func test3(fn: (() -> ()) -> ()) {
28+
{ myfn in myfn { fn {} } }(fn) // expected-warning {{can allow re-entrant modification}}
29+
}
30+
31+
func test4(fn: (() -> ()) -> ()) { // expected-note {{parameter 'fn' is implicitly non-escaping}}
32+
// TODO: infer that this function is noescape from its captures
33+
func foo() {
34+
fn {}
35+
// expected-error@-1 {{declaration closing over non-escaping parameter 'fn' may allow it to escape}}
36+
// FIXME: if the above is ever not an error, we should diagnose at the call below
37+
}
38+
39+
fn(foo)
40+
}

0 commit comments

Comments
 (0)