8000 [Sema]: diagnose implicit strong captures of weak capture list items · swiftlang/swift@47bf774 · GitHub
[go: up one dir, main page]
Skip to content

Commit 47bf774

Browse files
jamieQjamieQ
committed
[Sema]: diagnose implicit strong captures of weak capture list items
Add a diagnostic to warn when a capture list item creates a weak or unowned binding to a strong Decl and in so doing induces an implicit strong capture of the Decl in a parent closure scope.
1 parent afb4d13 commit 47bf774

File tree

4 files changed

+404
-0
lines changed

4 files changed

+404
-0
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4980,6 +4980,12 @@ ERROR(capture_across_type_decl,none,
49804980
ERROR(reference_to_invalid_decl,none,
49814981
"cannot reference invalid declaration %0", (const ValueDecl *))
49824982

4983+
WARNING(implicit_nonstrong_to_strong_capture,none,
4984+
"%0 capture list item %base1 implicitly captured as strong reference in outer scope",
4985+
(ReferenceOwnership, const Decl *))
4986+
NOTE(implicit_nonstrong_to_strong_capture_loc,none,
4987+
"%base0 implicitly captured here",(const Decl *))
4988+
49834989
//------------------------------------------------------------------------------
49844990
// MARK: Type Check Statements
49854991
//------------------------------------------------------------------------------

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 302 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2587,6 +2587,306 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
25872587
const_cast<Expr *>(E)->walk(DiagnoseWalker(ctx, ACE));
25882588
}
25892589

2590+
// MARK: -
2591+
2592+
/// Diagnose cases where binding a strong reference to a weak/unowned capture list
2593+
/// entry implicitly creates a strong capture of the referenced value in an ancestor
2594+
/// escaping closure.
2595+
static void diagnoseImplicitWeakToStrongCapture(const Expr *E,
2596+
const DeclContext *DC) {
2597+
if (!E || isa<ErrorExpr>(E) || !E->getType())
2598+
return;
2599+
2600+
class Util {
2601+
public:
2602+
static ReferenceOwnership getDeclOwnership(const Decl *D) {
2603+
if (auto attr = D->getAttrs().getAttribute<ReferenceOwnershipAttr>()) {
2604+
return attr->get();
2605+
}
2606+
// Default to strong if unspecified
2607+
return ReferenceOwnership::Strong;
2608+
}
2609+
2610+
static bool isLessThanStrongOwnership(const ReferenceOwnership ownership) {
2611+
return isLessStrongThan(ownership, ReferenceOwnership::Strong);
2612+
} 10000
2613+
2614+
static bool isEscapingClosure(const AbstractClosureExpr *ACE) {
2615+
return !ACE->getType()->isNoEscape();
2616+
}
2617+
2618+
static bool
2619+
isDeclACaptureListItemOfClosure(const Decl *D,
2620+
const AbstractClosureExpr *ACE) {
2621+
if (auto VD = dyn_cast<VarDecl>(D))
2622+
if (auto CLE = VD->getParentCaptureList())
2623+
if (CLE->getClosureBody() == ACE)
2624+
return true;
2625+
2626+
return false;
2627+
}
2628+
2629+
static std::optional<AbstractClosureExpr *>
2630+
ancestorEscapingClosureOfDecl(const Decl *D) {
2631+
DeclContext *DC = D->getDeclContext();
2632+
while (DC) {
2633+
if (auto ACE = dyn_cast<AbstractClosureExpr>(DC))
2634+
if (isEscapingClosure(ACE))
2635+
return ACE;
2636+
2637+
DC = DC->getParent();
2638+
}
2639+
return std::nullopt;
2640+
}
2641+
2642+
static bool isDeclDescendantOfEscapingClosure(const Decl *D) {
2643+
return ancestorEscapingClosureOfDecl(D).has_value();
2644+
}
2645+
};
2646+
2647+
/// Info regarding weak/unowned capture list items who have simple inits
2648+
/// binding to a DeclRefExpr with a corresponding Decl that has strong
2649+
/// ownership (i.e. the capture list item changed ownership from strong
2650+
/// to weak/unowned).
2651+
struct WeakToStrongCaptureItemInfo {
2652+
VarDecl *itemDecl;
2653+
ValueDecl *itemReferent;
2654+
DeclRefExpr *itemInitDRE;
2655+
ReferenceOwnership itemDeclOwnership;
2656+
};
2657+
2658+
class ImplicitWeakToStrongCaptureWalker : public BaseDiagnosticWalker {
2659+
ASTContext &Ctx;
2660+
2661+
public:
2662+
ImplicitWeakToStrongCaptureWalker(ASTContext &ctx) : Ctx(ctx) {}
2663+
2664+
/// Collection of capture item info to potentially diagnose.
2665+
llvm::SmallVector<WeakToStrongCaptureItemInfo, 8> WeakToStrongCaptureItems;
2666+
2667+
/// Stack for tracking the current closure expression context.
2668+
llvm::SmallVector<AbstractClosureExpr *, 8> ClosureStack;
2669+
2670+
/// Tracks DeclRefExprs and associates them with the currently-known closure
2671+
/// context.
2672+
llvm::SmallDenseMap<AbstractClosureExpr *,
2673+
llvm::SmallSetVector<DeclRefExpr *, 16>>
2674+
ClosuresToDeclRefs;
2675+
2676+
/// Number of escaping closures in the `ClosureStack`.
2677+
unsigned AncestorEscapingClosureCount;
2678+
2679+
// We're looking for captures like [weak a], [unowned a = b]
2680+
// here, where the bound DeclRef has strong ownership. If we find a
2681+
// candidate pattern, dig out a DeclRefExpr from some expected locations
2682+
// and record it. These will be used to diagnose capture list entries that
2683+
// bind a weak/unowned var but in so doing induce an implicit strong
2684+
// capture of the associated Decl in an ancestor escaping closure.
2685+
void recordWeakifiedCaptureListDeclIfNeeded(Decl *D) {
2686+
// If there's no ancestor escaping closure, no need to record anything
2687+
// since we can't produce a diagnostic in such cases.
2688+
if (!AncestorEscapingClosureCount)
2689+
return;
2690+
2691+
// We only care about pattern bindings
2692+
auto PBD = dyn_cast<PatternBindingDecl>(D);
2693+
if (!PBD)
2694+
return;
2695+
2696+
// We only handle single variable bindings currently
2697+
auto VD = PBD->getSingleVar();
2698+
if (!VD)
2699+
return;
2700+
2701+
// If this isn't part of a capture list, ignore it
2702+
if (!VD->isCaptureList())
2703+
return;
2704+
2705+
// If the capture list Decl is not weak/unowned, ignore it
2706+
auto ownership = Util::getDeclOwnership(VD);
2707+
if (!Util::isLessThanStrongOwnership(ownership))
2708+
return;
2709+
2710+
// Get the initialization expression
2711+
auto init = PBD->getInit(0);
2712+
if (!init) // TODO: is this possible?
2713+
return;
2714+
2715+
ValueDecl *referencedDecl = init->getReferencedDecl().getDecl();
2716+
2717+
// If we didn't find a referenced Decl for some reason, or it doesn't
2718+
// have strong ownership, there's nothing to diagnose.
2719+
if (!referencedDecl || Util::isLessThanStrongOwnership(
2720+
Util::getDeclOwnership(referencedDecl)))
2721+
return;
2722+
2723+
// TODO: does getSemanticsProvidingExpr make sense to use here?
2724+
2725+
if (auto DRE = dyn_cast<DeclRefExpr>(init)) {
2726+
// Handle DeclRefExpr
2727+
WeakToStrongCaptureItems.push_back(
2728+
{VD, referencedDecl, DRE, ownership});
< F422 /td>
2729+
} else if (auto IIO = dyn_cast<InjectIntoOptionalExpr>(init)) {
2730+
// Handle InjectIntoOptionalExpr
2731+
if (auto DRE = dyn_cast<DeclRefExpr>(IIO->getSubExpr())) {
2732+
WeakToStrongCaptureItems.push_back(
2733+
{VD, referencedDecl, DRE, ownership});
2734+
}
2735+
}
2736+
}
2737+
2738+
void diagnoseImplicitCaptureListOwnershipEscalation() {
2739+
// Set of all the 'weakified' DeclRefExprs for use in the loop below
2740+
SmallPtrSet<DeclRefExpr *, 8> weakifiedDeclRefs;
2741+
for (auto captureItem : WeakToStrongCaptureItems) {
2742+
weakifiedDeclRefs.insert(captureItem.itemInitDRE);
2743+
}
2744+
2745+
// TODO: is this the most straightforward algorithm for this?
2746+
// Go through the list of 'weakified' capture list items and check
2747+
// for ones that reference a Decl for which:
2748+
// - There exists an escaping closure in the DeclContext hierarchy
2749+
// from the capture list entry to the corresponding Decl's context.
2750+
// - Every closure DeclContext between the capture list item & its
2751+
// referenced Decl has no other references to the Decl other than
2752+
// possibly other 'weakified' capture list item bindings.
2753+
for (auto captureItem : WeakToStrongCaptureItems) {
2754+
const auto captureListItemReferent = captureItem.itemReferent;
2755+
bool foundNonWeakifiedReference = false;
2756+
2757+
// Capture list items' DeclContext is the parent of the corresponding
2758+
// closure despite being lexically contained in that closure's syntax.
2759+
DeclContext *DC = captureItem.itemDecl->getDeclContext();
2760+
DeclContext *referentDC = captureListItemReferent->getDeclContext();
2761+
std::optional<AbstractClosureExpr *> ancestorEscapingClosure;
2762+
while (DC && DC != referentDC && !foundNonWeakifiedReference) {
2763+
SWIFT_DEFER { DC = DC->getParent(); };
2764+
2765+
// We only care about closure DeclContexts. All DeclRefs from child
2766+
// DC's of closures should be transitively added during the AST walk,
2767+
// so we'll see them in iteration of a parent DC that is a closure.
2768+
if (!isa<AbstractClosureExpr>(DC))
2769+
continue;
2770+
2771+
auto ACE = cast<AbstractClosureExpr>(DC);
2772+
2773+
// TODO: is this logic correct?
2774+
// If we find an escaping closure, record it as we will want to
2775+
// diagnose it if there are no additional references to the
2776+
// 'weakified' capture list item's referent.
2777+
if (Util::isEscapingClosure(ACE) &&
2778+
!Util::isDeclACaptureListItemOfClosure(captureListItemReferent,
2779+
ACE)) {
2780+
ancestorEscapingClosure = ACE;
2781+
}
2782+
2783+
// Check for references to the capture list item's referent within the
2784+
// current closure context. If we find one that is _not_ also a
2785+
// reference from the initializer expression of a 'weakified' capture
2786+
// list item binding, then that indicates there is an 'explicit' use
2787+
// of the capture so we should _not_ emit a diagnostic. Conversely, if
2788+
// we find no such other references, it indicates every reference
2789+
// we've seen is also a 'weakified' one, so we must keep searching up
2790+
for (auto DRE : ClosuresToDeclRefs[ACE]) {
2791+
// If the reference doesn't refer to the relevant Decl, ignore it
2792+
if (DRE->getReferencedDecl().getDecl() != captureListItemReferent)
2793+
continue;
2794+
2795+
// If the reference is one of the 'weakified' captures, don't
2796+
// treat it as indicating a non-'weakified' use.
2797+
if (weakifiedDeclRefs.contains(DRE))
2798+
continue;
2799+
2800+
// We found a reference that wasn't a 'weakified' capture. Exit the
2801+
// loop and maybe report it.
2802+
foundNonWeakifiedReference = true;
2803+
break;
2804+
}
2805+
}
2806+
// TODO: given our setup we presumably should always have found _some_
2807+
// escaping closure at this point. Should we assert that?
2808+
2809+
// If we detected no additional references within an escaping closure
2810+
// then we should diagnose it as it may be an unintentional strong
2811+
// capture.
2812+
if (!foundNonWeakifiedReference && ancestorEscapingClosure) {
2813+
Ctx.Diags.diagnose(captureItem.itemInitDRE->getLoc(),
2814+
diag::implicit_nonstrong_to_strong_capture,
2815+
captureItem.itemDeclOwnership,
2816+
captureItem.itemReferent);
2817+
Ctx.Diags.diagnose(ancestorEscapingClosure.value()->getLoc(),
2818+
diag::implicit_nonstrong_to_strong_capture_loc,
2819+
captureItem.itemReferent);
2820+
}
2821+
}
2822+
}
2823+
2824+
// MARK: ASTWalker
2825+
2826+
PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
2827+
if (!E || isa<ErrorExpr>(E) || !E->getType())
2828+
return Action::SkipNode(E);
2829+
2830+
// Record info about DeclRefs for later checks for unique references
2831+
if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
2832+
// Record the DRE and associate it with the currently-tracked ACE
2833+
if (!ClosureStack.empty()) {
2834+
ClosuresToDeclRefs[ClosureStack.back()].insert(DRE);
2835+
}
2836+
} else if (auto *ACE = dyn_cast<AbstractClosureExpr>(E)) {
2837+
// Push the new closure context for DeclRef tracking
2838+
ClosureStack.push_back(ACE);
2839+
if (Util::isEscapingClosure(ACE))
2840+
AncestorEscapingClosureCount += 1;
2841+
}
2842+
2843+
return Action::Continue(E);
2844+
}
2845+
2846+
PostWalkResult<Expr *> walkToExprPost(Expr *E) override {
2847+
if (isa<AbstractClosureExpr>(E)) {
2848+
assert(!ClosureStack.empty());
2849+
auto closure = ClosureStack.pop_back_val();
2850+
if (Util::isEscapingClosure(closure))
2851+
AncestorEscapingClosureCount -= 1;
2852+
2853+
// TODO: is this right?
2854+
/* We may need something like it to properly handle situations
2855+
like this:
2856+
2857+
```
2858+
escaping {
2859+
escaping { [weak self] in ... }
2860+
let aNewScope = { self.stuff() }
2861+
}
2862+
```
2863+
2864+
We probably don't want to warn in such cases since there are
2865+
'explicit' references to the weak capture lexically contained
2866+
in the outer-most escaping closure.
2867+
*/
2868+
// When we exit a closure, add all recorded references to its parent.
2869+
if (!ClosureStack.empty()) {
2870+
auto parent = ClosuresToDeclRefs[ClosureStack.back()];
2871+
for (auto descendantDeclRef : ClosuresToDeclRefs[closure])
2872+
parent.insert(descendantDeclRef);
2873+
}
2874+
}
2875+
return Action::Continue(E);
2876+
}
2877+
2878+
PreWalkAction walkToDeclPre(Decl *D) override {
2879+
// Check for capture list entries to record info about them
2880+
recordWeakifiedCaptureListDeclIfNeeded(D);
2881+
return Action::Continue();
2882+
}
2883+
};
2884+
2885+
ImplicitWeakToStrongCaptureWalker Walker(DC->getASTContext());
2886+
const_cast<Expr *>(E)->walk(Walker);
2887+
Walker.diagnoseImplicitCaptureListOwnershipEscalation();
2888+
}
2889+
25902890
bool TypeChecker::getDefaultGenericArgumentsString(
25912891
SmallVectorImpl<char> &buf,
25922892
const swift::GenericTypeDecl *typeDecl,
@@ -6229,6 +6529,8 @@ void swift::performSyntacticExprDiagnostics(const Expr *E,
62296529
diagSyntacticUseRestrictions(E, DC, isExprStmt);
62306530
diagRecursivePropertyAccess(E, DC);
62316531
diagnoseImplicitSelfUseInClosure(E, DC);
6532+
// jqTestDiag(E, DC);
6533+
diagnoseImplicitWeakToStrongCapture(E, DC);
62326534
diagnoseUnintendedOptionalBehavior(E, DC);
62336535
maybeDiagnoseCallToKeyValueObserveMethod(E, DC);
62346536
diagnoseExplicitUseOfLazyVariableStorage(E, DC);

‎test/expr/closure/closures.swift

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,8 @@ do {
552552

553553
class C {
554554
init() {
555+
// expected-note@+3{{'self' implicitly captured here}}
556+
// expected-warning@+2{{'unowned' capture list item 'self' implicitly captured as strong reference in outer scope}}
555557
// expected-warning@+1{{capture 'self' was never used}}
556558
let _ = f { fn in { [unowned self, fn] x in x != 1000 ? fn(x + 1) : "success" } }(0)
557559
}

0 commit comments

Comments
 (0)
0