[Sema] A couple more MiscDiagnostics cleanups/fixes #77554
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hamishknight
requested review from
hborla,
slavapestov,
xedin,
AnthonyLatsis and
tshortli
as code owners
I missed this in my previous PR, but this is needed to ensure we visit macro arguments for macro expansion exprs that have substitute MacroExpansionDecls since we prefer to visit the arguments on the decl once the expression has been expanded.
Instead, ensure we walk into expressions in SyntacticDiagnosticWalker, allowing `performStmtDiagnostics` to be called there for all statements present in the target. This avoids a case where we'd double diagnose. While here, inherit the walker from BaseDiagnosticWalker.
The walker here is really doing 2 completely separate things, split it into 2 walkers. For ExprAvailabilityWalker, we just want to recursively continue walking looking for any sub-expressions to diagnose. For StmtAvailabilityWalker we only want to walk the top-level statement, and are only interested in any TypeReprs it has. This lets us get rid of the special handling for BraceStmt.
This used to have some logic when multi-statement closures were type-checked differently, it's always true now.
Simplifies things a tiny amount, and ensures we don't attempt to walk into non-PatternBindingDecls, same as other MiscDiagnostic passes. I don't think this currently makes a difference since I don't believe we have any interesting cases where a Decl is nested in an Expr without an intermediate Stmt, but it's the right thing to do regardless.
Previously we would check if we have a SwitchStmt, and apply diagnostics such as `checkExistentialTypes` to the CaseStmts individually. This however would have been missed for `catch` statements. The change to consistently call `performStmtDiagnostics` in closures fixed this for `do-catch`'s in closures, this commit fixes it for those outside of closures. Because this is source breaking, the existential diagnostic is downgraded to a warning until Swift 7 for catch statements specifically. While here, also apply the ambiguous where clause diagnostic to `catch` statements.
hamishknight
force-pushed
the
statement-misc-diags
branch
from
b9d09b6 to
b644cd8
Compare
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
tshortli
approved these changes
Nov 13, 2024
xedin
approved these changes
Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
performStmtDiagnosticsin CSApply, instead leaving it up to the SyntacticDiagnosticWalkerperformStmtDiagnosticsforcatchclauses, including allowing the ambiguous where clause diagnostic to be emitted for itdiagnoseStmtAvailability+ExprAvailabilityWalkera bitResolves #77453
Resolves #77553