Repository: swiftlang/swift-syntax (swift)

Issue:
SwiftParser accepts array literals where elements aren’t separated by commas
### Issue Kind

Other

### Source Code

```swift
[() ()]
```

### Description

This is parsed correctly but should emit an error that a comma is missing between the array elements.

Pull request:
Fixes #2255

Patch:
diff --git a/Sources/SwiftParser/Expressions.swift b/Sources/SwiftParser/Expressions.swift
index caece56ebdb..25d9c7f0d45 100644
--- a/Sources/SwiftParser/Expressions.swift
+++ b/Sources/SwiftParser/Expressions.swift
@@ -1497,17 +1497,38 @@ extension Parser {
     var elements = [RawSyntax]()
     do {
       var collectionProgress = LoopProgressCondition()
-      COLLECTION_LOOP: while self.hasProgressed(&collectionProgress) {
+      var keepGoing: RawTokenSyntax?
+      COLLECTION_LOOP: repeat {
         elementKind = self.parseCollectionElement(elementKind)
 
+        /// Whether expression of an array element or the value of a dictionary
+        /// element is missing. If this is the case, we shouldn't recover from
+        /// a missing comma since most likely the closing `]` is missing.
+        var elementIsMissingExpression: Bool {
+          switch elementKind! {
+          case .dictionary(_, _, _, let value):
+            return value.is(RawMissingExprSyntax.self)
+          case .array(let rawExprSyntax):
+            return rawExprSyntax.is(RawMissingExprSyntax.self)
+          }
+        }
+
         // Parse the ',' if exists.
-        let comma = self.consume(if: .comma)
+        if let token = self.consume(if: .comma) {
+          keepGoing = token
+        } else if !self.at(.rightSquare, .endOfFile) && !self.atStartOfLine && !elementIsMissingExpression && !self.atStartOfDeclaration()
+          && !self.atStartOfStatement(preferExpr: false)
+        {
+          keepGoing = missingToken(.comma)
+        } else {
+          keepGoing = nil
+        }
 
         switch elementKind! {
         case .array(let el):
           let element = RawArrayElementSyntax(
             expression: el,
-            trailingComma: comma,
+            trailingComma: keepGoing,
             arena: self.arena
           )
           if element.isEmpty {
@@ -1521,7 +1542,7 @@ extension Parser {
             unexpectedBeforeColon,
             colon: colon,
             value: value,
-            trailingComma: comma,
+            trailingComma: keepGoing,
             arena: self.arena
           )
           if element.isEmpty {
@@ -1530,29 +1551,7 @@ extension Parser {
             elements.append(RawSyntax(element))
           }
         }
-
-        // If we saw a comma, that's a strong indicator we have more elements
-        // to process. If that's not the case, we have to do some legwork to
-        // determine if we should bail out.
-        guard comma == nil || self.at(.rightSquare, .endOfFile) else {
-          continue
-        }
-
-        // If we found EOF or the closing square bracket, bailout.
-        if self.at(.rightSquare, .endOfFile) {
-          break
-        }
-
-        // If the next token is at the beginning of a new line and can never start
-        // an element, break.
-        if self.atStartOfLine
-          && (self.at(.rightBrace, .poundEndif)
-            || self.atStartOfDeclaration()
-            || self.atStartOfStatement(preferExpr: false))
-        {
-          break
-        }
-      }
+      } while keepGoing != nil && self.hasProgressed(&collectionProgress)
     }
 
     let (unexpectedBeforeRSquare, rsquare) = self.expect(.rightSquare)


Test patch:
diff --git a/Tests/SwiftParserTest/ExpressionTests.swift b/Tests/SwiftParserTest/ExpressionTests.swift
index 6f400ea68a0..fab76ddd87d 100644
--- a/Tests/SwiftParserTest/ExpressionTests.swift
+++ b/Tests/SwiftParserTest/ExpressionTests.swift
@@ -2762,11 +2762,15 @@ final class StatementExpressionTests: ParserTestCase {
       experimentalFeatures: .typedThrows
     )
     assertParse(
-      "[() try(MyError) async -> Void]()",
+      "[() 1️⃣try(MyError) async -> Void]()",
+      diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
+      fixedSource: "[(), try(MyError) async -> Void]()",
       experimentalFeatures: .typedThrows
     )
     assertParse(
-      "[() try async(MyError) -> Void]()",
+      "[() 1️⃣try async(MyError) -> Void]()",
+      diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
+      fixedSource: "[(), try async(MyError) -> Void]()",
       experimentalFeatures: .typedThrows
     )
     assertParse(
@@ -2787,30 +2791,49 @@ final class StatementExpressionTests: ParserTestCase {
       experimentalFeatures: .typedThrows
     )
     assertParse(
-      "[() try(MyError) await 1️⃣-> Void]()",
+      "[() 1️⃣try(MyError) 2️⃣await 3️⃣-> Void]()",
       diagnostics: [
         DiagnosticSpec(
+          locationMarker: "1️⃣",
+          message: "expected ',' in array element",
+          fixIts: ["insert ','"]
+        ),
+        DiagnosticSpec(
+          locationMarker: "2️⃣",
+          message: "expected ',' in array element",
+          fixIts: ["insert ','"]
+        ),
+        DiagnosticSpec(
+          locationMarker: "3️⃣",
           message: "expected expression in 'await' expression",
           fixIts: ["insert expression"]
-        )
+        ),
       ],
-      fixedSource: "[() try(MyError) await <#expression#> -> Void]()",
+      fixedSource: "[(), try(MyError), await <#expression#> -> Void]()",
       experimentalFeatures: .typedThrows
     )
     assertParse(
-      "[() try await(MyError) -> Void]()",
+      "[() 1️⃣try await(MyError) -> Void]()",
+      diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
+      fixedSource: "[(), try await(MyError) -> Void]()",
       experimentalFeatures: .typedThrows
     )
     assertParse(
-      "[() async(MyError) -> Void]()",
+      "[() 1️⃣async(MyError) -> Void]()",
+      diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
+      fixedSource: "[(), async(MyError) -> Void]()",
       experimentalFeatures: .typedThrows
     )
     assertParse(
-      "[() await(MyError) -> Void]()",
+      "[() 1️⃣await(MyError) -> Void]()",
+      diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
+      fixedSource: "[(), await(MyError) -> Void]()",
       experimentalFeatures: .typedThrows
     )
     assertParse(
-      "[() try(MyError) -> Void]()",
+      "[() 1️⃣try(MyError) -> Void]()",
+      diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
+      fixedSource: "[(), try(MyError) -> Void]()",
       experimentalFeatures: .typedThrows
     )
     assertParse(
@@ -2826,4 +2849,65 @@ final class StatementExpressionTests: ParserTestCase {
       experimentalFeatures: .typedThrows
     )
   }
+
+  func testArrayExprWithNoCommas() {
+    assertParse("[() ()]")
+
+    assertParse(
+      "[1 1️⃣2]",
+      diagnostics: [
+        DiagnosticSpec(
+          message: "expected ',' in array element",
+          fixIts: ["insert ','"]
+        )
+      ],
+      fixedSource: "[1, 2]"
+    )
+
+    assertParse(
+      #"["hello" 1️⃣"world"]"#,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "expected ',' in array element",
+          fixIts: ["insert ','"]
+        )
+      ],
+      fixedSource: #"["hello", "world"]"#
+    )
+  }
+
+  func testDictionaryExprWithNoCommas() {
+    assertParse(
+      "[1: () 1️⃣2: ()]",
+      diagnostics: [
+        DiagnosticSpec(
+          message: "expected ',' in dictionary element",
+          fixIts: ["insert ','"]
+        )
+      ],
+      fixedSource: #"[1: (), 2: ()]"#
+    )
+
+    assertParse(
+      #"["foo": 1 1️⃣"bar": 2]"#,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "expected ',' in dictionary element",
+          fixIts: ["insert ','"]
+        )
+      ],
+      fixedSource: #"["foo": 1, "bar": 2]"#
+    )
+
+    assertParse(
+      #"[1: "hello" 1️⃣2: "world"]"#,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "expected ',' in dictionary element",
+          fixIts: ["insert ','"]
+        )
+      ],
+      fixedSource: #"[1: "hello", 2: "world"]"#
+    )
+  }
 }
diff --git a/Tests/SwiftParserTest/translated/ObjectLiteralsTests.swift b/Tests/SwiftParserTest/translated/ObjectLiteralsTests.swift
index 9e7a084aaa0..21db15f792a 100644
--- a/Tests/SwiftParserTest/translated/ObjectLiteralsTests.swift
+++ b/Tests/SwiftParserTest/translated/ObjectLiteralsTests.swift
@@ -18,13 +18,14 @@ final class ObjectLiteralsTests: ParserTestCase {
   func testObjectLiterals1a() {
     assertParse(
       """
-      let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha)#1️⃣]
+      let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha)1️⃣#2️⃣]
       """,
       diagnostics: [
-        DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
       ],
       fixedSource: """
-        let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha)#<#identifier#>]
+        let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha), #<#identifier#>]
         """
     )
   }
@@ -32,13 +33,14 @@ final class ObjectLiteralsTests: ParserTestCase {
   func testObjectLiterals1b() {
     assertParse(
       """
-      let _ = [#Image(imageLiteral: localResourceNameAsString)#1️⃣]
+      let _ = [#Image(imageLiteral: localResourceNameAsString)1️⃣#2️⃣]
       """,
       diagnostics: [
-        DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
       ],
       fixedSource: """
-        let _ = [#Image(imageLiteral: localResourceNameAsString)#<#identifier#>]
+        let _ = [#Image(imageLiteral: localResourceNameAsString), #<#identifier#>]
         """
     )
   }
@@ -46,13 +48,14 @@ final class ObjectLiteralsTests: ParserTestCase {
   func testObjectLiterals1c() {
     assertParse(
       """
-      let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString)#1️⃣]
+      let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString)1️⃣#2️⃣]
       """,
       diagnostics: [
-        DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
       ],
       fixedSource: """
-        let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString)#<#identifier#>]
+        let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString), #<#identifier#>]
         """
     )
   }
@@ -112,10 +115,11 @@ final class ObjectLiteralsTests: ParserTestCase {
       """,
       diagnostics: [
         DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
         DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
       ],
       fixedSource: """
-        let _ = [#<#identifier#> #<#identifier#>]
+        let _ = [#<#identifier#>, #<#identifier#>]
         """
     )
   }
@@ -123,11 +127,10 @@ final class ObjectLiteralsTests: ParserTestCase {
   func testObjectLiterals5() {
     assertParse(
       """
-      let _ = ℹ️[#Color(_: 1, green: 1, 2)2️⃣
+      let _ = ℹ️[#Color(_: 1, green: 1, 2)1️⃣
       """,
       diagnostics: [
         DiagnosticSpec(
-          locationMarker: "2️⃣",
           message: "expected ']' to end array",
           notes: [NoteSpec(message: "to match this opening '['")],
           fixIts: ["insert ']'"]
@@ -142,23 +145,28 @@ final class ObjectLiteralsTests: ParserTestCase {
   func testObjectLiterals6() {
     assertParse(
       """
-      let _ = ℹ️[1️⃣#Color(red: 1, green: 1, blue: 1)#2️⃣3️⃣
+      let _ = ℹ️[#Color(red: 1, green: 1, blue: 1)1️⃣#2️⃣
       """,
       diagnostics: [
+        DiagnosticSpec(
+          locationMarker: "1️⃣",
+          message: "expected ',' in array element",
+          fixIts: ["insert ','"]
+        ),
         DiagnosticSpec(
           locationMarker: "2️⃣",
           message: "expected identifier in macro expansion",
           fixIts: ["insert identifier"]
         ),
         DiagnosticSpec(
-          locationMarker: "3️⃣",
+          locationMarker: "2️⃣",
           message: "expected ']' to end array",
           notes: [NoteSpec(message: "to match this opening '['")],
           fixIts: ["insert ']'"]
         ),
       ],
       fixedSource: """
-        let _ = [#Color(red: 1, green: 1, blue: 1)#<#identifier#>]
+        let _ = [#Color(red: 1, green: 1, blue: 1), #<#identifier#>]
         """
     )
   }
@@ -166,13 +174,14 @@ final class ObjectLiteralsTests: ParserTestCase {
   func testObjectLiterals7() {
     assertParse(
       """
-      let _ = [#Color(withRed: 1, green: 1, whatever: 2)#1️⃣]
+      let _ = [#Color(withRed: 1, green: 1, whatever: 2)1️⃣#2️⃣]
       """,
       diagnostics: [
-        DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
+        DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
+        DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
       ],
       fixedSource: """
-        let _ = [#Color(withRed: 1, green: 1, whatever: 2)#<#identifier#>]
+        let _ = [#Color(withRed: 1, green: 1, whatever: 2), #<#identifier#>]
         """
     )
   }
diff --git a/Tests/SwiftParserTest/translated/RecoveryTests.swift b/Tests/SwiftParserTest/translated/RecoveryTests.swift
index 752b6a7679c..1f2f9428685 100644
--- a/Tests/SwiftParserTest/translated/RecoveryTests.swift
+++ b/Tests/SwiftParserTest/translated/RecoveryTests.swift
@@ -2433,6 +2433,11 @@ final class RecoveryTests: ParserTestCase {
           locationMarker: "4️⃣",
           message: "unexpected code ') -> Int {}' in closure"
         ),
+        DiagnosticSpec(
+          locationMarker: "5️⃣",
+          message: "expected ',' in array element",
+          fixIts: ["insert ','"]
+        ),
         DiagnosticSpec(
           locationMarker: "5️⃣",
           message: "expected ']' to end array",
@@ -2449,7 +2454,7 @@ final class RecoveryTests: ParserTestCase {
         struct Foo19605164 {
         func a(s: S)
         }[{{g) -> Int {}
-        }}]}
+        }},]}
         #endif
         """
     )