Merge pull request swiftlang#122 from dylansturg/handle_nonwhitespace_linting

Diagnostics for trailing comma changes.

Author: allevato

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()
   }
 }