Merge pull request #623 from ahoppen/pr/diag-changes

Several improvements to diagnostics in the new parser

Author: ahoppen

Package.swift

@@ -58,6 +58,10 @@ let package = Package(
         "README.md"
       ]
     ),
+    .target(
+      name: "SwiftDiagnostics",
+      dependencies: ["SwiftSyntax"]
+    ),
     .target(
       name: "SwiftSyntax",
       dependencies: ["_CSwiftSyntax"],
@@ -105,7 +109,7 @@ let package = Package(
     ),
     .target(
       name: "SwiftParser",
-      dependencies: ["SwiftSyntax"],
+      dependencies: ["SwiftDiagnostics", "SwiftSyntax"],
       exclude: [
         "README.md"
       ]
@@ -116,7 +120,7 @@ let package = Package(
     ),
     .executableTarget(
       name: "swift-parser-test",
-      dependencies: ["SwiftSyntax", "SwiftParser", .product(name: "ArgumentParser", package: "swift-argument-parser")]
+      dependencies: ["SwiftDiagnostics", "SwiftSyntax", "SwiftParser", .product(name: "ArgumentParser", package: "swift-argument-parser")]
     ),
     .executableTarget(
         name: "generate-swift-syntax-builder",
@@ -162,7 +166,7 @@ let package = Package(
     ),
     .testTarget(
       name: "SwiftParserTest",
-      dependencies: ["SwiftParser", "_SwiftSyntaxTestSupport"]
+      dependencies: ["SwiftDiagnostics", "SwiftParser", "_SwiftSyntaxTestSupport"]
     ),
   ]
 )

Sources/SwiftParser/Diagnostics/Diagnostic.swift renamed to Sources/SwiftDiagnostics/Diagnostic.swift

@@ -12,16 +12,25 @@
 
 import SwiftSyntax
 
-public struct Diagnostic {
+public struct Diagnostic: CustomDebugStringConvertible {
   /// The message that should be displayed to the user
   public let diagMessage: DiagnosticMessage
 
   /// The node at whose start location the message should be displayed.
   public let node: Syntax
 
-  init(node: Syntax, message: DiagnosticMessage) {
+  /// Nodes that should be highlighted in the source code.
+  public let highlights: [Syntax]
+
+  /// Fix-Its that can be applied to resolve this diagnostic.
+  /// Each Fix-It offers a different way to resolve the diagnostic. Usually, there's only one.
+  public let fixIts: [FixIt]
+
+  public init(node: Syntax, message: DiagnosticMessage, highlights: [Syntax] = [], fixIts: [FixIt] = []) {
     self.diagMessage = message
     self.node = node
+    self.highlights = highlights
+    self.fixIts = fixIts
   }
 
   /// The message that should be displayed to the user.
@@ -30,14 +39,24 @@ public struct Diagnostic {
   }
 
   /// An ID that identifies the diagnostic's message.
-  /// See ``DiagnosticMessageID``.
-  public var diagnosticID: DiagnosticMessageID {
+  /// See ``MessageID``.
+  public var diagnosticID: MessageID {
     return diagMessage.diagnosticID
   }
 
   /// The location at which the diagnostic should be displayed.
   public func location(converter: SourceLocationConverter) -> SourceLocation {
     return node.startLocation(converter: converter)
   }
+
+  public var debugDescription: String {
+    if let root = node.root.as(SourceFileSyntax.self) {
+      let locationConverter = SourceLocationConverter(file: "", tree: root)
+      let location = location(converter: locationConverter)
+      return "\(location): \(message)"
+    } else {
+      return "<unknown>: \(message)"
+    }
+  }
 }
 

Sources/SwiftDiagnostics/FixIt.swift

@@ -0,0 +1,46 @@
+//===--- FixIt.swift ------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2022 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import SwiftSyntax
+
+/// Types conforming to this protocol represent Fix-It messages that can be
+/// shown to the client.
+/// The messages should describe the change that the Fix-It will perform
+public protocol FixItMessage {
+  /// The diagnostic message that should be displayed in the client.
+  var message: String { get }
+
+  /// See ``MessageID``.
+  var fixItID: MessageID { get }
+}
+
+
+/// A Fix-It that can be applied to resolve a diagnostic.
+public struct FixIt {
+  public enum Change {
+    /// Replace `oldNode` by `newNode`.
+    case replace(oldNode: Syntax, newNode: Syntax)
+  }
+
+  /// A description of what this Fix-It performs.
+  public let message: FixItMessage
+
+  /// The changes that need to be performed when the Fix-It is applied.
+  public let changes: [Change]
+
+  public init(message: FixItMessage, changes: [Change]) {
+    assert(!changes.isEmpty, "A Fix-It must have at least one diagnostic associated with it")
+    self.message = message
+    self.changes = changes
+  }
+}
+

Sources/SwiftParser/Diagnostics/DiagnosticMessage.swift renamed to Sources/SwiftDiagnostics/Message.swift

@@ -1,4 +1,4 @@
-//===--- DiagnosticMessage.swift ------------------------------------------===//
+//===--- Message.swift ----------------------------------------------------===//
 //
 // This source file is part of the Swift.org open source project
 //
@@ -17,35 +17,30 @@
 /// Two diagnostics with the same ID don’t need to necessarily have the exact
 /// same wording. Eg. it’s possible that the message contains more context when
 /// available.
-public struct DiagnosticMessageID: Hashable {
-  private let value: String
+public struct MessageID: Hashable {
+  private let domain: String
+  private let id: String
 
-  public init(_ value: String) {
-    self.value = value
+  public init(domain: String, id: String) {
+    self.domain = domain
+    self.id = id
   }
 }
 
+public enum DiagnosticSeverity {
+  case error
+  case warning
+  case note
+}
+
 /// Types conforming to this protocol represent diagnostic messages that can be
 /// shown to the client.
 public protocol DiagnosticMessage {
   /// The diagnostic message that should be displayed in the client.
   var message: String { get }
 
-  /// See ``DiagnosticMessageID``.
-  var diagnosticID: DiagnosticMessageID { get }
-}
-
-/// A diagnostic how's ID is determined by the diagnostic's type.
-public protocol TypedDiagnosticMessage: DiagnosticMessage {
-  var diagnosticID: DiagnosticMessageID { get }
-}
-
-public extension TypedDiagnosticMessage {
-  static var diagnosticID: DiagnosticMessageID {
-    return DiagnosticMessageID("\(self)")
-  }
+  /// See ``MessageID``.
+  var diagnosticID: MessageID { get }
 
-  var diagnosticID: DiagnosticMessageID {
-    return Self.diagnosticID
-  }
+  var severity: DiagnosticSeverity { get }
 }

Sources/SwiftParser/Declarations.swift

@@ -1155,21 +1155,25 @@ extension Parser {
       arena: self.arena)
   }
 
+  /// If a `throws` keyword appears right in front of the `arrow`, it is returned as `misplacedThrowsKeyword` so it can be synthesized in front of the arrow.
   @_spi(RawSyntax)
-  public mutating func parseFunctionReturnClause() -> RawReturnClauseSyntax {
+  public mutating func parseFunctionReturnClause() -> (returnClause: RawReturnClauseSyntax, misplacedThrowsKeyword: RawTokenSyntax?) {
     let arrow = self.eat(.arrow)
+    var misplacedThrowsKeyword: RawTokenSyntax? = nil
     let unexpectedBeforeReturnType: RawUnexpectedNodesSyntax?
-    if let unexpectedToken = self.consume(ifAny: .tryKeyword, .throwKeyword, .throwsKeyword) {
-      unexpectedBeforeReturnType = RawUnexpectedNodesSyntax(elements: [RawSyntax(unexpectedToken)], arena: self.arena)
+    if let throwsKeyword = self.consume(ifAny: .rethrowsKeyword, .throwsKeyword) {
+      misplacedThrowsKeyword = throwsKeyword
+      unexpectedBeforeReturnType = RawUnexpectedNodesSyntax(elements: [RawSyntax(throwsKeyword)], arena: self.arena)
     } else {
       unexpectedBeforeReturnType = nil
     }
     let result = self.parseType()
-    return RawReturnClauseSyntax(
+    let returnClause = RawReturnClauseSyntax(
       arrow: arrow,
       unexpectedBeforeReturnType,
       returnType: result,
       arena: self.arena)
+    return (returnClause, misplacedThrowsKeyword)
   }
 }
 
@@ -1227,7 +1231,7 @@ extension Parser {
       async = nil
     }
 
-    let throwsKeyword: RawTokenSyntax?
+    var throwsKeyword: RawTokenSyntax?
     if self.at(.throwsKeyword) || self.at(.rethrowsKeyword) {
       throwsKeyword = self.consumeAnyToken()
     } else {
@@ -1236,7 +1240,11 @@ extension Parser {
 
     let output: RawReturnClauseSyntax?
     if self.at(.arrow) {
-      output = self.parseFunctionReturnClause()
+      let result = self.parseFunctionReturnClause()
+      output = result.returnClause
+      if let misplacedThrowsKeyword = result.misplacedThrowsKeyword, throwsKeyword == nil {
+        throwsKeyword = RawTokenSyntax(missing: misplacedThrowsKeyword.tokenKind, arena: self.arena)
+      }
     } else {
       output = nil
     }
@@ -1275,7 +1283,7 @@ extension Parser {
 
     let result: RawReturnClauseSyntax
     if self.at(.arrow) {
-      result = self.parseFunctionReturnClause()
+      result = self.parseFunctionReturnClause().returnClause
     } else {
       result = RawReturnClauseSyntax(
         arrow: RawTokenSyntax(missing: .arrow, arena: self.arena),

Sources/SwiftParser/Diagnostics/ParseDiagnosticsGenerator.swift

@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+import SwiftDiagnostics
 import SwiftSyntax
 
 extension UnexpectedNodesSyntax {
@@ -22,6 +23,25 @@ extension UnexpectedNodesSyntax {
   }
 }
 
+fileprivate extension FixIt.Change {
+  /// Replaced a present node with a missing node
+  static func makeMissing(node: TokenSyntax) -> FixIt.Change {
+    assert(node.presence == .present)
+    return .replace(
+      oldNode: Syntax(node),
+      newNode: Syntax(TokenSyntax(node.tokenKind, leadingTrivia: [], trailingTrivia: [], presence: .missing))
+    )
+  }
+
+  static func makePresent(node: TokenSyntax, leadingTrivia: Trivia = [], trailingTrivia: Trivia = []) -> FixIt.Change {
+    assert(node.presence == .missing)
+    return .replace(
+      oldNode: Syntax(node),
+      newNode: Syntax(TokenSyntax(node.tokenKind, leadingTrivia: leadingTrivia, trailingTrivia: trailingTrivia, presence: .present))
+    )
+  }
+}
+
 public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
   private var diagnostics: [Diagnostic] = []
 
@@ -42,8 +62,13 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
   // MARK: - Private helper functions
 
   /// Produce a diagnostic.
-  private func addDiagnostic<T: SyntaxProtocol>(_ node: T, _ message: DiagnosticMessage) {
-    diagnostics.append(Diagnostic(node: Syntax(node), message: message))
+  private func addDiagnostic<T: SyntaxProtocol>(_ node: T, _ message: DiagnosticMessage, highlights: [Syntax] = [], fixIts: [FixIt] = []) {
+    diagnostics.append(Diagnostic(node: Syntax(node), message: message, highlights: highlights, fixIts: fixIts))
+  }
+
+  /// Produce a diagnostic.
+  private func addDiagnostic<T: SyntaxProtocol>(_ node: T, _ message: StaticParserError, highlights: [Syntax] = [], fixIts: [FixIt] = []) {
+    addDiagnostic(node, message as DiagnosticMessage, highlights: highlights, fixIts: fixIts)
   }
 
   /// If a diagnostic is generated that covers multiple syntax nodes, mark them as handles so they don't produce the generic diagnostics anymore.
@@ -70,7 +95,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
     if shouldSkip(node) {
       return .skipChildren
     }
-    addDiagnostic(node, UnexpectedNodesDiagnostic(unexpectedNodes: node))
+    addDiagnostic(node, UnexpectedNodesError(unexpectedNodes: node))
     return .skipChildren
   }
 
@@ -79,7 +104,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
       return .skipChildren
     }
     if node.presence == .missing {
-      addDiagnostic(node, MissingTokenDiagnostic(missingToken: node))
+      addDiagnostic(node, MissingTokenError(missingToken: node))
     }
     return .skipChildren
   }
@@ -90,13 +115,25 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
     if shouldSkip(node) {
       return .skipChildren
     }
+    // Detect C-style for loops based on two semicolons which could not be parsed between the 'for' keyword and the '{'
     // This is mostly a proof-of-concept implementation to produce more complex diagnostics.
-    if let unexpectedCondition = node.body.unexpectedBeforeLeftBrace {
-      // Detect C-style for loops based on two semicolons which could not be parsed between the 'for' keyword and the '{'
-      if unexpectedCondition.tokens(withKind: .semicolon).count == 2 {
-        addDiagnostic(node, CStyleForLoopDiagnostic())
-        markNodesAsHandled(node.inKeyword.id, unexpectedCondition.id)
-      }
+    if let unexpectedCondition = node.body.unexpectedBeforeLeftBrace,
+       unexpectedCondition.tokens(withKind: .semicolon).count == 2 {
+      // FIXME: This is aweful. We should have a way to either get all children between two cursors in a syntax node or highlight a range from one node to another.
+      addDiagnostic(node, .cStyleForLoop, highlights: ([
+        Syntax(node.pattern),
+        Syntax(node.unexpectedBetweenPatternAndTypeAnnotation),
+        Syntax(node.typeAnnotation),
+        Syntax(node.unexpectedBetweenTypeAnnotationAndInKeyword),
+        Syntax(node.inKeyword),
+        Syntax(node.unexpectedBetweenInKeywordAndSequenceExpr),
+        Syntax(node.sequenceExpr),
+        Syntax(node.unexpectedBetweenSequenceExprAndWhereClause),
+        Syntax(node.whereClause),
+        Syntax(node.unexpectedBetweenWhereClauseAndBody),
+        Syntax(unexpectedCondition)
+      ] as [Syntax?]).compactMap({ $0 }))
+      markNodesAsHandled(node.inKeyword.id, unexpectedCondition.id)
     }
     return .visitChildren
   }
@@ -105,12 +142,19 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
     if shouldSkip(node) {
       return .skipChildren
     }
-    if let output = node.output, let unexpectedBeforeReturnType = output.unexpectedBetweenArrowAndReturnType {
-      if let throwsInReturnPosition = unexpectedBeforeReturnType.tokens(withKind: .throwsKeyword).first {
-        addDiagnostic(throwsInReturnPosition, ThrowsInReturnPositionDiagnostic())
-        markNodesAsHandled(unexpectedBeforeReturnType.id, throwsInReturnPosition.id)
-        return .visitChildren
-      }
+    if let output = node.output,
+       let missingThrowsKeyword = node.throwsOrRethrowsKeyword,
+       missingThrowsKeyword.presence == .missing,
+       let unexpectedBeforeReturnType = output.unexpectedBetweenArrowAndReturnType,
+       let throwsInReturnPosition = unexpectedBeforeReturnType.tokens(withKind: .throwsKeyword).first {
+      addDiagnostic(throwsInReturnPosition, .throwsInReturnPosition, fixIts: [
+        FixIt(message: StaticParserFixIt.moveThrowBeforeArrow, changes: [
+          .makeMissing(node: throwsInReturnPosition),
+          .makePresent(node: missingThrowsKeyword, trailingTrivia: .space),
+        ])
+      ])
+      markNodesAsHandled(unexpectedBeforeReturnType.id, missingThrowsKeyword.id, throwsInReturnPosition.id)
+      return .visitChildren
     }
     return .visitChildren
   }