Merge pull request #1283 from ahoppen/ahoppen/missing-expr-in-sequence

Improve diagnostics for missing expressions in a sequence expression

Author: ahoppen

CodeGeneration/Sources/SyntaxSupport/gyb_generated/ExprNodes.swift

@@ -197,7 +197,7 @@ public let EXPR_NODES: [Node] = [
        elementName: "Expression"),
 
   Node(name: "PrefixOperatorExpr",
-       nameForDiagnostics: "prefix operator expression",
+       nameForDiagnostics: "operator",
        kind: "Expr",
        children: [
          Child(name: "OperatorToken",
@@ -208,7 +208,7 @@ public let EXPR_NODES: [Node] = [
        ]),
 
   Node(name: "BinaryOperatorExpr",
-       nameForDiagnostics: nil,
+       nameForDiagnostics: "operator",
        kind: "Expr",
        children: [
          Child(name: "OperatorToken",
@@ -363,7 +363,7 @@ public let EXPR_NODES: [Node] = [
        ]),
 
   Node(name: "UnresolvedTernaryExpr",
-       nameForDiagnostics: nil,
+       nameForDiagnostics: "ternary operator",
        kind: "Expr",
        children: [
          Child(name: "QuestionMark",
@@ -407,15 +407,15 @@ public let EXPR_NODES: [Node] = [
        ]),
 
   Node(name: "UnresolvedIsExpr",
-       nameForDiagnostics: nil,
+       nameForDiagnostics: "'is'",
        kind: "Expr",
        children: [
          Child(name: "IsTok",
                kind: .token(choices: [.keyword(text: "is")]))
        ]),
 
   Node(name: "IsExpr",
-       nameForDiagnostics: "'is' expression",
+       nameForDiagnostics: "'is'",
        kind: "Expr",
        children: [
          Child(name: "Expression",
@@ -427,7 +427,7 @@ public let EXPR_NODES: [Node] = [
        ]),
 
   Node(name: "UnresolvedAsExpr",
-       nameForDiagnostics: nil,
+       nameForDiagnostics: "'as'",
        kind: "Expr",
        children: [
          Child(name: "AsTok",
@@ -438,7 +438,7 @@ public let EXPR_NODES: [Node] = [
        ]),
 
   Node(name: "AsExpr",
-       nameForDiagnostics: "'as' expression",
+       nameForDiagnostics: "'as'",
        kind: "Expr",
        children: [
          Child(name: "Expression",

Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift

@@ -96,12 +96,11 @@ extension FixIt.Changes {
         ),
         .replaceLeadingTrivia(token: nextToken, newTrivia: []),
       ]
-    } else if let firstToken = node.firstToken(viewMode: .all),
+    } else if node.leadingTrivia?.isEmpty ?? true,
       let previousToken = node.previousToken(viewMode: .fixedUp),
-      !firstToken.tokenKind.isPunctuation,
-      !previousToken.tokenKind.isPunctuation,
-      firstToken.leadingTrivia.isEmpty,
-      (previousToken.presence == .missing ? BasicFormat().visit(previousToken).trailingTrivia : previousToken.trailingTrivia).isEmpty,
+      previousToken.presence == .present,
+      previousToken.trailingTrivia.isEmpty,
+      BasicFormat().requiresTrailingSpace(previousToken),
       leadingTrivia == nil
     {
       /// If neither this nor the previous token are punctionation make sure they

Sources/SwiftParserDiagnostics/MissingNodesError.swift

@@ -223,6 +223,25 @@ public struct MissingNodesError: ParserError {
       }
     }
 
+    let missingExpr: ExprSyntax?
+    if let expr = firstMissingNode.as(MissingExprSyntax.self) {
+      missingExpr = ExprSyntax(expr)
+    } else if let typeExpr = firstMissingNode.parent?.as(TypeExprSyntax.self) {
+      missingExpr = ExprSyntax(typeExpr)
+    } else {
+      missingExpr = nil
+    }
+    if let missingExpr = missingExpr,
+      let exprList = missingExpr.parent?.as(ExprListSyntax.self),
+      exprList.parent?.is(SequenceExprSyntax.self) ?? false,
+      let previousSiblingIndex = exprList.index(missingExpr.index, offsetBy: -1, limitedBy: exprList.startIndex)
+    {
+      let previousSibling = exprList[previousSiblingIndex]
+      if let previousSiblingName = previousSibling.nodeTypeNameForDiagnostics(allowBlockNames: false) {
+        return "after \(previousSiblingName)"
+      }
+    }
+
     return nil
   }
 
@@ -261,8 +280,7 @@ public struct MissingNodesError: ParserError {
     var message = "expected \(description)"
     if let afterClause = afterClause {
       message += " \(afterClause)"
-    }
-    if let parentContextClause = parentContextClause(anchor: anchor?.parent ?? findCommonAncestor(missingNodes)) {
+    } else if let parentContextClause = parentContextClause(anchor: anchor?.parent ?? findCommonAncestor(missingNodes)) {
       message += " \(parentContextClause)"
     }
     return message

Sources/SwiftSyntax/generated/Misc.swift

@@ -836,7 +836,7 @@ extension SyntaxKind {
     case .arrowExpr: 
       return nil
     case .asExpr: 
-      return "'as' expression"
+      return "'as'"
     case .assignmentExpr: 
       return nil
     case .associatedtypeDecl: 
@@ -868,7 +868,7 @@ extension SyntaxKind {
     case .backDeployAttributeSpecList: 
       return "'@_backDeploy' arguments"
     case .binaryOperatorExpr: 
-      return nil
+      return "operator"
     case .booleanLiteralExpr: 
       return "bool literal"
     case .borrowExpr: 
@@ -1082,7 +1082,7 @@ extension SyntaxKind {
     case .integerLiteralExpr: 
       return "integer literal"
     case .isExpr: 
-      return "'is' expression"
+      return "'is'"
     case .isTypePattern: 
       return "'is' pattern"
     case .keyPathComponentList: 
@@ -1202,7 +1202,7 @@ extension SyntaxKind {
     case .precedenceGroupRelation: 
       return "'relation' property of precedencegroup"
     case .prefixOperatorExpr: 
-      return "prefix operator expression"
+      return "operator"
     case .primaryAssociatedTypeClause: 
       return "primary associated type clause"
     case .primaryAssociatedTypeList: 
@@ -1302,13 +1302,13 @@ extension SyntaxKind {
     case .unexpectedNodes: 
       return nil
     case .unresolvedAsExpr: 
-      return nil
+      return "'as'"
     case .unresolvedIsExpr: 
-      return nil
+      return "'is'"
     case .unresolvedPatternExpr: 
       return nil
     case .unresolvedTernaryExpr: 
-      return nil
+      return "ternary operator"
     case .valueBindingPattern: 
       return "value binding pattern"
     case .variableDecl: 

Tests/SwiftDiagnosticsTest/DiagnosticsFormatterTests.swift

@@ -30,10 +30,10 @@ final class DiagnosticsFormatterTests: XCTestCase {
       """
     let expectedOutput = """
       1 │ var foo = bar +
-        ∣                ╰─ expected expression in variable
+        ∣                ╰─ expected expression after operator
 
       """
-    AssertStringsEqualWithDiff(expectedOutput, annotate(source: source))
+    AssertStringsEqualWithDiff(annotate(source: source), expectedOutput)
   }
 
   func testMultipleDiagnosticsInOneLine() {
@@ -47,7 +47,7 @@ final class DiagnosticsFormatterTests: XCTestCase {
         ∣     ╰─ expected name in member access
 
       """
-    AssertStringsEqualWithDiff(expectedOutput, annotate(source: source))
+    AssertStringsEqualWithDiff(annotate(source: source), expectedOutput)
   }
 
   func testLineSkipping() {
@@ -78,7 +78,7 @@ final class DiagnosticsFormatterTests: XCTestCase {
          ∣         ╰─ expected value and ')' to end function call
 
       """
-    AssertStringsEqualWithDiff(expectedOutput, annotate(source: source))
+    AssertStringsEqualWithDiff(annotate(source: source), expectedOutput)
   }
 
   func testTwoDiagnosticsAtSameLocation() throws {
@@ -91,7 +91,7 @@ final class DiagnosticsFormatterTests: XCTestCase {
 
       """
 
-    AssertStringsEqualWithDiff(expectedOutput, annotate(source: source))
+    AssertStringsEqualWithDiff(annotate(source: source), expectedOutput)
   }
 
   func testAddsColoringToSingleErrorDiagnostic() {
@@ -101,10 +101,10 @@ final class DiagnosticsFormatterTests: XCTestCase {
 
     let expectedOutput = """
       1 │ var foo = bar +
-        ∣                ╰─ \u{001B}[1;31merror: expected expression in variable\u{001B}[0;0m
+        ∣                ╰─ \u{001B}[1;31merror: expected expression after operator\u{001B}[0;0m
 
       """
-    AssertStringsEqualWithDiff(expectedOutput, annotate(source: source, colorize: true))
+    AssertStringsEqualWithDiff(annotate(source: source, colorize: true), expectedOutput)
   }
 
   func testAddsColoringToMultipleDiagnosticsInOneLine() {
@@ -118,6 +118,6 @@ final class DiagnosticsFormatterTests: XCTestCase {
         ∣     ╰─ \u{001B}[1;31merror: expected name in member access\u{001B}[0;0m
 
       """
-    AssertStringsEqualWithDiff(expectedOutput, annotate(source: source, colorize: true))
+    AssertStringsEqualWithDiff(annotate(source: source, colorize: true), expectedOutput)
   }
 }

Tests/SwiftParserTest/DeclarationTests.swift

@@ -597,7 +597,7 @@ final class DeclarationTests: XCTestCase {
       }
       """,
       diagnostics: [
-        DiagnosticSpec(message: "expected declaration after 'public' modifier in struct")
+        DiagnosticSpec(message: "expected declaration after 'public' modifier")
       ]
     )
   }
@@ -929,7 +929,7 @@ final class DeclarationTests: XCTestCase {
         DiagnosticSpec(locationMarker: "1️⃣", message: "expected '{' in struct"),
         DiagnosticSpec(locationMarker: "2️⃣", message: "expected condition in conditional compilation clause"),
         DiagnosticSpec(locationMarker: "3️⃣", message: "expected name in attribute"),
-        DiagnosticSpec(locationMarker: "3️⃣", message: "expected declaration after attribute in conditional compilation clause"),
+        DiagnosticSpec(locationMarker: "3️⃣", message: "expected declaration after attribute"),
         DiagnosticSpec(locationMarker: "3️⃣", message: "expected '#endif' in conditional compilation block"),
         DiagnosticSpec(locationMarker: "3️⃣", message: "expected '}' to end struct"),
       ]
@@ -1394,6 +1394,20 @@ final class DeclarationTests: XCTestCase {
       """
     )
   }
+
+  func testMissingExpressionInVariableAssignment() {
+    AssertParse(
+      """
+      let a =1️⃣
+      """,
+      diagnostics: [
+        DiagnosticSpec(message: "expected expression in variable")
+      ],
+      fixedSource: """
+        let a = <#expression#>
+        """
+    )
+  }
 }
 
 extension Parser.DeclAttributes {

Tests/SwiftParserTest/ExpressionTests.swift

@@ -27,7 +27,7 @@ final class ExpressionTests: XCTestCase {
     AssertParse(
       "a ? b :1️⃣",
       diagnostics: [
-        DiagnosticSpec(message: "expected expression")
+        DiagnosticSpec(message: "expected expression after ternary operator")
       ]
     )
   }
@@ -788,7 +788,7 @@ final class ExpressionTests: XCTestCase {
       }
       """,
       diagnostics: [
-        DiagnosticSpec(message: "expected expression in 'do' statement")
+        DiagnosticSpec(message: "expected expression after ternary operator")
       ]
     )
   }
@@ -1099,6 +1099,56 @@ final class ExpressionTests: XCTestCase {
         """#
     )
   }
+
+  func testMissingExpresssionInSequenceExpression() {
+    AssertParse(
+      """
+      a ? b :1️⃣
+      """,
+      diagnostics: [
+        DiagnosticSpec(message: "expected expression after ternary operator")
+      ],
+      fixedSource: """
+        a ? b : <#expression#>
+        """
+    )
+
+    AssertParse(
+      """
+      a +1️⃣
+      """,
+      diagnostics: [
+        DiagnosticSpec(message: "expected expression after operator")
+      ],
+      fixedSource: """
+        a + <#expression#>
+        """
+    )
+
+    AssertParse(
+      """
+      a as1️⃣
+      """,
+      diagnostics: [
+        DiagnosticSpec(message: "expected type after 'as'")
+      ],
+      fixedSource: """
+        a as <#type#>
+        """
+    )
+
+    AssertParse(
+      """
+      a is1️⃣
+      """,
+      diagnostics: [
+        DiagnosticSpec(message: "expected type after 'is'")
+      ],
+      fixedSource: """
+        a is <#type#>
+        """
+    )
+  }
 }
 
 final class MemberExprTests: XCTestCase {

Tests/SwiftParserTest/translated/ConflictMarkersTests.swift

@@ -82,7 +82,7 @@ final class ConflictMarkersTests: XCTestCase {
       diagnostics: [
         // TODO: Old parser expected error on line 1: source control conflict marker in source file
         DiagnosticSpec(locationMarker: "1️⃣", message: "unexpected code '<<<<<<< HEAD:conflict_markers.swift' before variable"),
-        DiagnosticSpec(locationMarker: "2️⃣", message: "expected expression in variable"),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "expected expression after operator"),
         DiagnosticSpec(locationMarker: "3️⃣", message: "'b' is not a valid digit in integer literal"),
         DiagnosticSpec(locationMarker: "4️⃣", message: "extraneous code at top level"),
       ]

Tests/SwiftParserTest/translated/ErrorsTests.swift

@@ -87,7 +87,7 @@ final class ErrorsTests: XCTestCase {
       """,
       diagnostics: [
         // TODO: Old parser expected error on line 3: expected expression after '? ... :' in ternary expression
-        DiagnosticSpec(locationMarker: "1️⃣", message: "expected expression in 'do' statement")
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected expression after ternary operator")
       ]
     )
   }

Tests/SwiftParserTest/translated/RecoveryTests.swift

@@ -80,7 +80,7 @@ final class RecoveryTests: XCTestCase {
         DiagnosticSpec(locationMarker: "1️⃣", message: "expected '>' to end generic argument clause"),
         DiagnosticSpec(locationMarker: "2️⃣", message: "consecutive statements on a line must be separated by ';'"),
         DiagnosticSpec(locationMarker: "3️⃣", message: "unexpected code 'this greater: >' in subscript"),
-        DiagnosticSpec(locationMarker: "4️⃣", message: "expected expression in function"),
+        DiagnosticSpec(locationMarker: "4️⃣", message: "expected expression after operator"),
         DiagnosticSpec(locationMarker: "4️⃣", message: "unexpected code in function"),
       ]
     )