Skip to content

Replaces the MultiLineTrailingCommas phase 1 rule with Pretty Printer tokens. #120

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
Jan 21, 2020
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
17 changes: 5 additions & 12 deletions Sources/SwiftFormat/Pipelines+Generated.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ import SwiftSyntax

extension LintPipeline {

func visit(_ node: ArrayExprSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(MultiLineTrailingCommas.visit, in: context, for: node)
return .visitChildren
}

func visit(_ node: AsExprSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(NeverForceUnwrap.visit, in: context, for: node)
return .visitChildren
Expand Down Expand Up @@ -71,11 +66,6 @@ extension LintPipeline {
return .visitChildren
}

func visit(_ node: DictionaryExprSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(MultiLineTrailingCommas.visit, in: context, for: node)
return .visitChildren
}

func visit(_ node: EnumCaseElementSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(AlwaysUseLowerCamelCase.visit, in: context, for: node)
visitIfEnabled(NoLeadingUnderscores.visit, in: context, for: node)
Expand Down Expand Up @@ -243,8 +233,12 @@ extension LintPipeline {
return .visitChildren
}

func visit(_ node: SwitchStmtSyntax) -> SyntaxVisitorContinueKind {
func visit(_ node: SwitchCaseListSyntax) -> SyntaxVisitorContinueKind {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, crap, I never regenerated this, did I 😞

At some point we should add a test that verifies that this file is up-to-date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a pre-commit hook? We might want to do the same for running the formatter on the formatter soon too.

Copy link
Member

Choose a reason for hiding this comment

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

We could make a test that invokes generate-pipeline and then verifies that the output is the same as the file in the source tree. Then once we're on CI, failure to regenerate would catch it.

A pre-commit hook that just auto-generates it could be smoother since it would prevent failure, but I'm a little wary of executing that much logic in a pre-commit hook (I'm not sure if there's ever a time where we wouldn't want that thing to run automatically).

visitIfEnabled(NoCasesWithOnlyFallthrough.visit, in: context, for: node)
return .visitChildren
}

func visit(_ node: SwitchStmtSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(NoParensAroundConditions.visit, in: context, for: node)
return .visitChildren
}
Expand Down Expand Up @@ -285,7 +279,6 @@ extension FormatPipeline {
node = DoNotUseSemicolons(context: context).visit(node)
node = FullyIndirectEnum(context: context).visit(node)
node = GroupNumericLiterals(context: context).visit(node)
node = MultiLineTrailingCommas(context: context).visit(node)
node = NoAccessLevelOnExtensionDeclaration(context: context).visit(node)
node = NoBlockComments(context: context).visit(node)
node = NoCasesWithOnlyFallthrough(context: context).visit(node)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ enum RuleRegistry {
"FullyIndirectEnum": true,
"GroupNumericLiterals": true,
"IdentifiersMustBeASCII": true,
"MultiLineTrailingCommas": true,
"NeverForceUnwrap": true,
"NeverUseForceTry": true,
"NeverUseImplicitlyUnwrappedOptionals": true,
Expand Down
39 changes: 39 additions & 0 deletions Sources/SwiftFormatPrettyPrint/PrettyPrint.swift
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ public class PrettyPrinter {
/// Keeps track of the continuation line state as you go into and out of open-close break groups.
private var continuationStack: [Bool] = []

/// Keeps track of the line number where comma regions started. Line numbers are removed as their
/// corresponding end token are encountered.
private var commaDelimitedRegionStack: [Int] = []

/// Keeps track of the most recent number of consecutive newlines that have been printed.
///
/// This value is reset to zero whenever non-newline content is printed.
Expand Down Expand Up @@ -443,6 +447,18 @@ public class PrettyPrinter {
case .enableBreaking:
activeBreakSuppressionCount -= 1
}

case .commaDelimitedRegionStart:
commaDelimitedRegionStack.append(openCloseBreakCompensatingLineNumber)

case .commaDelimitedRegionEnd:
guard let startLineNumber = commaDelimitedRegionStack.popLast() else {
fatalError("Found trailing comma end with no corresponding start.")
}

if startLineNumber != openCloseBreakCompensatingLineNumber {
write(",")
}
}
}

Expand Down Expand Up @@ -535,6 +551,21 @@ public class PrettyPrinter {
case .printerControl:
// Control tokens have no length. They aren't printed.
lengths.append(0)

case .commaDelimitedRegionStart:
lengths.append(0)

case .commaDelimitedRegionEnd:
// The trailing comma needs to be included in the length of the preceding break, but is not
// included in the length of the enclosing group. A trailing comma cannot cause the group
// to break onto multiple lines, because the comma isn't printed for a single line group.
if let index = delimIndexStack.last, case .break = tokens[index] {
lengths[index] += 1
}
// If the closest delimiter token is an open, instead of a break, then adding the comma's
// length isn't necessary. In that case, the comma is printed if the preceding break fires.

lengths.append(1)
}
}

Expand Down Expand Up @@ -621,6 +652,14 @@ public class PrettyPrinter {
case .printerControl(let kind):
printDebugIndent()
print("[PRINTER CONTROL Kind: \(kind) Idx: \(idx)]")

case .commaDelimitedRegionStart:
printDebugIndent()
print("[COMMA DELIMITED START Idx: \(idx)]")

case .commaDelimitedRegionEnd:
printDebugIndent()
print("[COMMA DELIMITED END Idx: \(idx)]")
}
}

Expand Down
8 changes: 8 additions & 0 deletions Sources/SwiftFormatPrettyPrint/Token.swift
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,14 @@ enum Token {
case verbatim(Verbatim)
case printerControl(kind: PrinterControlKind)

/// Marks the beginning of a comma delimited collection, where a trailing comma should be inserted
/// at `commaDelimitedRegionEnd` if and only if the collection spans multiple lines.
case commaDelimitedRegionStart

/// 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

// Convenience overloads for the enum types
static let open = Token.open(.inconsistent, 0)

Expand Down
20 changes: 19 additions & 1 deletion Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ private final class TokenStreamCreator: SyntaxVisitor {
/// stream.
private var pendingMultilineStringSegmentPrefixLengths = [TokenSyntax: Int]()

/// Lists tokens that shouldn't be appended to the token stream as `syntax` tokens. They will be
/// printed conditionally using a different type of token.
private var ignoredTokens = Set<TokenSyntax>()

init(configuration: Configuration, operatorContext: OperatorContext) {
self.config = configuration
self.operatorContext = operatorContext
Expand Down Expand Up @@ -698,6 +702,13 @@ private final class TokenStreamCreator: SyntaxVisitor {

func visit(_ node: ArrayElementListSyntax) -> SyntaxVisitorContinueKind {
insertTokens(.break(.same), betweenElementsOf: node)
if let firstElement = node.first, let lastElement = node.last {
before(firstElement.firstToken, tokens: .commaDelimitedRegionStart)
after(lastElement.lastToken, tokens: .commaDelimitedRegionEnd)
if let existingTrailingComma = lastElement.trailingComma {
ignoredTokens.insert(existingTrailingComma)
}
}
return .visitChildren
}

Expand All @@ -715,6 +726,13 @@ private final class TokenStreamCreator: SyntaxVisitor {

func visit(_ node: DictionaryElementListSyntax) -> SyntaxVisitorContinueKind {
insertTokens(.break(.same), betweenElementsOf: node)
if let firstElement = node.first, let lastElement = node.last {
before(firstElement.firstToken, tokens: .commaDelimitedRegionStart)
after(lastElement.lastToken, tokens: .commaDelimitedRegionEnd)
if let existingTrailingComma = lastElement.trailingComma {
ignoredTokens.insert(existingTrailingComma)
}
}
return .visitChildren
}

Expand Down Expand Up @@ -1936,7 +1954,7 @@ private final class TokenStreamCreator: SyntaxVisitor {

if let pendingSegmentIndex = pendingMultilineStringSegmentPrefixLengths.index(forKey: token) {
appendMultilineStringSegments(at: pendingSegmentIndex)
} else {
} else if !ignoredTokens.contains(token) {
// Otherwise, it's just a regular token, so add the text as-is.
appendToken(.syntax(token.text))
}
Expand Down
88 changes: 0 additions & 88 deletions Sources/SwiftFormatRules/MultiLineTrailingCommas.swift

This file was deleted.

18 changes: 14 additions & 4 deletions Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,42 @@ public class ArrayDeclTests: PrettyPrintTestCase {
public func testBasicArrays() {
let input =
"""
let a = [1, 2, 3]
let a = [1, 2, 3,]
let a: [Bool] = [false, true, true, false]
let a = [11111111, 2222222, 33333333, 444444]
let a = [11111111, 2222222, 33333333, 4444444]
let a: [String] = ["One", "Two", "Three", "Four"]
let a: [String] = ["One", "Two", "Three", "Four", "Five", "Six", "Seven"]
let a: [String] = ["One", "Two", "Three", "Four", "Five", "Six", "Seven",]
let a: [String] = [
"One", "Two", "Three", "Four", "Five",
"Six", "Seven", "Eight",
]
"""

let expected =
"""
let a = [1, 2, 3]
let a: [Bool] = [false, true, true, false]
let a = [11111111, 2222222, 33333333, 444444]
let a = [
11111111, 2222222, 33333333, 4444444
11111111, 2222222, 33333333, 4444444,
]
let a: [String] = [
"One", "Two", "Three", "Four"
"One", "Two", "Three", "Four",
]
let a: [String] = [
"One", "Two", "Three", "Four", "Five",
"Six", "Seven"
"Six", "Seven",
]
let a: [String] = [
"One", "Two", "Three", "Four", "Five",
"Six", "Seven",
]
let a: [String] = [
"One", "Two", "Three", "Four", "Five",
"Six", "Seven", "Eight",
]

"""

Expand Down
10 changes: 5 additions & 5 deletions Tests/SwiftFormatPrettyPrintTests/CommentTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -220,30 +220,30 @@ public class CommentTests: PrettyPrintTestCase {
// Array comment
let a = [
456, // small comment
789
789,
]

// Dictionary comment
let b = [
"abc": 456, // small comment
"def": 789
"def": 789,
]

// Trailing comment
let c = [
123, 456 // small comment
123, 456, // small comment
]

/* Array comment */
let a = [
456, /* small comment */
789
789,
]

/* Dictionary comment */
let b = [
"abc": 456, /* small comment */
"def": 789
"def": 789,
]

"""
Expand Down
Loading