[SwiftParser] Improve attribute parsing and recovering

Sources/SwiftParser/Attributes.swift

@@ -17,18 +17,56 @@
 #endif
 
 extension Parser {
-  mutating func parseAttributeList() -> RawAttributeListSyntax {
-    guard self.at(.atSign, .poundIf) else {
-      return self.emptyCollection(RawAttributeListSyntax.self)
+  private mutating func parseAttributeListElement() -> RawAttributeListSyntax.Element {
+    if self.at(.poundIf) {
+      // 'consumeIfConfigOfAttributes()' check in 'parseAttributeList()' already guarantees
+      // that this '#if' only contains attribute list elements. We don't check
+      // `consumeIfConfigOfAttributes()` here again because it's not only redundant, but it
+      // does *not* work for cases like:
+      //
+      //    #if COND1
+      //       @attr
+      //       #if COND2
+      //       #endif
+      //    #endif
+      //    func fn() {}
+      //
+      // In such cases, the second `#if` is not `consumeIfConfigOfAttributes()`.
+      return .ifConfigDecl(
+        self.parsePoundIfDirective { (parser, _) -> RawAttributeListSyntax.Element in
+          return parser.parseAttributeListElement()
+        } syntax: { parser, attributes in
+          return .attributes(RawAttributeListSyntax(elements: attributes, arena: parser.arena))
+        }
+      )
+    } else {
+      return .attribute(self.parseAttribute())
     }
+  }
 
+  mutating func parseAttributeList() -> RawAttributeListSyntax {
     var elements = [RawAttributeListSyntax.Element]()
+
+    func shouldContinue() -> Bool {
+      if self.at(.atSign) {
+        return true
+      }
+      if self.at(.poundIf) && self.withLookahead({ $0.consumeIfConfigOfAttributes() }) {
+        return true
+      }
+      return false
+    }
+
     var loopProgress = LoopProgressCondition()
-    repeat {
-      let attribute = self.parseAttribute()
+    while self.hasProgressed(&loopProgress) && shouldContinue() {
+      let attribute = self.parseAttributeListElement()
       elements.append(attribute)
-    } while self.at(.atSign, .poundIf) && self.hasProgressed(&loopProgress)
-    return RawAttributeListSyntax(elements: elements, arena: self.arena)
+    }
+    if elements.isEmpty {
+      return self.emptyCollection(RawAttributeListSyntax.self)
+    } else {
+      return RawAttributeListSyntax(elements: elements, arena: self.arena)
+    }
   }
 }
 
@@ -148,7 +186,7 @@ extension Parser {
     parseMissingArguments: (
       (inout Parser) -> (unexpectedBefore: RawUnexpectedNodesSyntax?, arguments: RawAttributeSyntax.Arguments)
     )? = nil
-  ) -> RawAttributeListSyntax.Element {
+  ) -> RawAttributeSyntax {
     var (unexpectedBeforeAtSign, atSign) = self.expect(.atSign)
     if atSign.trailingTriviaByteLength > 0 || self.currentToken.leadingTriviaByteLength > 0 {
       let diagnostic = TokenDiagnostic(
@@ -163,9 +201,7 @@ extension Parser {
     case .required:
       shouldParseArgument = true
     case .customAttribute:
-      shouldParseArgument =
-        self.withLookahead { $0.atAttributeOrSpecifierArgument() }
-        && self.at(TokenSpec(.leftParen, allowAtStartOfLine: false))
+      shouldParseArgument = self.withLookahead { $0.atAttributeOrSpecifierArgument() }
     case .optional:
       shouldParseArgument = self.at(.leftParen)
     case .noArgument:
@@ -190,46 +226,32 @@ extension Parser {
         (unexpectedBeforeArguments, argument) = parseArguments(&self)
       }
       let (unexpectedBeforeRightParen, rightParen) = self.expect(.rightParen)
-      return .attribute(
-        RawAttributeSyntax(
-          unexpectedBeforeAtSign,
-          atSign: atSign,
-          attributeName: attributeName,
-          unexpectedBeforeLeftParen,
-          leftParen: leftParen,
-          unexpectedBeforeArguments,
-          arguments: argument,
-          unexpectedBeforeRightParen,
-          rightParen: rightParen,
-          arena: self.arena
-        )
+      return RawAttributeSyntax(
+        unexpectedBeforeAtSign,
+        atSign: atSign,
+        attributeName: attributeName,
+        unexpectedBeforeLeftParen,
+        leftParen: leftParen,
+        unexpectedBeforeArguments,
+        arguments: argument,
+        unexpectedBeforeRightParen,
+        rightParen: rightParen,
+        arena: self.arena
       )
     } else {
-      return .attribute(
-        RawAttributeSyntax(
-          unexpectedBeforeAtSign,
-          atSign: atSign,
-          attributeName: attributeName,
-          leftParen: nil,
-          arguments: nil,
-          rightParen: nil,
-          arena: self.arena
-        )
+      return RawAttributeSyntax(
+        unexpectedBeforeAtSign,
+        atSign: atSign,
+        attributeName: attributeName,
+        leftParen: nil,
+        arguments: nil,
+        rightParen: nil,
+        arena: self.arena
       )
     }
   }
 
-  mutating func parseAttribute() -> RawAttributeListSyntax.Element {
-    if self.at(.poundIf) {
-      return .ifConfigDecl(
-        self.parsePoundIfDirective { (parser, _) -> RawAttributeListSyntax.Element in
-          return parser.parseAttribute()
-        } syntax: { parser, attributes in
-          return .attributes(RawAttributeListSyntax(elements: attributes, arena: parser.arena))
-        }
-      )
-    }
-
+  mutating func parseAttribute() -> RawAttributeSyntax {
     switch peek(isAtAnyIn: DeclarationAttributeWithSpecialSyntax.self) {
     case .abi:
       return parseAttribute(argumentMode: .required) { parser in
@@ -299,17 +321,15 @@ extension Parser {
     case .rethrows:
       let (unexpectedBeforeAtSign, atSign) = self.expect(.atSign)
       let (unexpectedBeforeAttributeName, attributeName) = self.expect(TokenSpec(.rethrows, remapping: .identifier))
-      return .attribute(
-        RawAttributeSyntax(
-          unexpectedBeforeAtSign,
-          atSign: atSign,
-          unexpectedBeforeAttributeName,
-          attributeName: RawIdentifierTypeSyntax(name: attributeName, genericArgumentClause: nil, arena: self.arena),
-          leftParen: nil,
-          arguments: nil,
-          rightParen: nil,
-          arena: self.arena
-        )
+      return RawAttributeSyntax(
+        unexpectedBeforeAtSign,
+        atSign: atSign,
+        unexpectedBeforeAttributeName,
+        attributeName: RawIdentifierTypeSyntax(name: attributeName, genericArgumentClause: nil, arena: self.arena),
+        leftParen: nil,
+        arguments: nil,
+        rightParen: nil,
+        arena: self.arena
       )
     case .Sendable:
       return parseAttribute(argumentMode: .noArgument) { parser in
@@ -1023,6 +1043,10 @@ extension Parser {
 
 extension Parser.Lookahead {
   mutating func atAttributeOrSpecifierArgument() -> Bool {
+    if !self.at(TokenSpec(.leftParen, allowAtStartOfLine: false)) {
+      return false
+    }
+
     var lookahead = self.lookahead()
     lookahead.skipSingle()
 
@@ -1055,9 +1079,7 @@ extension Parser.Lookahead {
       return false
     }
 
-    if self.at(TokenSpec(.leftParen, allowAtStartOfLine: false))
-      && self.withLookahead({ $0.atAttributeOrSpecifierArgument() })
-    {
+    if self.withLookahead({ $0.atAttributeOrSpecifierArgument() }) {
       self.skipSingle()
     }
 

Sources/SwiftParser/Declarations.swift

@@ -65,9 +65,16 @@ extension TokenConsumer {
 
     var hasAttribute = false
     var attributeProgress = LoopProgressCondition()
-    while subparser.hasProgressed(&attributeProgress) && subparser.at(.atSign) {
-      hasAttribute = true
-      _ = subparser.consumeAttributeList()
+    while subparser.hasProgressed(&attributeProgress) {
+      if subparser.at(.atSign) {
+        _ = subparser.consumeAttributeList()
+        hasAttribute = true
+      } else if subparser.at(.poundIf) && subparser.consumeIfConfigOfAttributes() {
+        subparser.skipSingle()
+        hasAttribute = true
+      } else {
+        break
+      }
     }
 
     var hasModifier = false
@@ -94,11 +101,6 @@ extension TokenConsumer {
       }
     }
 
-    if subparser.at(.poundIf) {
-      var attrLookahead = subparser.lookahead()
-      return attrLookahead.consumeIfConfigOfAttributes()
-    }
-
     let declStartKeyword: DeclarationKeyword?
     if allowRecovery {
       declStartKeyword =
@@ -175,6 +177,10 @@ extension TokenConsumer {
           allowRecovery: allowRecovery
         )
       }
+      if allowRecovery && (hasAttribute || hasModifier) {
+        // If we found any attributes or modifiers, consider it's a missing decl.
+        return true
+      }
       return false
     }
   }
@@ -492,11 +498,11 @@ extension Parser {
       arena: self.arena
     )
 
-    if self.at(.atSign), case .attribute(let attribute) = self.parseAttribute() {
+    if self.at(.atSign) {
       return RawUsingDeclSyntax(
         unexpectedBeforeUsingKeyword,
         usingKeyword: usingKeyword,
-        specifier: .attribute(attribute),
+        specifier: .attribute(self.parseAttribute()),
         arena: self.arena
       )
     }

Sources/SwiftParser/Lookahead.swift

@@ -221,11 +221,8 @@ extension Parser.Lookahead {
         _ = self.consumeGenericArguments()
       } while self.consume(if: .period) != nil
 
-      if self.consume(if: .leftParen) != nil {
-        while !self.at(.endOfFile, .rightParen, .poundEndif) {
-          self.skipSingle()
-        }
-        self.consume(if: .rightParen)
+      if self.atAttributeOrSpecifierArgument() {
+        self.skipSingle()
       }
     }
     return true

Sources/SwiftParser/TokenSpecSet.swift

@@ -267,9 +267,9 @@ enum ContextualDeclKeyword: TokenSpecSet {
   }
 }
 
-/// A `DeclarationKeyword` that is not a `ValueBindingPatternSyntax.BindingSpecifierOptions`.
+/// A `DeclarationKeyword` that is not a `VariableDeclSyntax.BindingSpecifierOptions`.
 ///
-/// `ValueBindingPatternSyntax.BindingSpecifierOptions` are injected into
+/// `VariableDeclSyntax.BindingSpecifierOptions` are injected into
 /// `DeclarationKeyword` via an `EitherTokenSpecSet`.
 enum PureDeclarationKeyword: TokenSpecSet {
   case actor
@@ -344,7 +344,7 @@ enum PureDeclarationKeyword: TokenSpecSet {
 
 typealias DeclarationKeyword = EitherTokenSpecSet<
   PureDeclarationKeyword,
-  ValueBindingPatternSyntax.BindingSpecifierOptions
+  VariableDeclSyntax.BindingSpecifierOptions
 >
 
 enum DeclarationModifier: TokenSpecSet {

Sources/SwiftParser/TopLevel.swift

@@ -251,10 +251,10 @@ extension Parser {
       return self.parseStatementItem()
     } else if self.atStartOfExpression() {
       return .expr(self.parseExpression(flavor: .basic, pattern: .none))
-    } else if self.atStartOfDeclaration(isAtTopLevel: isAtTopLevel, allowInitDecl: allowInitDecl, allowRecovery: true) {
-      return .decl(self.parseDeclaration())
     } else if self.atStartOfStatement(allowRecovery: true, preferExpr: false) {
       return self.parseStatementItem()
+    } else if self.atStartOfDeclaration(isAtTopLevel: isAtTopLevel, allowInitDecl: allowInitDecl, allowRecovery: true) {
+      return .decl(self.parseDeclaration())
     } else {
       return .init(expr: RawMissingExprSyntax(arena: self.arena))
     }

Sources/SwiftParser/Types.swift

@@ -1308,17 +1308,21 @@ extension Parser {
       return .attribute(self.parseDifferentiableAttribute())
 
     case .isolated:
-      return parseAttribute(argumentMode: .required) { parser in
-        return (nil, .argumentList(parser.parseIsolatedAttributeArguments()))
-      }
+      return .attribute(
+        parseAttribute(argumentMode: .required) { parser in
+          return (nil, .argumentList(parser.parseIsolatedAttributeArguments()))
+        }
+      )
     case .convention, ._opaqueReturnTypeOf, nil:  // Custom attribute
-      return parseAttribute(argumentMode: .customAttribute) { parser in
-        let arguments = parser.parseArgumentListElements(
-          pattern: .none,
-          allowTrailingComma: true
-        )
-        return (nil, .argumentList(RawLabeledExprListSyntax(elements: arguments, arena: parser.arena)))
-      }
+      return .attribute(
+        parseAttribute(argumentMode: .customAttribute) { parser in
+          let arguments = parser.parseArgumentListElements(
+            pattern: .none,
+            allowTrailingComma: true
+          )
+          return (nil, .argumentList(RawLabeledExprListSyntax(elements: arguments, arena: parser.arena)))
+        }
+      )
 
     }
   }

Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

@@ -482,21 +482,36 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
       otherNode.lastToken(viewMode: .sourceAccurate)?.tokenKind == .poundEndif
     {
       let diagnoseOn = parent.parent ?? parent
-      addDiagnostic(
-        diagnoseOn,
-        IfConfigDeclNotAllowedInContext(context: diagnoseOn),
-        highlights: [Syntax(node), Syntax(otherNode)],
-        fixIts: [
-          FixIt(
-            message: RemoveNodesFixIt([Syntax(node), Syntax(otherNode)]),
-            changes: [
-              .makeMissing([Syntax(node)], transferTrivia: false),
-              .makeMissing([Syntax(otherNode)], transferTrivia: false),
-            ]
-          )
-        ],
-        handledNodes: [node.id, otherNode.id]
-      )
+      if node == otherNode {
+        addDiagnostic(
+          diagnoseOn,
+          IfConfigDeclNotAllowedInContext(context: diagnoseOn),
+          highlights: [Syntax(node)],
+          fixIts: [
+            FixIt(
+              message: RemoveNodesFixIt([Syntax(node)]),
+              changes: .makeMissing([Syntax(node)], transferTrivia: false)
+            )
+          ],
+          handledNodes: [node.id]
+        )
+      } else {
+        addDiagnostic(
+          diagnoseOn,
+          IfConfigDeclNotAllowedInContext(context: diagnoseOn),
+          highlights: [Syntax(node), Syntax(otherNode)],
+          fixIts: [
+            FixIt(
+              message: RemoveNodesFixIt([Syntax(node), Syntax(otherNode)]),
+              changes: [
+                .makeMissing([Syntax(node)], transferTrivia: false),
+                .makeMissing([Syntax(otherNode)], transferTrivia: false),
+              ]
+            )
+          ],
+          handledNodes: [node.id, otherNode.id]
+        )
+      }
     } else {
       addDiagnostic(node, UnexpectedNodesError(unexpectedNodes: node), highlights: [Syntax(node)])
     }