[SwiftParser] Improve statement level recovery by rintaro · Pull Request #3156 · swiftlang/swift-syntax

rintaro · 2025-09-26T23:13:15Z

Consider the following code:

}
@attr1 func foo() {}

Previously, this was parsed as a FunctionDeclSyntax with the unexpected code } @attr attached as unexpectedBeforeFuncKeyword. This was problematic because @attr was effectively ignored. The TokenPrecedence-based recovery strategy couldn't handle this well, since @ is not a declaration keyword and doesn't by itself signal the start of a declaration.

One option would be to check atStartOfDeclaration() after each skipSingle() and recognize @ as the start of a declaration. However, then the preceding } would need to be attached to the first attribute in the attribute list, which would require threading recovery logic into parseAttributeList() and only applying it to the first parseAttribute(). This would make the code more complex and harder to maintain.

This PR introduces a new syntax node UnexpectedCodeDeclSyntax

It represents unexpected code at the statement/declaration level.
All statement-level recovery is now handled by parseUnexpectedCodeDeclaration().
This simplifies recovery handling significantly.

Also:

Improve #if related recovery:
- orphan #endif et al at top-level is now UnexpectedCodeDecl instead of making everything after it "unexpected"
```
#endif
func foo() {}
```
- Unbalanced #endif et al closes code/member block with missing }.
```
struct S {
  #if FOO
  func foo() {
  #endif
}
```
- Simplified parsePoundDirective(). It now receives only one closure instead of 3.
Don't attach ';' to compiler directives. For example:
```
#sourceLocation(file: "<file>", line: 100)
;
```
; is now diagnosed as ;-only statement, or unexpected ; separator depending on the position.
Adjust TokenPrecedence for decl/statement keyword so. Decl keyword recovery won't go beyond } or statements. E.g.
```
func foo() {
  @attr
}
struct S {}
```
```
func foo() {
  @attr
  if true {}
  struct S {}
}
```
cont...

rintaro · 2025-09-27T01:58:17Z

Sources/SwiftParser/Declarations.swift

        _ = subparser.consumeAttributeList()
        hasAttribute = true
      } else if subparser.at(.poundIf) && subparser.consumeIfConfigOfAttributes() {
-        subparser.skipSingle()


subparser.consumeIfConfigOfAttributes() already skipped #if .... #endif

rintaro · 2025-09-27T03:54:09Z

CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift

    base: .syntax,
    nameForDiagnostics: "declaration",
-    parserFunction: "parseDeclaration"
+    parserFunction: "parseDeclarationOrIfConfig"


parseDeclaration() don't parse #if ... #endif. Newly introduced parseDeclarationOrIfConfig() just for DeclSyntax.parse(from:)

rintaro · 2025-09-27T04:01:32Z

Sources/SwiftParser/Declarations.swift

  mutating func atStartOfDeclaration(
-    isAtTopLevel: Bool = false,
    allowInitDecl: Bool = true,
-    allowRecovery: Bool = false


Removed allowRecovery from atStartOfDeclaration()/atStartOfStatement()/atStartOfSwitchCase() because all statement-level recoveries are now done by parseUnexpectedCodeDecl()

rintaro · 2025-09-27T04:07:14Z

Sources/SwiftParser/Declarations.swift

  mutating func parseDeclaration(in context: DeclarationParseContext = .topLevelOrCodeBlock) -> RawDeclSyntax {
-    // If we are at a `#if` of attributes, the `#if` directive should be
-    // parsed when we're parsing the attributes.
-    if self.at(.poundIf) && !self.withLookahead({ $0.consumeIfConfigOfAttributes() }) {


This block is basically hoisted to parseMemberBlockItem()

To align with parseCodeBlockItem() scheme

Don't want to attach ; to IfConfigDeclSyntax

rintaro · 2025-09-27T04:08:23Z

Sources/SwiftParser/Declarations.swift

-      let isProbablyVarDecl = self.at(.identifier, .wildcard) && self.peek(isAt: .colon, .equal, .comma)
-      let isProbablyTupleDecl = self.at(.leftParen) && self.peek(isAt: .identifier, .wildcard)
-
-      if isProbablyVarDecl || isProbablyTupleDecl {
-        return RawDeclSyntax(self.parseBindingDeclaration(attrs, .missing(.keyword(.var)), in: context))
-      }


Factored out to atBindingDeclarationWithoutVarKeyword() and move the handling to recoveryResult above, aligning with atFunctionDeclarationWithoutFuncKeyword().

rintaro · 2025-09-27T04:09:36Z

Sources/SwiftParser/Declarations.swift

-
-      if atFunctionDeclarationWithoutFuncKeyword() {
-        return RawDeclSyntax(self.parseFuncDeclaration(attrs, .missing(.keyword(.func))))
-      }


This is already handled in recoveryResult above.

rintaro · 2025-09-27T04:46:12Z

Sources/SwiftParser/Statements.swift

    if self.atStartOfStatement(preferExpr: true) || self.atStartOfDeclaration() {
      return false
    }
+    if self.atStartOfLine && self.withLookahead({ $0.atStartOfSwitchCase() }) {


Previously case was a start of statement, but not anymore. This prevents the second case below is parsed as the returning expression.

case .foo: return case bar: ...

rintaro · 2025-09-27T04:50:59Z

Sources/SwiftParser/TopLevel.swift

+
+    let item: RawCodeBlockItemSyntax.Item
+    let attachSemi: Bool
+    if self.at(.poundIf) && !self.withLookahead({ $0.consumeIfConfigOfAttributes() }) {


Inlined parseItem() because we want to choose whether to attach ; (attachSemi) depending on what we parse.

rintaro · 2025-09-27T04:54:43Z

Sources/SwiftParser/TopLevel.swift

+    } else {
+      // Otherwise, eat the unexpected tokens into an "decl".
+      item = .decl(
+        RawDeclSyntax(
+          self.parseUnexpectedCodeDeclaration(allowInitDecl: allowInitDecl, requiresDecl: false, until: stopCondition)
+        )
+      )
+      attachSemi = true


Previously in parseItem(), we tried atStartOfStatement(allowRecovery: true) and atStartOfDeclaration(allowRecovery: true) for recoveries.
Now unexpected tokens at this position are always parsed as UnexpectedCodeDeclSyntax until we find a decl, statement, or expression.

rintaro · 2025-09-27T04:58:04Z

Tests/SwiftDiagnosticsTest/ParserDiagnosticsFormatterIntegrationTests.swift

      1 | func o() {
      2 | }👨‍👩‍👧‍👦}
-        |  |`- error: extraneous braces at top level
+        |  |`- error: unexpected braces in source file


This message change is because previously the UnexpectedNodesSyntax was stored as SourceFileSyntax.unexpectedBetweenStatementsAndEndOfFileToken, but now it's UnexpectedCodeDeclSyntax in SourceFileSyntax.statements.

rintaro · 2025-09-27T05:01:38Z

Tests/SwiftParserTest/AttributeTests.swift

        DiagnosticSpec(
          locationMarker: "2️⃣",
-          message: "unexpected code '))' before macro"
+          message: "unexpected code '))' in class"


This and similar changes are because the unexpected code are now separate UnexpectedCodeDeclSyntax in the statements.

So IIUC this parses as an attribute on a class with a macro member? The change from unexpected to a node seems fine to me diagnostic wise, there's not much difference in the "before" vs "in"

hamishknight

Nice cleanup!

Sources/SwiftParser/Attributes.swift

Sources/SwiftParser/TopLevel.swift

bnbarham · 2025-10-02T00:36:54Z

Tests/SwiftParserTest/AttributeTests.swift

        DiagnosticSpec(
          locationMarker: "2️⃣",
-          message: "unexpected code '))' before macro"
+          message: "unexpected code '))' in class"


So IIUC this parses as an attribute on a class with a macro member? The change from unexpected to a node seems fine to me diagnostic wise, there's not much difference in the "before" vs "in"

Tests/SwiftParserTest/DirectiveTests.swift

bnbarham · 2025-10-02T00:43:43Z

Tests/SwiftParserTest/DirectiveTests.swift

+        ),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "unexpected code in source file"),
+      ],
+      fixedSource: """


Why does the fixed source still have the #endif and extra }? Shouldn't both be unexpected and thus removed in the fixed source? Is the new UnexpectedDeclNode not being treated the same as unexpected?

I'm also unsure about closing the struct at the endif. It makes sense if there's a matching #if, but when there isn't... 🤔

Diagnostics are conservative about removing user code. UnexpectedNodesSyntax in general might contain important user code and we don't want to delete them in "fix-it all"

I'm also unsure about closing the struct at the endif.

We can't know if there's } after #endif until we parse it.
IMO #endif et al should always be stronger than }, because, unlike { or } which editor often insert, the user probably intentionally wrote #endif for some reason.

This specific test case might be controversial, but for example, for a source file like this:

struct MyStruct { func foo() {} // ... #endif struct Other { .... }

This is probably missing } before #endif

bnbarham · 2025-10-02T00:52:18Z

Tests/SwiftParserTest/ExpressionTests.swift

+      diagnostics: [
+        DiagnosticSpec(
+          locationMarker: "1️⃣",
+          // FIXME: "expected attribute name after '@'".


Could you file an issue for this rather than have the FIXME? Or add a specific test for the message, with the FIXME + a link to the issue? Same for the one below

sure. filed
#3158
#3159

bnbarham · 2025-10-02T00:53:19Z

Tests/SwiftParserTest/Parser+EntryTests.swift

+    )
+  }
+
+  func testDeclSyntaxParseIfConfigAttr() throws {


Thanks for adding these 🙇

bnbarham · 2025-10-02T03:45:32Z

Tests/SwiftParserTest/StatementTests.swift

+          message: "expected declaration and ';' after attribute",
+          fixIts: ["insert declaration and ';'"]


Why do we default to ; instead of a newline for this case?

This is modeled as

CodeBlockItem( item: .decl(MissingDecl(...)), semicolon: .semiColonToken(precence: .missing) )

And ParseDiagnosticsGenerator generates single diagnostic for consecutive missing nodes.

"consecutive statements ... insert newline or ';'" is a special diagnostics that is generated when the previous node was not missing.

Tests/SwiftParserTest/translated/ErrorsTests.swift

Tests/SwiftParserTest/translated/ForwardSlashRegexSkippingInvalidTests.swift

rintaro · 2025-10-03T06:33:13Z

swiftlang/swift#84553
@swift-ci Please test

rintaro · 2025-10-03T06:57:01Z

swiftlang/swift#84553
@swift-ci Please test Windows

rintaro · 2025-10-03T14:33:56Z

swiftlang/swift#84553
@swift-ci Please test Windows

rintaro force-pushed the parser-recover-stmtlevel branch from c706d4c to 2ca55fe Compare September 26, 2025 23:27

rintaro commented Sep 27, 2025

View reviewed changes

rintaro force-pushed the parser-recover-stmtlevel branch from ef1bf91 to dc4e479 Compare September 27, 2025 03:58

rintaro commented Sep 27, 2025

View reviewed changes

rintaro added 11 commits September 27, 2025 08:38

[SwiftParser] Improve statement level recovery

bda7f10

[SwiftParser] Improve '#if' recovery

41286de

tbs

6026c78

tbs

91f9d63

tbs

ad00841

tbs

130a51b

cleanup

bfddfd6

tbs

0eccca1

comment

6d34216

cleanup

7d97ad9

format

a61a1a7

rintaro mentioned this pull request Sep 27, 2025

[ASTGen] Update for UnexpectedCodeDeclSyntax addition swiftlang/swift#84553

Merged

fix

f9a97cf

rintaro force-pushed the parser-recover-stmtlevel branch from 5ceb6e8 to f9a97cf Compare September 28, 2025 03:08

format

229d5dc

rintaro marked this pull request as ready for review September 29, 2025 05:16

rintaro requested review from ahoppen, bnbarham and hamishknight as code owners September 29, 2025 05:16

hamishknight approved these changes Sep 29, 2025

View reviewed changes

Sources/SwiftParser/Attributes.swift Outdated Show resolved Hide resolved

Sources/SwiftParser/TopLevel.swift Outdated Show resolved Hide resolved

Sources/SwiftParser/TopLevel.swift Outdated Show resolved Hide resolved

rintaro added 3 commits September 29, 2025 10:30

[Parser] @unknown default at statement level

dfab212

comment

161643b

Don't interleave UexpectedNodes in UnexpectedCodeDeclSyntax

e8bbace

bnbarham reviewed Oct 2, 2025

View reviewed changes

rintaro mentioned this pull request Oct 3, 2025

Unsound parser diagnostics for missing attribute name for switch case/default attributes #3159

Open

tweak

ce47be2

bnbarham approved these changes Oct 3, 2025

View reviewed changes

rintaro merged commit 8ea19b6 into swiftlang:main Oct 3, 2025
34 checks passed

beccadax mentioned this pull request Oct 4, 2025

Adjust test to reflect _Concurrency module hiding swiftlang/sourcekit-lsp#2313

Merged

		message: "expected declaration and ';' after attribute",
		fixIts: ["insert declaration and ';'"]

Conversation

rintaro commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rintaro commented Oct 3, 2025

Uh oh!

rintaro commented Oct 3, 2025

Uh oh!

rintaro commented Oct 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rintaro commented Sep 26, 2025 •

edited

Loading

rintaro Sep 27, 2025 •

edited

Loading