Skip to content

[IDE] Ensure every walkToPre call is matched with a walkToPost call in SemaAnnotator #36628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2021
Merged

[IDE] Ensure every walkToPre call is matched with a walkToPost call in SemaAnnotator #36628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions include/swift/IDE/SourceEntityWalker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ namespace swift {
/// An abstract class used to traverse the AST and provide source information.
/// Visitation happens in source-order and compiler-generated semantic info,
/// like implicit declarations, is ignored.
///
/// If \c walkTo*Pre returns \c true, the children are visited and \c
/// walkTo*Post is called after all children have been visited.
/// If \c walkTo*Pre returns \c false, the corresponding \c walkTo*Post call
/// will not be issued.
///
/// If \c walkTo*Post returns \c false, the traversal is terminated. No more
/// \c walk* calls are issued. Nodes that have already received a \c walkTo*Pre
/// call will *not* receive a \c walkTo*Post call.
/// If \c walkTo*Post returns \c true, the traversal continues.
class SourceEntityWalker {
public:
/// Walks the provided source file.
Expand Down
146 changes: 89 additions & 57 deletions lib/IDE/SourceEntityWalker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,13 @@ bool SemaAnnotator::walkToDeclPre(Decl *D) {
if (isDone())
return false;

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

if (!handleCustomAttributes(D)) {
Cancelled = true;
Expand All @@ -134,15 +139,15 @@ bool SemaAnnotator::walkToDeclPre(Decl *D) {
if (!SEWalker.visitDeclarationArgumentName(PD->getArgumentName(), Loc,
VD)) {
Cancelled = true;
return true;
return false;
Comment on lines -137 to +142
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh. Nathan and I were bamboozled by this one as well. This is actually in a lambda where the true/false is reversed. I'm happy for that to be changed/something else, but the false here actually means "no error". Ie. the check below is:

if (ReportParamList(ParamList)) return false;

So this should be true as is, not false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But… why? Before anyone gets hit by this again, I’m inverting the return value of ReportParamList.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why indeed 😆. Sounds good to me!

}
}
return false;
return true;
};

if (isa<AbstractFunctionDecl>(VD) || isa<SubscriptDecl>(VD)) {
auto ParamList = getParameterList(VD);
if (ReportParamList(ParamList))
if (!ReportParamList(ParamList))
return false;
}
} else if (auto *ED = dyn_cast<ExtensionDecl>(D)) {
Expand Down Expand Up @@ -208,6 +213,10 @@ bool SemaAnnotator::walkToDeclPost(Decl *D) {
}

std::pair<bool, Stmt *> SemaAnnotator::walkToStmtPre(Stmt *S) {
if (isDone()) {
return { false, nullptr };
}

bool TraverseChildren = SEWalker.walkToStmtPre(S);
if (TraverseChildren) {
if (auto *DeferS = dyn_cast<DeferStmt>(S)) {
Expand All @@ -230,6 +239,10 @@ std::pair<bool, Stmt *> SemaAnnotator::walkToStmtPre(Stmt *S) {
}

Stmt *SemaAnnotator::walkToStmtPost(Stmt *S) {
if (isDone()) {
return nullptr;
}

bool Continue = SEWalker.walkToStmtPost(S);
if (!Continue)
Cancelled = true;
Expand All @@ -247,23 +260,36 @@ static SemaReferenceKind getReferenceKind(Expr *Parent, Expr *E) {
std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
assert(E);

std::pair<bool, Expr *> stopTraversal = { false, nullptr };
std::pair<bool, Expr *> skipChildren = { false, E };
if (isDone()) {
return { false, nullptr };
}

auto doSkipChildren = [&]() -> std::pair<bool, Expr *> {
if (!SEWalker.walkToExprPost(E))
return stopTraversal;
return skipChildren;
};
if (ExprsToSkip.count(E) != 0) {
// We are skipping the expression. Call neither walkToExprPr nor
// walkToExprPost on it
return { false, E };
}

if (isDone())
return stopTraversal;
if (!SEWalker.walkToExprPre(E)) {
return { false, E };
}

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

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

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

return doSkipChildren();
}
Expand Down Expand Up @@ -311,12 +337,12 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
if (auto *module = dyn_cast<ModuleDecl>(DRE->getDecl())) {
if (!passReference(ModuleEntity(module),
{module->getName(), E->getLoc()}))
return stopTraversal;
return doStopTraversal();
} else if (!passReference(DRE->getDecl(), DRE->getType(),
DRE->getNameLoc(),
ReferenceMetaData(getReferenceKind(Parent.getAsExpr(), DRE),
OpAccess))) {
return stopTraversal;
return doStopTraversal();
}
} else if (auto *MRE = dyn_cast<MemberRefExpr>(E)) {
{
Expand All @@ -335,14 +361,14 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {

// Visit in source order.
if (!MRE->getBase()->walk(*this))
return stopTraversal;
return doStopTraversal();
}

if (!passReference(MRE->getMember().getDecl(), MRE->getType(),
MRE->getNameLoc(),
ReferenceMetaData(SemaReferenceKind::DeclMemberRef,
OpAccess)))
return stopTraversal;
return doStopTraversal();

// We already visited the children.
return doSkipChildren();
Expand All @@ -352,12 +378,12 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
OtherCtorE->getConstructorLoc(),
ReferenceMetaData(SemaReferenceKind::DeclConstructorRef,
OpAccess)))
return stopTraversal;
return doStopTraversal();

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

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

if (SubscrD) {
if (!passSubscriptReference(SubscrD, E->getLoc(), data, true))
return stopTraversal;
return doStopTraversal();
}

if (!SE->getIndex()->walk(*this))
return stopTraversal;
return doStopTraversal();

if (SubscrD) {
if (!passSubscriptReference(SubscrD, E->getEndLoc(), data, false))
return stopTraversal;
return doStopTraversal();
}

// We already visited the children.
Expand Down Expand Up @@ -414,37 +440,35 @@ std::pair<bool, Expr *> SemaAnnotator::walkToExprPre(Expr *E) {
} else if (auto *BinE = dyn_cast<BinaryExpr>(E)) {
// Visit in source order.
if (!BinE->getArg()->getElement(0)->walk(*this))
return stopTraversal;
return doStopTraversal();
if (!BinE->getFn()->walk(*this))
return stopTraversal;
return doStopTraversal();
if (!BinE->getArg()->getElement(1)->walk(*this))
return stopTraversal;
return doStopTraversal();

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

} else if (auto TupleE = dyn_cast<TupleExpr>(E)) {
if (auto CallE = dyn_cast_or_null<CallExpr>(Parent.getAsExpr())) {
if (!passCallArgNames(CallE->getFn(), TupleE))
return stopTraversal;
return doStopTraversal();
}
} else if (auto IOE = dyn_cast<InOutExpr>(E)) {
llvm::SaveAndRestore<Optional<AccessKind>>
C(this->OpAccess, AccessKind::ReadWrite);

if (!IOE->getSubExpr()->walk(*this))
return stopTraversal;
return doStopTraversal();

// We already visited the children.
if (!walkToExprPost(E))
return stopTraversal;
return skipChildren;
return doSkipChildren();
} else if (auto LE = dyn_cast<LoadExpr>(E)) {
llvm::SaveAndRestore<Optional<AccessKind>>
C(this->OpAccess, AccessKind::Read);

if (!LE->getSubExpr()->walk(*this))
return stopTraversal;
return doStopTraversal();

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

if (AE->getDest() && !AE->getDest()->walk(*this))
return stopTraversal;
return doStopTraversal();
}

if (AE->getSrc() && !AE->getSrc()->walk(*this))
return stopTraversal;
return doStopTraversal();

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

if (!OEE->getSubExpr()->walk(*this))
return stopTraversal;
return doStopTraversal();

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

// Original non-escaping closure.
if (!MTEE->getNonescapingClosureValue()->walk(*this))
return stopTraversal;
return doStopTraversal();

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

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

return doSkipChildren();
} else if (auto OVE = dyn_cast<OpaqueValueExpr>(E)) {
// Walk into mapped value.
auto value = OpaqueValueMap.find(OVE);
if (value != OpaqueValueMap.end()) {
if (!value->second->walk(*this))
return stopTraversal;
return doStopTraversal();

return doSkipChildren();
}
} else if (auto DMRE = dyn_cast<DynamicMemberRefExpr>(E)) {
// Visit in source order.
if (!DMRE->getBase()->walk(*this))
return stopTraversal;
return doStopTraversal();
if (!passReference(DMRE->getMember().getDecl(), DMRE->getType(),
DMRE->getNameLoc(),
ReferenceMetaData(SemaReferenceKind::DynamicMemberRef,
OpAccess)))
return stopTraversal;
return doStopTraversal();
// We already visited the children.
return doSkipChildren();
}

return { true, E };
}

Expr *SemaAnnotator::walkToExprPost(Expr *E) {
if (isDone()) {
return nullptr;
}

if (isa<ConstructorRefCallExpr>(E)) {
assert(CtorRefs.back() == E);
CtorRefs.pop_back();
}

bool Continue = SEWalker.walkToExprPost(E);
if (!Continue)
Cancelled = true;
return Continue ? E : nullptr;
}

bool SemaAnnotator::walkToTypeReprPre(TypeRepr *T) {
if (isDone())
return false;
Expand All @@ -538,23 +578,15 @@ bool SemaAnnotator::walkToTypeReprPre(TypeRepr *T) {
return true;
}

Expr *SemaAnnotator::walkToExprPost(Expr *E) {
if (isa<ConstructorRefCallExpr>(E)) {
assert(CtorRefs.back() == E);
CtorRefs.pop_back();
}

bool Continue = SEWalker.walkToExprPost(E);
if (!Continue)
Cancelled = true;
return Continue ? E : nullptr;
}

bool SemaAnnotator::walkToTypeReprPost(TypeRepr *T) {
return !isDone();
}

std::pair<bool, Pattern *> SemaAnnotator::walkToPatternPre(Pattern *P) {
if (isDone()) {
return { false, nullptr };
}

if (P->isImplicit())
return { true, P };

Expand Down
15 changes: 15 additions & 0 deletions test/IDE/range_info_expr.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ func testWithoutActuallyEscaping(closure: (Int) -> Void) {
}
}

class SR12957 {
static let main = SR12957()
}

func bar(x: SR12957 = .main) {}

// RUN: %target-swift-ide-test -range -pos=7:8 -end-pos=7:19 -source-filename %s | %FileCheck %s -check-prefix=CHECK-BOOL
// CHECK-BOOL: <Type>Bool</Type>

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

// CHECK-PART-EXPR: <Kind>PartOfExpression</Kind>
// CHECK-PART-EXPR-NEXT: <Content>getSelf()</Content>
Expand Down Expand Up @@ -107,3 +114,11 @@ func testWithoutActuallyEscaping(closure: (Int) -> Void) {
// CHECK-MTEE-EXPR-3-NEXT: <Referenced>escapable</Referenced><Type>(Int) -> Void</Type>
// CHECK-MTEE-EXPR-3-NEXT: <ASTNodes>1</ASTNodes>
// CHECK-MTEE-EXPR-3-NEXT: <end>

// CHECK-DEFAULT-ARGUMENT: <Kind>SingleExpression</Kind>
// CHECK-DEFAULT-ARGUMENT-NEXT: <Content>.main</Content>
// CHECK-DEFAULT-ARGUMENT-NEXT: <Type>SR12957</Type><Exit>false</Exit>
// CHECK-DEFAULT-ARGUMENT-NEXT: <Context>swift_ide_test.(file).bar(x:)</Context>
// CHECK-DEFAULT-ARGUMENT-NEXT: <ASTNodes>1</ASTNodes>
// CHECK-DEFAULT-ARGUMENT-NEXT: <end>