Add missing comma diagnostic if it's inside an array or dictionary

[kimdv]

Fixes #2255

[kimdv]

It also says there is an error in

    assertParse(
      "[() try await(MyError) -> Void]()",
      experimentalFeatures: .typedThrows
    )

Not sure if that is true or not

[ahoppen]

I’m a little worried about the fact that this duplicates the logic of whether we should continue parsing, once for deciding whether to synthesize a comma and then whether to break a loop. We should decide whether to continue parsing another element based on whether we parsed/synthesized a comma.

Something like

Diff

diff --git a/Sources/SwiftParser/Expressions.swift b/Sources/SwiftParser/Expressions.swift
index 0eef0bdd..2b43aebe 100644
--- a/Sources/SwiftParser/Expressions.swift
+++ b/Sources/SwiftParser/Expressions.swift
@@ -1497,33 +1497,36 @@ 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)
 
-        var hasElementKindError: Bool {
+        /// 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 key, _, let colon, let value):
-            return key.hasError || colon.hasError || value.hasError
+          case .dictionary(_, _, _, let value):
+            return value.is(RawMissingExprSyntax.self)
           case .array(let rawExprSyntax):
-            return rawExprSyntax.hasError
+            return rawExprSyntax.is(RawMissingExprSyntax.self)
           }
         }
 
         // Parse the ',' if exists.
-        let comma: Token?
         if let token = self.consume(if: .comma) {
-          comma = token
-        } else if !self.atStartOfLine, !self.at(.rightSquare, .endOfFile), !hasElementKindError {
-          comma = missingToken(.comma)
+          keepGoing = token
+        } else if !self.at(.rightSquare, .endOfFile) && !self.atStartOfLine && !elementIsMissingExpression && !self.atStartOfDeclaration() && !self.atStartOfStatement(preferExpr: false) {
+          keepGoing = missingToken(.comma)
         } else {
-          comma = nil
+          keepGoing = nil
         }
 
         switch elementKind! {
         case .array(let el):
           let element = RawArrayElementSyntax(
             expression: el,
-            trailingComma: comma,
+            trailingComma: keepGoing,
             arena: self.arena
           )
           if element.isEmpty {
@@ -1537,7 +1540,7 @@ extension Parser {
             unexpectedBeforeColon,
             colon: colon,
             value: value,
-            trailingComma: comma,
+            trailingComma: keepGoing,
             arena: self.arena
           )
           if element.isEmpty {
@@ -1546,29 +1549,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)

[ahoppen]

It also says there is an error in

    assertParse(
      "[() try await(MyError) -> Void]()",
      experimentalFeatures: .typedThrows
    )

Not sure if that is true or not

Yes, that expression absolutely has an error.

[kimdv]

I’m a little worried about the fact that this duplicates the logic of whether we should continue parsing, once for deciding whether to synthesize a comma and then whether to break a loop. We should decide whether to continue parsing another element based on whether we parsed/synthesized a comma.

Something like

Diff

Thanks!
I think you can take a look again

[ahoppen]

Cool. LGTM.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows