Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions Sources/SwiftFormat/Rules/OrderedImports.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
}

Expand Down
48 changes: 43 additions & 5 deletions Tests/SwiftFormatTests/Rules/LintOrFormatRuleTestCase.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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)
}
}
}
34 changes: 34 additions & 0 deletions Tests/SwiftFormatTests/Rules/OrderedImportsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
]
)
}
}