Skip to content

Merge adjacent .line and .docLine comments into a single element. #732

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.

Sign up for GitHub

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 2 commits into from
May 9, 2024
Merged

Merge adjacent .line and .docLine comments into a single element. #732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cf4ea0
Merge adjacent .line and .docLine comments into a single element.
shawnhyam May 7, 2024
b065fa3
Improvements based on feedback and further testing.
shawnhyam May 8, 2024
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
7 changes: 4 additions & 3 deletions Sources/SwiftFormat/PrettyPrint/Comment.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ struct Comment {

switch kind {
case .line, .docLine:
self.text = [text.trimmingTrailingWhitespace()]
self.text = [text]
self.text[0].removeFirst(kind.prefixLength)
self.length = self.text.reduce(0, { $0 + $1.count + kind.prefixLength + 1 })

Expand All @@ -88,8 +88,9 @@ struct Comment {
func print(indent: [Indent]) -> String {
switch self.kind {
case .line, .docLine:
let separator = "\n" + kind.prefix
return kind.prefix + self.text.joined(separator: separator)
let separator = "\n" + indent.indentation() + kind.prefix
let trimmedLines = self.text.map { $0.trimmingTrailingWhitespace() }
return kind.prefix + trimmedLines.joined(separator: separator)
case .block, .docBlock:
let separator = "\n"
return kind.prefix + self.text.joined(separator: separator) + "*/"
Expand Down
35 changes: 29 additions & 6 deletions Sources/SwiftFormat/PrettyPrint/TokenStreamCreator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3384,14 +3384,37 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
/// This function also handles collapsing neighboring tokens in situations where that is
/// desired, like merging adjacent comments and newlines.
private func appendToken(_ token: Token) {
func breakAllowsCommentMerge(_ breakKind: BreakKind) -> Bool {
return breakKind == .same || breakKind == .continue || breakKind == .contextual
}

if let last = tokens.last {
switch (last, token) {
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, wasEndOfLine: false)
return
case (.break(let breakKind, _, .soft(1, _)), .comment(let c2, _))
where breakAllowsCommentMerge(breakKind) && (c2.kind == .docLine || c2.kind == .line):
// we are search for the pattern of [line comment] - [soft break 1] - [line comment]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit ourselves to just .same type breaks? Here's a weird example:

for x: Int
  // foo
  // bar
in y {}

This produces the following token stream around the comments:

  [BREAK Kind: continue Size: 1 Length: 107 NL: soft(count: 1, discretionary: true) Idx: 10]
  [COMMENT Line Length: 7 EOL: false Idx: 11]
  // foo
  [BREAK Kind: continue Size: 0 Length: 107 NL: soft(count: 1, discretionary: true) Idx: 12]
  [COMMENT Line Length: 7 EOL: false Idx: 13]
  // bar
  [BREAK Kind: continue Size: 0 Length: 104 NL: soft(count: 1, discretionary: true) Idx: 14]

This uses .continue breaks, so the logic as written here wouldn't merge those, even though visually they look like a multiline comment.

We'd also want to distinguish that from a similar case:

for x: Int  // foo
  // bar
in y {}

Presumably we wouldn't want to merge those comments together. I think the logic you have here is fine for that one, because the token stream looks like this:

  [SPACE Size: 2 Flexible: true Length: 2 Idx: 9]
  [COMMENT Line Length: 7 EOL: true Idx: 10]
  // foo
  [BREAK Kind: close(mustBreak: false) Size: 0 Length: 0 NL: elective(ignoresDiscretionary: false) Idx: 11]
  [BREAK Kind: continue Size: 1 Length: 107 NL: soft(count: 1, discretionary: true) Idx: 12]
  [COMMENT Line Length: 7 EOL: false Idx: 13]
  // bar
  [BREAK Kind: continue Size: 0 Length: 104 NL: soft(count: 1, discretionary: true) Idx: 14]

There are two breaks after foo (the first closes the indenting break group around the type annotation, the second is for the newline), so this logic (correctly) wouldn't trigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to allow merging of .continue and .contextual breaks, based on your feedback and examining the token stream from an existing codebase.

// where the comment type is the same; these can be merged into a single comment
if let nextToLast = tokens.dropLast().last,
case let .comment(c1, false) = nextToLast,
c1.kind == c2.kind
{
var mergedComment = c1
mergedComment.addText(c2.text)
tokens.removeLast() // remove the soft break
// replace the original comment with the merged one
tokens[tokens.count - 1] = .comment(mergedComment, wasEndOfLine: false)

// need to fix lastBreakIndex because we just removed the last break
lastBreakIndex = tokens.lastIndex(where: {
switch $0 {
case .break: return true
default: return false
}
})
canMergeNewlinesIntoLastBreak = false

return
}

// If we see a pair of spaces where one or both are flexible, combine them into a new token
// with the maximum of their counts.
Expand Down
72 changes: 72 additions & 0 deletions Tests/SwiftFormatTests/PrettyPrint/CommentTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ final class CommentTests: PrettyPrintTestCase {
func testLineComments() {
let input =
"""
// Line Comment0

// Line Comment1
// Line Comment2
let a = 123
Expand All @@ -93,6 +95,7 @@ final class CommentTests: PrettyPrintTestCase {
// Comment 4

let reallyLongVariableName = 123 // This comment should not wrap
// and should not combine with this comment

func MyFun() {
// just a comment
Expand Down Expand Up @@ -135,6 +138,8 @@ final class CommentTests: PrettyPrintTestCase {

let expected =
"""
// Line Comment0

// Line Comment1
// Line Comment2
let a = 123
Expand All @@ -145,6 +150,7 @@ final class CommentTests: PrettyPrintTestCase {
// Comment 4

let reallyLongVariableName = 123 // This comment should not wrap
// and should not combine with this comment

func MyFun() {
// just a comment
Expand Down Expand Up @@ -208,6 +214,13 @@ final class CommentTests: PrettyPrintTestCase {
let c = [123, 456 // small comment
]

// Multiline comment
let d = [123,
// comment line 1
// comment line 2
456
]

/* Array comment */
let a = [456, /* small comment */
789]
Expand Down Expand Up @@ -236,6 +249,14 @@ final class CommentTests: PrettyPrintTestCase {
123, 456, // small comment
]

// Multiline comment
let d = [
123,
// comment line 1
// comment line 2
456,
]

/* Array comment */
let a = [
456, /* small comment */
Expand Down Expand Up @@ -760,4 +781,55 @@ final class CommentTests: PrettyPrintTestCase {
]
)
}

func testLineWithDocLineComment() {
// none of these should be merged if/when there is comment formatting
let input =
"""
/// Doc line comment
// Line comment
/// Doc line comment
// Line comment

// Another line comment

"""
assertPrettyPrintEqual(input: input, expected: input, linelength: 80)
}

func testNonmergeableComments() {
// none of these should be merged if/when there is comment formatting
let input =
"""
let x = 1 // end of line comment
//

let y = // eol comment
1 // another
+ 2 // and another

"""

assertPrettyPrintEqual(input: input, expected: input, linelength: 80)
}

func testMergeableComments() {
// these examples should be merged and formatted if/when there is comment formatting
let input =
"""
let z =
// one comment
// and another comment
1 + 2

let w = [1, 2, 3]
.foo()
// this comment
// could be merged with this one
.bar()

"""

assertPrettyPrintEqual(input: input, expected: input, linelength: 80)
}
}