Allow #warning and #error directives in case position of a switch#3363
Allow #warning and #error directives in case position of a switch#3363nenadvulic wants to merge 5 commits into
#warning and #error directives in case position of a switch#3363Conversation
The C++ parser accepts pound diagnostic directives between switch cases, but SwiftParser diagnosed them as statements not covered by a case label. Add macroExpansionDecl as an element choice of SwitchCaseListSyntax and parse #warning/#error in parseSwitchCases. Fixes swiftlang#3251.
|
a little reminder about this PR @ahoppen thanks 😊 |
hamishknight
left a comment
There was a problem hiding this comment.
Thank you! Can you also please follow the RFC process for this change?
| /// The expansion of a freestanding macro in a position that expects a declaration. | ||
| case macroExpansionDecl(RawMacroExpansionDeclSyntax) |
There was a problem hiding this comment.
Hmm this is an unfortunate doc comment given we only allow #warning/#error. Would be nice if we could customize the doc comments here, but that's a bigger change so happy to leave that for a subsequent PR
Per review feedback: this change targets a future release rather than 6.4, so move the SwitchCaseListSyntax.Element entry into a new 605.md.
|
@hamishknight RFC thread on the Swift forums: https://forums.swift.org/t/rfc-allow-warning-and-error-directives-in-case-position-of-a-switch/87464 I've also moved the release note entry from |
Per review feedback: exercise pound diagnostic directives nested in an #if config clause inside a switch case list.
| ) | ||
| } | ||
|
|
||
| func testSwitchWithPoundDiagnosticInIfConfig() { |
There was a problem hiding this comment.
These tests are only showing what happens before the first case. Let's cover a couple other situations:
I'm assuming that these still parse the #warning as a statement of the case body and not as a new case?
switch x {
case 1:
print()
#warning("something")
}
switch x {
case 1:
print()
#warning("something")
case 2:
break
}There's also some fairly involved lookahead when #if is inside a case body to determine whether it's surrounding regular statements or another case list. What happens with these?
switch x {
case 1:
print()
#if COND
case 2: break
#else
#warning("Something")
#endif
}
switch x {
case 1:
print()
#if COND
#warning("Something")
#else
case 2: break
#endif
}
switch x {
case 1:
print()
#if COND
#if COND2
#warning("Something")
#endif
#else
#if COND2
case 2: break
#endif
#endif
}My expectation is that those would all parse correctly as IfConfigs wrapping SwitchCaseLists, not as regular statements inside the body. (Although, I have no idea what the C++ parser does here, either.)
There was a problem hiding this comment.
@allevato I tested all three against swiftc (Apple Swift 6.3) and SwiftSyntax already
matches the compiler here. The disambiguation is on the first branch: if it
starts with case/default the #if is case-list level, otherwise it's
statement level and a case in a later branch is an error.
Example 1 (first branch is a case) compiles fine, only the #warning fires:
switch x {
case "1": print()
#if COND
case "2": break
#else
#warning("Something")
#endif
default: break
}Example 2 (first branch is #warning, case in the #else) is rejected by
the compiler:
switch x {
case "1": print()
#if COND
#warning("Something")
#else
case "2": break
#endif
default: break
}error: 'case' label can only appear inside a 'switch' statement
Note this fires even with -D COND (so with the #else inactive), so it's
structural, not a fold-time thing. SwiftSyntax emits the equivalent diagnostic.
Example 3 behaves the same way.
For the ambiguous case (a #if with only #warning/#error, no case label in
any branch) the compiler attaches it to the preceding case's body, same as
SwiftSyntax does now.
So I think the current behavior is correct: matching it to accept examples 2/3
would diverge from the compiler. I can add tests pinning this down if you want.
Or if there's a compiler version where these parse differently, point me at it
and I'll match that instead.
There was a problem hiding this comment.
Ok, my expectations were incorrect then, but the reasoning makes sense why it does what it does. Thanks for checking!
If we don't have any existing tests covering this situation, adding some seems like a good idea just to avoid future regressions.
One more edge case that just occurred to me:
switch x {
case 1: break
#if COND
case 2: break
#else
case 3: break
#endif
#warning("Something")
default: break
Does the C++ parser allow this (with or without the default: break)? I think swift-syntax misparses it if C++ allows it (you get a malformed SwitchCaseItem with the #warning in a CodeBlockItem, where I think I would expect #warning to be parsed as a MacroExpansionDecl as a sibling to the IfConfigDecl.
There was a problem hiding this comment.
Checked the new one too. swiftc accepts it (only the #warning fires, no
errors), and swift-syntax actually parses it the way you'd expect: the
#warning comes out as a MacroExpansionDecl sibling to the IfConfigDecl in
the case list, not as a malformed SwitchCaseItem. Tree for
switch x {
case 1: break
#if COND
case 2: break
#else
case 3: break
#endif
#warning("Something")
default: break
}is [SwitchCase(case 1), IfConfigDecl(case 2 / case 3), MacroExpansionDecl(#warning), SwitchCase(default)],
no diagnostics. Same with the default removed (you just get the usual
non-exhaustive error).
And your two simpler ones parse the #warning as a statement in the case body,
like you assumed. So no misparse here.
I'll add tests for all of these so it doesn't regress.
Cover the cases raised in review: pound diagnostics in a case body, an #if whose first clause starts with a case (parsed at case-list level), an #if whose first clause starts with a pound diagnostic with a case in a later clause (diagnosed, matching the compiler), and a pound diagnostic following a case-list-level #if (parsed as a MacroExpansionDecl sibling).
|
@swift-ci please test |
@nenadvulic Can you open a PR for https://github.com/swiftlang/swift that fixes the missing case? We can cross-repo test the changes together |
@hamishknight Sure, I'll open a PR on swiftlang/swift and link it here so we can cross-repo test the two together. |
swift-syntax now parses #warning/#error in case position of a switch as a MacroExpansionDecl element of the case list (swiftlang/swift-syntax#3363). Generate the builtin pound for its diagnostic and skip it, since it is not a case.
|
@hamishknight I opened the companion PR: swiftlang/swift#90074 |
|
@swift-ci please test |
A '#warning'/'#error' at the start of a switch case list was checked after the '#if' recovery path, so a '#if' appearing later in the source caused the directive to be consumed as unexpected code before that '#if'. Check for the pound diagnostic first.
|
@swift-ci please test |
|
@hamishknight The cross-repo smoke test there passes (macOS + Linux). This PR's standalone macOS/Linux CI only fails because it builds against compiler |
|
@nenadvulic I did do a cross-repo test for this PR. Seems like the recent update-checkout changes have broken things here, let me investigate |
swift-syntax now parses #warning/#error in case position of a switch as a MacroExpansionDecl element of the case list (swiftlang/swift-syntax#3363). Generate the builtin pound for its diagnostic and skip it, since it is not a case.
|
@swift-ci please test |
Fixes #3251.
The C++ parser accepts
#warningand#errorbetween switch cases (parseStmtCaseshas a dedicatedpound_warning/pound_errorbranch), but SwiftParser sent them through the missing-case-label recovery path. As a result, swiftc reportederror: new Swift parser generated errors for code that C++ parser acceptedfor code like:Changes:
macroExpansionDeclto the element choices ofSwitchCaseListSyntax, since#warning/#errorare represented asMacroExpansionDeclSyntax(there is no dedicated pound-diagnostic node).#warning/#errorinparseSwitchCasesasmacroExpansionDeclelements. Only these two directives are accepted, mirroring the C++ parser; other macro expansions in case position still go through case-label recovery.SwitchCaseListSyntax.Elementcase is source-breaking for exhaustive switches.