Skip to content

[SourceKit] Don't use SourceEntitityWalker for placeholder expansion #73394

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions test/SourceKit/CodeExpand/code-expand-no-typecheck.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// RUN: %sourcekitd-test -req=expand-placeholder %s | %FileCheck %s

// FIXME: Make it acccept '-debug-forbid-typecheck-prefix' and ensure no typecheck happens.'

protocol MyProto {}

enum MyEnum: Hashable, MyProto {
case foo
case bar
}

func test(array: [MyEnum]) -> [NyEnum] {
array.filter(<#T##isIncluded: (MyEnum) throws -> Bool##(MyEnum) throws -> Bool#>)
// CHECK: array.filter { <#MyEnum#> in
// CHECK-NEXT: <#code#>
// CHECK-NEXT: }
}
38 changes: 27 additions & 11 deletions tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1695,7 +1695,7 @@ class PlaceholderExpansionScanner {
std::pair<ArgumentList *, bool> enclosingCallExprArg(SourceFile &SF,
SourceLoc SL) {

class CallExprFinder : public SourceEntityWalker {
class CallExprFinder : public ASTWalker {
public:
const SourceManager &SM;
SourceLoc TargetLoc;
Expand All @@ -1719,7 +1719,11 @@ class PlaceholderExpansionScanner {
return true;
}

bool walkToExprPre(Expr *E) override {
MacroWalking getMacroWalkingBehavior() const override {
return MacroWalking::Arguments;
}

PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
auto SR = E->getSourceRange();
if (SR.isValid() && SM.rangeContainsTokenLoc(SR, TargetLoc) &&
!checkCallExpr(E) && !EnclosingCallAndArg.first) {
Expand All @@ -1732,13 +1736,13 @@ class PlaceholderExpansionScanner {
OuterExpr = E;
}
}
return true;
return Action::Continue(E);
}

bool walkToExprPost(Expr *E) override {
PostWalkResult<Expr *> walkToExprPost(Expr *E) override {
if (E->getStartLoc() == TargetLoc)
return false; // found what we needed to find, stop walking.
return true;
return Action::Stop(); // found what we needed to find, stop walking.
return Action::Continue(E);
Comment on lines 1743 to +1745
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use Action::StopIf here if you want

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thanks for the tip! but let me merge this as is for now 🙏

}

/// Whether this statement body consists of only an implicit "return",
Expand All @@ -1756,7 +1760,7 @@ class PlaceholderExpansionScanner {

// Before pre-checking, the implicit return will not have been
// inserted. Look for a single expression body in a closure.
if (auto *ParentE = getWalker().Parent.getAsExpr()) {
if (auto *ParentE = Parent.getAsExpr()) {
if (isa<ClosureExpr>(ParentE)) {
if (auto *innerE = BS->getSingleActiveExpression())
return innerE->getStartLoc() == TargetLoc;
Expand All @@ -1767,7 +1771,7 @@ class PlaceholderExpansionScanner {
return false;
}

bool walkToStmtPre(Stmt *S) override {
PreWalkResult<Stmt *> walkToStmtPre(Stmt *S) override {
auto SR = S->getSourceRange();
if (SR.isValid() && SM.rangeContainsTokenLoc(SR, TargetLoc) &&
!isImplicitReturnBody(S)) {
Expand All @@ -1791,17 +1795,29 @@ class PlaceholderExpansionScanner {
break;
}
}
return true;
return Action::Continue(S);
}

bool shouldWalkInactiveConfigRegion() override { return true; }
PreWalkAction walkToDeclPre(Decl *D) override {
if (auto *ICD = dyn_cast<IfConfigDecl>(D)) {
for (auto Clause : ICD->getClauses()) {
// Active clase elements are visited normally.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Active clase elements are visited normally.
// Active clause elements are visited normally.

if (Clause.isActive)
continue;
for (auto Member : Clause.Elements)
Member.walk(*this);
}
return Action::SkipNode();
}
return Action::Continue();
}

ArgumentList *findEnclosingCallArg(SourceFile &SF, SourceLoc SL) {
EnclosingCallAndArg = {nullptr, nullptr};
OuterExpr = nullptr;
OuterStmt = nullptr;
TargetLoc = SL;
walk(SF);
SF.walk(*this);
return EnclosingCallAndArg.second;
}
};
Expand Down