From 4eee6f89b972ad886bf66db1f8e55184fb05ecef Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Mon, 20 Dec 2021 06:58:02 -0800 Subject: [PATCH 01/37] Unconditionally permit implicit self in weak closures --- lib/Sema/MiscDiagnostics.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 2569bde8c4a6a..5ee7d7ade2d30 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1523,7 +1523,18 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // capturing it will create a reference cycle. if (!ty->hasReferenceSemantics()) return false; - + + // If self was captured weakly, but has since been unwrapped, + // then we can permit implicit self (since it doesn't + // introduce a new strong reference). + if (auto *closureExpr = dyn_cast(inClosure)) { + // if the closure captured self weakly: + if (closureExpr->getCapturedSelfDecl()->getType()->is()) { + // TODO: how to check if `self` has been unwrapped or not? + return false; + } + } + return true; } From 2292e38a3869b193afd1cdff06d5433e3ab619f7 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Thu, 23 Dec 2021 10:51:22 -0800 Subject: [PATCH 02/37] Fix issue where implicit self wasn't rejected before self is unwrapped --- lib/Sema/MiscDiagnostics.cpp | 2 -- lib/Sema/PreCheckExpr.cpp | 12 ++++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 5ee7d7ade2d30..d25e750417f86 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1528,9 +1528,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // then we can permit implicit self (since it doesn't // introduce a new strong reference). if (auto *closureExpr = dyn_cast(inClosure)) { - // if the closure captured self weakly: if (closureExpr->getCapturedSelfDecl()->getType()->is()) { - // TODO: how to check if `self` has been unwrapped or not? return false; } } diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index fa257972099a1..5ff4461ae3d6f 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -744,6 +744,18 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, BaseExpr = TypeExpr::createImplicitForDecl( UDRE->getNameLoc(), NTD, BaseDC, DC->mapTypeIntoContext(NTD->getInterfaceType())); + } else if (Base->getBaseIdentifier().is("self")) { + // If this is an implicit self call, we don't actually + // know if self is Self or Optional (e.g. in a weak self + // closure before self is unwrapped). + // - Since we don't know the correct type of self yet, + // we leave this as an UnresolvedDeclRefExpr that is + // populated with the actual type later. + auto selfNameRef = new DeclNameRef(Base->getBaseIdentifier()); + BaseExpr = new (Context) UnresolvedDeclRefExpr(*selfNameRef, + DeclRefKind::Ordinary, + UDRE->getNameLoc()); + BaseExpr->setImplicit(); } else { BaseExpr = new (Context) DeclRefExpr(Base, UDRE->getNameLoc(), /*Implicit=*/true); From 30f07e13dcd48d1a98e91304667c3023aa578a99 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Thu, 23 Dec 2021 18:23:59 -0800 Subject: [PATCH 03/37] Fix edge cases --- lib/Sema/MiscDiagnostics.cpp | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index d25e750417f86..a3321d2a840f6 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1504,10 +1504,6 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!DRE || !DRE->isImplicit()) return false; - auto var = dyn_cast(DRE->getDecl()); - if (!var || !isEnclosingSelfReference(var, inClosure)) - return false; - // Defensive check for type. If the expression doesn't have type here, it // should have been diagnosed somewhere else. Type ty = DRE->getType(); @@ -1524,11 +1520,9 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!ty->hasReferenceSemantics()) return false; - // If self was captured weakly, but has since been unwrapped, - // then we can permit implicit self (since it doesn't - // introduce a new strong reference). - if (auto *closureExpr = dyn_cast(inClosure)) { - if (closureExpr->getCapturedSelfDecl()->getType()->is()) { + // If `self` is explicitly captured, then implicit self is always allowed + if (auto closureExpr = dyn_cast(inClosure)) { + if (closureExpr->getCapturedSelfDecl()) { return false; } } From b1ebd95ed0389f42fe2804a29f20410c27a9e9b8 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Thu, 23 Dec 2021 19:27:05 -0800 Subject: [PATCH 04/37] Improve diagnostic for invalid use of implicit self before self is unwrapped --- include/swift/AST/DiagnosticsSema.def | 6 ++++++ lib/Sema/CSDiagnostics.cpp | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index ad48bb67aa453..0675333e8c24a 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -1172,6 +1172,12 @@ NOTE(optional_key_path_root_base_chain, none, NOTE(optional_key_path_root_base_unwrap, none, "unwrap the optional using '!.' to access unwrapped type member %0", (DeclNameRef)) + +ERROR(optional_self_not_unwrapped,none, + "explicit use of 'self' is required when 'self' is optional, " + "to make control flow explicit", ()) +NOTE(optional_self_chain,none, + "reference 'self?.' explicitly", ()) ERROR(missing_unwrap_optional_try,none, "value of optional type %0 not unwrapped; did you mean to use 'try!' " diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp index 25236fdcd6ce7..82313a012ea25 100644 --- a/lib/Sema/CSDiagnostics.cpp +++ b/lib/Sema/CSDiagnostics.cpp @@ -1272,6 +1272,22 @@ bool MemberAccessOnOptionalBaseFailure::diagnoseAsError() { .fixItInsert(sourceRange.End, "!."); } } else { + // Check whether or not the base of this optional unwrap is implicit self + // - This can only happen with a [weak self] capture, and is not permitted. + if (auto dotExpr = getAsExpr(locator->getAnchor())) { + if (auto base = dotExpr->getBase()) { + if (auto baseDeclRef = dyn_cast(base)) { + if (baseDeclRef->isImplicit() && baseDeclRef->getDecl()->getBaseIdentifier().is("self")) { + emitDiagnostic(diag::optional_self_not_unwrapped); + + emitDiagnostic(diag::optional_self_chain) + .fixItInsertAfter(sourceRange.End, "self?."); + return true; + } + } + } + } + emitDiagnostic(diag::optional_base_not_unwrapped, baseType, Member, unwrappedBaseType); From d909365e2ea91cce8d555d652e7591c128f0f2aa Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Fri, 24 Dec 2021 08:16:56 -0800 Subject: [PATCH 05/37] Update tests --- test/expr/closure/closures.swift | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 49508cc7a8fd2..b94bb14003ca5 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -177,8 +177,8 @@ class ExplicitSelfRequiredTest { 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.}} doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in x += 1 }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [weak self] in x+1 }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [weak self] in x += 1 }) // expected-warning {{variable 'self' was written to, but never read}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self?.' explicitly}} + doStuff({ [weak self] in x+1 }) // expected-warning {{variable 'self' was written to, but never read}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self?.' explicitly}} doVoidStuff({ [self = self.x] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} doStuff({ [self = self.x] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} @@ -189,8 +189,8 @@ class ExplicitSelfRequiredTest { doVoidStuff { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} doVoidStuff { [y = self] in _ = method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' 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}} {{37-37=self.}} doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in _ = method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [weak self] in method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [weak self] in _ = method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} + doStuff({ [weak self] in method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} doVoidStuff({ [self = self.x] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} doStuff({ [self = self.x] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} doVoidStuff { _ = self.method() } @@ -709,3 +709,20 @@ public class TestImplicitCaptureOfExplicitCaptureOfSelfInEscapingClosure { } } } + +public class TestImplicitSelfForWeakSelfCapture { + func method() { } + + private init() { + doVoidStuff { [weak self] in + guard let self = self else { return } + method() + } + + doVoidStuff { [weak self] in + if let self = self { + method() + } + } + } +} From 5f8e567b79dcd70002c9714a38d7d672723e1cee Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Fri, 24 Dec 2021 08:17:17 -0800 Subject: [PATCH 06/37] Fix crash --- lib/Sema/MiscDiagnostics.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index a3321d2a840f6..3dbf93024ace8 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1580,10 +1580,15 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // 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(cast(selfRef)->getDecl()) - ->isSelfParameter(); + if (auto declRef = dyn_cast(selfRef)) { + if (auto decl = declRef->getDecl()) { + if (auto varDecl = dyn_cast(decl)) { + return !varDecl->isSelfParameter(); + } + } + } + + return false; }; SourceLoc memberLoc = SourceLoc(); From 72074b30be89fc3cfc971f282da577fe5ee3af1a Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Fri, 24 Dec 2021 16:32:51 -0800 Subject: [PATCH 07/37] Fix some test failures --- lib/Sema/MiscDiagnostics.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 3dbf93024ace8..8d8fdcb60a7e9 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1527,6 +1527,21 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, } } + if (auto var = dyn_cast(DRE->getDecl())) { + // If this `self` decl was not captured explicitly by this closure, + // but is actually from an outer `weak` capture's `if let self = self` + // or `guard let self = self`, then we don't allow implicit self. + if (auto parentStmt = var->getParentPatternStmt()) { + if (isa(parentStmt) || isa(parentStmt)) { + return true; + } + } + + if (!isEnclosingSelfReference(var, inClosure)) { + return false; + } + } + return true; } @@ -1619,7 +1634,6 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, return { false, 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(shouldOnlyWarn(E), 6); From 854f024ec174106e1dc8a5d06f19a55f7601df0a Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Fri, 24 Dec 2021 16:44:35 -0800 Subject: [PATCH 08/37] Fix issue where '@autoclosure () -> String = String()' would be rejected as an invalid implicit self --- lib/Sema/MiscDiagnostics.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 8d8fdcb60a7e9..9b0972dba2f1e 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1541,6 +1541,9 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, return false; } } + + if (auto value = dyn_cast(DRE->getDecl())) + return false; return true; } From a5b3312da53a29d5d7d362836f6698ea905f2b0d Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Fri, 24 Dec 2021 20:51:57 -0800 Subject: [PATCH 09/37] Fix more cases, write more test --- include/swift/AST/DiagnosticsSema.def | 2 - include/swift/AST/Stmt.h | 2 +- lib/Sema/MiscDiagnostics.cpp | 64 +++++++++++++++++++++------ test/expr/closure/closures.swift | 17 ++++++- 4 files changed, 67 insertions(+), 18 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 0675333e8c24a..1cffc1b23950b 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -3776,8 +3776,6 @@ NOTE(note_reference_self_explicitly,none, NOTE(note_other_self_capture,none, "variable other than 'self' captured here under the name 'self' does not " "enable implicit 'self'", ()) -NOTE(note_self_captured_weakly,none, - "weak capture of 'self' here does not enable implicit 'self'", ()) ERROR(implicit_use_of_self_in_closure,none, "implicit use of 'self' in closure; use 'self.' to make" " capture semantics explicit", ()) diff --git a/include/swift/AST/Stmt.h b/include/swift/AST/Stmt.h index 58d29225be543..3ebb938a8dbd1 100644 --- a/include/swift/AST/Stmt.h +++ b/include/swift/AST/Stmt.h @@ -562,7 +562,7 @@ class DoStmt : public LabeledStmt { }; /// Either an "if let" case or a simple boolean expression can appear as the -/// condition of an 'if' or 'while' statement. +/// condition of an 'if', 'guard', or 'while' statement. using StmtCondition = MutableArrayRef; /// This is the common base class between statements that can have labels, and diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 9b0972dba2f1e..d1d35e7506076 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1520,20 +1520,60 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!ty->hasReferenceSemantics()) return false; - // If `self` is explicitly captured, then implicit self is always allowed + auto isExplicitWeakSelfCapture = false; if (auto closureExpr = dyn_cast(inClosure)) { - if (closureExpr->getCapturedSelfDecl()) { - return false; + if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { + // If `weak self` captured explicitly, then implicit self + // is always allowed allowed once `self` is unwrapped. + // - If `self` is still an Optional, compilation would have failed already. + if (selfDecl->getType()->is()) { + isExplicitWeakSelfCapture = true; + } + + // If this capture is using the name `self` actually referring + // to some other variable (e.g. with `[self = "hello"]`) + // then implicit self is not allowed. + else if (!selfDecl->isSelfParamCapture()) { + return true; + } } } if (auto var = dyn_cast(DRE->getDecl())) { - // If this `self` decl was not captured explicitly by this closure, - // but is actually from an outer `weak` capture's `if let self = self` - // or `guard let self = self`, then we don't allow implicit self. if (auto parentStmt = var->getParentPatternStmt()) { if (isa(parentStmt) || isa(parentStmt)) { - return true; + // If this `self` decl was not captured explicitly by this closure, + // but is actually from an outer `weak` capture's `if let self = self` + // or `guard let self = self`, then we don't allow implicit self. + if (!isExplicitWeakSelfCapture) { + return true; + } + + // If this is an explicit `weak self` capture, then we need + // to validate that the unwrapped `self` decl is actually + // referring to `self`, and not something else like in + // `guard let self = .someOptionalVariable else { return }` + auto hasCorrectSelfBindingCondition = false; + for (auto cond : dyn_cast(parentStmt)->getCond()) { + if (cond.getKind() == StmtConditionElement::CK_PatternBinding) { + if (auto optionalPattern = dyn_cast(cond.getPattern())) { + // if the lhs of the optional binding is `self`... + if (optionalPattern->getSubPattern()->getBoundName().is("self")) { + if (auto loadExpr = dyn_cast(cond.getInitializer())) { + if (auto declRefExpr = dyn_cast(loadExpr->getSubExpr())) { + // and the rhs of the optional binding is `self`... + if (declRefExpr->getDecl()->getBaseIdentifier().is("self")) { + // then we can permit implicit self in this closure + hasCorrectSelfBindingCondition = true; + } + } + } + } + } + } + } + + return !hasCorrectSelfBindingCondition; } } @@ -1597,6 +1637,9 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // Until Swift 6, only emit a warning when we get this with an // explicit capture, since we used to not diagnose this at all. + // FIXME: We should always emit an error here for weak captures, + // which don't have the same implicit self source compat requirements + // as strong captures. auto shouldOnlyWarn = [&](Expr *selfRef) { if (auto declRef = dyn_cast(selfRef)) { if (auto decl = declRef->getDecl()) { @@ -1667,7 +1710,6 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // capture 'self' explicitly will result in an error, and using // 'self.' explicitly will be accessing something other than the // self param. - // FIXME: We could offer a special fixit in the [weak self] case to insert 'self?.'... return; } emitFixItsForExplicitClosure(Diags, memberLoc, CE); @@ -1688,11 +1730,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // If we've already captured something with the name "self" other than // the actual self param, offer special diagnostics. if (auto *VD = closureExpr->getCapturedSelfDecl()) { - // Either this is a weak capture of self... - if (VD->getType()->is()) { - Diags.diagnose(VD->getLoc(), diag::note_self_captured_weakly); - // ...or something completely different. - } else { + if (!VD->getType()->is()) { Diags.diagnose(VD->getLoc(), diag::note_other_self_capture); } diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index b94bb14003ca5..c47d6bf6eb69c 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -189,8 +189,8 @@ class ExplicitSelfRequiredTest { doVoidStuff { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} doVoidStuff { [y = self] in _ = method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' 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}} {{37-37=self.}} doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in _ = method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} - doStuff({ [weak self] in method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} + doVoidStuff({ [weak self] in _ = method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doStuff({ [weak self] in method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} doVoidStuff({ [self = self.x] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} doStuff({ [self = self.x] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} doVoidStuff { _ = self.method() } @@ -711,6 +711,7 @@ public class TestImplicitCaptureOfExplicitCaptureOfSelfInEscapingClosure { } public class TestImplicitSelfForWeakSelfCapture { + static var staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() func method() { } private init() { @@ -724,5 +725,17 @@ public class TestImplicitSelfForWeakSelfCapture { method() } } + + doVoidStuff { [weak self] in + guard let self = self else { return } + doVoidStuff { + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + } + + doVoidStuff { [weak self] in + guard let self = TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } } } From e21b4d7322aefb274c33a2bbab27de4b7db7aaba Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sat, 25 Dec 2021 10:21:29 -0800 Subject: [PATCH 10/37] Reject normal ValueDecls but allow ConstructorDecl from '@autoclosure @escaping () -> String = String()' --- lib/Sema/MiscDiagnostics.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index d1d35e7506076..b7109522b7b57 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1520,6 +1520,12 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!ty->hasReferenceSemantics()) return false; + // If this is a `ConstructorDecl`, like as a default @autoclosure in + // `@autoclosure @escaping () -> String = String()`, then the capture is fine + if (auto constructorDecl = dyn_cast(DRE->getDecl())) { + return false; + } + auto isExplicitWeakSelfCapture = false; if (auto closureExpr = dyn_cast(inClosure)) { if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { @@ -1582,9 +1588,6 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, } } - if (auto value = dyn_cast(DRE->getDecl())) - return false; - return true; } From 66c89f1ab106637444a64744b5f91785bf9ff3bb Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sat, 25 Dec 2021 10:49:29 -0800 Subject: [PATCH 11/37] More test fixes --- test/expr/closure/closures.swift | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index c47d6bf6eb69c..9bcf0a7b4e8f6 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -177,10 +177,10 @@ class ExplicitSelfRequiredTest { 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.}} doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in x += 1 }) // expected-warning {{variable 'self' was written to, but never read}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self?.' explicitly}} - doStuff({ [weak self] in x+1 }) // expected-warning {{variable 'self' was written to, but never read}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doVoidStuff({ [self = self.x] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [self = self.x] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doStuff({ [self = .init()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} // Methods follow the same rules as properties, uses of 'self' without capturing must be marked with "self." doStuff { method() } // expected-error {{call to method 'method' 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.}} @@ -191,8 +191,8 @@ class ExplicitSelfRequiredTest { doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} doVoidStuff({ [weak self] in _ = method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} doStuff({ [weak self] in method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doVoidStuff({ [self = self.x] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [self = self.x] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} doVoidStuff { _ = self.method() } doVoidStuff { [self] in _ = method() } doVoidStuff { [self = self] in _ = method() } From 6c682b719e6a3b113b95d1185460abd05ea035f5 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sat, 25 Dec 2021 13:57:27 -0800 Subject: [PATCH 12/37] Fix another test failure, from a defer FuncDecl --- lib/Sema/MiscDiagnostics.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index b7109522b7b57..1382390cc38dd 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1520,9 +1520,10 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!ty->hasReferenceSemantics()) return false; - // If this is a `ConstructorDecl`, like as a default @autoclosure in - // `@autoclosure @escaping () -> String = String()`, then the capture is fine - if (auto constructorDecl = dyn_cast(DRE->getDecl())) { + // If this is a `AbstractFunctionDecl` (`FuncDecl`, `ConstructorDecl`, or `DestructorDecl`. + // `@autoclosure @escaping () -> String = String()` as one example), + // then the capture is fine. + if (isa(DRE->getDecl())) { return false; } From 6fe7dc517ee44dbdcd4944362110ae24be40d233 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sat, 25 Dec 2021 17:14:35 -0800 Subject: [PATCH 13/37] Always emit an error for invalid inner closures trying to use implicit self from an outer closure --- lib/Sema/MiscDiagnostics.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 1382390cc38dd..8920c88e0e5e1 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1641,13 +1641,17 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // Until Swift 6, only emit a warning when we get this with an // explicit capture, since we used to not diagnose this at all. - // FIXME: We should always emit an error here for weak captures, - // which don't have the same implicit self source compat requirements - // as strong captures. auto shouldOnlyWarn = [&](Expr *selfRef) { if (auto declRef = dyn_cast(selfRef)) { if (auto decl = declRef->getDecl()) { if (auto varDecl = dyn_cast(decl)) { + // If the self decl was defined in an `if` or `guard` statement, we know this is an inner closure of some outer closure's `weak self` capture. Since this wasn't allowed in Swift 5.5, we should just always emit an error. + if (auto parentStmt = varDecl->getParentPatternStmt()) { + if (isa(parentStmt) || isa(parentStmt)) { + return false; + } + } + return !varDecl->isSelfParameter(); } } From c6b4a200e1e829da99f0927b5c089735f4132c9e Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sat, 25 Dec 2021 20:29:36 -0800 Subject: [PATCH 14/37] Make more tests pass --- test/expr/closure/closures.swift | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 9bcf0a7b4e8f6..fd17244ceee50 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -175,24 +175,24 @@ class ExplicitSelfRequiredTest { 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.}} - doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} + doVoidStuff({ [y = self] in 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}} {{20-20=self, }} expected-note{{reference 'self.' explicitly}} {{33-33=self.}} + doStuff({ [y = self] in 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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doVoidStuff({ [self = self.x] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [self = .init()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} + doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} // Methods follow the same rules as properties, uses of 'self' without capturing must be marked with "self." doStuff { method() } // expected-error {{call to method 'method' 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 { _ = method() } // expected-error {{call to method 'method' 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}} {{23-23=self.}} doVoidStuff { _ = "\(method())" } // expected-error {{call to method 'method' 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 { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} - doVoidStuff { [y = self] in _ = method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' 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}} {{37-37=self.}} - doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in _ = method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doStuff({ [weak self] in method() }) // expected-note {{weak capture of 'self' here does not enable implicit 'self'}} expected-warning {{variable 'self' was written to, but never read}} expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff { [y = self] in _ = method() } // expected-error {{call to method 'method' 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}} {{37-37=self.}} + doStuff({ [y = self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} + doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} doVoidStuff { _ = self.method() } doVoidStuff { [self] in _ = method() } doVoidStuff { [self = self] in _ = method() } @@ -246,9 +246,9 @@ class ExplicitSelfRequiredTest { // If we already have a capture list, self should be added to the list let y = 1 - doStuff { [y] in method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{22-22=self.}} + doStuff { [y] in method() } // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{22-22=self.}} doStuff { [ // expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} - y // expected-warning {{capture 'y' was never used}} + y ] in method() } // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{14-14=self.}} // "self." shouldn't be required in the initializer expression in a capture list @@ -257,7 +257,6 @@ class ExplicitSelfRequiredTest { // This should produce an error, since x is used within the inner closure. doStuff({ [myX = {x}] in 4 }) // 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}} {{23-23= [self] in }} expected-note{{reference 'self.' explicitly}} {{23-23=self.}} - // expected-warning @-1 {{capture 'myX' was never used}} return 42 } @@ -728,13 +727,13 @@ public class TestImplicitSelfForWeakSelfCapture { doVoidStuff { [weak self] in guard let self = self else { return } - doVoidStuff { - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} } } doVoidStuff { [weak self] in - guard let self = TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } } From 0cd9f1957267bd1f10a005f11da7ac7aa0459dda Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sat, 25 Dec 2021 20:36:57 -0800 Subject: [PATCH 15/37] Fix issue where 'let = MyCls()' in function body would unexpectedly permit implicit self --- lib/Sema/PreCheckExpr.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index 5ff4461ae3d6f..d93b46e07bb98 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -744,13 +744,15 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, BaseExpr = TypeExpr::createImplicitForDecl( UDRE->getNameLoc(), NTD, BaseDC, DC->mapTypeIntoContext(NTD->getInterfaceType())); - } else if (Base->getBaseIdentifier().is("self")) { + } else if (Base->getBaseIdentifier().is("self") && Base->getDeclContext()->getContextKind() == DeclContextKind::AbstractClosureExpr) { // If this is an implicit self call, we don't actually // know if self is Self or Optional (e.g. in a weak self // closure before self is unwrapped). // - Since we don't know the correct type of self yet, // we leave this as an UnresolvedDeclRefExpr that is // populated with the actual type later. + // - We only do this within a closure context, since that's + // the only place where self can be rebound. auto selfNameRef = new DeclNameRef(Base->getBaseIdentifier()); BaseExpr = new (Context) UnresolvedDeclRefExpr(*selfNameRef, DeclRefKind::Ordinary, From 0a5e4e039f6c66952a0528430c9ba922b15b6a10 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 26 Dec 2021 08:47:01 -0800 Subject: [PATCH 16/37] Fix incorrect DeclContext check --- lib/Sema/PreCheckExpr.cpp | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index d93b46e07bb98..a53bf7a121a4a 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -744,23 +744,30 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, BaseExpr = TypeExpr::createImplicitForDecl( UDRE->getNameLoc(), NTD, BaseDC, DC->mapTypeIntoContext(NTD->getInterfaceType())); - } else if (Base->getBaseIdentifier().is("self") && Base->getDeclContext()->getContextKind() == DeclContextKind::AbstractClosureExpr) { - // If this is an implicit self call, we don't actually - // know if self is Self or Optional (e.g. in a weak self - // closure before self is unwrapped). + } else { + // If this is an implicit self reference, the `VarDecl` will + // currently have a non-optional type. But at this point, + // we don't actually know if this is corret (it could be a + // weak self capture that hasn't been unwrapped yet). // - Since we don't know the correct type of self yet, // we leave this as an UnresolvedDeclRefExpr that is // populated with the actual type later. - // - We only do this within a closure context, since that's - // the only place where self can be rebound. - auto selfNameRef = new DeclNameRef(Base->getBaseIdentifier()); - BaseExpr = new (Context) UnresolvedDeclRefExpr(*selfNameRef, - DeclRefKind::Ordinary, - UDRE->getNameLoc()); - BaseExpr->setImplicit(); - } else { - BaseExpr = new (Context) DeclRefExpr(Base, UDRE->getNameLoc(), - /*Implicit=*/true); + bool isClosureImplicitSelfParameter = false; + if (DC->getContextKind() == DeclContextKind::AbstractClosureExpr) + if (auto varDecl = dyn_cast(Base)) + if (varDecl->isSelfParameter()) + isClosureImplicitSelfParameter = true; + + if (isClosureImplicitSelfParameter) { + auto selfNameRef = new DeclNameRef(Base->getBaseIdentifier()); + BaseExpr = new (Context) UnresolvedDeclRefExpr(*selfNameRef, + DeclRefKind::Ordinary, + UDRE->getNameLoc()); + BaseExpr->setImplicit(); + } else { + BaseExpr = new (Context) DeclRefExpr(Base, UDRE->getNameLoc(), + /*Implicit=*/true); + } } auto isInClosureContext = [&](ValueDecl *decl) -> bool { From 20ed27a5c62b6b0aa0939891dda013d418c3e3f6 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 26 Dec 2021 12:34:27 -0800 Subject: [PATCH 17/37] Tests pass, except for 'capture 'y' was never used' issue --- test/expr/closure/closures.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index fd17244ceee50..41224a0b702aa 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -175,9 +175,9 @@ class ExplicitSelfRequiredTest { 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-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.}} - doStuff({ [y = self] in 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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + 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.}} + doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} expected-warning {{variable 'self' was written to, but never read}} doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} @@ -187,7 +187,7 @@ class ExplicitSelfRequiredTest { doVoidStuff { _ = method() } // expected-error {{call to method 'method' 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}} {{23-23=self.}} doVoidStuff { _ = "\(method())" } // expected-error {{call to method 'method' 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 { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} - doVoidStuff { [y = self] in _ = method() } // expected-error {{call to method 'method' 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}} {{37-37=self.}} + doVoidStuff { [y = self] in _ = method() } // expectedexpected-error {{call to method 'method' 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}} {{37-37=self.}} doStuff({ [y = self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} From c03950b52eb9e3ba51461116405988e13b17e808 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 26 Dec 2021 14:09:57 -0800 Subject: [PATCH 18/37] Use LabeledConditionalStmt instead of just IfStmt / GuardStmt --- lib/Sema/MiscDiagnostics.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 8920c88e0e5e1..3520c1fecf4d1 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1548,10 +1548,10 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (auto var = dyn_cast(DRE->getDecl())) { if (auto parentStmt = var->getParentPatternStmt()) { - if (isa(parentStmt) || isa(parentStmt)) { + if (isa(parentStmt)) { // If this `self` decl was not captured explicitly by this closure, // but is actually from an outer `weak` capture's `if let self = self` - // or `guard let self = self`, then we don't allow implicit self. + // or `guard let self = self` etc, then we don't allow implicit self. if (!isExplicitWeakSelfCapture) { return true; } @@ -1645,9 +1645,11 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (auto declRef = dyn_cast(selfRef)) { if (auto decl = declRef->getDecl()) { if (auto varDecl = dyn_cast(decl)) { - // If the self decl was defined in an `if` or `guard` statement, we know this is an inner closure of some outer closure's `weak self` capture. Since this wasn't allowed in Swift 5.5, we should just always emit an error. + // If the self decl was defined in an conditional binding in an `if`/`guard`/`while`, + // then we know this is an inner closure of some outer closure's `weak self` capture. + // Since this wasn't allowed in Swift 5.5, we should just always emit an error. if (auto parentStmt = varDecl->getParentPatternStmt()) { - if (isa(parentStmt) || isa(parentStmt)) { + if (isa(parentStmt)) { return false; } } From ccdf807211696f8d0bb7fbadf699a7a274e57a9b Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 26 Dec 2021 14:14:31 -0800 Subject: [PATCH 19/37] Move weak self errors to their own file --- test/expr/closure/closures.swift | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 41224a0b702aa..0793d5540fd06 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -177,8 +177,6 @@ class ExplicitSelfRequiredTest { 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.}} doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} expected-warning {{variable 'self' was written to, but never read}} - doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} @@ -189,8 +187,6 @@ class ExplicitSelfRequiredTest { doVoidStuff { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} doVoidStuff { [y = self] in _ = method() } // expectedexpected-error {{call to method 'method' 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}} {{37-37=self.}} doStuff({ [y = self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} doVoidStuff { _ = self.method() } @@ -228,7 +224,7 @@ class ExplicitSelfRequiredTest { } // expected-note@+2 {{capture 'self' explicitly to enable implicit 'self' in this closure}} {{14-14= [self] in}} // Note: Trailing whitespace on the following line is intentional and should not be removed! - doStuff { + doStuff { method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{7-7=self.}} } // expected-note@+1 {{capture 'self' explicitly to enable implicit 'self' in this closure}} {{14-14= [self] in}} @@ -260,6 +256,16 @@ class ExplicitSelfRequiredTest { return 42 } + + // The error emitted by these cases cause `VarDeclUsageChecker` to not run analysis on this method, + // because its `sawError` flag is set to true. To preserve the "capture 'y' was never used" warnings + // above, we put these cases in their own method. + func weakSelfError() { + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} expected-warning {{variable 'self' was written to, but never read}} + doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + } } // If the implicit self is of value type, no diagnostic should be produced. From 369e20f64c05264c6a36aeb05a15b7655a09560a Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 26 Dec 2021 14:29:38 -0800 Subject: [PATCH 20/37] Put back unused capture warnings --- test/expr/closure/closures.swift | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 0793d5540fd06..ba75603f60f1f 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -185,8 +185,8 @@ class ExplicitSelfRequiredTest { doVoidStuff { _ = method() } // expected-error {{call to method 'method' 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}} {{23-23=self.}} doVoidStuff { _ = "\(method())" } // expected-error {{call to method 'method' 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 { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} - doVoidStuff { [y = self] in _ = method() } // expectedexpected-error {{call to method 'method' 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}} {{37-37=self.}} - doStuff({ [y = self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} + doVoidStuff { [y = self] in _ = method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' 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}} {{37-37=self.}} + doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} doVoidStuff { _ = self.method() } @@ -242,9 +242,9 @@ class ExplicitSelfRequiredTest { // If we already have a capture list, self should be added to the list let y = 1 - doStuff { [y] in method() } // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{22-22=self.}} + doStuff { [y] in method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{22-22=self.}} doStuff { [ // expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} - y + y // expected-warning {{capture 'y' was never used}} ] in method() } // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}} {{14-14=self.}} // "self." shouldn't be required in the initializer expression in a capture list @@ -253,6 +253,7 @@ class ExplicitSelfRequiredTest { // This should produce an error, since x is used within the inner closure. doStuff({ [myX = {x}] in 4 }) // 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}} {{23-23= [self] in }} expected-note{{reference 'self.' explicitly}} {{23-23=self.}} + // expected-warning @-1 {{capture 'myX' was never used}} return 42 } @@ -261,7 +262,7 @@ class ExplicitSelfRequiredTest { // because its `sawError` flag is set to true. To preserve the "capture 'y' was never used" warnings // above, we put these cases in their own method. func weakSelfError() { - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} @@ -625,7 +626,7 @@ func callitArgsFn(_ : Int, _ f: () -> () -> T) -> T { f()() } -func callitGenericArg(_ a: T, _ f: () -> T) -> T { +func callitGenericArg(_ a: T, _ f: () -> T) -> T { f() } @@ -641,7 +642,7 @@ func testSR13239_Tuple() -> Int { // expected-error@+2{{conflicting arguments to generic parameter 'T' ('()' vs. 'Int')}} // expected-note@+1:3{{generic parameter 'T' inferred as 'Int' from context}} callitTuple(1) { // expected-note@:18{{generic parameter 'T' inferred as '()' from closure return expression}} - (print("hello"), 0) + (print("hello"), 0) } } @@ -657,7 +658,7 @@ func testSR13239_Args() -> Int { // expected-error@+2{{conflicting arguments to generic parameter 'T' ('()' vs. 'Int')}} // expected-note@+1:3{{generic parameter 'T' inferred as 'Int' from context}} callitArgs(1) { // expected-note@:17{{generic parameter 'T' inferred as '()' from closure return expression}} - print("hello") + print("hello") } } @@ -665,13 +666,13 @@ func testSR13239_ArgsFn() -> Int { // expected-error@+2{{conflicting arguments to generic parameter 'T' ('()' vs. 'Int')}} // expected-note@+1:3{{generic parameter 'T' inferred as 'Int' from context}} callitArgsFn(1) { // expected-note@:19{{generic parameter 'T' inferred as '()' from closure return expression}} - { print("hello") } + { print("hello") } } } func testSR13239MultiExpr() -> Int { callit { - print("hello") + print("hello") return print("hello") // expected-error {{cannot convert return expression of type '()' to return type 'Int'}} } } From 0f336368eafea5f26d725b89988ec1a36c7836c9 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 26 Dec 2021 14:32:34 -0800 Subject: [PATCH 21/37] Update comments --- lib/Sema/MiscDiagnostics.cpp | 5 +++-- lib/Sema/PreCheckExpr.cpp | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 3520c1fecf4d1..222a0df9fd369 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1530,9 +1530,10 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, auto isExplicitWeakSelfCapture = false; if (auto closureExpr = dyn_cast(inClosure)) { if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { - // If `weak self` captured explicitly, then implicit self + // If `weak self` was captured explicitly, then implicit self // is always allowed allowed once `self` is unwrapped. - // - If `self` is still an Optional, compilation would have failed already. + // - If `self` is still an Optional, compilation would have failed already, + // so we don't need to check for that here. if (selfDecl->getType()->is()) { isExplicitWeakSelfCapture = true; } diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index a53bf7a121a4a..c9982b48ee927 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -747,7 +747,7 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, } else { // If this is an implicit self reference, the `VarDecl` will // currently have a non-optional type. But at this point, - // we don't actually know if this is corret (it could be a + // we don't actually know if this is correct (it could be a // weak self capture that hasn't been unwrapped yet). // - Since we don't know the correct type of self yet, // we leave this as an UnresolvedDeclRefExpr that is From 367b1120ab3f86ff7c03603afab98fcf8d6ffba3 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 26 Dec 2021 20:48:35 -0800 Subject: [PATCH 22/37] Fix last test failure --- lib/Sema/MiscDiagnostics.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 222a0df9fd369..5130326e90032 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1520,13 +1520,20 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!ty->hasReferenceSemantics()) return false; - // If this is a `AbstractFunctionDecl` (`FuncDecl`, `ConstructorDecl`, or `DestructorDecl`. - // `@autoclosure @escaping () -> String = String()` as one example), - // then the capture is fine. + // If this is an implicit self parameter to a `AbstractFunctionDecl` + // (`FuncDecl`, `ConstructorDecl`, or `DestructorDecl`, + // `@autoclosure @escaping () -> String = String()` as one example) + // then it isn't actually capturing the closure's 'self', and is fine. if (isa(DRE->getDecl())) { return false; } + // If this decl isn't named "self", then it isn't an implicit self capture + // and we have no reason to reject it. + if (!DRE->getDecl()->getBaseName().getIdentifier().is("self")) { + return false; + } + auto isExplicitWeakSelfCapture = false; if (auto closureExpr = dyn_cast(inClosure)) { if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { From 84896edaf26ef72bf9f86a25fd2adea09a9b1db7 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Thu, 15 Sep 2022 09:23:15 -0700 Subject: [PATCH 23/37] Use Ctx.Id_self and update comments --- lib/Sema/CSDiagnostics.cpp | 4 +++- lib/Sema/MiscDiagnostics.cpp | 7 ++++--- lib/Sema/PreCheckExpr.cpp | 12 +++++------- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp index 4fc473d90f0f4..6ee88bef91f21 100644 --- a/lib/Sema/CSDiagnostics.cpp +++ b/lib/Sema/CSDiagnostics.cpp @@ -1376,7 +1376,9 @@ bool MemberAccessOnOptionalBaseFailure::diagnoseAsError() { if (auto dotExpr = getAsExpr(locator->getAnchor())) { if (auto base = dotExpr->getBase()) { if (auto baseDeclRef = dyn_cast(base)) { - if (baseDeclRef->isImplicit() && baseDeclRef->getDecl()->getBaseIdentifier().is("self")) { + ASTContext &Ctx = baseDeclRef->getDecl()->getASTContext(); + if (baseDeclRef->isImplicit() + && baseDeclRef->getDecl()->getName().isSimpleName(Ctx.Id_self)) { emitDiagnostic(diag::optional_self_not_unwrapped); emitDiagnostic(diag::optional_self_chain) diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 842cbba22f2eb..4c95fc9061e29 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1591,7 +1591,8 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // If this decl isn't named "self", then it isn't an implicit self capture // and we have no reason to reject it. - if (!DRE->getDecl()->getBaseName().getIdentifier().is("self")) { + ASTContext &Ctx = DRE->getDecl()->getASTContext(); + if (!DRE->getDecl()->getName().isSimpleName(Ctx.Id_self)) { return false; } @@ -1634,11 +1635,11 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (cond.getKind() == StmtConditionElement::CK_PatternBinding) { if (auto optionalPattern = dyn_cast(cond.getPattern())) { // if the lhs of the optional binding is `self`... - if (optionalPattern->getSubPattern()->getBoundName().is("self")) { + if (optionalPattern->getSubPattern()->getBoundName() == Ctx.Id_self) { if (auto loadExpr = dyn_cast(cond.getInitializer())) { if (auto declRefExpr = dyn_cast(loadExpr->getSubExpr())) { // and the rhs of the optional binding is `self`... - if (declRefExpr->getDecl()->getBaseIdentifier().is("self")) { + if (declRefExpr->getDecl()->getName().isSimpleName(Ctx.Id_self)) { // then we can permit implicit self in this closure hasCorrectSelfBindingCondition = true; } diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index 7aff120b02125..bedaa8712a2fa 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -761,13 +761,11 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, UDRE->getNameLoc(), NTD, BaseDC, DC->mapTypeIntoContext(NTD->getInterfaceType())); } else { - // If this is an implicit self reference, the `VarDecl` will - // currently have a non-optional type. But at this point, - // we don't actually know if this is correct (it could be a - // weak self capture that hasn't been unwrapped yet). - // - Since we don't know the correct type of self yet, - // we leave this as an UnresolvedDeclRefExpr that is - // populated with the actual type later. + // If this is an implicit self reference, we replace the `DeclRefExpr` + // with an `UnresolvedDeclRefExpr` to make the type checker take another + // pass on the expr. This causes `getParentPatternStmt()` to be populated, + // which is required later to validate conditions for permitting + // implicit self for `[weak self]` captures. bool isClosureImplicitSelfParameter = false; if (DC->getContextKind() == DeclContextKind::AbstractClosureExpr) if (auto varDecl = dyn_cast(Base)) From 9e9f7be0f6a3504ad59b1207664c916f1dd2a5e0 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Thu, 15 Sep 2022 19:57:00 -0700 Subject: [PATCH 24/37] Resolve UDRE to DRE in PreCheckExpr --- lib/Sema/PreCheckExpr.cpp | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index bedaa8712a2fa..9519a28f99f37 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -761,26 +761,29 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, UDRE->getNameLoc(), NTD, BaseDC, DC->mapTypeIntoContext(NTD->getInterfaceType())); } else { - // If this is an implicit self reference, we replace the `DeclRefExpr` - // with an `UnresolvedDeclRefExpr` to make the type checker take another - // pass on the expr. This causes `getParentPatternStmt()` to be populated, - // which is required later to validate conditions for permitting - // implicit self for `[weak self]` captures. + BaseExpr = new (Context) DeclRefExpr(Base, UDRE->getNameLoc(), + /*Implicit=*/true); + + // Implicit self references default to always being `ParamDecl`s + // that reference the function or closure's `self` param. + // This isn't necessarily the case though, if `self` has been rebound + // (e.g. unwrapping a closure's `weak self` capture via `guard let self`). + // To find the actual `VarDecl` that the base refers to, we resolve the + // `DeclRefExpr` for `self` and return that instead. bool isClosureImplicitSelfParameter = false; if (DC->getContextKind() == DeclContextKind::AbstractClosureExpr) if (auto varDecl = dyn_cast(Base)) if (varDecl->isSelfParameter()) isClosureImplicitSelfParameter = true; - + if (isClosureImplicitSelfParameter) { - auto selfNameRef = new DeclNameRef(Base->getBaseIdentifier()); - BaseExpr = new (Context) UnresolvedDeclRefExpr(*selfNameRef, - DeclRefKind::Ordinary, - UDRE->getNameLoc()); - BaseExpr->setImplicit(); - } else { - BaseExpr = new (Context) DeclRefExpr(Base, UDRE->getNameLoc(), - /*Implicit=*/true); + auto &ctx = DC->getASTContext(); + auto *unresolvedExpr = new (Context) UnresolvedDeclRefExpr(DeclNameRef(ctx.Id_self), + DeclRefKind::Ordinary, + UDRE->getNameLoc()); + unresolvedExpr->setImplicit(); + if (auto *resolvedExpr = resolveDeclRefExpr(unresolvedExpr, DC, replaceInvalidRefsWithErrors)) + BaseExpr = resolvedExpr; } } From 95949cec476d96be7458be8201f1a485387c40e7 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sat, 17 Sep 2022 14:20:37 -0700 Subject: [PATCH 25/37] Use ASTScope::lookupUnqualified instead of TypeChecker::resolveDeclRefExpr --- lib/Sema/PreCheckExpr.cpp | 43 ++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index 9519a28f99f37..b9135569e80bf 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -728,7 +728,22 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, ValueDecl *Base = nullptr; DeclContext *BaseDC = nullptr; for (auto Result : Lookup) { - auto ThisBase = Result.getBaseDecl(); + // Perform an unqualified lookup for the base decl. This handles cases + // where self was rebound (e.g. `guard let self = self`) earlier in this scope. + // Only do this in closures, since implicit self isn't allowed to be rebound + // in other contexts. + ValueDecl* ThisBase = nullptr; + if (DC->getContextKind() == DeclContextKind::AbstractClosureExpr) { + auto &ctx = DC->getASTContext(); + auto localDecl = ASTScope::lookupSingleLocalDecl(DC->getParentSourceFile(), + DeclName(ctx.Id_self), + UDRE->getLoc()); + if (localDecl) + ThisBase = localDecl; + } + + if (!ThisBase) + ThisBase = Result.getBaseDecl(); // Track the base for member declarations. if (ThisBase && !isa(ThisBase)) { @@ -763,28 +778,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, } else { BaseExpr = new (Context) DeclRefExpr(Base, UDRE->getNameLoc(), /*Implicit=*/true); - - // Implicit self references default to always being `ParamDecl`s - // that reference the function or closure's `self` param. - // This isn't necessarily the case though, if `self` has been rebound - // (e.g. unwrapping a closure's `weak self` capture via `guard let self`). - // To find the actual `VarDecl` that the base refers to, we resolve the - // `DeclRefExpr` for `self` and return that instead. - bool isClosureImplicitSelfParameter = false; - if (DC->getContextKind() == DeclContextKind::AbstractClosureExpr) - if (auto varDecl = dyn_cast(Base)) - if (varDecl->isSelfParameter()) - isClosureImplicitSelfParameter = true; - - if (isClosureImplicitSelfParameter) { - auto &ctx = DC->getASTContext(); - auto *unresolvedExpr = new (Context) UnresolvedDeclRefExpr(DeclNameRef(ctx.Id_self), - DeclRefKind::Ordinary, - UDRE->getNameLoc()); - unresolvedExpr->setImplicit(); - if (auto *resolvedExpr = resolveDeclRefExpr(unresolvedExpr, DC, replaceInvalidRefsWithErrors)) - BaseExpr = resolvedExpr; - } } auto isInClosureContext = [&](ValueDecl *decl) -> bool { @@ -878,14 +871,14 @@ namespace { /// Update a direct callee expression node that has a function reference kind /// based on seeing a call to this callee. templategetFunctionRefKind())> + typename = decltype(((E*)nullptr)->getFunctionRefKind())> void tryUpdateDirectCalleeImpl(E *callee, int) { callee->setFunctionRefKind(addingDirectCall(callee->getFunctionRefKind())); } /// Version of tryUpdateDirectCalleeImpl for when the callee /// expression type doesn't carry a reference. - template + template void tryUpdateDirectCalleeImpl(E *callee, ...) { } /// The given expression is the direct callee of a call expression; mark it to From 03322bf716d94ca2590a915bd2efdd93646808a6 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Mon, 19 Sep 2022 21:09:28 -0700 Subject: [PATCH 26/37] Move implicit self lookup from resolveDeclRefExpr into ASTScope::unqualifiedLookup implementation --- include/swift/AST/NameLookup.h | 9 ++++++--- lib/AST/NameLookup.cpp | 3 +++ lib/AST/UnqualifiedLookup.cpp | 18 ++++++++++++++++-- lib/Sema/PreCheckExpr.cpp | 17 +---------------- lib/Sema/TypeCheckNameLookup.cpp | 11 +++++++---- 5 files changed, 33 insertions(+), 25 deletions(-) diff --git a/include/swift/AST/NameLookup.h b/include/swift/AST/NameLookup.h index d8b9bfe306958..4d1a233ff33b4 100644 --- a/include/swift/AST/NameLookup.h +++ b/include/swift/AST/NameLookup.h @@ -103,15 +103,18 @@ struct LookupResultEntry { /// extension (if it found something at that level). DeclContext *BaseDC; + /// The declaration that defines the base of the call to `Value` + ValueDecl *BaseDecl; + /// The declaration corresponds to the given name; i.e. the decl we are /// looking up. ValueDecl *Value; public: - LookupResultEntry(ValueDecl *value) : BaseDC(nullptr), Value(value) {} + LookupResultEntry(ValueDecl *value) : BaseDC(nullptr), BaseDecl(nullptr), Value(value) {} - LookupResultEntry(DeclContext *baseDC, ValueDecl *value) - : BaseDC(baseDC), Value(value) {} + LookupResultEntry(DeclContext *baseDC, ValueDecl *baseDecl, ValueDecl *value) + : BaseDC(baseDC), BaseDecl(baseDecl), Value(value) {} ValueDecl *getValueDecl() const { return Value; } diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp index c888081addbb9..fc58c2b5afe26 100644 --- a/lib/AST/NameLookup.cpp +++ b/lib/AST/NameLookup.cpp @@ -55,6 +55,9 @@ void VectorDeclConsumer::anchor() {} ValueDecl *LookupResultEntry::getBaseDecl() const { if (BaseDC == nullptr) return nullptr; + + if (BaseDecl) + return BaseDecl; if (auto *AFD = dyn_cast(BaseDC)) return AFD->getImplicitSelfDecl(); diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp index 207a2338bf1f5..f37e71cc1a740 100644 --- a/lib/AST/UnqualifiedLookup.cpp +++ b/lib/AST/UnqualifiedLookup.cpp @@ -343,8 +343,22 @@ void UnqualifiedLookupFactory::ResultFinderForTypeContext::findResults( SmallVector Lookup; contextForLookup->lookupQualified(selfBounds, Name, baseNLOptions, Lookup); for (auto Result : Lookup) { - results.emplace_back(const_cast(whereValueIsMember(Result)), - Result); + auto baseDC = const_cast(whereValueIsMember(Result)); + + // Perform an unqualified lookup for the base decl of this result. This handles cases + // where self was rebound (e.g. `guard let self = self`) earlier in the scope. + // Only do this in closures, since implicit self isn't allowed to be rebound + // in other contexts. Otherwise, we use the `ParamDecl` from `baseDC` if applicable. + ValueDecl* localBaseDecl = nullptr; + if (factory->DC->getContextKind() == DeclContextKind::AbstractClosureExpr && baseDC) { + auto *localDecl = ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), + DeclName(factory->Ctx.Id_self), + factory->Loc); + if (localDecl) + localBaseDecl = localDecl; + } + + results.emplace_back(baseDC, localBaseDecl, Result); #ifndef NDEBUG factory->addedResult(results.back()); #endif diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp index b9135569e80bf..24e2ec469cd75 100644 --- a/lib/Sema/PreCheckExpr.cpp +++ b/lib/Sema/PreCheckExpr.cpp @@ -728,22 +728,7 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, ValueDecl *Base = nullptr; DeclContext *BaseDC = nullptr; for (auto Result : Lookup) { - // Perform an unqualified lookup for the base decl. This handles cases - // where self was rebound (e.g. `guard let self = self`) earlier in this scope. - // Only do this in closures, since implicit self isn't allowed to be rebound - // in other contexts. - ValueDecl* ThisBase = nullptr; - if (DC->getContextKind() == DeclContextKind::AbstractClosureExpr) { - auto &ctx = DC->getASTContext(); - auto localDecl = ASTScope::lookupSingleLocalDecl(DC->getParentSourceFile(), - DeclName(ctx.Id_self), - UDRE->getLoc()); - if (localDecl) - ThisBase = localDecl; - } - - if (!ThisBase) - ThisBase = Result.getBaseDecl(); + auto ThisBase = Result.getBaseDecl(); // Track the base for member declarations. if (ThisBase && !isa(ThisBase)) { diff --git a/lib/Sema/TypeCheckNameLookup.cpp b/lib/Sema/TypeCheckNameLookup.cpp index a957da09715e1..8c4e5a4e93efd 100644 --- a/lib/Sema/TypeCheckNameLookup.cpp +++ b/lib/Sema/TypeCheckNameLookup.cpp @@ -92,18 +92,21 @@ namespace { /// \param baseDC The declaration context through which we found the /// declaration. /// + /// \param baseDecl The declaration that defines the base of the + /// call to `found` + /// /// \param foundInType The type through which we found the /// declaration. /// /// \param isOuter Whether this is an outer result (i.e. a result that isn't /// from the innermost scope with results) - void add(ValueDecl *found, DeclContext *baseDC, Type foundInType, + void add(ValueDecl *found, DeclContext *baseDC, ValueDecl *baseDecl, Type foundInType, bool isOuter) { DeclContext *foundDC = found->getDeclContext(); auto addResult = [&](ValueDecl *result) { if (Known.insert({{result, baseDC}, false}).second) { - Result.add(LookupResultEntry(baseDC, result), isOuter); + Result.add(LookupResultEntry(baseDC, baseDecl, result), isOuter); if (isOuter) FoundOuterDecls.push_back(result); else @@ -267,7 +270,7 @@ LookupResult TypeChecker::lookupUnqualified(DeclContext *dc, DeclNameRef name, assert(foundInType && "bogus base declaration?"); } - builder.add(found.getValueDecl(), found.getDeclContext(), foundInType, + builder.add(found.getValueDecl(), found.getDeclContext(), found.getBaseDecl(), foundInType, /*isOuter=*/idx >= lookup.getIndexOfFirstOuterResult()); } return result; @@ -327,7 +330,7 @@ LookupResult TypeChecker::lookupMember(DeclContext *dc, dc->lookupQualified(type, name, subOptions, lookupResults); for (auto found : lookupResults) - builder.add(found, nullptr, type, /*isOuter=*/false); + builder.add(found, nullptr, nullptr, type, /*isOuter=*/false); return result; } From 9dd56f9daff2f9b51143aa3ea363326efcad7869 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Thu, 22 Sep 2022 09:34:56 -0700 Subject: [PATCH 27/37] Move remaining logic in LookupResultEntry::getBaseDecl() to ASTScope::lookupUnqualified impl, add more extensive tests, fix failing tests --- include/swift/AST/NameLookup.h | 3 +- lib/AST/NameLookup.cpp | 23 +---- lib/AST/UnqualifiedLookup.cpp | 58 ++++++++++-- lib/Sema/MiscDiagnostics.cpp | 155 ++++++++++++++----------------- test/expr/closure/closures.swift | 78 +++++++++++++++- 5 files changed, 194 insertions(+), 123 deletions(-) diff --git a/include/swift/AST/NameLookup.h b/include/swift/AST/NameLookup.h index 4d1a233ff33b4..9cb748da7f407 100644 --- a/include/swift/AST/NameLookup.h +++ b/include/swift/AST/NameLookup.h @@ -103,7 +103,8 @@ struct LookupResultEntry { /// extension (if it found something at that level). DeclContext *BaseDC; - /// The declaration that defines the base of the call to `Value` + /// The declaration that defines the base of the call to `Value`. + /// This is always available, as long as `BaseDC` is not null. ValueDecl *BaseDecl; /// The declaration corresponds to the given name; i.e. the decl we are diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp index fc58c2b5afe26..41e1d2174005c 100644 --- a/lib/AST/NameLookup.cpp +++ b/lib/AST/NameLookup.cpp @@ -56,28 +56,7 @@ ValueDecl *LookupResultEntry::getBaseDecl() const { if (BaseDC == nullptr) return nullptr; - if (BaseDecl) - return BaseDecl; - - if (auto *AFD = dyn_cast(BaseDC)) - return AFD->getImplicitSelfDecl(); - - if (auto *PBI = dyn_cast(BaseDC)) { - auto *selfDecl = PBI->getImplicitSelfDecl(); - assert(selfDecl); - return selfDecl; - } - - if (auto *CE = dyn_cast(BaseDC)) { - auto *selfDecl = CE->getCapturedSelfDecl(); - assert(selfDecl); - assert(selfDecl->isSelfParamCapture()); - return selfDecl; - } - - auto *nominalDecl = BaseDC->getSelfNominalTypeDecl(); - assert(nominalDecl); - return nominalDecl; + return BaseDecl; } void LookupResult::filter( diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp index f37e71cc1a740..17b3074328d24 100644 --- a/lib/AST/UnqualifiedLookup.cpp +++ b/lib/AST/UnqualifiedLookup.cpp @@ -24,6 +24,7 @@ #include "swift/AST/NameLookupRequests.h" #include "swift/AST/PropertyWrappers.h" #include "swift/AST/SourceFile.h" +#include "swift/AST/Initializer.h" #include "swift/Basic/Debug.h" #include "swift/Basic/STLExtras.h" #include "swift/Basic/SourceManager.h" @@ -345,20 +346,59 @@ void UnqualifiedLookupFactory::ResultFinderForTypeContext::findResults( for (auto Result : Lookup) { auto baseDC = const_cast(whereValueIsMember(Result)); + // Retrieve the base decl for this lookup + ValueDecl* baseDecl; + // Perform an unqualified lookup for the base decl of this result. This handles cases // where self was rebound (e.g. `guard let self = self`) earlier in the scope. - // Only do this in closures, since implicit self isn't allowed to be rebound - // in other contexts. Otherwise, we use the `ParamDecl` from `baseDC` if applicable. + // - Only do this in closures that capture self weakly, since implicit self isn't + // allowed to be rebound in other contexts. In other contexts, implicit self + // _always_ refers to the context's self `ParamDecl`, even if there is another + // local decl with the name `self` that would be found by `lookupSingleLocalDecl`. ValueDecl* localBaseDecl = nullptr; - if (factory->DC->getContextKind() == DeclContextKind::AbstractClosureExpr && baseDC) { - auto *localDecl = ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), - DeclName(factory->Ctx.Id_self), - factory->Loc); - if (localDecl) - localBaseDecl = localDecl; + bool isInWeakSelfClosure = false; + if (auto closureExpr = dyn_cast(factory->DC)) { + if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { + auto *attr = selfDecl->getAttrs().getAttribute(); + isInWeakSelfClosure = attr->get() == ReferenceOwnership::Weak; + } + } + + if (isInWeakSelfClosure) { + localBaseDecl = ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), + DeclName(factory->Ctx.Id_self), + factory->Loc); + } + + if (baseDC == nullptr) + baseDecl = nullptr; + + else if (localBaseDecl) + baseDecl = localBaseDecl; + + else if (auto *AFD = dyn_cast(baseDC)) + baseDecl = AFD->getImplicitSelfDecl(); + + else if (auto *PBI = dyn_cast(baseDC)) { + auto *selfDecl = PBI->getImplicitSelfDecl(); + assert(selfDecl); + baseDecl = selfDecl; + } + + else if (auto *CE = dyn_cast(baseDC)) { + auto *selfDecl = CE->getCapturedSelfDecl(); + assert(selfDecl); + assert(selfDecl->isSelfParamCapture()); + baseDecl = selfDecl; + } + + else { + auto *nominalDecl = baseDC->getSelfNominalTypeDecl(); + assert(nominalDecl); + baseDecl = nominalDecl; } - results.emplace_back(baseDC, localBaseDecl, Result); + results.emplace_back(baseDC, baseDecl, Result); #ifndef NDEBUG factory->addedResult(results.back()); #endif diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 346dc37345c8f..f60fbae37663d 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1565,30 +1565,6 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!DRE || !DRE->isImplicit()) return false; - - // Defensive check for type. If the expression doesn't have type here, it - // should have been diagnosed somewhere else. - Type ty = DRE->getType(); - assert(ty && "Implicit self parameter ref without type"); - if (!ty) - return false; - - // Metatype self captures don't extend the lifetime of an object. - if (ty->is()) - return false; - - // If self does not have reference semantics, it is very unlikely that - // capturing it will create a reference cycle. - if (!ty->hasReferenceSemantics()) - return false; - - // If this is an implicit self parameter to a `AbstractFunctionDecl` - // (`FuncDecl`, `ConstructorDecl`, or `DestructorDecl`, - // `@autoclosure @escaping () -> String = String()` as one example) - // then it isn't actually capturing the closure's 'self', and is fine. - if (isa(DRE->getDecl())) { - return false; - } // If this decl isn't named "self", then it isn't an implicit self capture // and we have no reason to reject it. @@ -1600,66 +1576,68 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, auto isExplicitWeakSelfCapture = false; if (auto closureExpr = dyn_cast(inClosure)) { if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { - // If `weak self` was captured explicitly, then implicit self - // is always allowed allowed once `self` is unwrapped. - // - If `self` is still an Optional, compilation would have failed already, - // so we don't need to check for that here. if (selfDecl->getType()->is()) { isExplicitWeakSelfCapture = true; } - - // If this capture is using the name `self` actually referring - // to some other variable (e.g. with `[self = "hello"]`) - // then implicit self is not allowed. - else if (!selfDecl->isSelfParamCapture()) { - return true; - } } } - if (auto var = dyn_cast(DRE->getDecl())) { - if (auto parentStmt = var->getParentPatternStmt()) { - if (isa(parentStmt)) { - // If this `self` decl was not captured explicitly by this closure, - // but is actually from an outer `weak` capture's `if let self = self` - // or `guard let self = self` etc, then we don't allow implicit self. - if (!isExplicitWeakSelfCapture) { - return true; - } - - // If this is an explicit `weak self` capture, then we need - // to validate that the unwrapped `self` decl is actually - // referring to `self`, and not something else like in - // `guard let self = .someOptionalVariable else { return }` - auto hasCorrectSelfBindingCondition = false; - for (auto cond : dyn_cast(parentStmt)->getCond()) { - if (cond.getKind() == StmtConditionElement::CK_PatternBinding) { - if (auto optionalPattern = dyn_cast(cond.getPattern())) { + // If this is an explicit `weak self` capture, then implicit self is allowed + // once the closure's self param is unwrapped. We need to validate that the + // unwrapped `self` decl specifically refers to an unwrapped copy of the + // closure's `self` param, and not something else like in + // `guard let self = .someOptionalVariable else { return }` or + // `let self = someUnrelatedVariable`. If self hasn't been unwrapped yet + // and is still an optional, we would have already hit an error elsewhere. + if (isExplicitWeakSelfCapture) { + bool hasCorrectSelfBindingCondition = false; + if (auto var = dyn_cast(DRE->getDecl())) { + // if the `self` decls was defined in a `let`, `guard`, or `while` condition... + if (auto parentStmt = var->getParentPatternStmt()) + if (auto parentConditionalStmt = dyn_cast(parentStmt)) + for (auto cond : parentConditionalStmt->getCond()) + if (auto optionalPattern = dyn_cast_or_null(cond.getPattern())) // if the lhs of the optional binding is `self`... - if (optionalPattern->getSubPattern()->getBoundName() == Ctx.Id_self) { - if (auto loadExpr = dyn_cast(cond.getInitializer())) { - if (auto declRefExpr = dyn_cast(loadExpr->getSubExpr())) { + if (optionalPattern->getSubPattern()->getBoundName() == Ctx.Id_self) + if (auto loadExpr = dyn_cast(cond.getInitializer())) + if (auto declRefExpr = dyn_cast(loadExpr->getSubExpr())) // and the rhs of the optional binding is `self`... - if (declRefExpr->getDecl()->getName().isSimpleName(Ctx.Id_self)) { - // then we can permit implicit self in this closure + if (declRefExpr->getDecl()->getName().isSimpleName(Ctx.Id_self)) + // then we can permit implicit self in this case hasCorrectSelfBindingCondition = true; - } - } - } - } - } - } - } - - return !hasCorrectSelfBindingCondition; - } } - if (!isEnclosingSelfReference(var, inClosure)) { - return false; - } + return !hasCorrectSelfBindingCondition; } + // Defensive check for type. If the expression doesn't have type here, it + // should have been diagnosed somewhere else. + Type ty = DRE->getType(); + assert(ty && "Implicit self parameter ref without type"); + if (!ty) + return false; + + // Metatype self captures don't extend the lifetime of an object. + if (ty->is()) + return false; + + // If self does not have reference semantics, it is very unlikely that + // capturing it will create a reference cycle. + if (!ty->hasReferenceSemantics()) + return false; + + if (auto closureExpr = dyn_cast(inClosure)) + if (auto selfDecl = closureExpr->getCapturedSelfDecl()) + // If this capture is using the name `self` actually referring + // to some other variable (e.g. with `[self = "hello"]`) + // then implicit self is not allowed. + if (!selfDecl->isSelfParamCapture()) + return true; + + if (auto var = dyn_cast(DRE->getDecl())) + if (!isEnclosingSelfReference(var, inClosure)) + return false; + return true; } @@ -1667,6 +1645,13 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, /// use or capture of "self." for qualification of member references. static bool isClosureRequiringSelfQualification( const AbstractClosureExpr *CE) { + // If this closure capture self weakly, then we have to validate each usage + // of implicit self individually, even in a nonescaping closure + if (auto closureExpr = dyn_cast(CE)) + if (auto selfDecl = closureExpr->getCapturedSelfDecl()) + if (selfDecl->getType()->is()) + return true; + // If the closure's type was inferred to be noescape, then it doesn't // need qualification. if (AnyFunctionRef(const_cast(CE)) @@ -1713,23 +1698,19 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // 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) { - if (auto declRef = dyn_cast(selfRef)) { - if (auto decl = declRef->getDecl()) { - if (auto varDecl = dyn_cast(decl)) { - // If the self decl was defined in an conditional binding in an `if`/`guard`/`while`, - // then we know this is an inner closure of some outer closure's `weak self` capture. - // Since this wasn't allowed in Swift 5.5, we should just always emit an error. - if (auto parentStmt = varDecl->getParentPatternStmt()) { - if (isa(parentStmt)) { - return false; - } - } - + // If this implicit self decl is from a closure that captured self weakly, + // then we should always emit an error, since implicit self was only + // allowed starting in Swift 5.8 and later. + if (auto closureExpr = dyn_cast(ACE)) + if (auto selfDecl = closureExpr->getCapturedSelfDecl()) + if (selfDecl->getType()->is()) + return false; + + if (auto declRef = dyn_cast(selfRef)) + if (auto decl = declRef->getDecl()) + if (auto varDecl = dyn_cast(decl)) return !varDecl->isSelfParameter(); - } - } - } - + return false; }; diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 94471ac884b2e..622d7c65c21e2 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -176,8 +176,8 @@ class ExplicitSelfRequiredTest { 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.}} doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} - doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} + doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} // Methods follow the same rules as properties, uses of 'self' without capturing must be marked with "self." doStuff { method() } // expected-error {{call to method 'method' 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.}} @@ -186,8 +186,8 @@ class ExplicitSelfRequiredTest { doVoidStuff { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} doVoidStuff { [y = self] in _ = method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' 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}} {{37-37=self.}} doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} - doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit; this is an error in Swift 6}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} + doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} doVoidStuff { _ = self.method() } doVoidStuff { [self] in _ = method() } doVoidStuff { [self = self] in _ = method() } @@ -262,8 +262,10 @@ class ExplicitSelfRequiredTest { // above, we put these cases in their own method. func weakSelfError() { doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} } } @@ -745,6 +747,7 @@ public class TestImplicitSelfForWeakSelfCapture { private init() { doVoidStuff { [weak self] in + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} guard let self = self else { return } method() } @@ -766,6 +769,73 @@ public class TestImplicitSelfForWeakSelfCapture { guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } + + doVoidStuffNonEscaping { [weak self] in + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + guard let self = self else { return } + method() + } + + doVoidStuffNonEscaping { [weak self] in + if let self = self { + method() + } + } + + doVoidStuff { [weak self] in + let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional + guard let self = self else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional + guard let self = self else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + guard let self = self else { return } + doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} + } + } + + doVoidStuffNonEscaping { [weak self] in + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + } +} + +public class TestRebindingSelfIsDisallowed { + let count: Void = () + + private init() { + doVoidStuff { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } + + doVoidStuffNonEscaping { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } + + doVoidStuff { [weak self] in + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + } + + func method() { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} } } From 5946c66962d1b7237cb3731bded085a7e39d8bd1 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Thu, 22 Sep 2022 18:17:18 -0700 Subject: [PATCH 28/37] Style updates --- include/swift/AST/NameLookup.h | 7 +- lib/AST/NameLookup.cpp | 2 +- lib/AST/UnqualifiedLookup.cpp | 118 ++++++++++++----------- lib/Sema/CSDiagnostics.cpp | 22 ++--- lib/Sema/MiscDiagnostics.cpp | 155 +++++++++++++++++-------------- lib/Sema/TypeCheckNameLookup.cpp | 8 +- 6 files changed, 172 insertions(+), 140 deletions(-) diff --git a/include/swift/AST/NameLookup.h b/include/swift/AST/NameLookup.h index 9cb748da7f407..9133b0ba7d18a 100644 --- a/include/swift/AST/NameLookup.h +++ b/include/swift/AST/NameLookup.h @@ -106,16 +106,17 @@ struct LookupResultEntry { /// The declaration that defines the base of the call to `Value`. /// This is always available, as long as `BaseDC` is not null. ValueDecl *BaseDecl; - + /// The declaration corresponds to the given name; i.e. the decl we are /// looking up. ValueDecl *Value; public: - LookupResultEntry(ValueDecl *value) : BaseDC(nullptr), BaseDecl(nullptr), Value(value) {} + LookupResultEntry(ValueDecl *value) + : BaseDC(nullptr), BaseDecl(nullptr), Value(value) {} LookupResultEntry(DeclContext *baseDC, ValueDecl *baseDecl, ValueDecl *value) - : BaseDC(baseDC), BaseDecl(baseDecl), Value(value) {} + : BaseDC(baseDC), BaseDecl(baseDecl), Value(value) {} ValueDecl *getValueDecl() const { return Value; } diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp index 41e1d2174005c..9031e90e957aa 100644 --- a/lib/AST/NameLookup.cpp +++ b/lib/AST/NameLookup.cpp @@ -55,7 +55,7 @@ void VectorDeclConsumer::anchor() {} ValueDecl *LookupResultEntry::getBaseDecl() const { if (BaseDC == nullptr) return nullptr; - + return BaseDecl; } diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp index 17b3074328d24..e174bf7ac9fd6 100644 --- a/lib/AST/UnqualifiedLookup.cpp +++ b/lib/AST/UnqualifiedLookup.cpp @@ -19,12 +19,12 @@ #include "swift/AST/ASTVisitor.h" #include "swift/AST/DebuggerClient.h" #include "swift/AST/ImportCache.h" +#include "swift/AST/Initializer.h" #include "swift/AST/ModuleNameLookup.h" #include "swift/AST/NameLookup.h" #include "swift/AST/NameLookupRequests.h" #include "swift/AST/PropertyWrappers.h" #include "swift/AST/SourceFile.h" -#include "swift/AST/Initializer.h" #include "swift/Basic/Debug.h" #include "swift/Basic/STLExtras.h" #include "swift/Basic/SourceManager.h" @@ -77,6 +77,8 @@ namespace { private: SelfBounds findSelfBounds(const DeclContext *dc); + ValueDecl *lookupBaseDecl(const DeclContext *baseDC) const; + ValueDecl *getBaseDeclForResult(const DeclContext *baseDC) const; // Classify this declaration. // Types are formally members of the metatype. @@ -345,64 +347,74 @@ void UnqualifiedLookupFactory::ResultFinderForTypeContext::findResults( contextForLookup->lookupQualified(selfBounds, Name, baseNLOptions, Lookup); for (auto Result : Lookup) { auto baseDC = const_cast(whereValueIsMember(Result)); - - // Retrieve the base decl for this lookup - ValueDecl* baseDecl; - - // Perform an unqualified lookup for the base decl of this result. This handles cases - // where self was rebound (e.g. `guard let self = self`) earlier in the scope. - // - Only do this in closures that capture self weakly, since implicit self isn't - // allowed to be rebound in other contexts. In other contexts, implicit self - // _always_ refers to the context's self `ParamDecl`, even if there is another - // local decl with the name `self` that would be found by `lookupSingleLocalDecl`. - ValueDecl* localBaseDecl = nullptr; - bool isInWeakSelfClosure = false; - if (auto closureExpr = dyn_cast(factory->DC)) { - if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { - auto *attr = selfDecl->getAttrs().getAttribute(); - isInWeakSelfClosure = attr->get() == ReferenceOwnership::Weak; - } - } - - if (isInWeakSelfClosure) { - localBaseDecl = ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), - DeclName(factory->Ctx.Id_self), - factory->Loc); - } - - if (baseDC == nullptr) - baseDecl = nullptr; - - else if (localBaseDecl) - baseDecl = localBaseDecl; + auto baseDecl = getBaseDeclForResult(baseDC); + results.emplace_back(baseDC, baseDecl, Result); +#ifndef NDEBUG + factory->addedResult(results.back()); +#endif + } +} - else if (auto *AFD = dyn_cast(baseDC)) - baseDecl = AFD->getImplicitSelfDecl(); +ValueDecl * +UnqualifiedLookupFactory::ResultFinderForTypeContext::getBaseDeclForResult( + const DeclContext *baseDC) const { + if (baseDC == nullptr) { + return nullptr; + } - else if (auto *PBI = dyn_cast(baseDC)) { - auto *selfDecl = PBI->getImplicitSelfDecl(); - assert(selfDecl); - baseDecl = selfDecl; - } + if (auto localBaseDecl = lookupBaseDecl(baseDC)) { + return localBaseDecl; + } - else if (auto *CE = dyn_cast(baseDC)) { - auto *selfDecl = CE->getCapturedSelfDecl(); - assert(selfDecl); - assert(selfDecl->isSelfParamCapture()); - baseDecl = selfDecl; - } + if (auto *AFD = dyn_cast(baseDC)) { + return const_cast(AFD->getImplicitSelfDecl()); + } - else { - auto *nominalDecl = baseDC->getSelfNominalTypeDecl(); - assert(nominalDecl); - baseDecl = nominalDecl; + if (auto *PBI = dyn_cast(baseDC)) { + auto *selfDecl = PBI->getImplicitSelfDecl(); + assert(selfDecl); + return selfDecl; + } + + else if (auto *CE = dyn_cast(baseDC)) { + auto *selfDecl = CE->getCapturedSelfDecl(); + assert(selfDecl); + assert(selfDecl->isSelfParamCapture()); + return selfDecl; + } + + auto *nominalDecl = baseDC->getSelfNominalTypeDecl(); + assert(nominalDecl); + return nominalDecl; +} + +ValueDecl *UnqualifiedLookupFactory::ResultFinderForTypeContext::lookupBaseDecl( + const DeclContext *baseDC) const { + // Perform an unqualified lookup for the base decl of this result. This + // handles cases where self was rebound (e.g. `guard let self = self`) + // earlier in the scope. + // + // Only do this in closures that capture self weakly, since implicit self + // isn't allowed to be rebound in other contexts. In other contexts, implicit + // self _always_ refers to the context's self `ParamDecl`, even if there + // is another local decl with the name `self` that would be found by + // `lookupSingleLocalDecl`. + bool isInWeakSelfClosure = false; + if (auto closureExpr = dyn_cast(factory->DC)) { + if (auto decl = closureExpr->getCapturedSelfDecl()) { + if (auto a = decl->getAttrs().getAttribute()) { + isInWeakSelfClosure = a->get() == ReferenceOwnership::Weak; + } } - - results.emplace_back(baseDC, baseDecl, Result); -#ifndef NDEBUG - factory->addedResult(results.back()); -#endif } + + if (isInWeakSelfClosure) { + return ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), + DeclName(factory->Ctx.Id_self), + factory->Loc); + } + + return nullptr; } // TODO (someday): Instead of adding unavailable entries to Results, diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp index 6ee88bef91f21..2d382752d2869 100644 --- a/lib/Sema/CSDiagnostics.cpp +++ b/lib/Sema/CSDiagnostics.cpp @@ -1372,19 +1372,17 @@ bool MemberAccessOnOptionalBaseFailure::diagnoseAsError() { } } else { // Check whether or not the base of this optional unwrap is implicit self - // - This can only happen with a [weak self] capture, and is not permitted. + // This can only happen with a [weak self] capture, and is not permitted. if (auto dotExpr = getAsExpr(locator->getAnchor())) { - if (auto base = dotExpr->getBase()) { - if (auto baseDeclRef = dyn_cast(base)) { - ASTContext &Ctx = baseDeclRef->getDecl()->getASTContext(); - if (baseDeclRef->isImplicit() - && baseDeclRef->getDecl()->getName().isSimpleName(Ctx.Id_self)) { - emitDiagnostic(diag::optional_self_not_unwrapped); - - emitDiagnostic(diag::optional_self_chain) - .fixItInsertAfter(sourceRange.End, "self?."); - return true; - } + if (auto baseDeclRef = dyn_cast(dotExpr->getBase())) { + ASTContext &Ctx = baseDeclRef->getDecl()->getASTContext(); + if (baseDeclRef->isImplicit() && + baseDeclRef->getDecl()->getName().isSimpleName(Ctx.Id_self)) { + emitDiagnostic(diag::optional_self_not_unwrapped); + + emitDiagnostic(diag::optional_self_chain) + .fixItInsertAfter(sourceRange.End, "self?."); + return true; } } } diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index f60fbae37663d..9d90d75b00661 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1565,51 +1565,25 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (!DRE || !DRE->isImplicit()) return false; - + // If this decl isn't named "self", then it isn't an implicit self capture // and we have no reason to reject it. ASTContext &Ctx = DRE->getDecl()->getASTContext(); if (!DRE->getDecl()->getName().isSimpleName(Ctx.Id_self)) { return false; } - - auto isExplicitWeakSelfCapture = false; - if (auto closureExpr = dyn_cast(inClosure)) { - if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { - if (selfDecl->getType()->is()) { - isExplicitWeakSelfCapture = true; - } - } - } - - // If this is an explicit `weak self` capture, then implicit self is allowed - // once the closure's self param is unwrapped. We need to validate that the - // unwrapped `self` decl specifically refers to an unwrapped copy of the - // closure's `self` param, and not something else like in - // `guard let self = .someOptionalVariable else { return }` or - // `let self = someUnrelatedVariable`. If self hasn't been unwrapped yet - // and is still an optional, we would have already hit an error elsewhere. - if (isExplicitWeakSelfCapture) { - bool hasCorrectSelfBindingCondition = false; - if (auto var = dyn_cast(DRE->getDecl())) { - // if the `self` decls was defined in a `let`, `guard`, or `while` condition... - if (auto parentStmt = var->getParentPatternStmt()) - if (auto parentConditionalStmt = dyn_cast(parentStmt)) - for (auto cond : parentConditionalStmt->getCond()) - if (auto optionalPattern = dyn_cast_or_null(cond.getPattern())) - // if the lhs of the optional binding is `self`... - if (optionalPattern->getSubPattern()->getBoundName() == Ctx.Id_self) - if (auto loadExpr = dyn_cast(cond.getInitializer())) - if (auto declRefExpr = dyn_cast(loadExpr->getSubExpr())) - // and the rhs of the optional binding is `self`... - if (declRefExpr->getDecl()->getName().isSimpleName(Ctx.Id_self)) - // then we can permit implicit self in this case - hasCorrectSelfBindingCondition = true; - } - - return !hasCorrectSelfBindingCondition; + + // If this is an explicit `weak self` capture, then implicit self is + // allowed once the closure's self param is unwrapped. We need to validate + // that the unwrapped `self` decl specifically refers to an unwrapped copy + // of the closure's `self` param, and not something else like in `guard + // let self = .someOptionalVariable else { return }` or `let self = + // someUnrelatedVariable`. If self hasn't been unwrapped yet and is still + // an optional, we would have already hit an error elsewhere. + if (closureHasWeakSelfCapture(inClosure)) { + return !implicitWeakSelfReferenceIsValid(DRE); } - + // Defensive check for type. If the expression doesn't have type here, it // should have been diagnosed somewhere else. Type ty = DRE->getType(); @@ -1625,33 +1599,81 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // capturing it will create a reference cycle. if (!ty->hasReferenceSemantics()) return false; - - if (auto closureExpr = dyn_cast(inClosure)) - if (auto selfDecl = closureExpr->getCapturedSelfDecl()) + + if (auto closureExpr = dyn_cast(inClosure)) { + if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { // If this capture is using the name `self` actually referring // to some other variable (e.g. with `[self = "hello"]`) // then implicit self is not allowed. - if (!selfDecl->isSelfParamCapture()) + if (!selfDecl->isSelfParamCapture()) { return true; - - if (auto var = dyn_cast(DRE->getDecl())) - if (!isEnclosingSelfReference(var, inClosure)) + } + } + } + + if (auto var = dyn_cast(DRE->getDecl())) { + if (!isEnclosingSelfReference(var, inClosure)) { return false; - + } + } + return true; } + static bool implicitWeakSelfReferenceIsValid(const DeclRefExpr *DRE) { + ASTContext &Ctx = DRE->getDecl()->getASTContext(); + + // Check if the implicit self decl refers to a var in a conditional stmt + LabeledConditionalStmt *conditionalStmt = nullptr; + if (auto var = dyn_cast(DRE->getDecl())) { + if (auto parentStmt = var->getParentPatternStmt()) { + conditionalStmt = dyn_cast(parentStmt); + } + } + + if (conditionalStmt == nullptr) { + return false; + } + + // Find the condition that defined the self decl, + // and check that both its LHS and RHS are 'self' + for (auto cond : conditionalStmt->getCond()) { + if (auto OSP = dyn_cast(cond.getPattern())) { + if (OSP->getSubPattern()->getBoundName() != Ctx.Id_self) { + continue; + } + + if (auto LE = dyn_cast(cond.getInitializer())) { + if (auto selfDRE = dyn_cast(LE->getSubExpr())) { + return (selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self)); + } + } + } + } + + return false; + } + + static bool closureHasWeakSelfCapture(const AbstractClosureExpr *ACE) { + if (auto closureExpr = dyn_cast(ACE)) { + if (auto selfDecl = closureExpr->getCapturedSelfDecl()) { + return selfDecl->getType()->is(); + } + } + + return false; + } + /// Return true if this is a closure expression that will require explicit /// use or capture of "self." for qualification of member references. static bool isClosureRequiringSelfQualification( const AbstractClosureExpr *CE) { - // If this closure capture self weakly, then we have to validate each usage - // of implicit self individually, even in a nonescaping closure - if (auto closureExpr = dyn_cast(CE)) - if (auto selfDecl = closureExpr->getCapturedSelfDecl()) - if (selfDecl->getType()->is()) - return true; - + // If this closure capture self weakly, then we have to validate each + // usage of implicit self individually, even in a nonescaping closure + if (closureHasWeakSelfCapture(CE)) { + return true; + } + // If the closure's type was inferred to be noescape, then it doesn't // need qualification. if (AnyFunctionRef(const_cast(CE)) @@ -1698,22 +1720,19 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // 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) { - // If this implicit self decl is from a closure that captured self weakly, - // then we should always emit an error, since implicit self was only - // allowed starting in Swift 5.8 and later. - if (auto closureExpr = dyn_cast(ACE)) - if (auto selfDecl = closureExpr->getCapturedSelfDecl()) - if (selfDecl->getType()->is()) - return false; - - if (auto declRef = dyn_cast(selfRef)) - if (auto decl = declRef->getDecl()) - if (auto varDecl = dyn_cast(decl)) - return !varDecl->isSelfParameter(); - - return false; + // If this implicit self decl is from a closure that captured self + // weakly, then we should always emit an error, since implicit self was + // only allowed starting in Swift 5.8 and later. + if (closureHasWeakSelfCapture(ACE)) { + return false; + } + + // We know that isImplicitSelfParamUseLikelyToCauseCycle is true, + // which means all these casts are valid. + return !cast(cast(selfRef)->getDecl()) + ->isSelfParameter(); }; - + SourceLoc memberLoc = SourceLoc(); if (auto *MRE = dyn_cast(E)) if (isImplicitSelfParamUseLikelyToCauseCycle(MRE->getBase(), ACE)) { diff --git a/lib/Sema/TypeCheckNameLookup.cpp b/lib/Sema/TypeCheckNameLookup.cpp index 8c4e5a4e93efd..c0215a4894a19 100644 --- a/lib/Sema/TypeCheckNameLookup.cpp +++ b/lib/Sema/TypeCheckNameLookup.cpp @@ -100,12 +100,13 @@ namespace { /// /// \param isOuter Whether this is an outer result (i.e. a result that isn't /// from the innermost scope with results) - void add(ValueDecl *found, DeclContext *baseDC, ValueDecl *baseDecl, Type foundInType, - bool isOuter) { + void add(ValueDecl *found, DeclContext *baseDC, ValueDecl *baseDecl, + Type foundInType, bool isOuter) { DeclContext *foundDC = found->getDeclContext(); auto addResult = [&](ValueDecl *result) { if (Known.insert({{result, baseDC}, false}).second) { + // HERE, need to look up base decl Result.add(LookupResultEntry(baseDC, baseDecl, result), isOuter); if (isOuter) FoundOuterDecls.push_back(result); @@ -270,7 +271,8 @@ LookupResult TypeChecker::lookupUnqualified(DeclContext *dc, DeclNameRef name, assert(foundInType && "bogus base declaration?"); } - builder.add(found.getValueDecl(), found.getDeclContext(), found.getBaseDecl(), foundInType, + builder.add(found.getValueDecl(), found.getDeclContext(), + found.getBaseDecl(), foundInType, /*isOuter=*/idx >= lookup.getIndexOfFirstOuterResult()); } return result; From 94ef6c4ab463ecb6f0ce3a7454d6bdb8f228bc4a Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 25 Sep 2022 10:46:34 -0700 Subject: [PATCH 29/37] Permit implicit self for weak self captures in nonescaping closures in Swift 5 (this is an error in Swift 6) --- include/swift/AST/Pattern.h | 12 +++ lib/AST/UnqualifiedLookup.cpp | 7 ++ lib/Sema/MiscDiagnostics.cpp | 75 ++++++++++++---- test/expr/closure/closures.swift | 38 ++++---- test/expr/closure/closures_swift6.swift | 112 ++++++++++++++++++++++++ 5 files changed, 209 insertions(+), 35 deletions(-) diff --git a/include/swift/AST/Pattern.h b/include/swift/AST/Pattern.h index 4dd661cd87435..8302eb57304a0 100644 --- a/include/swift/AST/Pattern.h +++ b/include/swift/AST/Pattern.h @@ -608,6 +608,7 @@ class OptionalSomePattern : public Pattern { Pattern *SubPattern; SourceLoc QuestionLoc; EnumElementDecl *ElementDecl = nullptr; + bool EnablesImplicitSelfForWeakSelfCapture = false; public: explicit OptionalSomePattern(Pattern *SubPattern, @@ -627,6 +628,17 @@ class OptionalSomePattern : public Pattern { EnumElementDecl *getElementDecl() const { return ElementDecl; } void setElementDecl(EnumElementDecl *d) { ElementDecl = d; } + /// Whether or not this pattern is used to enable implicit self + /// for weak self captures in the following scope. This is used + /// in Swift 5 language modes to prevent a false-positive + /// "unused value" warning, and is not necessary in Swift 6+. + bool getEnablesImplicitSelfForWeakSelfCapture() { + return EnablesImplicitSelfForWeakSelfCapture; + } + void setEnablesImplicitSelfForWeakSelfCapture(bool value) { + EnablesImplicitSelfForWeakSelfCapture = value; + } + static bool classof(const Pattern *P) { return P->getKind() == PatternKind::OptionalSome; } diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp index e174bf7ac9fd6..1f775ea6eff1d 100644 --- a/lib/AST/UnqualifiedLookup.cpp +++ b/lib/AST/UnqualifiedLookup.cpp @@ -408,6 +408,13 @@ ValueDecl *UnqualifiedLookupFactory::ResultFinderForTypeContext::lookupBaseDecl( } } + // We can only change the behavior of lookup in Swift 6 and later, + // due to a bug in Swift 5 where implicit self is always allowed + // for weak self captures in non-escaping closures. + if (!factory->Ctx.LangOpts.isSwiftVersionAtLeast(6)) { + return nullptr; + } + if (isInWeakSelfClosure) { return ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), DeclName(factory->Ctx.Id_self), diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 9d90d75b00661..93c0d2f6d89d6 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1581,7 +1581,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // someUnrelatedVariable`. If self hasn't been unwrapped yet and is still // an optional, we would have already hit an error elsewhere. if (closureHasWeakSelfCapture(inClosure)) { - return !implicitWeakSelfReferenceIsValid(DRE); + return !implicitWeakSelfReferenceIsValid(DRE, inClosure); } // Defensive check for type. If the expression doesn't have type here, it @@ -1620,12 +1620,32 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, return true; } - static bool implicitWeakSelfReferenceIsValid(const DeclRefExpr *DRE) { + static bool + implicitWeakSelfReferenceIsValid(const DeclRefExpr *DRE, + const AbstractClosureExpr *inClosure) { ASTContext &Ctx = DRE->getDecl()->getASTContext(); + auto selfDecl = DRE->getDecl(); + + if (!Ctx.LangOpts.isSwiftVersionAtLeast(6)) { + // In Swift 5 language modes, the decl of this implicit `DeclRefExpr` + // incorrectly refers to the `self` `ParamDecl` of the closure + // (which is always non-optional), instead of the actual decl that + // an explicit `self` call would refer to. As a workaround, we can + // manually lookup what self "should" refer to, and then validate + // that decl instead. + auto lookupResult = ASTScope::lookupSingleLocalDecl( + inClosure->getParentSourceFile(), DeclName(Ctx.Id_self), + DRE->getLoc()); + if (!lookupResult) { + return false; + } + + selfDecl = lookupResult; + } // Check if the implicit self decl refers to a var in a conditional stmt LabeledConditionalStmt *conditionalStmt = nullptr; - if (auto var = dyn_cast(DRE->getDecl())) { + if (auto var = dyn_cast(selfDecl)) { if (auto parentStmt = var->getParentPatternStmt()) { conditionalStmt = dyn_cast(parentStmt); } @@ -1645,7 +1665,17 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (auto LE = dyn_cast(cond.getInitializer())) { if (auto selfDRE = dyn_cast(LE->getSubExpr())) { - return (selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self)); + if (selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self)) { + // Mark this `OptionalStatementPattern` as enabling implicit + // self for this weak self capture. This lets us avoid emitting + // a false-positive warning in `VarDeclUsageChecker` in + // Swift 5 mode (this is unnecessary in Swift 6). + if (!Ctx.LangOpts.isSwiftVersionAtLeast(6)) { + OSP->setEnablesImplicitSelfForWeakSelfCapture(true); + } + + return true; + } } } } @@ -1666,14 +1696,8 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, /// Return true if this is a closure expression that will require explicit /// use or capture of "self." for qualification of member references. - static bool isClosureRequiringSelfQualification( - const AbstractClosureExpr *CE) { - // If this closure capture self weakly, then we have to validate each - // usage of implicit self individually, even in a nonescaping closure - if (closureHasWeakSelfCapture(CE)) { - return true; - } - + static bool + isClosureRequiringSelfQualification(const AbstractClosureExpr *CE) { // If the closure's type was inferred to be noescape, then it doesn't // need qualification. if (AnyFunctionRef(const_cast(CE)) @@ -1702,8 +1726,11 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, std::pair walkToExprPre(Expr *E) override { if (auto *CE = dyn_cast(E)) { // If this is a potentially-escaping closure expression, start looking - // for references to self if we aren't already. - if (isClosureRequiringSelfQualification(CE)) + // for references to self if we aren't already. But if this closure + // captures self weakly, then we have to validate each usage of implicit + // self individually, even in a nonescaping closure + if (isClosureRequiringSelfQualification(CE) || + closureHasWeakSelfCapture(CE)) Closures.push_back(CE); } @@ -1724,7 +1751,10 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // weakly, then we should always emit an error, since implicit self was // only allowed starting in Swift 5.8 and later. if (closureHasWeakSelfCapture(ACE)) { - return false; + // Implicit self was incorrectly permitted for weak self captures + // in non-escaping closures in Swift 5.7, so in that case we can + // only warn until Swift 6. + return !isClosureRequiringSelfQualification(ACE); } // We know that isImplicitSelfParamUseLikelyToCauseCycle is true, @@ -3275,7 +3305,20 @@ VarDeclUsageChecker::~VarDeclUsageChecker() { auto initExpr = SC->getCond()[0].getInitializer(); if (initExpr->getStartLoc().isValid()) { unsigned noParens = initExpr->canAppendPostfixExpression(); - + + // Don't emit an "unused value" warning if this is a + // `let self = self` condition being used to enable + // implicit self for the remainder of this scope, + // since it would be a false positive. This is only + // necessary in Swift 5 mode, because in Swift 6 + // this new self decl is correctly used as the base of + // implicit self calls for the remainder of the scope. + auto &ctx = var->getASTContext(); + if (!ctx.LangOpts.isSwiftVersionAtLeast(6) && + OSP->getEnablesImplicitSelfForWeakSelfCapture()) { + continue; + } + // If the subexpr is an "as?" cast, we can rewrite it to // be an "is" test. ConditionalCheckedCastExpr *CCE = nullptr; diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 622d7c65c21e2..431003a5dfd85 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -261,12 +261,12 @@ class ExplicitSelfRequiredTest { // because its `sawError` flag is set to true. To preserve the "capture 'y' was never used" warnings // above, we put these cases in their own method. func weakSelfError() { - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doStuff({ [weak self] in x+1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doStuff({ [weak self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} } } @@ -742,12 +742,12 @@ public class TestImplicitCaptureOfExplicitCaptureOfSelfInEscapingClosure { } public class TestImplicitSelfForWeakSelfCapture { - static var staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() + static let staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() func method() { } private init() { doVoidStuff { [weak self] in - method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} guard let self = self else { return } method() } @@ -759,19 +759,19 @@ public class TestImplicitSelfForWeakSelfCapture { } doVoidStuff { [weak self] in - guard let self = self else { return } + guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} } } doVoidStuff { [weak self] in - guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in - method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} guard let self = self else { return } method() } @@ -784,26 +784,26 @@ public class TestImplicitSelfForWeakSelfCapture { doVoidStuff { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } + guard let self = self else { return } // // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + guard let self = self else { return } // expected-warning {{unexpected warning produced: value 'self' was defined but never used; consider replacing with boolean test}} + method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in - guard let self = self else { return } + guard let self = self else { return } // expected-warning {{unexpected warning produced: value 'self' was defined but never used; consider replacing with boolean test}} doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} } } doVoidStuffNonEscaping { [weak self] in - guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // expected-warning {{unexpected warning produced: value 'self' was defined but never used; consider replacing with boolean test}} + method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } } } @@ -824,12 +824,12 @@ public class TestRebindingSelfIsDisallowed { doVoidStuff { [weak self] in let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} } doVoidStuffNonEscaping { [weak self] in let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} } } diff --git a/test/expr/closure/closures_swift6.swift b/test/expr/closure/closures_swift6.swift index 92197278ff457..273bd0e78bd19 100644 --- a/test/expr/closure/closures_swift6.swift +++ b/test/expr/closure/closures_swift6.swift @@ -70,3 +70,115 @@ class C_56501 { } } } + +class ExplicitSelfRequiredTest_WeakSelf { + var x = 42 + + func method() { } + func weakSelfError() { + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} expected-error {{cannot convert value of type '()' to closure result type 'Int'}} + } +} + +public class TestImplicitSelfForWeakSelfCapture { + static let staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() + func method() { } + + private init() { + doVoidStuff { [weak self] in + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + guard let self = self else { return } + method() + } + + doVoidStuff { [weak self] in + if let self = self { + method() + } + } + + doVoidStuff { [weak self] in + guard let self = self else { return } + doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} + } + } + + doVoidStuff { [weak self] in + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + guard let self = self else { return } + method() + } + + doVoidStuffNonEscaping { [weak self] in + if let self = self { + method() + } + } + + doVoidStuff { [weak self] in + let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional + guard let self = self else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional + guard let self = self else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + guard let self = self else { return } + doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} + } + } + + doVoidStuffNonEscaping { [weak self] in + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + } +} + +public class TestRebindingSelfIsDisallowed { + let count: Void = () + + private init() { + doVoidStuff { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } + + doVoidStuffNonEscaping { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } + + doVoidStuff { [weak self] in + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + } + + func method() { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } +} \ No newline at end of file From dfa1fda3d394f6c582e106c6a0e225ee5e33b4a3 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 25 Sep 2022 16:19:57 -0700 Subject: [PATCH 30/37] fix warning annotation in test --- test/expr/closure/closures.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 431003a5dfd85..d62c2d25d1a3c 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -790,19 +790,19 @@ public class TestImplicitSelfForWeakSelfCapture { doVoidStuffNonEscaping { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } // expected-warning {{unexpected warning produced: value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in - guard let self = self else { return } // expected-warning {{unexpected warning produced: value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} } } doVoidStuffNonEscaping { [weak self] in - guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // expected-warning {{unexpected warning produced: value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } } From 35e028e5c266b0be3e2f50e1d3bdedb6f25e3c4e Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 25 Sep 2022 19:48:24 -0700 Subject: [PATCH 31/37] Suppress more false-positive 'self is unused' warnings --- include/swift/AST/Expr.h | 15 +++++++++++++++ lib/Sema/MiscDiagnostics.cpp | 24 ++++++++++++++++++------ test/expr/closure/closures.swift | 32 ++++++++++++++++---------------- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h index 391a3965e75b6..db2cef201d8ab 100644 --- a/include/swift/AST/Expr.h +++ b/include/swift/AST/Expr.h @@ -1169,6 +1169,7 @@ class DeclRefExpr : public Expr { ConcreteDeclRef D; DeclNameLoc Loc; ActorIsolation implicitActorHopTarget; + ValueDecl *DeclForUsageAnalysis = nullptr; public: DeclRefExpr(ConcreteDeclRef D, DeclNameLoc Loc, bool Implicit, @@ -1247,6 +1248,20 @@ class DeclRefExpr : public Expr { Bits.DeclRefExpr.FunctionRefKind = static_cast(refKind); } + /// The decl that should be used for usage analysis of this DRE. + /// This is potentially different from `getDecl()`, to handle + /// cases where performing usage analysis on the actual decl + /// causes false-positive warnings. + ValueDecl *getDeclForUsageAnalysis() { + if (DeclForUsageAnalysis) { + return DeclForUsageAnalysis; + } + + return getDecl(); + } + + void setDeclForUsageAnalysis(ValueDecl *vd) { DeclForUsageAnalysis = vd; } + static bool classof(const Expr *E) { return E->getKind() == ExprKind::DeclRef; } diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 93c0d2f6d89d6..b6d8e03b29c6c 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1580,8 +1580,15 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // let self = .someOptionalVariable else { return }` or `let self = // someUnrelatedVariable`. If self hasn't been unwrapped yet and is still // an optional, we would have already hit an error elsewhere. + // + // Always run the `implicitWeakSelfReferenceIsValid` check, since it + // annotates applicable decls / stmts with state used in + // `VarDeclUsageChecker`, and we need that state even in closures that + // don't directly capture self weakly (but are inner closures of a closure + // that does capture self weakly). + auto isValid = implicitWeakSelfReferenceIsValid(DRE, inClosure); if (closureHasWeakSelfCapture(inClosure)) { - return !implicitWeakSelfReferenceIsValid(DRE, inClosure); + return !isValid; } // Defensive check for type. If the expression doesn't have type here, it @@ -1621,7 +1628,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, } static bool - implicitWeakSelfReferenceIsValid(const DeclRefExpr *DRE, + implicitWeakSelfReferenceIsValid(DeclRefExpr *DRE, const AbstractClosureExpr *inClosure) { ASTContext &Ctx = DRE->getDecl()->getASTContext(); auto selfDecl = DRE->getDecl(); @@ -1640,6 +1647,11 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, return false; } + // When `VarDeclUsageChecker` checks this DRE, we need to use + // the base we looked up here instead, since otherwise + // we'll emit confusing false-positive warnings. + DRE->setDeclForUsageAnalysis(lookupResult); + selfDecl = lookupResult; } @@ -3537,7 +3549,7 @@ void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) { // If we found a decl that is being assigned to, then mark it. if (auto *DRE = dyn_cast(E)) { - addMark(DRE->getDecl(), Flags); + addMark(DRE->getDeclForUsageAnalysis(), Flags); return; } @@ -3622,10 +3634,10 @@ std::pair VarDeclUsageChecker::walkToExprPre(Expr *E) { // If this is a DeclRefExpr found in a random place, it is a load of the // vardecl. if (auto *DRE = dyn_cast(E)) { - addMark(DRE->getDecl(), RK_Read); + addMark(DRE->getDeclForUsageAnalysis(), RK_Read); // If the Expression is a read of a getter, track for diagnostics - if (auto VD = dyn_cast(DRE->getDecl())) { + if (auto VD = dyn_cast(DRE->getDeclForUsageAnalysis())) { AssociatedGetterRefExpr.insert(std::make_pair(VD, DRE)); } } @@ -3699,7 +3711,7 @@ void VarDeclUsageChecker::handleIfConfig(IfConfigDecl *ICD) { // conservatively mark it read and written. This will silence "variable // unused" and "could be marked let" warnings for it. if (auto *DRE = dyn_cast(E)) - VDUC.addMark(DRE->getDecl(), RK_Read|RK_Written); + VDUC.addMark(DRE->getDeclForUsageAnalysis(), RK_Read | RK_Written); else if (auto *declRef = dyn_cast(E)) { auto name = declRef->getName(); auto loc = declRef->getLoc(); diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index d62c2d25d1a3c..7d2a39c9d7fb9 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -176,8 +176,8 @@ class ExplicitSelfRequiredTest { 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.}} doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} - doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} // Methods follow the same rules as properties, uses of 'self' without capturing must be marked with "self." doStuff { method() } // expected-error {{call to method 'method' 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.}} @@ -186,8 +186,8 @@ class ExplicitSelfRequiredTest { doVoidStuff { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} doVoidStuff { [y = self] in _ = method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' 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}} {{37-37=self.}} doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} - doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} doVoidStuff { _ = self.method() } doVoidStuff { [self] in _ = method() } doVoidStuff { [self = self] in _ = method() } @@ -261,12 +261,12 @@ class ExplicitSelfRequiredTest { // because its `sawError` flag is set to true. To preserve the "capture 'y' was never used" warnings // above, we put these cases in their own method. func weakSelfError() { - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} - doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} - doStuff({ [weak self] in x+1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} - doVoidStuff({ [weak self] in _ = method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} - doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} - doStuff({ [weak self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doStuff({ [weak self] in x+1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doStuff({ [weak self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } } @@ -759,14 +759,14 @@ public class TestImplicitSelfForWeakSelfCapture { } doVoidStuff { [weak self] in - guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self else { return } doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} } } doVoidStuff { [weak self] in - guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } @@ -784,25 +784,25 @@ public class TestImplicitSelfForWeakSelfCapture { doVoidStuff { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } // // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self else { return } method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self else { return } method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in - guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self else { return } doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} } } doVoidStuffNonEscaping { [weak self] in - guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } } From e4b8a829fe6a33a579e7661282d516b0ff5860f1 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Tue, 27 Sep 2022 21:15:06 -0700 Subject: [PATCH 32/37] Remove properties from AST nodes --- include/swift/AST/Expr.h | 15 ---------- include/swift/AST/Pattern.h | 12 -------- lib/Sema/MiscDiagnostics.cpp | 54 ++++++++++++++++-------------------- 3 files changed, 24 insertions(+), 57 deletions(-) diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h index db2cef201d8ab..391a3965e75b6 100644 --- a/include/swift/AST/Expr.h +++ b/include/swift/AST/Expr.h @@ -1169,7 +1169,6 @@ class DeclRefExpr : public Expr { ConcreteDeclRef D; DeclNameLoc Loc; ActorIsolation implicitActorHopTarget; - ValueDecl *DeclForUsageAnalysis = nullptr; public: DeclRefExpr(ConcreteDeclRef D, DeclNameLoc Loc, bool Implicit, @@ -1248,20 +1247,6 @@ class DeclRefExpr : public Expr { Bits.DeclRefExpr.FunctionRefKind = static_cast(refKind); } - /// The decl that should be used for usage analysis of this DRE. - /// This is potentially different from `getDecl()`, to handle - /// cases where performing usage analysis on the actual decl - /// causes false-positive warnings. - ValueDecl *getDeclForUsageAnalysis() { - if (DeclForUsageAnalysis) { - return DeclForUsageAnalysis; - } - - return getDecl(); - } - - void setDeclForUsageAnalysis(ValueDecl *vd) { DeclForUsageAnalysis = vd; } - static bool classof(const Expr *E) { return E->getKind() == ExprKind::DeclRef; } diff --git a/include/swift/AST/Pattern.h b/include/swift/AST/Pattern.h index 8302eb57304a0..4dd661cd87435 100644 --- a/include/swift/AST/Pattern.h +++ b/include/swift/AST/Pattern.h @@ -608,7 +608,6 @@ class OptionalSomePattern : public Pattern { Pattern *SubPattern; SourceLoc QuestionLoc; EnumElementDecl *ElementDecl = nullptr; - bool EnablesImplicitSelfForWeakSelfCapture = false; public: explicit OptionalSomePattern(Pattern *SubPattern, @@ -628,17 +627,6 @@ class OptionalSomePattern : public Pattern { EnumElementDecl *getElementDecl() const { return ElementDecl; } void setElementDecl(EnumElementDecl *d) { ElementDecl = d; } - /// Whether or not this pattern is used to enable implicit self - /// for weak self captures in the following scope. This is used - /// in Swift 5 language modes to prevent a false-positive - /// "unused value" warning, and is not necessary in Swift 6+. - bool getEnablesImplicitSelfForWeakSelfCapture() { - return EnablesImplicitSelfForWeakSelfCapture; - } - void setEnablesImplicitSelfForWeakSelfCapture(bool value) { - EnablesImplicitSelfForWeakSelfCapture = value; - } - static bool classof(const Pattern *P) { return P->getKind() == PatternKind::OptionalSome; } diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index b6d8e03b29c6c..ba855b92ebebf 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1526,6 +1526,15 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) { const_cast(E)->walk(walker); } +namespace DiagnosticsContext { +/// A mapping of replacement `ValueDecl`s to use in `VarDeclUsageAnalysis` +/// instead of the actual `ValueDecl` of the associated `DeclRefExpr`. +/// This lets `VarDeclUsageAnalysis` avoid emitting false-positive warnings +/// in certain circumstances. +static llvm::SmallDenseMap + DeclMappingForUsageAnalysis; +} + /// Look for any property references in closures that lack a 'self.' qualifier. /// Within a closure, we require that the source code contain 'self.' explicitly /// (or that the closure explicitly capture 'self' in the capture list) because @@ -1650,7 +1659,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // When `VarDeclUsageChecker` checks this DRE, we need to use // the base we looked up here instead, since otherwise // we'll emit confusing false-positive warnings. - DRE->setDeclForUsageAnalysis(lookupResult); + DiagnosticsContext::DeclMappingForUsageAnalysis[DRE] = lookupResult; selfDecl = lookupResult; } @@ -1677,17 +1686,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (auto LE = dyn_cast(cond.getInitializer())) { if (auto selfDRE = dyn_cast(LE->getSubExpr())) { - if (selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self)) { - // Mark this `OptionalStatementPattern` as enabling implicit - // self for this weak self capture. This lets us avoid emitting - // a false-positive warning in `VarDeclUsageChecker` in - // Swift 5 mode (this is unnecessary in Swift 6). - if (!Ctx.LangOpts.isSwiftVersionAtLeast(6)) { - OSP->setEnablesImplicitSelfForWeakSelfCapture(true); - } - - return true; - } + return selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self); } } } @@ -2636,7 +2635,15 @@ class VarDeclUsageChecker : public ASTWalker { VarDecls[vd] |= Flag; } - + + ValueDecl *getDecl(DeclRefExpr *DRE) { + if (auto decl = DiagnosticsContext::DeclMappingForUsageAnalysis[DRE]) { + return decl; + } + + return DRE->getDecl(); + } + void markBaseOfStorageUse(Expr *E, ConcreteDeclRef decl, unsigned flags); void markBaseOfStorageUse(Expr *E, bool isMutating); @@ -3318,19 +3325,6 @@ VarDeclUsageChecker::~VarDeclUsageChecker() { if (initExpr->getStartLoc().isValid()) { unsigned noParens = initExpr->canAppendPostfixExpression(); - // Don't emit an "unused value" warning if this is a - // `let self = self` condition being used to enable - // implicit self for the remainder of this scope, - // since it would be a false positive. This is only - // necessary in Swift 5 mode, because in Swift 6 - // this new self decl is correctly used as the base of - // implicit self calls for the remainder of the scope. - auto &ctx = var->getASTContext(); - if (!ctx.LangOpts.isSwiftVersionAtLeast(6) && - OSP->getEnablesImplicitSelfForWeakSelfCapture()) { - continue; - } - // If the subexpr is an "as?" cast, we can rewrite it to // be an "is" test. ConditionalCheckedCastExpr *CCE = nullptr; @@ -3549,7 +3543,7 @@ void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) { // If we found a decl that is being assigned to, then mark it. if (auto *DRE = dyn_cast(E)) { - addMark(DRE->getDeclForUsageAnalysis(), Flags); + addMark(getDecl(DRE), Flags); return; } @@ -3634,10 +3628,10 @@ std::pair VarDeclUsageChecker::walkToExprPre(Expr *E) { // If this is a DeclRefExpr found in a random place, it is a load of the // vardecl. if (auto *DRE = dyn_cast(E)) { - addMark(DRE->getDeclForUsageAnalysis(), RK_Read); + addMark(getDecl(DRE), RK_Read); // If the Expression is a read of a getter, track for diagnostics - if (auto VD = dyn_cast(DRE->getDeclForUsageAnalysis())) { + if (auto VD = dyn_cast(getDecl(DRE))) { AssociatedGetterRefExpr.insert(std::make_pair(VD, DRE)); } } @@ -3711,7 +3705,7 @@ void VarDeclUsageChecker::handleIfConfig(IfConfigDecl *ICD) { // conservatively mark it read and written. This will silence "variable // unused" and "could be marked let" warnings for it. if (auto *DRE = dyn_cast(E)) - VDUC.addMark(DRE->getDeclForUsageAnalysis(), RK_Read | RK_Written); + VDUC.addMark(VDUC.getDecl(DRE), RK_Read | RK_Written); else if (auto *declRef = dyn_cast(E)) { auto name = declRef->getName(); auto loc = declRef->getLoc(); From eae9b4e526ebc9387584d9b6c8efcda1966bed78 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Wed, 28 Sep 2022 10:30:20 -0700 Subject: [PATCH 33/37] Add SE-0365 to CHANGELOG.md --- CHANGELOG.md | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3116a4c8f5b18..4108a87bb77de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,46 @@ _**Note:** This is in reverse chronological order, so newer entries are added to ## Swift 5.8 +* [SE-0365][]: + + Implicit `self` is now permitted for `weak self` captures, after `self` is unwrapped. + + For example, the usage of implicit `self` below is now permitted: + + ```swift + class ViewController { + let button: Button + + func setup() { + button.tapHandler = { [weak self] in + guard let self else { return } + dismiss() // refers to `self.dismiss()` + } + } + + func dismiss() { ... } + } + ``` + + In Swift 5 language modes, implicit `self` is permitted for `weak self` captures in _non-escaping_ closures even before `self` is unwrapped. For example, this code compiles successfully in Swift 5 language modes: + + ```swift + class ExampleClass { + func makeArray() -> [String] { + // `Array.map` takes a non-escaping closure: + ["foo", "bar", "baaz"].map { [weak self] string in + double(string) // implicitly refers to `self!.double(string)` + } + } + + func double(_ string: String) -> String { + string + string + } + } + ``` + + Starting in Swift 6, the above code will no longer compile. `weak self` captures in non-escaping closures will have the same behavior as captures in escaping closures (as described in [SE-0365][]). Code relying on the previous behavior will need to be updated to either unwrap `self` (e.g. by adding a `guard let self else return` statement), or to use a different capture method (e.g. using `[self]` or `[unowned self]` instead of `[weak self]`). + * [SE-0362][]: The compiler flag `-enable-upcoming-feature X` can now be used to enable a specific feature `X` that has been accepted by the evolution process, but whose introduction into the language is waiting for the next major version (e.g., version 6). The `X` is specified by any proposal that falls into this category: @@ -9559,6 +9599,7 @@ Swift 1.0 [SE-0357]: [SE-0358]: [SE-0362]: +[SE-0365]: [SR-75]: [SR-106]: From a4fac800deb50eb09fab3b3ae49acff411b01b9e Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Wed, 28 Sep 2022 15:22:59 -0700 Subject: [PATCH 34/37] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4108a87bb77de..1bcfafb7d5cfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ _**Note:** This is in reverse chronological order, so newer entries are added to } ``` - In Swift 5 language modes, implicit `self` is permitted for `weak self` captures in _non-escaping_ closures even before `self` is unwrapped. For example, this code compiles successfully in Swift 5 language modes: + In Swift 5 language modes, implicit `self` is permitted for `weak self` captures in _non-escaping_ closures even before `self` is unwrapped. For example, this code compiles successfully in Swift 5 language mode: ```swift class ExampleClass { From 21e94dec3aad3e616d1bd5e601651bd75662449a Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Wed, 28 Sep 2022 17:41:35 -0700 Subject: [PATCH 35/37] Move SE-0365 to Swift 6 section in changelog --- CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bcfafb7d5cfd..92bc39b389b35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ CHANGELOG _**Note:** This is in reverse chronological order, so newer entries are added to the top._ -## Swift 5.8 +## Swift 6.0 * [SE-0365][]: @@ -43,7 +43,9 @@ _**Note:** This is in reverse chronological order, so newer entries are added to } ``` - Starting in Swift 6, the above code will no longer compile. `weak self` captures in non-escaping closures will have the same behavior as captures in escaping closures (as described in [SE-0365][]). Code relying on the previous behavior will need to be updated to either unwrap `self` (e.g. by adding a `guard let self else return` statement), or to use a different capture method (e.g. using `[self]` or `[unowned self]` instead of `[weak self]`). + In Swift 6, the above code will no longer compile. `weak self` captures in non-escaping closures now have the same behavior as captures in escaping closures (as described in [SE-0365][]). Code relying on the previous behavior will need to be updated to either unwrap `self` (e.g. by adding a `guard let self else return` statement), or to use a different capture method (e.g. using `[self]` or `[unowned self]` instead of `[weak self]`). + +## Swift 5.8 * [SE-0362][]: From cc92f76a9e1f3a941937861afc1c8029d9a0791a Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Wed, 28 Sep 2022 17:43:11 -0700 Subject: [PATCH 36/37] Revert changes for handling SE-0365 in Swift 5 Revert "Remove properties from AST nodes" This reverts commit e4b8a829fe6a33a579e7661282d516b0ff5860f1. Revert "Suppress more false-positive 'self is unused' warnings" This reverts commit 35e028e5c266b0be3e2f50e1d3bdedb6f25e3c4e. Revert "fix warning annotation in test" This reverts commit dfa1fda3d394f6c582e106c6a0e225ee5e33b4a3. Revert "Permit implicit self for weak self captures in nonescaping closures in Swift 5 (this is an error in Swift 6)" This reverts commit 94ef6c4ab463ecb6f0ce3a7454d6bdb8f228bc4a. --- lib/AST/UnqualifiedLookup.cpp | 7 -- lib/Sema/MiscDiagnostics.cpp | 91 +++++-------------- test/expr/closure/closures.swift | 34 +++---- test/expr/closure/closures_swift6.swift | 112 ------------------------ 4 files changed, 38 insertions(+), 206 deletions(-) diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp index 1f775ea6eff1d..e174bf7ac9fd6 100644 --- a/lib/AST/UnqualifiedLookup.cpp +++ b/lib/AST/UnqualifiedLookup.cpp @@ -408,13 +408,6 @@ ValueDecl *UnqualifiedLookupFactory::ResultFinderForTypeContext::lookupBaseDecl( } } - // We can only change the behavior of lookup in Swift 6 and later, - // due to a bug in Swift 5 where implicit self is always allowed - // for weak self captures in non-escaping closures. - if (!factory->Ctx.LangOpts.isSwiftVersionAtLeast(6)) { - return nullptr; - } - if (isInWeakSelfClosure) { return ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), DeclName(factory->Ctx.Id_self), diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index ba855b92ebebf..9d90d75b00661 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1526,15 +1526,6 @@ static void diagRecursivePropertyAccess(const Expr *E, const DeclContext *DC) { const_cast(E)->walk(walker); } -namespace DiagnosticsContext { -/// A mapping of replacement `ValueDecl`s to use in `VarDeclUsageAnalysis` -/// instead of the actual `ValueDecl` of the associated `DeclRefExpr`. -/// This lets `VarDeclUsageAnalysis` avoid emitting false-positive warnings -/// in certain circumstances. -static llvm::SmallDenseMap - DeclMappingForUsageAnalysis; -} - /// Look for any property references in closures that lack a 'self.' qualifier. /// Within a closure, we require that the source code contain 'self.' explicitly /// (or that the closure explicitly capture 'self' in the capture list) because @@ -1589,15 +1580,8 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // let self = .someOptionalVariable else { return }` or `let self = // someUnrelatedVariable`. If self hasn't been unwrapped yet and is still // an optional, we would have already hit an error elsewhere. - // - // Always run the `implicitWeakSelfReferenceIsValid` check, since it - // annotates applicable decls / stmts with state used in - // `VarDeclUsageChecker`, and we need that state even in closures that - // don't directly capture self weakly (but are inner closures of a closure - // that does capture self weakly). - auto isValid = implicitWeakSelfReferenceIsValid(DRE, inClosure); if (closureHasWeakSelfCapture(inClosure)) { - return !isValid; + return !implicitWeakSelfReferenceIsValid(DRE); } // Defensive check for type. If the expression doesn't have type here, it @@ -1636,37 +1620,12 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, return true; } - static bool - implicitWeakSelfReferenceIsValid(DeclRefExpr *DRE, - const AbstractClosureExpr *inClosure) { + static bool implicitWeakSelfReferenceIsValid(const DeclRefExpr *DRE) { ASTContext &Ctx = DRE->getDecl()->getASTContext(); - auto selfDecl = DRE->getDecl(); - - if (!Ctx.LangOpts.isSwiftVersionAtLeast(6)) { - // In Swift 5 language modes, the decl of this implicit `DeclRefExpr` - // incorrectly refers to the `self` `ParamDecl` of the closure - // (which is always non-optional), instead of the actual decl that - // an explicit `self` call would refer to. As a workaround, we can - // manually lookup what self "should" refer to, and then validate - // that decl instead. - auto lookupResult = ASTScope::lookupSingleLocalDecl( - inClosure->getParentSourceFile(), DeclName(Ctx.Id_self), - DRE->getLoc()); - if (!lookupResult) { - return false; - } - - // When `VarDeclUsageChecker` checks this DRE, we need to use - // the base we looked up here instead, since otherwise - // we'll emit confusing false-positive warnings. - DiagnosticsContext::DeclMappingForUsageAnalysis[DRE] = lookupResult; - - selfDecl = lookupResult; - } // Check if the implicit self decl refers to a var in a conditional stmt LabeledConditionalStmt *conditionalStmt = nullptr; - if (auto var = dyn_cast(selfDecl)) { + if (auto var = dyn_cast(DRE->getDecl())) { if (auto parentStmt = var->getParentPatternStmt()) { conditionalStmt = dyn_cast(parentStmt); } @@ -1686,7 +1645,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (auto LE = dyn_cast(cond.getInitializer())) { if (auto selfDRE = dyn_cast(LE->getSubExpr())) { - return selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self); + return (selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self)); } } } @@ -1707,8 +1666,14 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, /// Return true if this is a closure expression that will require explicit /// use or capture of "self." for qualification of member references. - static bool - isClosureRequiringSelfQualification(const AbstractClosureExpr *CE) { + static bool isClosureRequiringSelfQualification( + const AbstractClosureExpr *CE) { + // If this closure capture self weakly, then we have to validate each + // usage of implicit self individually, even in a nonescaping closure + if (closureHasWeakSelfCapture(CE)) { + return true; + } + // If the closure's type was inferred to be noescape, then it doesn't // need qualification. if (AnyFunctionRef(const_cast(CE)) @@ -1737,11 +1702,8 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, std::pair walkToExprPre(Expr *E) override { if (auto *CE = dyn_cast(E)) { // If this is a potentially-escaping closure expression, start looking - // for references to self if we aren't already. But if this closure - // captures self weakly, then we have to validate each usage of implicit - // self individually, even in a nonescaping closure - if (isClosureRequiringSelfQualification(CE) || - closureHasWeakSelfCapture(CE)) + // for references to self if we aren't already. + if (isClosureRequiringSelfQualification(CE)) Closures.push_back(CE); } @@ -1762,10 +1724,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // weakly, then we should always emit an error, since implicit self was // only allowed starting in Swift 5.8 and later. if (closureHasWeakSelfCapture(ACE)) { - // Implicit self was incorrectly permitted for weak self captures - // in non-escaping closures in Swift 5.7, so in that case we can - // only warn until Swift 6. - return !isClosureRequiringSelfQualification(ACE); + return false; } // We know that isImplicitSelfParamUseLikelyToCauseCycle is true, @@ -2635,15 +2594,7 @@ class VarDeclUsageChecker : public ASTWalker { VarDecls[vd] |= Flag; } - - ValueDecl *getDecl(DeclRefExpr *DRE) { - if (auto decl = DiagnosticsContext::DeclMappingForUsageAnalysis[DRE]) { - return decl; - } - - return DRE->getDecl(); - } - + void markBaseOfStorageUse(Expr *E, ConcreteDeclRef decl, unsigned flags); void markBaseOfStorageUse(Expr *E, bool isMutating); @@ -3324,7 +3275,7 @@ VarDeclUsageChecker::~VarDeclUsageChecker() { auto initExpr = SC->getCond()[0].getInitializer(); if (initExpr->getStartLoc().isValid()) { unsigned noParens = initExpr->canAppendPostfixExpression(); - + // If the subexpr is an "as?" cast, we can rewrite it to // be an "is" test. ConditionalCheckedCastExpr *CCE = nullptr; @@ -3543,7 +3494,7 @@ void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) { // If we found a decl that is being assigned to, then mark it. if (auto *DRE = dyn_cast(E)) { - addMark(getDecl(DRE), Flags); + addMark(DRE->getDecl(), Flags); return; } @@ -3628,10 +3579,10 @@ std::pair VarDeclUsageChecker::walkToExprPre(Expr *E) { // If this is a DeclRefExpr found in a random place, it is a load of the // vardecl. if (auto *DRE = dyn_cast(E)) { - addMark(getDecl(DRE), RK_Read); + addMark(DRE->getDecl(), RK_Read); // If the Expression is a read of a getter, track for diagnostics - if (auto VD = dyn_cast(getDecl(DRE))) { + if (auto VD = dyn_cast(DRE->getDecl())) { AssociatedGetterRefExpr.insert(std::make_pair(VD, DRE)); } } @@ -3705,7 +3656,7 @@ void VarDeclUsageChecker::handleIfConfig(IfConfigDecl *ICD) { // conservatively mark it read and written. This will silence "variable // unused" and "could be marked let" warnings for it. if (auto *DRE = dyn_cast(E)) - VDUC.addMark(VDUC.getDecl(DRE), RK_Read | RK_Written); + VDUC.addMark(DRE->getDecl(), RK_Read|RK_Written); else if (auto *declRef = dyn_cast(E)) { auto name = declRef->getName(); auto loc = declRef->getLoc(); diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 7d2a39c9d7fb9..622d7c65c21e2 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -176,8 +176,8 @@ class ExplicitSelfRequiredTest { 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.}} doStuff({ [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}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in x += 1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} + doStuff({ [self = ExplicitSelfRequiredTest()] in x+1 }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} // Methods follow the same rules as properties, uses of 'self' without capturing must be marked with "self." doStuff { method() } // expected-error {{call to method 'method' 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.}} @@ -186,8 +186,8 @@ class ExplicitSelfRequiredTest { doVoidStuff { () -> () in _ = method() } // expected-error {{call to method 'method' 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]}} expected-note{{reference 'self.' explicitly}} {{35-35=self.}} doVoidStuff { [y = self] in _ = method() } // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' 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}} {{37-37=self.}} doStuff({ [y = self] in method() }) // expected-warning {{capture 'y' was never used}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}} {{16-16=self, }} expected-note{{reference 'self.' explicitly}} {{29-29=self.}} - doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [self = ExplicitSelfRequiredTest()] in _ = method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} + doStuff({ [self = ExplicitSelfRequiredTest()] in method() }) // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{capture 'self' was never used}} doVoidStuff { _ = self.method() } doVoidStuff { [self] in _ = method() } doVoidStuff { [self = self] in _ = method() } @@ -261,12 +261,12 @@ class ExplicitSelfRequiredTest { // because its `sawError` flag is set to true. To preserve the "capture 'y' was never used" warnings // above, we put these cases in their own method. func weakSelfError() { - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} - doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [weak self] in x+1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} - doVoidStuff({ [weak self] in _ = method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - doStuff({ [weak self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} } } @@ -742,12 +742,12 @@ public class TestImplicitCaptureOfExplicitCaptureOfSelfInEscapingClosure { } public class TestImplicitSelfForWeakSelfCapture { - static let staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() + static var staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() func method() { } private init() { doVoidStuff { [weak self] in - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} guard let self = self else { return } method() } @@ -771,7 +771,7 @@ public class TestImplicitSelfForWeakSelfCapture { } doVoidStuffNonEscaping { [weak self] in - method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} guard let self = self else { return } method() } @@ -791,7 +791,7 @@ public class TestImplicitSelfForWeakSelfCapture { doVoidStuffNonEscaping { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional guard let self = self else { return } - method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in @@ -803,7 +803,7 @@ public class TestImplicitSelfForWeakSelfCapture { doVoidStuffNonEscaping { [weak self] in guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } - method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } } } @@ -824,12 +824,12 @@ public class TestRebindingSelfIsDisallowed { doVoidStuff { [weak self] in let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} } } diff --git a/test/expr/closure/closures_swift6.swift b/test/expr/closure/closures_swift6.swift index 273bd0e78bd19..92197278ff457 100644 --- a/test/expr/closure/closures_swift6.swift +++ b/test/expr/closure/closures_swift6.swift @@ -70,115 +70,3 @@ class C_56501 { } } } - -class ExplicitSelfRequiredTest_WeakSelf { - var x = 42 - - func method() { } - func weakSelfError() { - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} expected-error {{cannot convert value of type '()' to closure result type 'Int'}} - } -} - -public class TestImplicitSelfForWeakSelfCapture { - static let staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() - func method() { } - - private init() { - doVoidStuff { [weak self] in - method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - guard let self = self else { return } - method() - } - - doVoidStuff { [weak self] in - if let self = self { - method() - } - } - - doVoidStuff { [weak self] in - guard let self = self else { return } - doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} - } - } - - doVoidStuff { [weak self] in - guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - } - - doVoidStuffNonEscaping { [weak self] in - method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - guard let self = self else { return } - method() - } - - doVoidStuffNonEscaping { [weak self] in - if let self = self { - method() - } - } - - doVoidStuff { [weak self] in - let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - } - - doVoidStuffNonEscaping { [weak self] in - let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - } - - doVoidStuffNonEscaping { [weak self] in - guard let self = self else { return } - doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} - } - } - - doVoidStuffNonEscaping { [weak self] in - guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - } - } -} - -public class TestRebindingSelfIsDisallowed { - let count: Void = () - - private init() { - doVoidStuff { - let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} - } - - doVoidStuffNonEscaping { - let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} - } - - doVoidStuff { [weak self] in - let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} - } - - doVoidStuffNonEscaping { [weak self] in - let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} - } - } - - func method() { - let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} - } -} \ No newline at end of file From 5bfdfd822c7db7f942a017d5b70308c817c498f2 Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Wed, 28 Sep 2022 18:25:16 -0700 Subject: [PATCH 37/37] Only enable SE-0365 in Swift 6 --- lib/AST/UnqualifiedLookup.cpp | 7 ++ lib/Sema/MiscDiagnostics.cpp | 42 +++++---- test/expr/closure/closures.swift | 49 +++++------ test/expr/closure/closures_swift6.swift | 110 +++++++++++++++++++++++- 4 files changed, 162 insertions(+), 46 deletions(-) diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp index e174bf7ac9fd6..1f775ea6eff1d 100644 --- a/lib/AST/UnqualifiedLookup.cpp +++ b/lib/AST/UnqualifiedLookup.cpp @@ -408,6 +408,13 @@ ValueDecl *UnqualifiedLookupFactory::ResultFinderForTypeContext::lookupBaseDecl( } } + // We can only change the behavior of lookup in Swift 6 and later, + // due to a bug in Swift 5 where implicit self is always allowed + // for weak self captures in non-escaping closures. + if (!factory->Ctx.LangOpts.isSwiftVersionAtLeast(6)) { + return nullptr; + } + if (isInWeakSelfClosure) { return ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(), DeclName(factory->Ctx.Id_self), diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp index 9d90d75b00661..a2961b3c3cf74 100644 --- a/lib/Sema/MiscDiagnostics.cpp +++ b/lib/Sema/MiscDiagnostics.cpp @@ -1580,7 +1580,13 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // let self = .someOptionalVariable else { return }` or `let self = // someUnrelatedVariable`. If self hasn't been unwrapped yet and is still // an optional, we would have already hit an error elsewhere. - if (closureHasWeakSelfCapture(inClosure)) { + // + // We can only enable this behavior in Swift 6 and later, due to a + // bug in Swift 5 where implicit self was always allowed for + // weak self captures (even before self was unwrapped) in + // non-escaping closures. + if (Ctx.LangOpts.isSwiftVersionAtLeast(6) && + closureHasWeakSelfCapture(inClosure)) { return !implicitWeakSelfReferenceIsValid(DRE); } @@ -1666,11 +1672,16 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, /// Return true if this is a closure expression that will require explicit /// use or capture of "self." for qualification of member references. - static bool isClosureRequiringSelfQualification( - const AbstractClosureExpr *CE) { + static bool + isClosureRequiringSelfQualification(const AbstractClosureExpr *CE, + ASTContext &Ctx) { // If this closure capture self weakly, then we have to validate each - // usage of implicit self individually, even in a nonescaping closure - if (closureHasWeakSelfCapture(CE)) { + // usage of implicit self individually, even in a nonescaping closure. + // + // We can only do this in Swift 6 mode, since we didn't do this in Swift 5 + // (and changing this behavior causes new errors to be emitted). + if (Ctx.LangOpts.isSwiftVersionAtLeast(6) && + closureHasWeakSelfCapture(CE)) { return true; } @@ -1703,7 +1714,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, if (auto *CE = dyn_cast(E)) { // If this is a potentially-escaping closure expression, start looking // for references to self if we aren't already. - if (isClosureRequiringSelfQualification(CE)) + if (isClosureRequiringSelfQualification(CE, Ctx)) Closures.push_back(CE); } @@ -1720,13 +1731,6 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, // 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) { - // If this implicit self decl is from a closure that captured self - // weakly, then we should always emit an error, since implicit self was - // only allowed starting in Swift 5.8 and later. - if (closureHasWeakSelfCapture(ACE)) { - return false; - } - // We know that isImplicitSelfParamUseLikelyToCauseCycle is true, // which means all these casts are valid. return !cast(cast(selfRef)->getDecl()) @@ -1770,7 +1774,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, Expr *walkToExprPost(Expr *E) override { if (auto *CE = dyn_cast(E)) { - if (isClosureRequiringSelfQualification(CE)) { + if (isClosureRequiringSelfQualification(CE, Ctx)) { assert(Closures.size() > 0); Closures.pop_back(); } @@ -1893,16 +1897,16 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E, } }; + auto &ctx = DC->getASTContext(); AbstractClosureExpr *ACE = nullptr; if (DC->isLocalContext()) { while (DC->getParent()->isLocalContext() && !ACE) { if (auto *closure = dyn_cast(DC)) - if (DiagnoseWalker::isClosureRequiringSelfQualification(closure)) + if (DiagnoseWalker::isClosureRequiringSelfQualification(closure, ctx)) ACE = const_cast(closure); DC = DC->getParent(); } } - auto &ctx = DC->getASTContext(); const_cast(E)->walk(DiagnoseWalker(ctx, ACE)); } @@ -2594,7 +2598,7 @@ class VarDeclUsageChecker : public ASTWalker { VarDecls[vd] |= Flag; } - + void markBaseOfStorageUse(Expr *E, ConcreteDeclRef decl, unsigned flags); void markBaseOfStorageUse(Expr *E, bool isMutating); @@ -3275,7 +3279,7 @@ VarDeclUsageChecker::~VarDeclUsageChecker() { auto initExpr = SC->getCond()[0].getInitializer(); if (initExpr->getStartLoc().isValid()) { unsigned noParens = initExpr->canAppendPostfixExpression(); - + // If the subexpr is an "as?" cast, we can rewrite it to // be an "is" test. ConditionalCheckedCastExpr *CCE = nullptr; @@ -3656,7 +3660,7 @@ void VarDeclUsageChecker::handleIfConfig(IfConfigDecl *ICD) { // conservatively mark it read and written. This will silence "variable // unused" and "could be marked let" warnings for it. if (auto *DRE = dyn_cast(E)) - VDUC.addMark(DRE->getDecl(), RK_Read|RK_Written); + VDUC.addMark(DRE->getDecl(), RK_Read | RK_Written); else if (auto *declRef = dyn_cast(E)) { auto name = declRef->getName(); auto loc = declRef->getLoc(); diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift index 622d7c65c21e2..1f8eb3f7b7ae7 100644 --- a/test/expr/closure/closures.swift +++ b/test/expr/closure/closures.swift @@ -257,16 +257,13 @@ class ExplicitSelfRequiredTest { return 42 } - // The error emitted by these cases cause `VarDeclUsageChecker` to not run analysis on this method, - // because its `sawError` flag is set to true. To preserve the "capture 'y' was never used" warnings - // above, we put these cases in their own method. func weakSelfError() { - doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} - doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} - doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-warning {{variable 'self' was written to, but never read}} + doStuff({ [weak self] in x+1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} + doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-warning {{variable 'self' was written to, but never read}} + doStuff({ [weak self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}} } } @@ -747,19 +744,21 @@ public class TestImplicitSelfForWeakSelfCapture { private init() { doVoidStuff { [weak self] in - method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} guard let self = self else { return } - method() + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + self.method() } doVoidStuff { [weak self] in if let self = self { - method() + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + self.method() } } doVoidStuff { [weak self] in - guard let self = self else { return } + guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} } @@ -768,42 +767,40 @@ public class TestImplicitSelfForWeakSelfCapture { doVoidStuff { [weak self] in guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + self.method() } doVoidStuffNonEscaping { [weak self] in - method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + method() guard let self = self else { return } method() + self.method() } doVoidStuffNonEscaping { [weak self] in if let self = self { method() + self.method() } } doVoidStuff { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional - guard let self = self else { return } + guard let self = self else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}} method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} } doVoidStuffNonEscaping { [weak self] in let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional guard let self = self else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} - } - - doVoidStuffNonEscaping { [weak self] in - guard let self = self else { return } - doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} - } + method() + self.method() } doVoidStuffNonEscaping { [weak self] in guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } - method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + method() + self.method() } } } @@ -824,12 +821,12 @@ public class TestRebindingSelfIsDisallowed { doVoidStuff { [weak self] in let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} } doVoidStuffNonEscaping { [weak self] in let `self` = "self shouldn't become a string" - let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} } } diff --git a/test/expr/closure/closures_swift6.swift b/test/expr/closure/closures_swift6.swift index 92197278ff457..e25567cbfcdda 100644 --- a/test/expr/closure/closures_swift6.swift +++ b/test/expr/closure/closures_swift6.swift @@ -7,8 +7,18 @@ func doVoidStuffNonEscaping(_ fn: () -> ()) {} class ExplicitSelfRequiredTest { var x = 42 - func method() { + func method() -> Int { 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.}} + return 42 + } + + func weakSelfError() { + doVoidStuff({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doStuff({ [weak self] in x+1 }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + doVoidStuff({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} + doStuff({ [weak self] in method() }) // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}} } } @@ -70,3 +80,101 @@ class C_56501 { } } } + +public class TestImplicitSelfForWeakSelfCapture { + static let staticOptional: TestImplicitSelfForWeakSelfCapture? = .init() + func method() { } + + private init() { + doVoidStuff { [weak self] in + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + guard let self = self else { return } + method() + } + + doVoidStuff { [weak self] in + if let self = self { + method() + } + } + + doVoidStuff { [weak self] in + guard let self = self else { return } + doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} + } + } + + doVoidStuff { [weak self] in + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + method() // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note {{reference 'self?.' explicitly}} + guard let self = self else { return } + method() + } + + doVoidStuffNonEscaping { [weak self] in + if let self = self { + method() + } + } + + doVoidStuff { [weak self] in + let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional + guard let self = self else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + let `self`: TestImplicitSelfForWeakSelfCapture? = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional + guard let self = self else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + guard let self = self else { return } + doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}} + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}} + } + } + + doVoidStuffNonEscaping { [weak self] in + guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } + method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + } +} + +public class TestRebindingSelfIsDisallowed { + let count: Void = () + + private init() { + doVoidStuff { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } + + doVoidStuffNonEscaping { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } + + doVoidStuff { [weak self] in + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + + doVoidStuffNonEscaping { [weak self] in + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{reference to property 'count' in closure requires explicit use of 'self' to make capture semantics explicit}} + } + } + + func method() { + let `self` = "self shouldn't become a string" + let _: Int = count // expected-error{{cannot convert value of type 'Void' to specified type 'Int'}} + } +} \ No newline at end of file