Skip to content
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

[Sema] A couple of fixes for MiscDiagnostics on macro expansions #77479

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

hamishknight
Copy link
Contributor

@hamishknight hamishknight commented Nov 8, 2024

  • Delay expanding macros until the end of CSApply to ensure the solution is applied to any parent expression nodes, avoiding a crash for the implicit-self diagnostic logic
  • Avoid double-diagnosing macro expansions in MiscDiagnostics, ensuring we only walk expansions in MiscDiagnostics when type-checking the expansion itself

rdar://138997009

@hamishknight
Copy link
Contributor Author

Going to drop the performStmtDiagnostics change for now since that's going to be a larger change to fix

@hamishknight
Copy link
Contributor Author

@swift-ci please test source compatibility

Avoid walking into macro expansions for
MiscDiagnostics, the expansions will instead be
visited when they're type-checked on their own.
Always bothered me the constructor for
ExprRewriter is buried in the middle of the class.
Attempting to expand macros in the middle of
CSApply can result in attempting to run
MiscDiagnostics within a closure that hasn't yet
had the solution applied to the AST, which can
crash the implicit-self diagnostic logic. Move
the expansion to the end of CSApply such that
expansions are type-checked along with local
decls, ensuring it's run after the solution has
been applied to the AST.

rdar://138997009
@hamishknight
Copy link
Contributor Author

@swift-ci please test

return MacroWalking::Expansion;
// Macro expansions will be walked when they're type-checked, not as
// part of the surrounding code.
return MacroWalking::None;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please separately evaluate whether it would be possible to subtype all of this walkers from BaseDiagnosticWalker and remove all of these overrides?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I have that exact change in a follow-up PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect! :)

@hamishknight hamishknight merged commit fab09c5 into swiftlang:main Nov 11, 2024
4 of 5 checks passed
@hamishknight hamishknight deleted the macro-misc-diag-fixes branch November 11, 2024 19:28
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.

2 participants