Navigation Menu

Skip to content

Commit

Permalink
Fix the condition for warning about implicit capture of self captures.
Browse files Browse the repository at this point in the history
We've always emitted an error if we saw an implicit use of a self
parameter of class type from an escaping closure.  In PR #35898, I fixed
this to also emit an error if the reference was to an explicit capture
of self that wasn't made in the current closure.  That was causing
some source incompatibilities that we decided were too severe, so in
PR #38947 I weakened that to a warning when the diagnostic walk was
within multiple levels of closures, because I have always thought of
this as a fix to nested closures.  However, this was the wrong condition
in two ways.

First, the diagnostic walk does not always start from the outermost
function declaration; it can also start from a multi-statement closure.
In that case, we'll still end up emitting an error when we see uses
of explicit captures from the closure when we walk it, and so we still
have a source incompatibility.  That is rdar://82545600.

Second, the old diagnostic did actually fire correctly in nested
closures as long as the code was directly referring to the original
self parameter and not any intervening captures.  Therefore, #38947
actually turned some things into warnings that had always been errors.

The fix is to produce a warning exactly when the referenced declaration
was an explicit capture.
  • Loading branch information
rjmccall committed Sep 1, 2021
1 parent 6e35487 commit 1711df4
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 8 deletions.
15 changes: 12 additions & 3 deletions lib/Sema/MiscDiagnostics.cpp
Expand Up @@ -1615,6 +1615,15 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
// Diagnostics should correct the innermost closure
auto *ACE = Closures[Closures.size() - 1];
assert(ACE);

// Until Swift 6, only emit a warning when we get this with an
// explicit capture, since we used to not diagnose this at all.
auto shouldOnlyWarn = [&](Expr *selfRef) {
// We know that isImplicitSelfParamUseLikelyToCauseCycle is true,
// which means all these casts are valid.
return !cast<VarDecl>(cast<DeclRefExpr>(selfRef)->getDecl())
->isSelfParameter();
};

SourceLoc memberLoc = SourceLoc();
if (auto *MRE = dyn_cast<MemberRefExpr>(E))
Expand All @@ -1624,7 +1633,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
Diags.diagnose(memberLoc,
diag::property_use_in_closure_without_explicit_self,
baseName.getIdentifier())
.warnUntilSwiftVersionIf(Closures.size() > 1, 6);
.warnUntilSwiftVersionIf(shouldOnlyWarn(MRE->getBase()), 6);
}

// Handle method calls with a specific diagnostic + fixit.
Expand All @@ -1636,7 +1645,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
Diags.diagnose(DSCE->getLoc(),
diag::method_call_in_closure_without_explicit_self,
MethodExpr->getDecl()->getBaseIdentifier())
.warnUntilSwiftVersionIf(Closures.size() > 1, 6);
.warnUntilSwiftVersionIf(shouldOnlyWarn(DSCE->getBase()), 6);
}

if (memberLoc.isValid()) {
Expand All @@ -1647,7 +1656,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
// Catch any other implicit uses of self with a generic diagnostic.
if (isImplicitSelfParamUseLikelyToCauseCycle(E, ACE))
Diags.diagnose(E->getLoc(), diag::implicit_use_of_self_in_closure)
.warnUntilSwiftVersionIf(Closures.size() > 1, 6);
.warnUntilSwiftVersionIf(shouldOnlyWarn(E), 6);

return { true, E };
}
Expand Down
8 changes: 4 additions & 4 deletions test/attr/attr_noescape.swift
Expand Up @@ -91,25 +91,25 @@ class SomeClass {

let _: () -> Void = { // expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{26-26= [self] in}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{26-26= [self] in}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{26-26= [self] in}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{26-26= [self] in}}
func inner() { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{22-22=self.}}
let inner2 = { foo() } // expected-warning {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{21-21= [self] in}} expected-note{{reference 'self.' explicitly}} {{22-22=self.}}
let inner2 = { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{21-21= [self] in}} expected-note{{reference 'self.' explicitly}} {{22-22=self.}}
let _ = inner2
func multi() -> Int { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{29-29=self.}}
let multi2: () -> Int = { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{32-32= [self] in}} expected-note{{reference 'self.' explicitly}} {{33-33=self.}}
let _ = multi2
doesEscape { foo() } // expected-warning {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{19-19= [self] in}} expected-note{{reference 'self.' explicitly}} {{20-20=self.}}
doesEscape { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{19-19= [self] in}} expected-note{{reference 'self.' explicitly}} {{20-20=self.}}
takesNoEscapeClosure { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{30-30=self.}}
doesEscape { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{19-19= [self] in}} expected-note{{reference 'self.' explicitly}} {{20-20=self.}}
takesNoEscapeClosure { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{30-30=self.}}
}

doesEscape { //expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{17-17= [self] in}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{17-17= [self] in}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{17-17= [self] in}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{17-17= [self] in}}
func inner() { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{22-22=self.}}
let inner2 = { foo() } // expected-warning {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{21-21= [self] in}} expected-note{{reference 'self.' explicitly}} {{22-22=self.}}
let inner2 = { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{21-21= [self] in}} expected-note{{reference 'self.' explicitly}} {{22-22=self.}}
_ = inner2
func multi() -> Int { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{29-29=self.}}
let multi2: () -> Int = { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{32-32= [self] in}} expected-note{{reference 'self.' explicitly}} {{33-33=self.}}
_ = multi2
doesEscape { foo() } // expected-warning {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{19-19= [self] in}} expected-note{{reference 'self.' explicitly}} {{20-20=self.}}
doesEscape { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{19-19= [self] in}} expected-note{{reference 'self.' explicitly}} {{20-20=self.}}
takesNoEscapeClosure { foo() } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{30-30=self.}}
doesEscape { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{19-19= [self] in}} expected-note{{reference 'self.' explicitly}} {{20-20=self.}}
takesNoEscapeClosure { foo(); return 0 } // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{30-30=self.}}
Expand Down
17 changes: 16 additions & 1 deletion test/expr/closure/closures.swift
Expand Up @@ -172,7 +172,7 @@ class ExplicitSelfRequiredTest {
doStuff({ [unowned(unsafe) self] in x+1 })
doStuff({ [unowned self = self] in x+1 })
doStuff({ x+1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{14-14= [self] in}} expected-note{{reference 'self.' explicitly}} {{15-15=self.}}
doVoidStuff({ doStuff({ x+1 })}) // expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{28-28= [self] in}} expected-note{{reference 'self.' explicitly}} {{29-29=self.}}
doVoidStuff({ doStuff({ x+1 })}) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{28-28= [self] in}} expected-note{{reference 'self.' explicitly}} {{29-29=self.}}
doVoidStuff({ x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{18-18= [self] in}} expected-note{{reference 'self.' explicitly}} {{19-19=self.}}
doVoidStuff({ _ = "\(x)"}) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{18-18= [self] in}} expected-note{{reference 'self.' explicitly}} {{26-26=self.}}
doVoidStuff({ [y = self] in x += 1 }) // expected-warning {{capture 'y' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{20-20=self, }} expected-note{{reference 'self.' explicitly}} {{33-33=self.}}
Expand Down Expand Up @@ -694,3 +694,18 @@ func testSR13239_Variadic_Twos() -> Int {
print("hello")
})
}

// rdar://82545600: this should just be a warning until Swift 6
public class TestImplicitCaptureOfExplicitCaptureOfSelfInEscapingClosure {
var property = false

private init() {
doVoidStuff { [unowned self] in
doVoidStuff {}
doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}}
doVoidStuff {}
property = false // expected-warning {{reference to property 'property' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}}
}
}
}
}

0 comments on commit 1711df4

Please sign in to comment.