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

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented May 2, 2024

Placeholder expansion should be a syntactic operation, but SourceEntityWalker can invoke type checking operations, which causes unexpected bahaviors including crashes.

rdar://121360941`

Placeholder expansion should be a syntactic operation, but
`SourceEntityWalker` can invoke type checking operations, which causes
unexpected bahaviors including crashes.

rdar://121360941`
@rintaro
Copy link
Member Author

rintaro commented May 2, 2024

@swift-ci Please smoke test

Comment on lines 1743 to +1745
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);
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 🙏

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants