Merge pull request #1521 from kimdv/kimdv/chery-pick-unexpected-second-identifier

[5.9] Add diagnostic for unexpected second identifier

Author: ahoppen

Sources/SwiftParser/Patterns.swift

@@ -217,9 +217,22 @@ extension Parser {
       while !self.at(.eof, .rightParen) && keepGoing && loopProgress.evaluate(currentToken) {
         // If the tuple element has a label, parse it.
         let labelAndColon = self.consume(if: .identifier, followedBy: .colon)
-        let (label, colon) = (labelAndColon?.0, labelAndColon?.1)
+        var (label, colon) = (labelAndColon?.0, labelAndColon?.1)
+
+        /// If we have something like `x SomeType`, use the indication that `SomeType` starts with a capital letter (and is thus probably a type name)
+        /// as an indication that the user forgot to write the colon instead of forgetting to write the comma to separate two elements.
+        if label == nil, colon == nil, peek().rawTokenKind == .identifier, peek().tokenText.isStartingWithUppercase {
+          label = consumeAnyToken()
+          colon = self.missingToken(.colon)
+        }
+
         let pattern = self.parsePattern()
-        let trailingComma = self.consume(if: .comma)
+        var trailingComma = self.consume(if: .comma)
+
+        if trailingComma == nil && self.at(TokenSpec(.identifier, allowAtStartOfLine: false)) {
+          trailingComma = self.missingToken(RawTokenKind.comma)
+        }
+
         keepGoing = trailingComma != nil
         elements.append(
           RawTuplePatternElementSyntax(

Sources/SwiftParser/SyntaxUtils.swift

@@ -88,6 +88,14 @@ extension SyntaxText {
   var isEditorPlaceholder: Bool {
     return self.starts(with: SyntaxText("<#")) && self.hasSuffix(SyntaxText("#>"))
   }
+
+  var isStartingWithUppercase: Bool {
+    if !self.isEmpty, let firstCharacterByte = self.baseAddress?.pointee {
+      return 65 <= firstCharacterByte && firstCharacterByte <= 90
+    } else {
+      return false
+    }
+  }
 }
 
 extension RawTokenKind {

Sources/SwiftParser/Types.swift

@@ -548,7 +548,7 @@ extension Parser {
         if let first = first,
           second == nil,
           colon?.isMissing == true,
-          first.tokenText.description.first?.isUppercase == true
+          first.tokenText.isStartingWithUppercase
         {
           elements.append(
             RawTupleTypeElementSyntax(

Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

@@ -327,7 +327,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
         FixIt(
           message: .joinIdentifiers,
           changes: [
-            FixIt.MultiNodeChange(.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joined), presence: .present)))),
+            FixIt.MultiNodeChange(.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joined), trailingTrivia: tokens.last?.trailingTrivia ?? [], presence: .present)))),
             .makeMissing(tokens),
           ]
         )
@@ -338,7 +338,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
           FixIt(
             message: .joinIdentifiersWithCamelCase,
             changes: [
-              FixIt.MultiNodeChange(.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joinedUsingCamelCase), presence: .present)))),
+              FixIt.MultiNodeChange(.replace(oldNode: Syntax(previousToken), newNode: Syntax(TokenSyntax(.identifier(joinedUsingCamelCase), trailingTrivia: tokens.last?.trailingTrivia ?? [], presence: .present)))),
               .makeMissing(tokens),
             ]
           )

Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift

@@ -383,9 +383,9 @@ public struct SpaceSeparatedIdentifiersError: ParserError {
     if let name = firstToken.parent?.ancestorOrSelf(mapping: {
       $0.nodeTypeNameForDiagnostics(allowBlockNames: false)
     }) {
-      return "found an unexpected second identifier in \(name)"
+      return "found an unexpected second identifier in \(name); is there an accidental break?"
     } else {
-      return "found an unexpected second identifier"
+      return "found an unexpected second identifier; is there an accidental break?"
     }
   }
 }

Tests/SwiftParserTest/ParserTests.swift

@@ -54,7 +54,7 @@ public class ParserTests: XCTestCase {
     }
   }
 
-  /// Run parsr tests on all of the Swift files in the given path, recursively.
+  /// Run parser tests on all of the Swift files in the given path, recursively.
   func runParserTests(
     name: String,
     path: URL,

Tests/SwiftParserTest/TypeTests.swift

@@ -15,15 +15,17 @@
 import XCTest
 
 final class TypeTests: XCTestCase {
-
   func testMissingColonInType() {
     assertParse(
       """
       var foo 1️⃣Bar = 1
       """,
       diagnostics: [
-        DiagnosticSpec(message: "expected ':' in type annotation")
-      ]
+        DiagnosticSpec(message: "expected ':' in type annotation", fixIts: ["insert ':'"])
+      ],
+      fixedSource: """
+        var foo: Bar = 1
+        """
     )
   }
 
@@ -65,7 +67,6 @@ final class TypeTests: XCTestCase {
   }
 
   func testClosureSignatures() {
-
     assertParse(
       """
       simple { [] str in

Tests/SwiftParserTest/translated/InvalidTests.swift

@@ -388,92 +388,85 @@ final class InvalidTests: XCTestCase {
     )
   }
 
-  func testInvalid23a() {
-    assertParse(
-      """
-      func dog 1️⃣cow() {}
-      """,
-      diagnostics: [
-        DiagnosticSpec(
-          message: "found an unexpected second identifier in function",
-          fixIts: [
-            "join the identifiers together",
-            "join the identifiers together with camel-case",
-          ]
-        )
-      ],
-      applyFixIts: ["join the identifiers together"],
-      fixedSource: "func dogcow() {}"
-    )
-  }
-
-  func testInvalid23b() {
-    assertParse(
-      """
-      func dog 1️⃣cow() {}
-      """,
-      diagnostics: [
-        DiagnosticSpec(
-          message: "found an unexpected second identifier in function",
-          fixIts: [
-            "join the identifiers together",
-            "join the identifiers together with camel-case",
-          ]
-        )
-      ],
-      applyFixIts: ["join the identifiers together with camel-case"],
-      fixedSource: "func dogCow() {}"
-    )
+  func testInvalid23() {
+    let testCases: [UInt: (fixIt: String, fixedSource: String)] = [
+      #line: ("join the identifiers together", "func dogcow() {}"),
+      #line: ("join the identifiers together with camel-case", "func dogCow() {}"),
+    ]
+
+    for (line, testCase) in testCases {
+      assertParse(
+        """
+        func dog 1️⃣cow() {}
+        """,
+        diagnostics: [
+          DiagnosticSpec(
+            message: "found an unexpected second identifier in function; is there an accidental break?",
+            fixIts: [
+              "join the identifiers together",
+              "join the identifiers together with camel-case",
+            ]
+          )
+        ],
+        applyFixIts: [testCase.fixIt],
+        fixedSource: testCase.fixedSource,
+        line: line
+      )
+    }
   }
 
   func testThreeIdentifersForFunctionName() {
-    assertParse(
-      """
-      func dog 1️⃣cow sheep() {}
-      """,
-      diagnostics: [
-        DiagnosticSpec(
-          message: "found an unexpected second identifier in function",
-          fixIts: [
-            "join the identifiers together",
-            "join the identifiers together with camel-case",
-          ]
-        )
-      ],
-      applyFixIts: ["join the identifiers together with camel-case"],
-      fixedSource: "func dogCowSheep() {}"
-    )
-  }
+    let testCases: [UInt: (fixIt: String, fixedSource: String)] = [
+      #line: ("join the identifiers together", "func dogcowsheep() {}"),
+      #line: ("join the identifiers together with camel-case", "func dogCowSheep() {}"),
+    ]
 
-  func testInvalid24() {
-    assertParse(
-      """
-      func cat 1️⃣Mouse() {}
-      """,
-      diagnostics: [
-        DiagnosticSpec(message: "found an unexpected second identifier in function", fixIts: ["join the identifiers together"])
-      ],
-      fixedSource: "func catMouse() {}"
-    )
+    for (line, testCase) in testCases {
+      assertParse(
+        """
+        func dog 1️⃣cow sheep() {}
+        """,
+        diagnostics: [
+          DiagnosticSpec(
+            message: "found an unexpected second identifier in function; is there an accidental break?",
+            fixIts: [
+              "join the identifiers together",
+              "join the identifiers together with camel-case",
+            ]
+          )
+        ],
+        applyFixIts: [testCase.fixIt],
+        fixedSource: testCase.fixedSource,
+        line: line
+      )
+    }
   }
 
   func testInvalid25() {
-    assertParse(
-      """
-      func friend 1️⃣ship<T>(x: T) {}
-      """,
-      diagnostics: [
-        DiagnosticSpec(
-          message: "found an unexpected second identifier in function",
-          fixIts: [
-            "join the identifiers together",
-            "join the identifiers together with camel-case",
-          ]
-        )
-      ],
-      applyFixIts: ["join the identifiers together with camel-case"],
-      fixedSource: "func friendShip<T>(x: T) {}"
-    )
+    let testCases: [UInt: (fixIt: String, fixedSource: String)] = [
+      #line: ("join the identifiers together", "func friendship<T>(x: T) {}"),
+      #line: ("join the identifiers together with camel-case", "func friendShip<T>(x: T) {}"),
+    ]
+
+    for (line, testCase) in testCases {
+      assertParse(
+        """
+        func friend 1️⃣ship<T>(x: T) {}
+        """,
+        diagnostics: [
+          DiagnosticSpec(
+            message: "found an unexpected second identifier in function; is there an accidental break?",
+            fixIts: [
+              "join the identifiers together",
+              "join the identifiers together with camel-case",
+            ]
+          )
+        ],
+        applyFixIts: [testCase.fixIt],
+        fixedSource: testCase.fixedSource,
+        line: line
+      )
+    }
   }
 
   func testInvalid26() {

Tests/SwiftParserTest/translated/RecoveryTests.swift

@@ -742,14 +742,30 @@ final class RecoveryTests: XCTestCase {
 
   //===--- Recovery for multiple identifiers in decls
   func testRecovery58() {
-    assertParse(
-      """
-      protocol Multi 1️⃣ident {}
-      """,
-      diagnostics: [
-        DiagnosticSpec(message: "found an unexpected second identifier in protocol", highlight: "ident")
-      ]
-    )
+    let testCases: [UInt: (fixIt: String, fixedSource: String)] = [
+      #line: ("join the identifiers together", "protocol Multiident {}"),
+      #line: ("join the identifiers together with camel-case", "protocol MultiIdent {}"),
+    ]
+
+    for (line, testCase) in testCases {
+      assertParse(
+        """
+        protocol Multi 1️⃣ident {}
+        """,
+        diagnostics: [
+          DiagnosticSpec(
+            message: "found an unexpected second identifier in protocol; is there an accidental break?",
+            highlight: "ident",
+            fixIts: ["join the identifiers together", "join the identifiers together with camel-case"],
+            line: line
+          )
+        ],
+        applyFixIts: [testCase.fixIt],
+        fixedSource: testCase.fixedSource,
+        line: line
+      )
+    }
+
   }
 
   func testRecovery60() {
@@ -758,7 +774,11 @@ final class RecoveryTests: XCTestCase {
       class CCC 1️⃣CCC<T> {}
       """,
       diagnostics: [
-        DiagnosticSpec(message: "found an unexpected second identifier in class")
+        DiagnosticSpec(
+          message: "found an unexpected second identifier in class; is there an accidental break?",
+          highlight: "CCC<T>",
+          fixIts: ["join the identifiers together"]
+        )
       ],
       fixedSource: """
         class CCCCCC<T> {}
@@ -775,7 +795,7 @@ final class RecoveryTests: XCTestCase {
       }
       """,
       diagnostics: [
-        DiagnosticSpec(locationMarker: "1️⃣", message: "found an unexpected second identifier in enum"),
+        DiagnosticSpec(locationMarker: "1️⃣", message: "found an unexpected second identifier in enum; is there an accidental break?", fixIts: ["join the identifiers together"]),
         DiagnosticSpec(locationMarker: "2️⃣", message: "consecutive declarations on a line must be separated by ';'"),
         DiagnosticSpec(locationMarker: "3️⃣", message: "unexpected code 'a' before enum case"),
       ]
@@ -794,24 +814,39 @@ final class RecoveryTests: XCTestCase {
       }
       """#,
       diagnostics: [
-        DiagnosticSpec(locationMarker: "1️⃣", message: "found an unexpected second identifier in struct"),
+        DiagnosticSpec(locationMarker: "1️⃣", message: "found an unexpected second identifier in struct; is there an accidental break?", fixIts: ["join the identifiers together"]),
         DiagnosticSpec(locationMarker: "2️⃣", message: "expected ':' in type annotation"),
         DiagnosticSpec(locationMarker: "3️⃣", message: #"unexpected code ': Int = ""' before function"#),
-        // TODO: (good first issue) Old parser expected error on line 4: found an unexpected second identifier in variable declaration; is there an accidental break?
         DiagnosticSpec(locationMarker: "4️⃣", message: "expected ':' in type annotation"),
       ]
     )
   }
 
-  func testRecovery64() {
+  func testRecovery64a() {
     assertParse(
       """
       let (efg 1️⃣hij, foobar) = (5, 6)
       """,
       diagnostics: [
-        // TODO: (good first issue) Old parser expected error on line 1: found an unexpected second identifier in constant declaration; is there an accidental break?
-        DiagnosticSpec(message: "unexpected code 'hij, foobar' in tuple pattern")
-      ]
+        DiagnosticSpec(message: "expected ',' in tuple pattern", fixIts: ["insert ','"])
+      ],
+      fixedSource: """
+        let (efg, hij, foobar) = (5, 6)
+        """
+    )
+  }
+
+  func testRecovery64b() {
+    assertParse(
+      """
+      let (efg 1️⃣Hij, foobar) = (5, 6)
+      """,
+      diagnostics: [
+        DiagnosticSpec(message: "expected ':' in tuple pattern", fixIts: ["insert ':'"])
+      ],
+      fixedSource: """
+        let (efg: Hij, foobar) = (5, 6)
+        """
     )
   }