Diagnostics for trailing comma changes.

As part of the migration from a phase 1 rule for trailing commas to using the PrettyPrinter, we still need to emit diagnostics when the source's use of trailing commas is incorrect. These diagnostics are similar to those from the original phase 1 rule, except they don't specify the type of collection (instead saying "collection literal"). I think context of the line and column number + the general term "collection" are sufficient for the user to understand the intent.

I added support to PrettyPrintTestCase for collecting diagnostics so that they can be tested.

Finally, the WhitespaceLinter is only able to handle changes in whitespace. By adding/removing trailing commas, the whitespace linter would fail. Instead of checking the whitespace linter to support skipping comma changes, I've created a "mode" in the PrettyPrinter to print the text of the source exactly as it was while only changing whitespace. If we ever introduce other types of text edits in the PrettyPrinter, this should work for that too.

Author: dylansturg

Sources/SwiftFormat/SwiftFormatter.swift

@@ -109,7 +109,8 @@ public final class SwiftFormatter {
       context: context,
       operatorContext: operatorContext,
       node: transformedSyntax,
-      printTokenStream: debugOptions.contains(.dumpTokenStream))
+      printTokenStream: debugOptions.contains(.dumpTokenStream),
+      whitespaceOnly: false)
     outputStream.write(printer.prettyPrint())
   }
 }

Sources/SwiftFormat/SwiftLinter.swift

@@ -92,7 +92,8 @@ public final class SwiftLinter {
       context: context,
       operatorContext: operatorContext,
       node: syntax,
-      printTokenStream: debugOptions.contains(.dumpTokenStream))
+      printTokenStream: debugOptions.contains(.dumpTokenStream),
+      whitespaceOnly: true)
     let formatted = printer.prettyPrint()
     let ws = WhitespaceLinter(user: syntax.description, formatted: formatted, context: context)
     ws.lint()

Sources/SwiftFormatPrettyPrint/PrettyPrint.swift

@@ -66,6 +66,11 @@ public class PrettyPrinter {
   /// If true, the token stream is printed to the console for debugging purposes.
   private var printTokenStream: Bool
 
+  /// Whether the pretty printer should restrict its changes to whitespace. When true, only
+  /// whitespace (e.g. spaces, newlines) are modified. Otherwise, text changes (e.g. add/remove
+  /// trailing commas) are performed in addition to whitespace.
+  private let whitespaceOnly: Bool
+
   /// Keeps track of the line numbers and indentation states of the open (and unclosed) breaks seen
   /// so far.
   private var activeOpenBreaks: [ActiveOpenBreak] = []
@@ -139,8 +144,10 @@ public class PrettyPrinter {
   ///   - node: The node to be pretty printed.
   ///   - printTokenStream: Indicates whether debug information about the token stream should be
   ///     printed to standard output.
+  ///   - whitespaceOnly: Whether only whitespace changes should be made.
   public init(
-    context: Context, operatorContext: OperatorContext, node: Syntax, printTokenStream: Bool
+    context: Context, operatorContext: OperatorContext, node: Syntax, printTokenStream: Bool,
+    whitespaceOnly: Bool
   ) {
     self.context = context
     let configuration = context.configuration
@@ -149,6 +156,7 @@ public class PrettyPrinter {
     self.maxLineLength = configuration.lineLength
     self.spaceRemaining = self.maxLineLength
     self.printTokenStream = printTokenStream
+    self.whitespaceOnly = whitespaceOnly
   }
 
   /// Append the given string to the output buffer.
@@ -451,12 +459,20 @@ public class PrettyPrinter {
     case .commaDelimitedRegionStart:
       commaDelimitedRegionStack.append(openCloseBreakCompensatingLineNumber)
 
-    case .commaDelimitedRegionEnd:
+    case .commaDelimitedRegionEnd(let hasTrailingComma, let position):
       guard let startLineNumber = commaDelimitedRegionStack.popLast() else {
         fatalError("Found trailing comma end with no corresponding start.")
       }
 
-      if startLineNumber != openCloseBreakCompensatingLineNumber {
+      let shouldHaveTrailingComma = startLineNumber != openCloseBreakCompensatingLineNumber
+      if shouldHaveTrailingComma && !hasTrailingComma {
+        diagnose(.addTrailingComma, at: position)
+      } else if !shouldHaveTrailingComma && hasTrailingComma {
+        diagnose(.removeTrailingComma, at: position)
+      }
+
+      let shouldWriteComma = whitespaceOnly ? hasTrailingComma : shouldHaveTrailingComma
+      if shouldWriteComma {
         write(",")
       }
     }
@@ -679,4 +695,10 @@ extension Diagnostic.Message {
 
   static let moveEndOfLineComment = Diagnostic.Message(
     .warning, "move end-of-line comment that exceeds the line length")
+
+  static let addTrailingComma = Diagnostic.Message(
+    .warning, "add trailing comma to the last element in multiline collection literal")
+
+  static let removeTrailingComma = Diagnostic.Message(
+    .warning, "remove trailing comma from the last element in single line collection literal")
 }

Sources/SwiftFormatPrettyPrint/Token.swift

@@ -177,7 +177,7 @@ enum Token {
 
   /// 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
+  case commaDelimitedRegionEnd(hasTrailingComma: Bool, position: AbsolutePosition)
 
   // Convenience overloads for the enum types
   static let open = Token.open(.inconsistent, 0)

Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift

@@ -709,7 +709,9 @@ private final class TokenStreamCreator: SyntaxVisitor {
     insertTokens(.break(.same), betweenElementsOf: node)
     if let firstElement = node.first, let lastElement = node.last {
       before(firstElement.firstToken, tokens: .commaDelimitedRegionStart)
-      after(lastElement.lastToken, tokens: .commaDelimitedRegionEnd)
+      let endToken = Token.commaDelimitedRegionEnd(
+        hasTrailingComma: lastElement.trailingComma != nil, position: lastElement.endPosition)
+      after(lastElement.lastToken, tokens: endToken)
       if let existingTrailingComma = lastElement.trailingComma {
         ignoredTokens.insert(existingTrailingComma)
       }
@@ -733,7 +735,9 @@ private final class TokenStreamCreator: SyntaxVisitor {
     insertTokens(.break(.same), betweenElementsOf: node)
     if let firstElement = node.first, let lastElement = node.last {
       before(firstElement.firstToken, tokens: .commaDelimitedRegionStart)
-      after(lastElement.lastToken, tokens: .commaDelimitedRegionEnd)
+      let endToken = Token.commaDelimitedRegionEnd(
+        hasTrailingComma: lastElement.trailingComma != nil, position: lastElement.endPosition)
+      after(lastElement.lastToken, tokens: endToken)
       if let existingTrailingComma = lastElement.trailingComma {
         ignoredTokens.insert(existingTrailingComma)
       }

Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift

@@ -1,3 +1,7 @@
+import SwiftSyntax
+
+@testable import SwiftFormatPrettyPrint
+
 public class ArrayDeclTests: PrettyPrintTestCase {
   public func testBasicArrays() {
     let input =
@@ -60,4 +64,35 @@ public class ArrayDeclTests: PrettyPrintTestCase {
 
     assertPrettyPrintEqual(input: input, expected: expected, linelength: 45)
   }
+
+  public func testWhitespaceOnlyDoesNotChangeTrailingComma() {
+    let input =
+      """
+      let a = [1, 2, 3,]
+      let a: [String] = [
+        "One", "Two", "Three", "Four", "Five",
+        "Six", "Seven", "Eight"
+      ]
+      """
+
+    assertPrettyPrintEqual(
+      input: input, expected: input + "\n", linelength: 45, whitespaceOnly: true)
+  }
+
+  public func testTrailingCommaDiagnostics() {
+    let input =
+      """
+      let a = [1, 2, 3,]
+      let a: [String] = [
+        "One", "Two", "Three", "Four", "Five",
+        "Six", "Seven", "Eight"
+      ]
+      """
+
+    assertPrettyPrintEqual(
+      input: input, expected: input + "\n", linelength: 45, whitespaceOnly: true)
+
+    XCTAssertDiagnosed(Diagnostic.Message.removeTrailingComma, line: 1, column: 18)
+    XCTAssertDiagnosed(Diagnostic.Message.addTrailingComma, line: 4, column: 26)
+  }
 }

Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift

@@ -1,3 +1,7 @@
+import SwiftSyntax
+
+@testable import SwiftFormatPrettyPrint
+
 public class DictionaryDeclTests: PrettyPrintTestCase {
   public func testBasicDictionaries() {
     let input =
@@ -43,4 +47,35 @@ public class DictionaryDeclTests: PrettyPrintTestCase {
 
     assertPrettyPrintEqual(input: input, expected: expected, linelength: 50)
   }
+
+  public func testWhitespaceOnlyDoesNotChangeTrailingComma() {
+    let input =
+      """
+      let a = [1: "a", 2: "b", 3: "c",]
+      let a: [Int: String] = [
+        1: "a", 2: "b", 3: "c", 4: "d", 5: "e", 6: "f",
+        7: "g", 8: "i"
+      ]
+      """
+
+    assertPrettyPrintEqual(
+      input: input, expected: input + "\n", linelength: 50, whitespaceOnly: true)
+  }
+
+  public func testTrailingCommaDiagnostics() {
+    let input =
+      """
+      let a = [1: "a", 2: "b", 3: "c",]
+      let a: [Int: String] = [
+        1: "a", 2: "b", 3: "c", 4: "d", 5: "e", 6: "f",
+        7: "g", 8: "i"
+      ]
+      """
+
+    assertPrettyPrintEqual(
+      input: input, expected: input + "\n", linelength: 50, whitespaceOnly: true)
+
+    XCTAssertDiagnosed(Diagnostic.Message.removeTrailingComma, line: 1, column: 33)
+    XCTAssertDiagnosed(Diagnostic.Message.addTrailingComma, line: 4, column: 17)
+  }
 }

Tests/SwiftFormatPrettyPrintTests/PrettyPrintTestCase.swift

@@ -6,37 +6,90 @@ import XCTest
 @testable import SwiftFormatPrettyPrint
 
 public class PrettyPrintTestCase: XCTestCase {
+  /// A helper that will keep track of which diagnostics have been emitted, and their locations.
+  private var consumer: DiagnosticTrackingConsumer? = nil
+
+  /// Implements `DiagnosticConsumer` to keep track which diagnostics have been raised and their
+  /// locations.
+  private class DiagnosticTrackingConsumer: DiagnosticConsumer {
+    var registeredDiagnostics = [(String, line: Int?, column: Int?)]()
+
+    func handle(_ diagnostic: Diagnostic) {
+      let loc = diagnostic.location
+      registeredDiagnostics.append((diagnostic.message.text, line: loc?.line, column: loc?.column))
+    }
+
+    func finalize() {}
+  }
+
+  /// Asserts that a specific diagnostic message was emitted. This should be called to check for
+  /// diagnostics after `assertPrettyPrintEqual`.
+  ///
+  /// - Parameters:
+  ///   - message: The diagnostic message to check for.
+  ///   - line: The line number of the diagnotic message within the ueser's input text.
+  ///   - column: The column number of the diagnostic message within the user's input text.
+  ///   - file: The file the test resides in (defaults to the current caller's file).
+  ///   - sourceLine: The line the test resides in (defaults to the current caller's file).
+  func XCTAssertDiagnosed(
+    _ message: Diagnostic.Message,
+    line: Int? = nil,
+    column: Int? = nil,
+    file: StaticString = #file,
+    sourceLine: UInt = #line
+  ) {
+    let maybeIdx = consumer?.registeredDiagnostics.firstIndex {
+      $0 == (message.text, line: line, column: column)
+    }
+
+    guard let idx = maybeIdx else {
+      XCTFail("diagnostic '\(message.text)' not raised", file: file, line: sourceLine)
+      return
+    }
+
+    consumer?.registeredDiagnostics.remove(at: idx)
+  }
 
   public func assertPrettyPrintEqual(
     input: String,
     expected: String,
     linelength: Int,
     configuration: Configuration = Configuration(),
+    whitespaceOnly: Bool = false,
     file: StaticString = #file,
     line: UInt = #line
   ) {
     var configuration = configuration
     configuration.lineLength = linelength
+    let firstPassConsumer = DiagnosticTrackingConsumer()
+    consumer = firstPassConsumer
 
     // Assert that the input, when formatted, is what we expected.
-    if let formatted = prettyPrintedSource(input, configuration: configuration) {
+    if let formatted = prettyPrintedSource(
+      input, configuration: configuration, whitespaceOnly: whitespaceOnly,
+      consumer: firstPassConsumer)
+    {
       XCTAssertEqual(
         expected, formatted,
         "Pretty-printed result was not what was expected",
         file: file, line: line)
 
       // Idempotency check: Running the formatter multiple times should not change the outcome.
       // Assert that running the formatter again on the previous result keeps it the same.
-      if let reformatted = prettyPrintedSource(formatted, configuration: configuration) {
+      if let reformatted = prettyPrintedSource(
+        formatted, configuration: configuration, whitespaceOnly: whitespaceOnly)
+      {
         XCTAssertEqual(
           formatted, reformatted, "Pretty printer is not idempotent", file: file, line: line)
       }
     }
   }
 
   /// Returns the given source code reformatted with the pretty printer.
-  private func prettyPrintedSource(_ source: String, configuration: Configuration) -> String?
-  {
+  private func prettyPrintedSource(
+    _ source: String, configuration: Configuration, whitespaceOnly: Bool,
+    consumer: DiagnosticConsumer? = nil
+  ) -> String? {
     let sourceFileSyntax: SourceFileSyntax
     do {
       sourceFileSyntax = try SyntaxParser.parse(source: source)
@@ -47,15 +100,19 @@ public class PrettyPrintTestCase: XCTestCase {
 
     let context = Context(
       configuration: configuration,
-      diagnosticEngine: nil,
+      diagnosticEngine: DiagnosticEngine(),
       fileURL: URL(fileURLWithPath: "/tmp/file.swift"),
       sourceFileSyntax: sourceFileSyntax)
+    if let consumer = consumer {
+      context.diagnosticEngine?.addConsumer(consumer)
+    }
 
     let printer = PrettyPrinter(
       context: context,
       operatorContext: OperatorContext.makeBuiltinOperatorContext(),
       node: sourceFileSyntax,
-      printTokenStream: false)
+      printTokenStream: false,
+      whitespaceOnly: whitespaceOnly)
     return printer.prettyPrint()
   }
 }