Merge pull request swiftlang#179 from dylansturg/ugh_commas

Prevent trailing commas from overflowing max line length.

Author: allevato

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),