Merge pull request swiftlang#185 from google/format-idempotent

Stop wrapping end-of-line comments, add idempotency checks.

Author: dabelknap

Sources/SwiftFormatPrettyPrint/Comment.swift

@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 import Foundation
+import SwiftSyntax
 
 struct Comment {
   enum Kind {
@@ -38,10 +39,13 @@ struct Comment {
 
   let kind: Kind
   var text: [String]
+  let position: AbsolutePosition?
   public var length: Int
 
-  init(kind: Kind, text: String) {
+  init(kind: Kind, text: String, position: AbsolutePosition? = nil) {
     self.kind = kind
+    self.position = position
+
     switch kind {
     case .line, .docLine:
       self.text = [text]

Sources/SwiftFormatPrettyPrint/PrettyPrint.swift

@@ -29,7 +29,8 @@ fileprivate let closeGroupMarker = "\u{27ED}"
 /// PrettyPrinter takes a Syntax node and outputs a well-formatted, re-indented reproduction of the
 /// code as a String.
 public class PrettyPrinter {
-  private let configuration: Configuration
+  private let context: Context
+  private var configuration: Configuration { return context.configuration }
   private let maxLineLength: Int
   private var tokens: [Token]
   private var outputBuffer: String = ""
@@ -85,10 +86,11 @@ public class PrettyPrinter {
   /// Creates a new PrettyPrinter with the provided formatting configuration.
   ///
   /// - Parameters:
-  ///   - configuration: The configuration used to decide whitespace or breaking behavior.
+  ///   - context: The formatter context.
   ///   - node: The node to be pretty printed.
-  public init(configuration: Configuration, node: Syntax, isDebugMode: Bool, printTokenStream: Bool) {
-    self.configuration = configuration
+  public init(context: Context, node: Syntax, isDebugMode: Bool, printTokenStream: Bool) {
+    self.context = context
+    let configuration = context.configuration
     self.tokens = node.makeTokenStream(configuration: configuration)
     self.maxLineLength = configuration.lineLength
     self.isDebugMode = isDebugMode
@@ -234,7 +236,7 @@ public class PrettyPrinter {
       }
       spaceRemaining -= syntaxToken.text.count
 
-    case .comment(let comment):
+    case .comment(let comment, let wasEndOfLine):
       if lastBreakConsecutive {
         // If the last token created a new line, we need to apply indentation.
         writeSpaces(lastBreakValue)
@@ -245,7 +247,13 @@ public class PrettyPrinter {
         lastBreakValue = 0
       }
       write(comment.print(indent: lastBreakValue))
-      spaceRemaining -= comment.length
+      if wasEndOfLine {
+        if comment.length > spaceRemaining {
+          diagnose(.moveEndOfLineComment, at: comment.position)
+        }
+      } else {
+        spaceRemaining -= comment.length
+      }
 
     case .verbatim(let verbatim):
       write(verbatim.print(indent: lastBreakValue))
@@ -351,9 +359,9 @@ public class PrettyPrinter {
           total += syntaxToken.text.count
         }
 
-      case .comment(let comment):
+      case .comment(let comment, let wasEndOfLine):
         lengths.append(comment.length)
-        total += comment.length
+        total += wasEndOfLine ? 0 : comment.length
 
       case .verbatim(let verbatim):
         var length: Int
@@ -476,17 +484,17 @@ public class PrettyPrinter {
       printDebugIndent()
       print("[RESET]")
 
-    case .comment(let comment):
+    case .comment(let comment, let wasEndOfLine):
       printDebugIndent()
       switch comment.kind {
       case .line:
-        print("[COMMENT Line Length: \(length)]")
+        print("[COMMENT Line Length: \(length) EOL: \(wasEndOfLine)]")
       case .docLine:
-        print("[COMMENT DocLine Length: \(length)]")
+        print("[COMMENT DocLine Length: \(length) EOL: \(wasEndOfLine)]")
       case .block:
-        print("[COMMENT Block Length: \(length)]")
+        print("[COMMENT Block Length: \(length) EOL: \(wasEndOfLine)]")
       case .docBlock:
-        print("[COMMENT DocBlock Length: \(length)]")
+        print("[COMMENT DocBlock Length: \(length) EOL: \(wasEndOfLine)]")
       }
       printDebugIndent()
       print(comment.print(indent: debugIndent))
@@ -514,6 +522,16 @@ public class PrettyPrinter {
       }
     }
   }
+
+  private func diagnose(_ message: Diagnostic.Message, at position: AbsolutePosition?) {
+    let location: SourceLocation?
+    if let position = position {
+      location = SourceLocation(file: context.fileURL.path, position: position)
+    } else {
+      location = nil
+    }
+    context.diagnosticEngine?.diagnose(message, location: location)
+  }
 }
 
 /// Convenience properties/functions to access ANSI color code strings, respecting whether or not
@@ -543,3 +561,9 @@ enum Ansi {
   /// The number 30 is added to the given index to determine the actual color code.
   static func color(_ index: Int) -> String { return isTerminal ? "\u{001b}[\(30 + index)m" : "" }
 }
+
+extension Diagnostic.Message {
+
+  static let moveEndOfLineComment =
+    Diagnostic.Message(.warning, "End-of-line comment exceeds the line length")
+}

Sources/SwiftFormatPrettyPrint/Token.swift

@@ -31,7 +31,7 @@ enum Token {
   case `break`(size: Int, offset: Int)
   case space(size: Int)
   case newlines(Int, offset: Int)
-  case comment(Comment)
+  case comment(Comment, wasEndOfLine: Bool)
   case reset
   case verbatim(Verbatim)
 

Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift

@@ -1371,24 +1371,23 @@ private final class TokenStreamCreator: SyntaxVisitor {
       return
     }
 
+    let commentToken: Comment
+    let position = token.endPositionAfterTrailingTrivia
     switch firstPiece {
     case .lineComment(let text):
-      appendToken(.break(size: 2, offset: 2))
-      appendToken(.comment(Comment(kind: .line, text: text)))
-      if isInContainer(token) {
-        appendToken(.newline)
-      }
-
+      commentToken = Comment(kind: .line, text: text, position: position)
     case .blockComment(let text):
-      appendToken(.break(size: 2, offset: 2))
-      appendToken(.comment(Comment(kind: .block, text: text)))
-      if isInContainer(token) {
-        appendToken(.newline)
-      }
-
+      commentToken = Comment(kind: .block, text: text, position: position)
     default:
       return
     }
+
+    appendToken(.break(size: 2, offset: 0))
+    appendToken(.comment(commentToken, wasEndOfLine: true))
+
+    if isInContainer(token) {
+      appendToken(.newline)
+    }
   }
 
   private func isInContainer(_ token: TokenSyntax) -> Bool {
@@ -1415,7 +1414,7 @@ private final class TokenStreamCreator: SyntaxVisitor {
             }
           }
 
-          appendToken(.comment(Comment(kind: .line, text: text)))
+          appendToken(.comment(Comment(kind: .line, text: text), wasEndOfLine: false))
 
           if token.withoutTrivia().text == "}" {
             appendToken(.newline(offset: -2))
@@ -1435,7 +1434,7 @@ private final class TokenStreamCreator: SyntaxVisitor {
             }
           }
 
-          appendToken(.comment(Comment(kind: .block, text: text)))
+          appendToken(.comment(Comment(kind: .block, text: text), wasEndOfLine: false))
 
           if token.withoutTrivia().text == "}" {
             appendToken(.newline(offset: -2))
@@ -1445,7 +1444,7 @@ private final class TokenStreamCreator: SyntaxVisitor {
         }
 
       case .docLineComment(let text):
-        appendToken(.comment(Comment(kind: .docLine, text: text)))
+        appendToken(.comment(Comment(kind: .docLine, text: text), wasEndOfLine: false))
         if case .newlines? = trivia[safe: index + 1],
            case .docLineComment? = trivia[safe: index + 2] {
           // do nothing
@@ -1454,7 +1453,7 @@ private final class TokenStreamCreator: SyntaxVisitor {
         }
 
       case .docBlockComment(let text):
-        appendToken(.comment(Comment(kind: .docBlock, text: text)))
+        appendToken(.comment(Comment(kind: .docBlock, text: text), wasEndOfLine: false))
         appendToken(.newline)
 
       case .newlines(let n), .carriageReturns(let n), .carriageReturnLineFeeds(let n):
@@ -1471,11 +1470,11 @@ private final class TokenStreamCreator: SyntaxVisitor {
   func appendToken(_ token: Token) {
     if let last = tokens.last {
       switch (last, token) {
-      case (.comment(let c1), .comment(let c2))
-        where c1.kind == .docLine && c2.kind == .docLine:
+      case (.comment(let c1, _), .comment(let c2, _))
+      where c1.kind == .docLine && c2.kind == .docLine:
         var newComment = c1
         newComment.addText(c2.text)
-        tokens[tokens.count - 1] = .comment(newComment)
+        tokens[tokens.count - 1] = .comment(newComment, wasEndOfLine: false)
         return
 
       case (.newlines(let N1, let offset1), .newlines(let N2, let offset2)):

Sources/swift-format/Run.swift

@@ -24,9 +24,7 @@ import SwiftSyntax
 /// - Returns: Zero if there were no lint errors, otherwise a non-zero number.
 public func lintMain(path: String) -> Int {
   let url = URL(fileURLWithPath: path)
-  let engine = DiagnosticEngine()
-  let consumer = PrintingDiagnosticConsumer()
-  engine.addConsumer(consumer)
+  let engine = makeDiagnosticEngine()
 
   let context = Context(
     configuration: Configuration(),
@@ -55,12 +53,8 @@ public func lintMain(path: String) -> Int {
 /// - Returns: Zero if there were no lint errors, otherwise a non-zero number.
 public func formatMain(path: String, isDebugMode: Bool, prettyPrint: Bool, printTokenStream: Bool) -> Int {
   let url = URL(fileURLWithPath: path)
-
-  let context = Context(
-    configuration: Configuration(),
-    diagnosticEngine: nil,
-    fileURL: url
-  )
+  let configuration = Configuration()
+  let context = Context(configuration: configuration, diagnosticEngine: nil, fileURL: url)
 
   let pipeline = FormatPipeline(context: context)
   populate(pipeline)
@@ -72,8 +66,15 @@ public func formatMain(path: String, isDebugMode: Bool, prettyPrint: Bool, print
     let formatted = pipeline.visit(file as Syntax)
 
     if prettyPrint {
+      // We create a different context here because we only want diagnostics from the pretty printer
+      // phase when formatting.
+      let prettyPrintContext = Context(
+        configuration: configuration,
+        diagnosticEngine: makeDiagnosticEngine(),
+        fileURL: url)
+
       let printer = PrettyPrinter(
-        configuration: context.configuration,
+        context: prettyPrintContext,
         node: formatted,
         isDebugMode: isDebugMode,
         printTokenStream: printTokenStream
@@ -87,3 +88,11 @@ public func formatMain(path: String, isDebugMode: Bool, prettyPrint: Bool, print
   }
   return 0
 }
+
+/// Makes and returns a new configured diagnostic engine.
+private func makeDiagnosticEngine() -> DiagnosticEngine {
+  let engine = DiagnosticEngine()
+  let consumer = PrintingDiagnosticConsumer()
+  engine.addConsumer(consumer)
+  return engine
+}

Tests/SwiftFormatPrettyPrintTests/CommentTests.swift

@@ -90,7 +90,7 @@ public class CommentTests: PrettyPrintTestCase {
       // Comment 3
       // Comment 4
 
-      let reallyLongVariableName = 123 // This comment should wrap
+      let reallyLongVariableName = 123 // This comment should not wrap
 
       func MyFun() {
         // just a comment
@@ -124,8 +124,7 @@ public class CommentTests: PrettyPrintTestCase {
       // Comment 3
       // Comment 4
 
-      let reallyLongVariableName = 123
-        // This comment should wrap
+      let reallyLongVariableName = 123  // This comment should not wrap
 
       func MyFun() {
         // just a comment
@@ -298,7 +297,7 @@ public class CommentTests: PrettyPrintTestCase {
       /* Comment 3
          Comment 4 */
 
-      let reallyLongVariableName = 123  /* This comment should wrap */
+      let reallyLongVariableName = 123  /* This comment should not wrap */
 
       let d = 123
       /* Trailing Comment */
@@ -315,8 +314,7 @@ public class CommentTests: PrettyPrintTestCase {
       /* Comment 3
          Comment 4 */
 
-      let reallyLongVariableName = 123
-        /* This comment should wrap */
+      let reallyLongVariableName = 123  /* This comment should not wrap */
 
       let d = 123
       /* Trailing Comment */

Tests/SwiftFormatPrettyPrintTests/PrettyPrintTestCase.swift

@@ -6,35 +6,48 @@ import XCTest
 @testable import SwiftFormatPrettyPrint
 
 public class PrettyPrintTestCase: XCTestCase {
+
   public func assertPrettyPrintEqual(
     input: String,
     expected: String,
     linelength: Int,
     file: StaticString = #file,
     line: UInt = #line
   ) {
-    let config = Configuration()
-    config.lineLength = linelength
+    let configuration = Configuration()
+    configuration.lineLength = linelength
 
     let context = Context(
-      configuration: config,
+      configuration: configuration,
       diagnosticEngine: nil,
-      fileURL: URL(fileURLWithPath: "/tmp/test.swift")
-    )
+      fileURL: URL(fileURLWithPath: "/tmp/file.swift"))
 
-    do {
-      let syntax = try SyntaxTreeParser.parse(input)
+    // Assert that the input, when formatted, is what we expected.
+    if let formatted = prettyPrintedSource(input, context: context) {
+      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, context: context) {
+        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(_ original: String, context: Context) -> String? {
+    do {
+      let syntax = try SyntaxTreeParser.parse(original)
       let printer = PrettyPrinter(
-        configuration: context.configuration,
-        node: syntax,
-        isDebugMode: false,
-        printTokenStream: false
-      )
-      let output = printer.prettyPrint()
-      XCTAssertEqual(expected, output, file: file, line: line)
+        context: context, node: syntax, isDebugMode: false, printTokenStream: false)
+      return printer.prettyPrint()
     } catch {
-      fatalError("\(error)")
+      XCTFail("Parsing failed with error: \(error)")
+      return nil
     }
   }
 }