Skip to content

Commit 981b663

Browse files
ahoppenahoppen
committed
[IDE] Ensure every walkToPre call is matched with a walkToPost call in SemaAnnotator
We had some unbalanced calls of `walkTo*Pre` and `walkTo*Post` in `SemaAnnotator`. The main fix was to set `Cancelled` to `true` if traversal is being stopped in `walkToExprPre`. While I was at it, I also - Added some more checks, ensuring that no more `walkTo*` calls are issued after `Cancelled` has been set to `true`. - Added some comments, describing the intended traversal behaviour. - Inverted the return value of the `ReportParamList` lambda to be in line with the return value of the enclosing `walkToDeclPre` - Moved `walkToExprPost` to be place right after `walkToExprPre` Resolves rdar://64139829 [SR-12957]
1 parent 74b5eb7 commit 981b663

File tree

3 files changed

+114
-57
lines changed

3 files changed

+114
-57
lines changed

‎include/swift/IDE/SourceEntityWalker.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,16 @@ namespace swift {
4141
/// An abstract class used to traverse the AST and provide source information.
4242
/// Visitation happens in source-order and compiler-generated semantic info,
4343
/// like implicit declarations, is ignored.
44+
///
45+
/// If \c walkTo*Pre returns \c true, the children are visited and \c
46+
/// walkTo*Post is called after all children have been visited.
47+
/// If \c walkTo*Pre returns \c false, the corresponding \c walkTo*Post call
48+
/// will not be issued.
49+
///
50+
/// If \c walkTo*Post returns \c false, the traversal is terminated. No more
51+
/// \c walk* calls are issued. Nodes that have already received a \c walkTo*Pre
52+
/// call will *not* receive a \c walkTo*Post call.
53+
/// If \c walkTo*Post returns \c true, the traversal continues.
4454
class SourceEntityWalker {
4555
public:
4656
/// Walks the provided source file.

‎lib/IDE/SourceEntityWalker.cpp

Lines changed: 89 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,13 @@ bool SemaAnnotator::walkToDeclPre(Decl *D) {
106106
if (isDone())
107107
return false;
108108

109-
if (shouldIgnore(D))
109+
if (shouldIgnore(D)) {
110+
// If we return true here, the children will still be visited, but we won't
111+
// call walkToDeclPre on SEWalker. The corresponding walkToDeclPost call
112+
// on SEWalker will be prevented by the check for shouldIgnore in
113+
// walkToDeclPost in SemaAnnotator.
110114
return isa<PatternBindingDecl>(D);
115+
}
111116

112117
if (!handleCustomAttributes(D)) {
113118
Cancelled = true;
@@ -134,15 +139,15 @@ bool SemaAnnotator::walkToDeclPre(Decl *D) {
134139
if (!SEWalker.visitDeclarationArgumentName(PD->getArgumentName(), Loc,
135140
VD)) {
136141
Cancelled = true;
137-
return true;
142+
return false;
138143
}
139144
}
140-
return false;
145+
return true;
141146
};
142147

143148
if (isa<AbstractFunctionDecl>(VD) || isa<SubscriptDecl>(VD)) {
144149
auto ParamList = getParameterList(VD);
145-
if (ReportParamList(ParamList))
150+
if (!ReportParamList(ParamList))
146151
return false;
147152
}
148153
} else if (auto *ED = dyn_cast<ExtensionDecl>(D)) {
@@ -208,6 +213,10 @@ bool SemaAnnotator::walkToDeclPost(Decl *D) {
208213
}
209214

210215
std::pair<bool, Stmt *> SemaAnnotator::walkToStmtPre(Stmt *S) {
216+
if (isDone()) {
217+
return { false, nullptr };
218+
}
219+
211220
bool TraverseChildren = SEWalker.walkToStmtPre(S);
212221
if (TraverseChildren) {
213222
if (auto *DeferS = dyn_cast<DeferStmt>(S)) {
@@ -230,6 +239,10 @@ std::pair<bool, Stmt *> SemaAnnotator::walkToStmtPre(Stmt *S) {
230239
}
231240

232241
Stmt *SemaAnnotator::walkToStmtPost(Stmt *S) {
242+
if (isDone()) {
243+
return nullptr;
244+
}
245+
233246
bool Continue = SEWalker.walkToStmtPost(S);
234247
if (!Continue)
235248
Cancelled = true;
@@ -247,23 +260,36 @@ static SemaReferenceKind getReferenceKind(Expr *Parent, Expr *E) {
247260
std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
248261
assert(E);
249262

250-
std::pair<bool, Expr *> stopTraversal = { false, nullptr };
251-
std::pair<bool, Expr *> skipChildren = { false, E };
263+
if (isDone()) {
264+
return { false, nullptr };
265+
}
252266

253-
auto doSkipChildren = [&]() -> std::pair<bool, Expr *> {
254-
if (!SEWalker.walkToExprPost(E))
255-
return stopTraversal;
256-
return skipChildren;
257-
};
267+
if (ExprsToSkip.count(E) != 0) {
268+
// We are skipping the expression. Call neither walkToExprPr nor
269+
// walkToExprPost on it
270+
return { false, E };
271+
}
258272

259-
if (isDone())
260-
return stopTraversal;
273+
if (!SEWalker.walkToExprPre(E)) {
274+
return { false, E };
275+
}
261276

262-
if (ExprsToSkip.count(E) != 0)
263-
return skipChildren;
277+
auto doSkipChildren = [&]() -> std::pair<bool, Expr *> {
278+
// If we decide to skip the children after having issued the call to
279+
// walkToExprPre, we need to simulate a corresponding call to walkToExprPost
280+
// which will not be issued by the ASTWalker if we return false in the first
281+
// component.
282+
if (!walkToExprPost(E)) {
283+
// walkToExprPost has cancelled the traversal. Stop.
284+
return { false, nullptr };
285+
}
286+
return { false, E };
287+
};
264288

265-
if (!SEWalker.walkToExprPre(E))
266-
return skipChildren;
289+
auto doStopTraversal = [&]() -> std::pair<bool, Expr *> {
290+
Cancelled = true;
291+
return { false, nullptr };
292+
};
267293

268294
if (auto *CtorRefE = dyn_cast<ConstructorRefCallExpr>(E))
269295
CtorRefs.push_back(CtorRefE);
@@ -275,7 +301,7 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
275301
DRE->getNameLoc(),
276302
ReferenceMetaData(getReferenceKind(Parent.getAsExpr(), DRE),
277303
OpAccess)))
278-
return stopTraversal;
304+
return doStopTraversal();
279305

280306
return doSkipChildren();
281307
}
@@ -311,12 +337,12 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
311337
if (auto *module = dyn_cast<ModuleDecl>(DRE->getDecl())) {
312338
if (!passReference(ModuleEntity(module),
313339
{module->getName(), E->getLoc()}))
314-
return stopTraversal;
340+
return doStopTraversal();
315341
} else if (!passReference(DRE->getDecl(), DRE->getType(),
316342
DRE->getNameLoc(),
317343
ReferenceMetaData(getReferenceKind(Parent.getAsExpr(), DRE),
318344
OpAccess))) {
319-
return stopTraversal;
345+
return doStopTraversal();
320346
}
321347
} else if (auto *MRE = dyn_cast<MemberRefExpr>(E)) {
322348
{
@@ -335,14 +361,14 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
335361

336362
// Visit in source order.
337363
if (!MRE->getBase()->walk(*this))
338-
return stopTraversal;
364+
return doStopTraversal();
339365
}
340366

341367
if (!passReference(MRE->getMember().getDecl(), MRE->getType(),
342368
MRE->getNameLoc(),
343369
ReferenceMetaData(SemaReferenceKind::DeclMemberRef,
344370
OpAccess)))
345-
return stopTraversal;
371+
return doStopTraversal();
346372

347373
// We already visited the children.
348374
return doSkipChildren();
@@ -352,12 +378,12 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
352378
OtherCtorE->getConstructorLoc(),
353379
ReferenceMetaData(SemaReferenceKind::DeclConstructorRef,
354380
OpAccess)))
355-
return stopTraversal;
381+
return doStopTraversal();
356382

357383
} else if (auto *SE = dyn_cast<SubscriptExpr>(E)) {
358384
// Visit in source order.
359385
if (!SE->getBase()->walk(*this))
360-
return stopTraversal;
386+
return doStopTraversal();
361387

362388
ValueDecl *SubscrD = nullptr;
363389
if (SE->hasDecl())
@@ -368,15 +394,15 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
368394

369395
if (SubscrD) {
370396
if (!passSubscriptReference(SubscrD, E->getLoc(), data, true))
371-
return stopTraversal;
397+
return doStopTraversal();
372398
}
373399

374400
if (!SE->getIndex()->walk(*this))
375-
return stopTraversal;
401+
return doStopTraversal();
376402

377403
if (SubscrD) {
378404
if (!passSubscriptReference(SubscrD, E->getEndLoc(), data, false))
379-
return stopTraversal;
405+
return doStopTraversal();
380406
}
381407

382408
// We already visited the children.
@@ -414,37 +440,35 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
414440
} else if (auto *BinE = dyn_cast<BinaryExpr>(E)) {
415441
// Visit in source order.
416442
if (!BinE->getArg()->getElement(0)->walk(*this))
417-
return stopTraversal;
443+
return doStopTraversal();
418444
if (!BinE->getFn()->walk(*this))
419-
return stopTraversal;
445+
return doStopTraversal();
420446
if (!BinE->getArg()->getElement(1)->walk(*this))
421-
return stopTraversal;
447+
return doStopTraversal();
422448

423449
// We already visited the children.
424450
return doSkipChildren();
425451

426452
} else if (auto TupleE = dyn_cast<TupleExpr>(E)) {
427453
if (auto CallE = dyn_cast_or_null<CallExpr>(Parent.getAsExpr())) {
428454
if (!passCallArgNames(CallE->getFn(), TupleE))
429-
return stopTraversal;
455+
return doStopTraversal();
430456
}
431457
} else if (auto IOE = dyn_cast<InOutExpr>(E)) {
432458
llvm::SaveAndRestore<Optional<AccessKind>>
433459
C(this->OpAccess, AccessKind::ReadWrite);
434460

435461
if (!IOE->getSubExpr()->walk(*this))
436-
return stopTraversal;
462+
return doStopTraversal();
437463

438464
// We already visited the children.
439-
if (!walkToExprPost(E))
440-
return stopTraversal;
441-
return skipChildren;
465+
return doSkipChildren();
442466
} else if (auto LE = dyn_cast<LoadExpr>(E)) {
443467
llvm::SaveAndRestore<Optional<AccessKind>>
444468
C(this->OpAccess, AccessKind::Read);
445469

446470
if (!LE->getSubExpr()->walk(*this))
447-
return stopTraversal;
471+
return doStopTraversal();
448472

449473
// We already visited the children.
450474
return doSkipChildren();
@@ -454,11 +478,11 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
454478
C(this->OpAccess, AccessKind::Write);
455479

456480
if (AE->getDest() && !AE->getDest()->walk(*this))
457-
return stopTraversal;
481+
return doStopTraversal();
458482
}
459483

460484
if (AE->getSrc() && !AE->getSrc()->walk(*this))
461-
return stopTraversal;
485+
return doStopTraversal();
462486

463487
// We already visited the children.
464488
return doSkipChildren();
@@ -470,7 +494,7 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
470494
};
471495

472496
if (!OEE->getSubExpr()->walk(*this))
473-
return stopTraversal;
497+
return doStopTraversal();
474498

475499
return doSkipChildren();
476500
} else if (auto MTEE = dyn_cast<MakeTemporarilyEscapableExpr>(E)) {
@@ -479,47 +503,63 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
479503

480504
// Original non-escaping closure.
481505
if (!MTEE->getNonescapingClosureValue()->walk(*this))
482-
return stopTraversal;
506+
return doStopTraversal();
483507

484508
// Body, which is called by synthesized CallExpr.
485509
auto *callExpr = cast<CallExpr>(MTEE->getSubExpr());
486510
if (!callExpr->getFn()->walk(*this))
487-
return stopTraversal;
511+
return doStopTraversal();
488512

489513
return doSkipChildren();
490514
} else if (auto CUCE = dyn_cast<CollectionUpcastConversionExpr>(E)) {
491515
// Ignore conversion expressions. We don't handle OpaqueValueExpr here
492516
// because it's only in conversion expressions. Instead, just walk into
493517
// sub expression.
494518
if (!CUCE->getSubExpr()->walk(*this))
495-
return stopTraversal;
519+
return doStopTraversal();
496520

497521
return doSkipChildren();
498522
} else if (auto OVE = dyn_cast<OpaqueValueExpr>(E)) {
499523
// Walk into mapped value.
500524
auto value = OpaqueValueMap.find(OVE);
501525
if (value != OpaqueValueMap.end()) {
502526
if (!value->second->walk(*this))
503-
return stopTraversal;
527+
return doStopTraversal();
504528

505529
return doSkipChildren();
506530
}
507531
} else if (auto DMRE = dyn_cast<DynamicMemberRefExpr>(E)) {
508532
// Visit in source order.
509533
if (!DMRE->getBase()->walk(*this))
510-
return stopTraversal;
534+
return doStopTraversal();
511535
if (!passReference(DMRE->getMember().getDecl(), DMRE->getType(),
512536
DMRE->getNameLoc(),
513537
ReferenceMetaData(SemaReferenceKind::DynamicMemberRef,
514538
OpAccess)))
515-
return stopTraversal;
539+
return doStopTraversal();
516540
// We already visited the children.
517541
return doSkipChildren();
518542
}
519543

520544
return { true, E };
521545
}
522546

547+
Expr *SemaAnnotator::walkToExprPost(Expr *E) {
548+
if (isDone()) {
549+
return nullptr;
550+
}
551+
552+
if (isa<ConstructorRefCallExpr>(E)) {
553+
assert(CtorRefs.back() == E);
554+
CtorRefs.pop_back();
555+
}
556+
557+
bool Continue = SEWalker.walkToExprPost(E);
558+
if (!Continue)
559+
Cancelled = true;
560+
return Continue ? E : nullptr;
561+
}
562+
523563
bool SemaAnnotator::walkToTypeReprPre(TypeRepr *T) {
524564
if (isDone())
525565
return false;
@@ -538,23 +578,15 @@ bool SemaAnnotator::walkToTypeReprPre(TypeRepr *T) {
538578
return true;
539579
}
540580

541-
Expr *SemaAnnotator::walkToExprPost(Expr *E) {
542-
if (isa<ConstructorRefCallExpr>(E)) {
543-
assert(CtorRefs.back() == E);
544-
CtorRefs.pop_back();
545-
}
546-
547-
bool Continue = SEWalker.walkToExprPost(E);
548-
if (!Continue)
549-
Cancelled = true;
550-
return Continue ? E : nullptr;
551-
}
552-
553581
bool SemaAnnotator::walkToTypeReprPost(TypeRepr *T) {
554582
return !isDone();
555583
}
556584

557585
std::pair<bool, Pattern *> SemaAnnotator::walkToPatternPre(Pattern *P) {
586+
if (isDone()) {
587+
return { false, nullptr };
588+
}
589+
558590
if (P->isImplicit())
559591
return { true, P };
560592

‎test/IDE/range_info_expr.swift

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ func testWithoutActuallyEscaping(closure: (Int) -> Void) {
3737
}
3838
}
3939

40+
class SR12957 {
41+
static let main = SR12957()
42+
}
43+
44+
func bar(x: SR12957 = .main) {}
45+
4046
// RUN: %target-swift-ide-test -range -pos=7:8 -end-pos=7:19 -source-filename %s | %FileCheck %s -check-prefix=CHECK-BOOL
4147
// CHECK-BOOL: <Type>Bool</Type>
4248

@@ -55,6 +61,7 @@ func testWithoutActuallyEscaping(closure: (Int) -> Void) {
5561
// RUN: %target-swift-ide-test -range -pos=35:1 -end-pos=38:1 -source-filename %s | %FileCheck %s -check-prefix=CHECK-MTEE-EXPR-1
5662
// RUN: %target-swift-ide-test -range -pos=35:27 -end-pos=35:34 -source-filename %s | %FileCheck %s -check-prefix=CHECK-MTEE-EXPR-2
5763
// RUN: %target-swift-ide-test -range -pos=35:36 -end-pos=37:4 -source-filename %s | %FileCheck %s -check-prefix=CHECK-MTEE-EXPR-3
64+
// RUN: %target-swift-ide-test -range -pos=44:23 -end-pos=44:28 -source-filename %s | %FileCheck %s -check-prefix=CHECK-DEFAULT-ARGUMENT
5865

5966
// CHECK-PART-EXPR: <Kind>PartOfExpression</Kind>
6067
// CHECK-PART-EXPR-NEXT: <Content>getSelf()</Content>
@@ -107,3 +114,11 @@ func testWithoutActuallyEscaping(closure: (Int) -> Void) {
107114
// CHECK-MTEE-EXPR-3-NEXT: <Referenced>escapable</Referenced><Type>(Int) -> Void</Type>
108115
// CHECK-MTEE-EXPR-3-NEXT: <ASTNodes>1</ASTNodes>
109116
// CHECK-MTEE-EXPR-3-NEXT: <end>
117+
118+
// CHECK-DEFAULT-ARGUMENT: <Kind>SingleExpression</Kind>
119+
// CHECK-DEFAULT-ARGUMENT-NEXT: <Content>.main</Content>
120+
// CHECK-DEFAULT-ARGUMENT-NEXT: <Type>SR12957</Type><Exit>false</Exit>
121+
// CHECK-DEFAULT-ARGUMENT-NEXT: <Context>swift_ide_test.(file).bar(x:)</Context>
122+
// CHECK-DEFAULT-ARGUMENT-NEXT: <ASTNodes>1</ASTNodes>
123+
// CHECK-DEFAULT-ARGUMENT-NEXT: <end>
124+

0 commit comments

Comments
 (0)