Add diagnostic to move 'async' and 'throws' in 'ArrowExpr' in front of '->'

Author: ahoppen

Sources/SwiftParser/Diagnostics/MissingNodesError.swift

@@ -17,9 +17,11 @@ import SwiftBasicFormat
 // MARK: - Shared code
 
 /// Returns a string that describes `missingNodes`.
-/// `missingNodes` are expected to all be children of `commonParent`.
-private func missingNodesDescription(missingNodes: [Syntax], commonParent: Syntax?) -> String {
-  assert(missingNodes.allSatisfy({ $0.parent == commonParent }))
+/// If `commonParent` is not `nil`, `missingNodes` are expected to all be children of `commonParent`.
+func missingNodesDescription(missingNodes: [Syntax], commonParent: Syntax?) -> String {
+  if commonParent != nil {
+    assert(missingNodes.allSatisfy({ $0.parent == commonParent }))
+  }
 
   // If all tokens in the parent are missing, return the parent type name.
   if let commonParent = commonParent,

Sources/SwiftParser/Diagnostics/ParseDiagnosticsGenerator.swift

@@ -95,6 +95,51 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
     return handledNodes.contains(node.id)
   }
 
+  /// Utility function to emit a diagnostic that removes a misplaced token and instead inserts an equivalent token at the corrected location.
+  ///
+  /// If `incorrectContainer` contains only tokens that satisfy `unexpectedTokenCondition`, emit a diagnostic with message `message` that marks this token as misplaced.
+  /// If `correctTokens` contains missing tokens, also emit a Fix-It with message `fixIt` that marks the unexpected token as missing and instead inserts `correctTokens`.
+  public func exchangeTokens(
+    unexpected: UnexpectedNodesSyntax?,
+    unexpectedTokenCondition: (TokenSyntax) -> Bool,
+    correctTokens: [TokenSyntax?],
+    message: (_ misplacedTokens: [TokenSyntax]) -> DiagnosticMessage,
+    fixIt: (_ misplacedTokens: [TokenSyntax]) -> FixItMessage
+  ) {
+    guard let incorrectContainer = unexpected,
+          let misplacedTokens = incorrectContainer.onlyTokens(satisfying: unexpectedTokenCondition) else {
+      // If there are no unexpected nodes or the unexpected contain multiple tokens, don't emit a diagnostic.
+      return
+    }
+
+    // Ignore `correctTokens` that are already present.
+    let correctTokens = correctTokens.compactMap({ $0 }).filter({ $0.presence == .missing })
+    var changes = misplacedTokens.map(FixIt.Changes.makeMissing)
+    for correctToken in correctTokens {
+      if misplacedTokens.count == 1, let misplacedToken = misplacedTokens.first,
+         misplacedToken.nextToken(viewMode: .all) == correctToken || misplacedToken.previousToken(viewMode: .all) == correctToken {
+        changes.append(FixIt.Changes.makePresent(
+          node: correctToken,
+          leadingTrivia: misplacedToken.leadingTrivia,
+          trailingTrivia: misplacedToken.trailingTrivia
+        ))
+      } else {
+        changes.append(FixIt.Changes.makePresent(
+          node: correctToken,
+          leadingTrivia: nil,
+          trailingTrivia: nil
+        ))
+      }
+    }
+    var fixIts: [FixIt] = []
+    if changes.count > 1 {
+      // Only emit a Fix-It if we are moving a token, i.e. also making a token present.
+      fixIts.append(FixIt(message: fixIt(misplacedTokens), changes: changes))
+    }
+
+    addDiagnostic(incorrectContainer, message(misplacedTokens), fixIts: fixIts, handledNodes: [incorrectContainer.id] + correctTokens.map(\.id))
+  }
+
   // MARK: - Generic diagnostic generation
 
   public override func visitAny(_ node: Syntax) -> SyntaxVisitorContinueKind {
@@ -180,6 +225,20 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
     return .visitChildren
   }
 
+  public override func visit(_ node: ArrowExprSyntax) -> SyntaxVisitorContinueKind {
+    if shouldSkip(node) {
+      return .skipChildren
+    }
+    exchangeTokens(
+      unexpected: node.unexpectedAfterArrowToken,
+      unexpectedTokenCondition: { $0.tokenKind == .contextualKeyword("async") || $0.tokenKind == .throwsKeyword },
+      correctTokens: [node.asyncKeyword, node.throwsToken],
+      message: { EffectsSpecifierAfterArrow(effectsSpecifiersAfterArrow: $0) },
+      fixIt: { MoveEffectsSpecifiersBeforeArrowFixIt(effectsSpecifiersAfterArrow: $0) }
+    )
+    return .visitChildren
+  }
+
   public override func visit(_ node: ForInStmtSyntax) -> SyntaxVisitorContinueKind {
     if shouldSkip(node) {
       return .skipChildren

Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift

@@ -83,6 +83,14 @@ public enum StaticParserError: String, DiagnosticMessage {
 
 // MARK: - Diagnostics (please sort alphabetically)
 
+public struct EffectsSpecifierAfterArrow: ParserError {
+  public let effectsSpecifiersAfterArrow: [TokenSyntax]
+
+  public var message: String {
+    "\(missingNodesDescription(missingNodes: effectsSpecifiersAfterArrow.map(Syntax.init), commonParent: nil)) may only occur before '->'"
+  }
+}
+
 public struct ExtaneousCodeAtTopLevel: ParserError {
   public let extraneousCode: UnexpectedNodesSyntax
 
@@ -149,3 +157,11 @@ public enum StaticParserFixIt: String, FixItMessage {
     MessageID(domain: diagnosticDomain, id: "\(type(of: self)).\(self)")
   }
 }
+
+public struct MoveEffectsSpecifiersBeforeArrowFixIt: ParserFixIt {
+  public let effectsSpecifiersAfterArrow: [TokenSyntax]
+
+  public var message: String {
+    "move \(missingNodesDescription(missingNodes: effectsSpecifiersAfterArrow.map(Syntax.init), commonParent: nil)) in front of '->'"
+  }
+}

Sources/SwiftParser/Diagnostics/SyntaxExtensions.swift

@@ -20,6 +20,30 @@ extension UnexpectedNodesSyntax {
   func tokens(withKind kind: TokenKind) -> [TokenSyntax] {
     return self.tokens(satisfying: { $0.tokenKind == kind })
   }
+
+  /// If this only contains a single item that is a token, return that token, otherwise return `nil`.
+  var onlyToken: TokenSyntax? {
+    return onlyToken(where: { _ in true })
+  }
+
+  /// If this only contains a single item, which is a token satisfying `condition`, return that token, otherwise return `nil`.
+  func onlyToken(where condition: (TokenSyntax) -> Bool) -> TokenSyntax? {
+    if self.count == 1, let token = self.first?.as(TokenSyntax.self), condition(token) {
+      return token
+    } else {
+      return nil
+    }
+  }
+
+  /// If this only contains tokens satisfying `condition`, return an array containing those tokens, otherwise return `nil`.
+  func onlyTokens(satisfying condition: (TokenSyntax) -> Bool) -> [TokenSyntax]? {
+    let tokens = tokens(satisfying: condition)
+    if tokens.count == self.count {
+      return tokens
+    } else {
+      return nil
+    }
+  }
 }
 
 extension Syntax {

Sources/SwiftParser/Expressions.swift

@@ -319,17 +319,42 @@ extension Parser {
         return nil
       }
     case (.arrow, _)?, (.throwsKeyword, _)?:
-      let asyncKeyword = self.consumeIfContextualKeyword("async")
+      var asyncKeyword = self.consumeIfContextualKeyword("async")
+
+      var throwsKeyword = self.consume(if: .throwsKeyword)
 
-      let throwsKeyword = self.consume(if: .throwsKeyword)
       let (unexpectedBeforeArrow, arrow) = self.expect(.arrow)
 
+      // Recover if effect modifiers are specified after '->' by eating them into
+      // unexpectedAfterArrow and inserting the corresponding effects modifier as
+      // missing into the ArrowExprSyntax. This reflect the semantics the user
+      // originally intended.
+      var effectModifiersAfterArrow: [RawTokenSyntax] = []
+      if let asyncAfterArrow = self.consumeIfContextualKeyword("async") {
+        effectModifiersAfterArrow.append(asyncAfterArrow)
+        if asyncKeyword == nil {
+          asyncKeyword = missingToken(.contextualKeyword, text: "async")
+        }
+      }
+      if let throwsAfterArrow = self.consume(if: .throwsKeyword) {
+        effectModifiersAfterArrow.append(throwsAfterArrow)
+        if throwsKeyword == nil {
+          throwsKeyword = missingToken(.throwsKeyword, text: nil)
+        }
+      }
+      let unexpectedAfterArrow = effectModifiersAfterArrow.isEmpty ? nil : RawUnexpectedNodesSyntax(
+        elements: effectModifiersAfterArrow.map { RawSyntax($0) },
+        arena: self.arena
+      )
+
       let op = RawArrowExprSyntax(
         asyncKeyword: asyncKeyword,
         throwsToken: throwsKeyword,
         unexpectedBeforeArrow,
         arrowToken: arrow,
-        arena: self.arena)
+        unexpectedAfterArrow,
+        arena: self.arena
+      )
 
       return (RawExprSyntax(op), nil)
 

Tests/SwiftParserTest/Expressions.swift

@@ -581,10 +581,17 @@ final class ExpressionTests: XCTestCase {
     AssertParse(
       "[(Int) -> #^DIAG^#throws Int]()",
       diagnostics: [
-        // FIXME: We should suggest to move 'throws' in front of '->'
-        DiagnosticSpec(message: "expected expression in array element"),
-        DiagnosticSpec(message: "unexpected text 'throws Int' in array"),
-      ]
+        DiagnosticSpec(message: "'throws' may only occur before '->'", fixIts: ["move 'throws' in front of '->'"]),
+      ],
+      fixedSource: "[(Int) throws -> Int]()"
+    )
+
+    AssertParse(
+      "[(Int) -> #^DIAG^#async throws Int]()",
+      diagnostics: [
+        DiagnosticSpec(message: "'async throws' may only occur before '->'", fixIts: ["move 'async throws' in front of '->'"]),
+      ],
+      fixedSource: "[(Int) async throws -> Int]()"
     )
 
     AssertParse(

Tests/SwiftParserTest/translated/AsyncTests.swift

@@ -91,16 +91,20 @@ final class AsyncTests: XCTestCase {
   func testAsync8() {
     AssertParse(
       """
-      func asyncGlobal8() async throws#^DIAG_1^# async -> async#^DIAG_2^# Int#^DIAG_3^# async {}
+      func asyncGlobal8() async throws#^DIAG_1^# async -> #^DIAG_2^#async Int#^DIAG_3^# async {}
       """,
       diagnostics: [
         DiagnosticSpec(locationMarker: "DIAG_1", message: "consecutive statements on a line must be separated by ';'"),
-        DiagnosticSpec(locationMarker: "DIAG_2", message: "consecutive statements on a line must be separated by ';'"),
+        DiagnosticSpec(locationMarker: "DIAG_2", message: "'async' may only occur before '->'"),
         DiagnosticSpec(locationMarker: "DIAG_3", message: "consecutive statements on a line must be separated by ';'"),
         // TODO: Old parser expected error on line 1: 'async' has already been specified, Fix-It replacements: 34 - 40 = ''
         // TODO: Old parser expected error on line 1: 'async' has already been specified, Fix-It replacements: 43 - 49 = ''
         // TODO: Old parser expected error on line 1: 'async' has already been specified, Fix-It replacements: 53 - 59 = ''
-      ]
+      ],
+      // TODO: This should not insert another 'async'
+      fixedSource: """
+      func asyncGlobal8() async throws; async async -> Int; async {}
+      """
     )
   }
 
@@ -135,16 +139,29 @@ final class AsyncTests: XCTestCase {
     )
   }
 
-  func testAsync10() {
+  func testAsync10a() {
     AssertParse(
       """
-      // Parsing function types with 'async'.
       typealias AsyncFunc1 = () async -> ()
+      """
+    )
+  }
+
+  func testAsync10b() {
+    AssertParse(
+      """
       typealias AsyncFunc2 = () async throws -> ()
+      """
+    )
+  }
+
+  func testAsync10c() {
+    AssertParse(
+      """
       typealias AsyncFunc3 = () throws async -> ()
       """,
       diagnostics: [
-        // TODO: Old parser expected error on line 4: 'async' must precede 'throws', Fix-It replacements: 34 - 40 = '', 27 - 27 = 'async '
+        // TODO: Old parser expected error on line 1: 'async' must precede 'throws', Fix-It replacements: 34 - 40 = '', 27 - 27 = 'async '
       ]
     )
   }
@@ -156,14 +173,14 @@ final class AsyncTests: XCTestCase {
       func testTypeExprs() {
         let _ = [() async -> ()]()
         let _ = [() async throws -> ()]()
-        let _ = [() throws #^DIAG^#async -> ()]()  
-        let _ = [() -> async ()]() 
+        let _ = [() throws #^DIAG_1^#async -> ()]()
+        let _ = [() -> #^DIAG_2^#async ()]()
       }
       """,
       diagnostics: [
         // TODO: Old parser expected error on line 5: 'async' must precede 'throws', Fix-It replacements: 22 - 28 = '', 15 - 15 = 'async '
-        DiagnosticSpec(message: "unexpected text 'async' in array element"),
-        // TODO: Old parser expected error on line 6: 'async' may only occur before '->', Fix-It replacements: 18 - 24 = '', 15 - 15 = 'async '
+        DiagnosticSpec(locationMarker: "DIAG_1", message: "unexpected text 'async' in array element"),
+        DiagnosticSpec(locationMarker: "DIAG_2", message: "'async' may only occur before '->'"),
       ]
     )
   }