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

[5.9] [Sema] Catch invalid if/switch exprs in more places #68653

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

hamishknight
Copy link
Contributor

5.9 cherry-pick of #68207

  • Explanation: Fixes missing diagnostics for if/switch expressions in property initializers, subscript default args, and custom attribute args. Most notably this fixes a miscompile when a non-exhaustive if expression is used in a property init, subscript default arg, or custom attribute arg.
  • Scope: Affects diagnostic logic for if/switch expressions
  • Issue: rdar://115785675
  • Risk: Low, this change applies existing diagnostic logic to more places.
  • Testing: Added tests to test suite
  • Reviewer: Pavel Yaskevich

Previously we would only look through a handful of
AST node types when determining if an if/switch
expression is in a valid position. However this
doesn't handle cases where we synthesize code
around an if/switch expression, such as
`init(erasing:)` calls. As such, relax the logic
such that it can look through any implicit
expression.

rdar://113435870
Move out-of-place SingleValueStmtExpr checking into
`performSyntacticExprDiagnostics`, to ensure we
catch all expressions. Previously we did the walk
as a part of Decl-based MiscDiagnostics, but it
turns out that can miss expressions in property
initializers, subscript default arguments, and
custom attrs.

This does mean that we'll now no longer diagnose
out-of-place if/switch exprs if the expression
didn't type-check, but that's consistent with the
rest of MiscDiagnostics, and I don't think it will
be a major issue in practice. We probably ought to
consider moving this checking into PreCheckExpr,
but that would require first separating out
SequenceExpr folding, which has other consequences,
and so I'm leaving as future work for now.
@hamishknight hamishknight added 🍒 release cherry pick Flag: Release branch cherry picks swift 5.9 labels Sep 20, 2023
@hamishknight hamishknight requested a review from a team as a code owner September 20, 2023 15:37
@hamishknight
Copy link
Contributor Author

Note we could leave 157e488 out of this cherry-pick, but I think it's low risk enough to take too, since it should only relax the out-of-place diagnostics.

@hamishknight
Copy link
Contributor Author

@swift-ci please test

@hamishknight
Copy link
Contributor Author

@swift-ci please test macOS

@bnbarham bnbarham merged commit 49fe7f7 into swiftlang:release/5.9 Sep 21, 2023
5 checks passed
@hamishknight hamishknight deleted the nestless-5.9 branch September 21, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks swift 5.9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants