Merge pull request swiftlang#228 from dylansturg/commas_crash

Handle existing commas in single element collections.

Author: allevato

Sources/SwiftFormatPrettyPrint/PrettyPrint.swift

@@ -541,12 +541,18 @@ public class PrettyPrinter {
     case .commaDelimitedRegionStart:
       commaDelimitedRegionStack.append(openCloseBreakCompensatingLineNumber)
 
-    case .commaDelimitedRegionEnd(let hasTrailingComma):
+    case .commaDelimitedRegionEnd(let hasTrailingComma, let isSingleElement):
       guard let startLineNumber = commaDelimitedRegionStack.popLast() else {
         fatalError("Found trailing comma end with no corresponding start.")
       }
 
-      let shouldHaveTrailingComma = startLineNumber != openCloseBreakCompensatingLineNumber
+      // We need to specifically disable trailing commas on elements of single item collections.
+      // The syntax library can't distinguish a collection's initializer (where the elements are
+      // types) from a literal (where the elements are the contents of a collection instance).
+      // We never want to add a trailing comma in an initializer so we disable trailing commas on
+      // single element collections.
+      let shouldHaveTrailingComma =
+        startLineNumber != openCloseBreakCompensatingLineNumber && !isSingleElement
       if shouldHaveTrailingComma && !hasTrailingComma {
         diagnose(.addTrailingComma)
       } else if !shouldHaveTrailingComma && hasTrailingComma {
@@ -653,14 +659,15 @@ public class PrettyPrinter {
       case .commaDelimitedRegionStart:
         lengths.append(0)
 
-      case .commaDelimitedRegionEnd:
+      case .commaDelimitedRegionEnd(_, let isSingleElement):
         // The token's length is only necessary when a comma will be printed, but it's impossible to
         // know at this point whether the region-start token will be on the same line as this token.
         // Without adding this length to the total, it would be possible for this comma to be
         // printed in column `maxLineLength`. Unfortunately, this can cause breaks to fire
         // unnecessarily when the enclosed tokens comma would fit within `maxLineLength`.
-        total += 1
-        lengths.append(1)
+        let length = isSingleElement ? 0 : 1
+        total += length
+        lengths.append(length)
       }
     }
 

Sources/SwiftFormatPrettyPrint/Token.swift

@@ -185,8 +185,8 @@ enum Token {
   case commaDelimitedRegionStart
 
   /// Marks the end of a comma delimited collection, where a trailing comma should be inserted
-  /// if and only if the collection spans multiple lines.
-  case commaDelimitedRegionEnd(hasTrailingComma: Bool)
+  /// if and only if the collection spans multiple lines and has multiple elements.
+  case commaDelimitedRegionEnd(hasTrailingComma: Bool, isSingleElement: Bool)
 
   /// Starts a scope where `contextual` breaks have consistent behavior.
   case contextualBreakingStart

Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift

@@ -795,16 +795,12 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
       if let trailingComma = lastElement.trailingComma {
         ignoredTokens.insert(trailingComma)
       }
-      // The syntax library can't distinguish an array initializer (where the elements are types)
-      // from an array literal (where the elements are the contents of the array). We never want to
-      // add a trailing comma in the initializer case and there's always exactly 1 element, so we
-      // never add a trailing comma to 1 element arrays.
-      if let firstElement = node.first, firstElement != lastElement {
-        before(firstElement.firstToken, tokens: .commaDelimitedRegionStart)
-        let endToken =
-          Token.commaDelimitedRegionEnd(hasTrailingComma: lastElement.trailingComma != nil)
-        after(lastElement.expression.lastToken, tokens: [endToken])
-      }
+      before(node.first?.firstToken, tokens: .commaDelimitedRegionStart)
+      let endToken =
+        Token.commaDelimitedRegionEnd(
+          hasTrailingComma: lastElement.trailingComma != nil,
+          isSingleElement: node.first == lastElement)
+      after(lastElement.expression.lastToken, tokens: [endToken])
     }
     return .visitChildren
   }
@@ -842,16 +838,12 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
       if let trailingComma = lastElement.trailingComma {
         ignoredTokens.insert(trailingComma)
       }
-      // The syntax library can't distinguish a dictionary initializer (where the elements are
-      // types) from a dictionary literal (where the elements are the contents of the dictionary).
-      // We never want to add a trailing comma in the initializer case and there's always exactly 1
-      //  element, so we never add a trailing comma to 1 element dictionaries.
-      if let firstElement = node.first, let lastElement = node.last, firstElement != lastElement {
-        before(firstElement.firstToken, tokens: .commaDelimitedRegionStart)
-        let endToken =
-          Token.commaDelimitedRegionEnd(hasTrailingComma: lastElement.trailingComma != nil)
-        after(lastElement.lastToken, tokens: endToken)
-      }
+      before(node.first?.firstToken, tokens: .commaDelimitedRegionStart)
+      let endToken =
+        Token.commaDelimitedRegionEnd(
+          hasTrailingComma: lastElement.trailingComma != nil,
+          isSingleElement: node.first == node.last)
+      after(lastElement.lastToken, tokens: endToken)
     }
     return .visitChildren
   }

Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift

@@ -103,6 +103,9 @@ final class ArrayDeclTests: PrettyPrintTestCase {
   func testWhitespaceOnlyDoesNotChangeTrailingComma() {
     let input =
       """
+      let a = [
+        "String",
+      ]
       let a = [1, 2, 3,]
       let a: [String] = [
         "One", "Two", "Three", "Four", "Five",

Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift

@@ -92,6 +92,9 @@ final class DictionaryDeclTests: PrettyPrintTestCase {
   func testWhitespaceOnlyDoesNotChangeTrailingComma() {
     let input =
       """
+      let a = [
+        1: "a",
+      ]
       let a = [1: "a", 2: "b", 3: "c",]
       let a: [Int: String] = [
         1: "a", 2: "b", 3: "c", 4: "d", 5: "e", 6: "f",