Prevent trailing commas from overflowing max line length.

The implementation of trailing commas, using a workaround of the length calculation algorithm, resulted in edge cases where the trailing comma could overflow the max line length. This edge case happens for elements that should break internally to create additional space for the trailing comma (e.g. arrays, dictionaries, tuples). For example, an array that contains arrays could have its final element overflow the line length if the last element + correct indentation is exactly `maxLineLength`. After the trailing comma is added, the last array element *should* have broken up onto multiple lines to make space for the comma.

The problem happens because the comma token was being added at the wrong syntax level, outside of the element's group, and its length wasn't propagating. Not propagating the comma's length was intentional to work around the fact that the trailing comma is only present when the comma delimited collection uses multiple lines. Unfortunately, moving the comma deeper into the element so that it can cause elements to break internally means its length has to be propagate as a normal token to also cause breaks at different syntax levels to fire too.

- Array/dictionary elements won't be allowed to overflow the configured line length.

- Arrays/dictionaries that could fit on 1 line without a trailing comma will be forced onto multiple lines with a trailing comma *if* they fit exactly at `maxLineLength` (without a trailing comma).

Author: dylansturg

Sources/SwiftFormatPrettyPrint/PrettyPrint.swift

@@ -651,15 +651,12 @@ public class PrettyPrinter {
         lengths.append(0)
 
       case .commaDelimitedRegionEnd:
-        // The trailing comma needs to be included in the length of the preceding break, but is not
-        // included in the length of the enclosing group. A trailing comma cannot cause the group
-        // to break onto multiple lines, because the comma isn't printed for a single line group.
-        if let index = delimIndexStack.last, case .break = tokens[index] {
-          lengths[index] += 1
-        }
-        // If the closest delimiter token is an open, instead of a break, then adding the comma's
-        // length isn't necessary. In that case, the comma is printed if the preceding break fires.
-
+        // 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)
       }
     }

Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift

@@ -728,28 +728,33 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
   override func visit(_ node: ArrayElementListSyntax) -> SyntaxVisitorContinueKind {
     insertTokens(.break(.same), betweenElementsOf: node)
 
-    // 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, let lastElement = node.last, firstElement != lastElement {
-      before(firstElement.firstToken, tokens: .commaDelimitedRegionStart)
-      let endToken =
-        Token.commaDelimitedRegionEnd(hasTrailingComma: lastElement.trailingComma != nil)
-      after(lastElement.lastToken, tokens: endToken)
-      if let existingTrailingComma = lastElement.trailingComma {
-        ignoredTokens.insert(existingTrailingComma)
+    for element in node {
+      before(element.firstToken, tokens: .open)
+      after(element.lastToken, tokens: .close)
+      if let trailingComma = element.trailingComma {
+        closingDelimiterTokens.insert(trailingComma)
+      }
+    }
+
+    if let lastElement = node.last {
+      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])
       }
     }
     return .visitChildren
   }
 
   override func visit(_ node: ArrayElementSyntax) -> SyntaxVisitorContinueKind {
-    before(node.firstToken, tokens: .open)
-    after(node.lastToken, tokens: .close)
-    if let trailingComma = node.trailingComma {
-      closingDelimiterTokens.insert(trailingComma)
-    }
     return .visitChildren
   }
 
@@ -762,17 +767,28 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
   override func visit(_ node: DictionaryElementListSyntax) -> SyntaxVisitorContinueKind {
     insertTokens(.break(.same), betweenElementsOf: node)
 
-    // 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)
-      if let existingTrailingComma = lastElement.trailingComma {
-        ignoredTokens.insert(existingTrailingComma)
+    for element in node {
+      before(element.firstToken, tokens: .open)
+      after(element.colon, tokens: .break)
+      after(element.lastToken, tokens: .close)
+      if let trailingComma = element.trailingComma {
+        closingDelimiterTokens.insert(trailingComma)
+      }
+    }
+
+    if let lastElement = node.last {
+      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)
       }
     }
     return .visitChildren
@@ -784,12 +800,6 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
   }
 
   override func visit(_ node: DictionaryElementSyntax) -> SyntaxVisitorContinueKind {
-    before(node.firstToken, tokens: .open)
-    after(node.colon, tokens: .break)
-    after(node.lastToken, tokens: .close)
-    if let trailingComma = node.trailingComma {
-      closingDelimiterTokens.insert(trailingComma)
-    }
     return .visitChildren
   }
 

Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift

@@ -7,7 +7,6 @@ final class ArrayDeclTests: PrettyPrintTestCase {
       """
       let a = [1, 2, 3,]
       let a: [Bool] = [false, true, true, false]
-      let a = [11111111, 2222222, 33333333, 444444]
       let a = [11111111, 2222222, 33333333, 4444444]
       let a: [String] = ["One", "Two", "Three", "Four"]
       let a: [String] = ["One", "Two", "Three", "Four", "Five", "Six", "Seven"]
@@ -16,13 +15,13 @@ final class ArrayDeclTests: PrettyPrintTestCase {
         "One", "Two", "Three", "Four", "Five",
         "Six", "Seven", "Eight",
       ]
+      let a = [11111111, 2222222, 33333333, 444444]
       """
 
     let expected =
       """
       let a = [1, 2, 3]
       let a: [Bool] = [false, true, true, false]
-      let a = [11111111, 2222222, 33333333, 444444]
       let a = [
         11111111, 2222222, 33333333, 4444444,
       ]
@@ -43,6 +42,15 @@ final class ArrayDeclTests: PrettyPrintTestCase {
       ]
 
       """
+      // Ideally, this array would be left on 1 line without a trailing comma. We don't know if the
+      // comma is required when calculating the length of array elements, so the comma's length is
+      // always added to last element and that 1 character causes the newlines inside of the array.
+      + """
+      let a = [
+        11111111, 2222222, 33333333, 444444,
+      ]
+
+      """
 
     assertPrettyPrintEqual(input: input, expected: expected, linelength: 45)
   }
@@ -134,4 +142,59 @@ final class ArrayDeclTests: PrettyPrintTestCase {
 
     assertPrettyPrintEqual(input: input, expected: expected, linelength: 32)
   }
+
+  func testInnerElementBreakingFromComma() {
+    let input =
+      """
+      let a = [("abc", "def", "xyz"),("this ", "string", "is long"),]
+      let a = [("abc", "def", "xyz"),("this ", "string", "is long")]
+      let a = [("this ", "string", "is long"),]
+      let a = [("this ", "string", "is long")]
+      let a = ["this ", "string", "is longer",]
+      let a = [("this", "str"), ("is", "lng")]
+      a = [("az", "by"), ("cf", "de")]
+      """
+
+    let expected =
+      """
+      let a = [
+        ("abc", "def", "xyz"),
+        (
+          "this ", "string", "is long"
+        ),
+      ]
+      let a = [
+        ("abc", "def", "xyz"),
+        (
+          "this ", "string", "is long"
+        ),
+      ]
+      let a = [
+        ("this ", "string", "is long")
+      ]
+      let a = [
+        ("this ", "string", "is long")
+      ]
+      let a = [
+        "this ", "string",
+        "is longer",
+      ]
+      let a = [
+        ("this", "str"),
+        ("is", "lng"),
+      ]
+
+      """
+      // Ideally, this array would be left on 1 line without a trailing comma. We don't know if the
+      // comma is required when calculating the length of array elements, so the comma's length is
+      // always added to last element and that 1 character causes the newlines inside of the array.
+      + """
+      a = [
+        ("az", "by"), ("cf", "de"),
+      ]
+
+      """
+
+    assertPrettyPrintEqual(input: input, expected: expected, linelength: 32)
+  }
 }

Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift

@@ -7,7 +7,6 @@ final class DictionaryDeclTests: PrettyPrintTestCase {
       """
       let a = [1: "a", 2: "b", 3: "c",]
       let a: [Int: String] = [1: "a", 2: "b", 3: "c"]
-      let a = [10000: "abc", 20000: "def", 30000: "ghi"]
       let a = [10000: "abc", 20000: "def", 30000: "ghij"]
       let a: [Int: String] = [1: "a", 2: "b", 3: "c", 4: "d"]
       let a: [Int: String] = [1: "a", 2: "b", 3: "c", 4: "d", 5: "e", 6: "f", 7: "g"]
@@ -16,13 +15,13 @@ final class DictionaryDeclTests: PrettyPrintTestCase {
         1: "a", 2: "b", 3: "c", 4: "d", 5: "e", 6: "f",
         7: "g", 8: "i",
       ]
+      let a = [10000: "abc", 20000: "def", 30000: "ghi"]
       """
 
     let expected =
       """
       let a = [1: "a", 2: "b", 3: "c"]
       let a: [Int: String] = [1: "a", 2: "b", 3: "c"]
-      let a = [10000: "abc", 20000: "def", 30000: "ghi"]
       let a = [
         10000: "abc", 20000: "def", 30000: "ghij",
       ]
@@ -43,6 +42,16 @@ final class DictionaryDeclTests: PrettyPrintTestCase {
       ]
 
       """
+      // Ideally, this dictionary would be left on 1 line without a trailing comma. We don't know if
+      // the comma is required when calculating the length of elements, so the comma's length is
+      // always added to last element and that 1 character causes the newlines inside of the
+      // dictionary.
+      + """
+      let a = [
+        10000: "abc", 20000: "def", 30000: "ghi",
+      ]
+
+      """
 
     assertPrettyPrintEqual(input: input, expected: expected, linelength: 50)
   }
@@ -175,4 +184,65 @@ final class DictionaryDeclTests: PrettyPrintTestCase {
 
     assertPrettyPrintEqual(input: input, expected: expected, linelength: 32)
   }
+
+  func testInnerElementBreakingFromComma() {
+    let input =
+      """
+      let a = [key1: ("abc", "def", "xyz"),key2: ("this ", "string", "is long"),]
+      let a = [key1: ("abc", "def", "xyz"),key2: ("this ", "string", "is long")]
+      let a = [key2: ("this ", "string", "is long")]
+      let a = [key2: ("this ", "string", "is long"),]
+      let a = [key2: ("this ", "string", "is long ")]
+      let a = [key1: ("a", "z"), key2: ("b ", "y")]
+      let a = [key1: ("ab", "z"), key2: ("b ", "y")]
+      a = [k1: ("ab", "z"), k2: ("bc", "y")]
+      """
+
+    let expected =
+      """
+      let a = [
+        key1: ("abc", "def", "xyz"),
+        key2: (
+          "this ", "string", "is long"
+        ),
+      ]
+      let a = [
+        key1: ("abc", "def", "xyz"),
+        key2: (
+          "this ", "string", "is long"
+        ),
+      ]
+      let a = [
+        key2: ("this ", "string", "is long")
+      ]
+      let a = [
+        key2: ("this ", "string", "is long")
+      ]
+      let a = [
+        key2: (
+          "this ", "string", "is long "
+        )
+      ]
+      let a = [
+        key1: ("a", "z"), key2: ("b ", "y"),
+      ]
+      let a = [
+        key1: ("ab", "z"),
+        key2: ("b ", "y"),
+      ]
+
+      """
+      // Ideally, this dictionary would be left on 1 line without a trailing comma. We don't know if
+      // the comma is required when calculating the length of elements, so the comma's length is
+      // always added to last element and that 1 character causes the newlines inside of the
+      // dictionary.
+      + """
+      a = [
+        k1: ("ab", "z"), k2: ("bc", "y"),
+      ]
+
+      """
+
+    assertPrettyPrintEqual(input: input, expected: expected, linelength: 38)
+  }
 }

Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift

@@ -25,6 +25,7 @@ extension ArrayDeclTests {
         ("testArrayOfFunctions", testArrayOfFunctions),
         ("testBasicArrays", testBasicArrays),
         ("testGroupsTrailingComma", testGroupsTrailingComma),
+        ("testInnerElementBreakingFromComma", testInnerElementBreakingFromComma),
         ("testNoTrailingCommasInTypes", testNoTrailingCommasInTypes),
         ("testTrailingCommaDiagnostics", testTrailingCommaDiagnostics),
         ("testWhitespaceOnlyDoesNotChangeTrailingComma", testWhitespaceOnlyDoesNotChangeTrailingComma),
@@ -196,6 +197,7 @@ extension DictionaryDeclTests {
         ("testBasicDictionaries", testBasicDictionaries),
         ("testDiscretionaryNewlineAfterColon", testDiscretionaryNewlineAfterColon),
         ("testGroupsTrailingComma", testGroupsTrailingComma),
+        ("testInnerElementBreakingFromComma", testInnerElementBreakingFromComma),
         ("testNoTrailingCommasInTypes", testNoTrailingCommasInTypes),
         ("testTrailingCommaDiagnostics", testTrailingCommaDiagnostics),
         ("testWhitespaceOnlyDoesNotChangeTrailingComma", testWhitespaceOnlyDoesNotChangeTrailingComma),