Merge pull request #674 from allevato/more-trailing-comment-fixes

[OrderedImports] Fix dropped trailing comments on top-level code items.

Author: allevato

Sources/SwiftFormat/Rules/OrderedImports.swift

@@ -315,17 +315,17 @@ fileprivate func generateLines(codeBlockItemList: CodeBlockItemListSyntax, conte
       blockWithoutTrailingTrivia.trailingTrivia = []
       currentLine.syntaxNode = .importCodeBlock(blockWithoutTrailingTrivia, sortable: sortable)
     } else {
-      guard let syntaxNode = currentLine.syntaxNode else {
-        currentLine.syntaxNode = .nonImportCodeBlocks([block])
-        continue
-      }
-      // Multiple code blocks can be merged, as long as there isn't an import statement.
-      switch syntaxNode {
-      case .importCodeBlock:
-        appendNewLine()
+      if let syntaxNode = currentLine.syntaxNode {
+        // Multiple code blocks can be merged, as long as there isn't an import statement.
+        switch syntaxNode {
+        case .importCodeBlock:
+          appendNewLine()
+          currentLine.syntaxNode = .nonImportCodeBlocks([block])
+        case .nonImportCodeBlocks(let existingCodeBlocks):
+          currentLine.syntaxNode = .nonImportCodeBlocks(existingCodeBlocks + [block])
+        }
+      } else {
         currentLine.syntaxNode = .nonImportCodeBlocks([block])
-      case .nonImportCodeBlocks(let existingCodeBlocks):
-        currentLine.syntaxNode = .nonImportCodeBlocks(existingCodeBlocks + [block])
       }
     }
 

Tests/SwiftFormatTests/Rules/LintOrFormatRuleTestCase.swift

@@ -112,7 +112,7 @@ class LintOrFormatRuleTestCase: DiagnosingTestCase {
 
     let formatter = formatType.init(context: context)
     let actual = formatter.visit(sourceFileSyntax)
-    assertStringsEqualWithDiff(actual.description, expected, file: file, line: line)
+    assertStringsEqualWithDiff("\(actual)", expected, file: file, line: line)
 
     assertFindings(
       expected: findings,
@@ -123,15 +123,22 @@ class LintOrFormatRuleTestCase: DiagnosingTestCase {
       line: line)
 
     // Verify that the pretty printer can consume the transformed tree (e.g., it does not contain
-    // any unfolded `SequenceExpr`s). We don't need to check the actual output here (we don't want
-    // the rule tests to be pretty-printer dependent), but this will catch invariants that aren't
-    // satisfied.
-    _ = PrettyPrinter(
+    // any unfolded `SequenceExpr`s). Then do a whitespace-insensitive comparison of the two trees
+    // to verify that the format rule didn't transform the tree in such a way that it caused the
+    // pretty-printer to drop important information (the most likely case is a format rule
+    // misplacing trivia in a way that the pretty-printer isn't able to handle).
+    let prettyPrintedSource = PrettyPrinter(
       context: context,
       node: Syntax(actual),
       printTokenStream: false,
       whitespaceOnly: false
     ).prettyPrint()
+    let prettyPrintedTree = Parser.parse(source: prettyPrintedSource)
+    XCTAssertEqual(
+      whitespaceInsensitiveText(of: actual),
+      whitespaceInsensitiveText(of: prettyPrintedTree),
+      "After pretty-printing and removing fluid whitespace, the files did not match",
+      file: file, line: line)
 
     var emittedPipelineFindings = [Finding]()
     // Disable default rules, so only select rule runs in pipeline
@@ -149,3 +156,34 @@ class LintOrFormatRuleTestCase: DiagnosingTestCase {
       emittedFindings: emittedPipelineFindings, context: context, file: file, line: line)
   }
 }
+
+/// Returns a string containing a whitespace-insensitive representation of the given source file.
+private func whitespaceInsensitiveText(of file: SourceFileSyntax) -> String {
+  var result = ""
+  for token in file.tokens(viewMode: .sourceAccurate) {
+    appendNonspaceTrivia(token.leadingTrivia, to: &result)
+    result.append(token.text)
+    appendNonspaceTrivia(token.trailingTrivia, to: &result)
+  }
+  return result
+}
+
+/// Appends any non-whitespace trivia pieces from the given trivia collection to the output string.
+private func appendNonspaceTrivia(_ trivia: Trivia, to string: inout String) {
+  for piece in trivia {
+    switch piece {
+    case .carriageReturnLineFeeds, .carriageReturns, .formfeeds, .newlines, .spaces, .tabs:
+      break
+    case .lineComment(let comment), .docLineComment(let comment):
+      // A tree transforming rule might leave whitespace at the end of a line comment, which the
+      // pretty printer will remove, so we should ignore that.
+      if let lastNonWhitespaceIndex = comment.lastIndex(where: { !$0.isWhitespace }) {
+        string.append(contentsOf: comment[...lastNonWhitespaceIndex])
+      } else {
+        string.append(comment)
+      }
+    default:
+      piece.write(to: &string)
+    }
+  }
+}

Tests/SwiftFormatTests/Rules/OrderedImportsTests.swift

@@ -618,4 +618,38 @@ final class OrderedImportsTests: LintOrFormatRuleTestCase {
       ]
     )
   }
+
+  func testTrailingCommentsOnTopLevelCodeItems() {
+    assertFormatting(
+      OrderedImports.self,
+      input: """
+        import Zebras
+        1️⃣import Apples
+        #if canImport(Darwin)
+          import Darwin
+        #elseif canImport(Glibc)
+          import Glibc
+        #endif  // canImport(Darwin)
+
+        foo()  // calls the foo
+        bar()  // calls the bar
+        """,
+      expected: """
+        import Apples
+        import Zebras
+
+        #if canImport(Darwin)
+          import Darwin
+        #elseif canImport(Glibc)
+          import Glibc
+        #endif  // canImport(Darwin)
+
+        foo()  // calls the foo
+        bar()  // calls the bar
+        """,
+      findings: [
+        FindingSpec("1️⃣", message: "sort import statements lexicographically"),
+      ]
+    )
+  }
 }