From 47b5015b9fd75b6a2fb3193b020e5969535e6a83 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Wed, 1 Apr 2020 13:42:49 -0700 Subject: [PATCH 01/23] Discard newlines when sorting imports. When the author inserts a newline between the `import` token and the imported path, that newline shouldn't be considered when sorting the paths. --- Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift | 2 -- 1 file changed, 2 deletions(-) diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index 173048dc8..d12c3db33 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -207,9 +207,7 @@ extension DifferentiationAttributeTests { // `swift test --generate-linuxmain` // to regenerate. static let __allTests__DifferentiationAttributeTests = [ - ("testDerivative", testDerivative), ("testDifferentiable", testDifferentiable), - ("testTranspose", testTranspose), ] } From fa522d03132a342f26064556312253b1c77b61e7 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Wed, 1 Apr 2020 14:21:51 -0700 Subject: [PATCH 02/23] Move conditional compilation directive inside tests. This ensures that the test function always exists, even if the symbol is not defined, so that `--generate-linuxmain` contains the symbol in both cases. When the symbol is undefined, the empty test will vacuously succeed. --- Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index d12c3db33..173048dc8 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -207,7 +207,9 @@ extension DifferentiationAttributeTests { // `swift test --generate-linuxmain` // to regenerate. static let __allTests__DifferentiationAttributeTests = [ + ("testDerivative", testDerivative), ("testDifferentiable", testDifferentiable), + ("testTranspose", testTranspose), ] } From 58ccbf38c070a524398fbbd5c3aecc9040c64cc7 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Wed, 1 Apr 2020 15:18:12 -0700 Subject: [PATCH 03/23] Restrict doc-block to doc-line transform to first doc comment. The transform is intentionally meant to only apply to comments that are documenting a declaration. The distinction is made by finding the comment "closest" to the decl, searching through the trivia backwards. Unfortunately, the search didn't handle the fact that there could be multiple doc comments in the leading trivia of a decl. Multiple comments usually occur at the beginning of the file, when there's a file level comment (either doc block or doc line), followed by decl documentation, then the first decl. It might make sense to convert doc-block comments in the file preamble to doc-line, but we've previously had issues converting general purpose block comments. This approach is safer because only comments that are documenting a decl (defined as closest to the decl) are converted. --- ...eTripleSlashForDocumentationComments.swift | 10 +- ...leSlashForDocumentationCommentsTests.swift | 91 +++++++++++++++++++ .../XCTestManifests.swift | 3 + 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/Sources/SwiftFormatRules/UseTripleSlashForDocumentationComments.swift b/Sources/SwiftFormatRules/UseTripleSlashForDocumentationComments.swift index 9daca71cb..d8f733bf5 100644 --- a/Sources/SwiftFormatRules/UseTripleSlashForDocumentationComments.swift +++ b/Sources/SwiftFormatRules/UseTripleSlashForDocumentationComments.swift @@ -73,17 +73,23 @@ public final class UseTripleSlashForDocumentationComments: SyntaxFormatRule { /// a docLineComment. private func convertDocBlockCommentToDocLineComment(_ decl: DeclSyntax) -> DeclSyntax { guard let commentText = decl.docComment else { return decl } - guard let declLeadinTrivia = decl.leadingTrivia else { return decl } + guard let declLeadingTrivia = decl.leadingTrivia else { return decl } let docComments = commentText.components(separatedBy: "\n") var pieces = [TriviaPiece]() // Ensures the documentation comment is a docLineComment. var hasFoundDocComment = false - for piece in declLeadinTrivia.reversed() { + for piece in declLeadingTrivia.reversed() { if case .docBlockComment(_) = piece, !hasFoundDocComment { hasFoundDocComment = true diagnose(.avoidDocBlockComment, on: decl) pieces.append(contentsOf: separateDocBlockIntoPieces(docComments).reversed()) + } else if case .docLineComment(_) = piece, !hasFoundDocComment { + // The comment was a doc-line comment all along. Leave it alone. + // This intentionally only considers the comment closest to the decl. There may be other + // comments, including block or doc-block comments, which are left as-is because they aren't + // necessarily related to the decl and are unlikely part of the decl's documentation. + return decl } else { pieces.append(piece) } diff --git a/Tests/SwiftFormatRulesTests/UseTripleSlashForDocumentationCommentsTests.swift b/Tests/SwiftFormatRulesTests/UseTripleSlashForDocumentationCommentsTests.swift index e986ce58d..bba730a97 100644 --- a/Tests/SwiftFormatRulesTests/UseTripleSlashForDocumentationCommentsTests.swift +++ b/Tests/SwiftFormatRulesTests/UseTripleSlashForDocumentationCommentsTests.swift @@ -54,4 +54,95 @@ final class UseTripleSlashForDocumentationCommentsTests: LintOrFormatRuleTestCas public var test = 1 """) } + + func testMultipleTypesOfDocComments() { + XCTAssertFormatting( + UseTripleSlashForDocumentationComments.self, + input: """ + /** + * This is my preamble. It could be important. + * This comment stays as-is. + */ + + /// This decl has a comment. + /// The comment is multiple lines long. + public class AClazz { + } + """, + expected: """ + /** + * This is my preamble. It could be important. + * This comment stays as-is. + */ + + /// This decl has a comment. + /// The comment is multiple lines long. + public class AClazz { + } + """) + } + + func testMultipleDocLineComments() { + XCTAssertFormatting( + UseTripleSlashForDocumentationComments.self, + input: """ + /// This is my preamble. It could be important. + /// This comment stays as-is. + /// + + /// This decl has a comment. + /// The comment is multiple lines long. + public class AClazz { + } + """, + expected: """ + /// This is my preamble. It could be important. + /// This comment stays as-is. + /// + + /// This decl has a comment. + /// The comment is multiple lines long. + public class AClazz { + } + """) + } + + func testManyDocComments() { + XCTAssertFormatting( + UseTripleSlashForDocumentationComments.self, + input: """ + /** + * This is my preamble. It could be important. + * This comment stays as-is. + */ + + /// This is a doc-line comment! + + /** This is a fairly short doc-block comment. */ + + /// Why are there so many comments? + /// Who knows! But there are loads. + + /** AClazz is a class with good name. */ + public class AClazz { + } + """, + expected: """ + /** + * This is my preamble. It could be important. + * This comment stays as-is. + */ + + /// This is a doc-line comment! + + /** This is a fairly short doc-block comment. */ + + /// Why are there so many comments? + /// Who knows! But there are loads. + + /// AClazz is a class with good name. + public class AClazz { + } + """) + } } diff --git a/Tests/SwiftFormatRulesTests/XCTestManifests.swift b/Tests/SwiftFormatRulesTests/XCTestManifests.swift index 41d3f412f..7c57d651a 100644 --- a/Tests/SwiftFormatRulesTests/XCTestManifests.swift +++ b/Tests/SwiftFormatRulesTests/XCTestManifests.swift @@ -369,6 +369,9 @@ extension UseTripleSlashForDocumentationCommentsTests { // `swift test --generate-linuxmain` // to regenerate. static let __allTests__UseTripleSlashForDocumentationCommentsTests = [ + ("testManyDocComments", testManyDocComments), + ("testMultipleDocLineComments", testMultipleDocLineComments), + ("testMultipleTypesOfDocComments", testMultipleTypesOfDocComments), ("testRemoveDocBlockComments", testRemoveDocBlockComments), ("testRemoveDocBlockCommentsWithoutStars", testRemoveDocBlockCommentsWithoutStars), ] From 744ca11ade217399daba2204de0c59d164a71d03 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Thu, 2 Apr 2020 09:52:33 -0700 Subject: [PATCH 04/23] Remove calls to `FileHandle.synchronizeFile()`. Standard output does not support `fsync`, which is the system call that this boils down to on Linux (and presumably on Darwin as well). In both cases, `fsync` sets `errno` to `EINVAL`, but unfortunately Foundation's behavior differs per platform: on Darwin the error is silently swallowed, but on Linux it traps. --- Sources/swift-format/Subcommands/Format.swift | 2 -- 1 file changed, 2 deletions(-) diff --git a/Sources/swift-format/Subcommands/Format.swift b/Sources/swift-format/Subcommands/Format.swift index 7ad2f55b9..c9d0279bc 100644 --- a/Sources/swift-format/Subcommands/Format.swift +++ b/Sources/swift-format/Subcommands/Format.swift @@ -108,7 +108,6 @@ private func formatMain( try bufferData.write(to: assumingFileURL, options: .atomic) } else { try formatter.format(source: source, assumingFileURL: assumingFileURL, to: &stdoutStream) - stdoutStream.synchronizeFile() } } catch SwiftFormatError.fileNotReadable { diagnosticEngine.diagnose( @@ -122,7 +121,6 @@ private func formatMain( return } stdoutStream.write(source) - stdoutStream.synchronizeFile() return } let location = SourceLocationConverter(file: path, source: source).location(for: position) From 428b33ca81b6b41653762c48883738e0324883c3 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Thu, 2 Apr 2020 14:39:18 -0700 Subject: [PATCH 05/23] Fix `@differentiable` attribute formatting. - Don't insert a space before `where` if it's the first thing in the attribute. - Apply formatting to comma-delimited parameter "tuples" following the `wrt:` clause. Fixes SR-12414. --- .../TokenStreamCreator.swift | 28 ++++++++- .../DifferentiationAttributeTests.swift | 62 +++++++++++++++++++ .../XCTestManifests.swift | 2 + 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index 9dd23752c..14d45e94b 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -2020,16 +2020,27 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { override func visit(_ node: DifferentiableAttributeArgumentsSyntax) -> SyntaxVisitorContinueKind { // This node encapsulates the entire list of arguments in a `@differentiable(...)` attribute. + after(node.diffParamsComma, tokens: .break(.same)) + + var needsBreakBeforeWhereClause = false + if let vjp = node.maybeVJP { - before(vjp.firstToken, tokens: .break(.same), .open) + before(vjp.firstToken, tokens: .open) after(vjp.lastToken, tokens: .close) + after(vjp.trailingComma, tokens: .break(.same)) + needsBreakBeforeWhereClause = true } if let jvp = node.maybeJVP { - before(jvp.firstToken, tokens: .break(.same), .open) + before(jvp.firstToken, tokens: .open) after(jvp.lastToken, tokens: .close) + after(jvp.trailingComma, tokens: .break(.same)) + needsBreakBeforeWhereClause = true } if let whereClause = node.whereClause { - before(whereClause.firstToken, tokens: .break(.same), .open) + if needsBreakBeforeWhereClause { + before(whereClause.firstToken, tokens: .break(.same)) + } + before(whereClause.firstToken, tokens: .open) after(whereClause.lastToken, tokens: .close) } return .visitChildren @@ -2044,6 +2055,17 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { return .visitChildren } + override func visit(_ node: DifferentiationParamsSyntax) -> SyntaxVisitorContinueKind { + after(node.leftParen, tokens: .break(.open, size: 0), .open) + before(node.rightParen, tokens: .break(.close, size: 0), .close) + return .visitChildren + } + + override func visit(_ node: DifferentiationParamSyntax) -> SyntaxVisitorContinueKind { + after(node.trailingComma, tokens: .break(.same)) + return .visitChildren + } + // `DerivativeRegistrationAttributeArguments` was added after the Swift 5.2 release was cut. #if HAS_DERIVATIVE_REGISTRATION_ATTRIBUTE override func visit(_ node: DerivativeRegistrationAttributeArgumentsSyntax) diff --git a/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift b/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift index 3d5440733..bf33ca825 100644 --- a/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift @@ -43,6 +43,68 @@ final class DifferentiationAttributeTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 43) } + func testDifferentiableWithOnlyWhereClause() { + let input = + """ + @differentiable(where T: D) + func foo(_ x: T) -> T {} + + @differentiable(where T: Differentiable) + func foo(_ x: T) -> T {} + """ + + let expected = + """ + @differentiable(where T: D) + func foo(_ x: T) -> T {} + + @differentiable( + where T: Differentiable + ) + func foo(_ x: T) -> T {} + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 28) + } + + func testDifferentiableWithMultipleParameters() { + let input = + """ + @differentiable(wrt: (x, y)) + func foo(_ x: T) -> T {} + + @differentiable(wrt: (self, x, y)) + func foo(_ x: T) -> T {} + + @differentiable(wrt: (theVariableNamedSelf, theVariableNamedX, theVariableNamedY)) + func foo(_ x: T) -> T {} + """ + + let expected = + """ + @differentiable(wrt: (x, y)) + func foo(_ x: T) -> T {} + + @differentiable( + wrt: (self, x, y) + ) + func foo(_ x: T) -> T {} + + @differentiable( + wrt: ( + theVariableNamedSelf, + theVariableNamedX, + theVariableNamedY + ) + ) + func foo(_ x: T) -> T {} + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 28) + } + func testDerivative() { #if HAS_DERIVATIVE_REGISTRATION_ATTRIBUTE let input = diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index 173048dc8..2735b5571 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -209,6 +209,8 @@ extension DifferentiationAttributeTests { static let __allTests__DifferentiationAttributeTests = [ ("testDerivative", testDerivative), ("testDifferentiable", testDifferentiable), + ("testDifferentiableWithMultipleParameters", testDifferentiableWithMultipleParameters), + ("testDifferentiableWithOnlyWhereClause", testDifferentiableWithOnlyWhereClause), ("testTranspose", testTranspose), ] } From c7db9060d091cad4800ca51bcefb0744f35864b4 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Thu, 2 Apr 2020 15:35:43 -0700 Subject: [PATCH 06/23] Add spaces after labels for overlooked stmt types. Some statement types that allow a label were lacking the space token after the label. This caused the formatter to remove spaces when it encountered those stmt types. --- .../TokenStreamCreator.swift | 4 ++ .../DoStmtTests.swift | 61 +++++++++++++++++++ .../IfStmtTests.swift | 27 ++++++++ .../RepeatStmtTests.swift | 27 ++++++++ .../SwitchStmtTests.swift | 38 ++++++++++++ .../XCTestManifests.swift | 14 +++++ 6 files changed, 171 insertions(+) create mode 100644 Tests/SwiftFormatPrettyPrintTests/DoStmtTests.swift diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index 14d45e94b..a5ff7f828 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -444,6 +444,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { // MARK: - Control flow statement nodes override func visit(_ node: IfStmtSyntax) -> SyntaxVisitorContinueKind { + after(node.labelColon, tokens: .space) after(node.ifKeyword, tokens: .space) // Add break groups, using open continuation breaks, around any conditions after the first so @@ -531,6 +532,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { } override func visit(_ node: RepeatWhileStmtSyntax) -> SyntaxVisitorContinueKind { + after(node.labelColon, tokens: .space) arrangeBracesAndContents(of: node.body, contentsKeyPath: \.statements) if config.lineBreakBeforeControlFlowKeywords { @@ -550,6 +552,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { } override func visit(_ node: DoStmtSyntax) -> SyntaxVisitorContinueKind { + after(node.labelColon, tokens: .space) arrangeBracesAndContents(of: node.body, contentsKeyPath: \.statements) return .visitChildren } @@ -591,6 +594,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { } override func visit(_ node: SwitchStmtSyntax) -> SyntaxVisitorContinueKind { + after(node.labelColon, tokens: .space) before(node.switchKeyword, tokens: .open) after(node.switchKeyword, tokens: .space) before(node.leftBrace, tokens: .break(.reset)) diff --git a/Tests/SwiftFormatPrettyPrintTests/DoStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/DoStmtTests.swift new file mode 100644 index 000000000..f3c458869 --- /dev/null +++ b/Tests/SwiftFormatPrettyPrintTests/DoStmtTests.swift @@ -0,0 +1,61 @@ +import SwiftFormatConfiguration + +final class DoStmtTests: PrettyPrintTestCase { + func testBasicDoStmt() { + let input = + """ + do {} + do { f() } + do { foo() } + do { let a = 123 + var b = "abc" + } + do { veryLongFunctionCallThatShouldBeBrokenOntoANewLine() } + """ + + let expected = + """ + do {} + do { f() } + do { foo() } + do { + let a = 123 + var b = "abc" + } + do { + veryLongFunctionCallThatShouldBeBrokenOntoANewLine() + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 25) + } + + func testLabeledDoStmt() { + let input = """ + someLabel:do { + bar() + baz() + } + somePrettyLongLabelThatTakesUpManyColumns: do { + bar() + baz() + } + """ + + let expected = """ + someLabel: do { + bar() + baz() + } + somePrettyLongLabelThatTakesUpManyColumns: do + { + bar() + baz() + } + + """ + assertPrettyPrintEqual(input: input, expected: expected, linelength: 45) + } +} + diff --git a/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift index 3caac6009..d59010de4 100644 --- a/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift @@ -488,4 +488,31 @@ final class IfStmtTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 50) } + + func testLabeledIfStmt() { + let input = + """ + someLabel:if foo && bar { + // do something + } + anotherVeryLongLabelThatTakesUpTooManyCharacters: if foo && bar { + // do something else + } + """ + + let expected = + """ + someLabel: if foo && bar { + // do something + } + anotherVeryLongLabelThatTakesUpTooManyCharacters: if foo + && bar + { + // do something else + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 50) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/RepeatStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/RepeatStmtTests.swift index 787d69e1b..474083baf 100644 --- a/Tests/SwiftFormatPrettyPrintTests/RepeatStmtTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/RepeatStmtTests.swift @@ -125,4 +125,31 @@ final class RepeatStmtTests: PrettyPrintTestCase { """ assertPrettyPrintEqual(input: input, expected: input + "\n", linelength: 45) } + + func testLabeledRepeat() { + let input = """ + someLabel:repeat { + bar() + baz() + } while condition + somePrettyLongLabelThatTakesUpManyColumns: repeat { + bar() + baz() + } while condition + """ + + let expected = """ + someLabel: repeat { + bar() + baz() + } while condition + somePrettyLongLabelThatTakesUpManyColumns: repeat + { + bar() + baz() + } while condition + + """ + assertPrettyPrintEqual(input: input, expected: expected, linelength: 45) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift index 2043bae93..33b70a886 100644 --- a/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift @@ -288,4 +288,42 @@ final class SwitchStmtTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 40) } + + func testLabeledSwitchStmt() { + let input = + """ + label:switch foo { + case bar: + callForBar() + case baz: + callForBaz() + } + someVeryExtremelyLongLabel: switch foo { + case bar: + callForBar() + case baz: + callForBaz() + } + """ + + let expected = + """ + label: switch foo { + case bar: + callForBar() + case baz: + callForBaz() + } + someVeryExtremelyLongLabel: switch foo + { + case bar: + callForBar() + case baz: + callForBaz() + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 20) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index 2735b5571..025ee9384 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -215,6 +215,16 @@ extension DifferentiationAttributeTests { ] } +extension DoStmtTests { + // DO NOT MODIFY: This is autogenerated, use: + // `swift test --generate-linuxmain` + // to regenerate. + static let __allTests__DoStmtTests = [ + ("testBasicDoStmt", testBasicDoStmt), + ("testLabeledDoStmt", testLabeledDoStmt), + ] +} + extension EnumDeclTests { // DO NOT MODIFY: This is autogenerated, use: // `swift test --generate-linuxmain` @@ -396,6 +406,7 @@ extension IfStmtTests { ("testIfElseStatement_noBreakBeforeElse", testIfElseStatement_noBreakBeforeElse), ("testIfLetStatements", testIfLetStatements), ("testIfStatement", testIfStatement), + ("testLabeledIfStmt", testLabeledIfStmt), ("testMatchingPatternConditions", testMatchingPatternConditions), ("testOptionalBindingConditions", testOptionalBindingConditions), ("testParenthesizedClauses", testParenthesizedClauses), @@ -557,6 +568,7 @@ extension RepeatStmtTests { static let __allTests__RepeatStmtTests = [ ("testBasicRepeatTests_breakBeforeWhile", testBasicRepeatTests_breakBeforeWhile), ("testBasicRepeatTests_noBreakBeforeWhile", testBasicRepeatTests_noBreakBeforeWhile), + ("testLabeledRepeat", testLabeledRepeat), ("testNestedRepeat", testNestedRepeat), ] } @@ -693,6 +705,7 @@ extension SwitchStmtTests { // to regenerate. static let __allTests__SwitchStmtTests = [ ("testBasicSwitch", testBasicSwitch), + ("testLabeledSwitchStmt", testLabeledSwitchStmt), ("testNestedSwitch", testNestedSwitch), ("testNewlinesDisambiguatingWhereClauses", testNewlinesDisambiguatingWhereClauses), ("testSwitchCases", testSwitchCases), @@ -816,6 +829,7 @@ public func __allTests() -> [XCTestCaseEntry] { testCase(DeinitializerDeclTests.__allTests__DeinitializerDeclTests), testCase(DictionaryDeclTests.__allTests__DictionaryDeclTests), testCase(DifferentiationAttributeTests.__allTests__DifferentiationAttributeTests), + testCase(DoStmtTests.__allTests__DoStmtTests), testCase(EnumDeclTests.__allTests__EnumDeclTests), testCase(ExtensionDeclTests.__allTests__ExtensionDeclTests), testCase(ForInStmtTests.__allTests__ForInStmtTests), From a50d4fe0cfb9e215d8fe3df3378f14d6b396a024 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Thu, 2 Apr 2020 16:04:07 -0700 Subject: [PATCH 07/23] Allow right paren of enum case decl to be on the same line as the last param. When enum cases have associated values, these are similar to function params. The right paren is allowed to stay on the same line for function params, and this makes enum cases consistent. The break allows discretionary line breaks. --- Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift | 2 +- Tests/SwiftFormatPrettyPrintTests/EnumDeclTests.swift | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index a5ff7f828..c73fc622e 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -1190,7 +1190,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { after(node.trailingComma, tokens: .break) if let associatedValue = node.associatedValue { - arrangeParameterClause(associatedValue, forcesBreakBeforeRightParen: true) + arrangeParameterClause(associatedValue, forcesBreakBeforeRightParen: false) } return .visitChildren diff --git a/Tests/SwiftFormatPrettyPrintTests/EnumDeclTests.swift b/Tests/SwiftFormatPrettyPrintTests/EnumDeclTests.swift index 67ef04cae..bf015c132 100644 --- a/Tests/SwiftFormatPrettyPrintTests/EnumDeclTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/EnumDeclTests.swift @@ -115,15 +115,14 @@ final class EnumDeclTests: PrettyPrintTestCase { fifth case sixth(Int) case seventh( - a: Int, b: Bool, c: Double - ) + a: Int, b: Bool, c: Double) } """ var config = Configuration() config.lineBreakBeforeEachArgument = false - assertPrettyPrintEqual(input: input, expected: expected, linelength: 30, configuration: config) + assertPrettyPrintEqual(input: input, expected: expected, linelength: 31, configuration: config) } func testIndirectEnum() { From ff7ac9a7eecbd77e18f2b81ae4a252b536bee75e Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Fri, 3 Apr 2020 15:48:32 -0700 Subject: [PATCH 08/23] Various fixes to `ReturnVoidInsteadOfEmptyTuple`. - Handle `()` in closure signature return types. - Handle `()` when nested in the argument lists of function types and closures. - Ensure leading/trailing trivia of the original `()` is preserved. - Don't replace `()` if it has internal comments, like `(/*foo*/)`. --- .../ReturnVoidInsteadOfEmptyTuple.swift | 78 +++++++- .../ReturnVoidInsteadOfEmptyTupleTests.swift | 167 ++++++++++++++++-- .../XCTestManifests.swift | 6 +- 3 files changed, 232 insertions(+), 19 deletions(-) diff --git a/Sources/SwiftFormatRules/ReturnVoidInsteadOfEmptyTuple.swift b/Sources/SwiftFormatRules/ReturnVoidInsteadOfEmptyTuple.swift index 90f98bad3..e6c53447e 100644 --- a/Sources/SwiftFormatRules/ReturnVoidInsteadOfEmptyTuple.swift +++ b/Sources/SwiftFormatRules/ReturnVoidInsteadOfEmptyTuple.swift @@ -25,14 +25,82 @@ public final class ReturnVoidInsteadOfEmptyTuple: SyntaxFormatRule { public override func visit(_ node: FunctionTypeSyntax) -> TypeSyntax { guard let returnType = node.returnType.as(TupleTypeSyntax.self), returnType.elements.count == 0 - else { return TypeSyntax(node) } - diagnose(.returnVoid, on: node.returnType) - let voidKeyword = SyntaxFactory.makeSimpleTypeIdentifier( + else { + return super.visit(node) + } + + diagnose(.returnVoid, on: returnType) + + // If the user has put non-whitespace trivia inside the empty tuple, like a comment, then we + // still diagnose it as a lint error but we don't replace it because it's not obvious where the + // comment should go. + if hasNonWhitespaceLeadingTrivia(returnType.rightParen) { + return super.visit(node) + } + + // Make sure that function types nested in the argument list are also rewritten (for example, + // `(Int -> ()) -> ()` should become `(Int -> Void) -> Void`). + let arguments = visit(node.arguments).as(TupleTypeElementListSyntax.self)! + let voidKeyword = makeVoidIdentifierType(toReplace: returnType) + return TypeSyntax(node.withArguments(arguments).withReturnType(TypeSyntax(voidKeyword))) + } + + public override func visit(_ node: ClosureSignatureSyntax) -> Syntax { + guard let output = node.output, + let returnType = output.returnType.as(TupleTypeSyntax.self), + returnType.elements.count == 0 + else { + return super.visit(node) + } + + diagnose(.returnVoid, on: returnType) + + // If the user has put non-whitespace trivia inside the empty tuple, like a comment, then we + // still diagnose it as a lint error but we don't replace it because it's not obvious where the + // comment should go. + if hasNonWhitespaceLeadingTrivia(returnType.rightParen) { + return super.visit(node) + } + + let input: Syntax? + if let parameterClause = node.input?.as(ParameterClauseSyntax.self) { + // If the closure input is a complete parameter clause (variables and types), make sure that + // nested function types are also rewritten (for example, `label: (Int -> ()) -> ()` should + // become `label: (Int -> Void) -> Void`). + input = visit(parameterClause) + } else { + // Otherwise, it's a simple signature (just variable names, no types), so there is nothing to + // rewrite. + input = node.input + } + let voidKeyword = makeVoidIdentifierType(toReplace: returnType) + return Syntax(node.withInput(input).withOutput(output.withReturnType(TypeSyntax(voidKeyword)))) + } + + /// Returns a value indicating whether the leading trivia of the given token contained any + /// non-whitespace pieces. + private func hasNonWhitespaceLeadingTrivia(_ token: TokenSyntax) -> Bool { + for piece in token.leadingTrivia { + switch piece { + case .blockComment, .docBlockComment, .docLineComment, .garbageText, .lineComment: + return true + default: + break + } + } + return false + } + + /// Returns a type syntax node with the identifier `Void` whose leading and trailing trivia have + /// been copied from the tuple type syntax node it is replacing. + private func makeVoidIdentifierType(toReplace node: TupleTypeSyntax) -> SimpleTypeIdentifierSyntax + { + return SyntaxFactory.makeSimpleTypeIdentifier( name: SyntaxFactory.makeIdentifier( "Void", - trailingTrivia: returnType.rightParen.trailingTrivia), + leadingTrivia: node.firstToken?.leadingTrivia ?? [], + trailingTrivia: node.lastToken?.trailingTrivia ?? []), genericArgumentClause: nil) - return TypeSyntax(node.withReturnType(TypeSyntax(voidKeyword))) } } diff --git a/Tests/SwiftFormatRulesTests/ReturnVoidInsteadOfEmptyTupleTests.swift b/Tests/SwiftFormatRulesTests/ReturnVoidInsteadOfEmptyTupleTests.swift index bbd7d8af3..2a78268c1 100644 --- a/Tests/SwiftFormatRulesTests/ReturnVoidInsteadOfEmptyTupleTests.swift +++ b/Tests/SwiftFormatRulesTests/ReturnVoidInsteadOfEmptyTupleTests.swift @@ -1,20 +1,161 @@ import SwiftFormatRules final class ReturnVoidInsteadOfEmptyTupleTests: LintOrFormatRuleTestCase { - func testEmptyTupleReturns() { + func testBasic() { XCTAssertFormatting( ReturnVoidInsteadOfEmptyTuple.self, - input: """ - let callback: () -> () - typealias x = Int -> () - func y() -> Int -> () { return } - func z(d: Bool -> ()) {} - """, - expected: """ - let callback: () -> Void - typealias x = Int -> Void - func y() -> Int -> Void { return } - func z(d: Bool -> Void) {} - """) + input: + """ + let callback: () -> () + typealias x = Int -> () + func y() -> Int -> () { return } + func z(d: Bool -> ()) {} + """, + expected: + """ + let callback: () -> Void + typealias x = Int -> Void + func y() -> Int -> Void { return } + func z(d: Bool -> Void) {} + """, + checkForUnassertedDiagnostics: true + ) + XCTAssertDiagnosed(.returnVoid, line: 1, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 2, column: 22) + XCTAssertDiagnosed(.returnVoid, line: 3, column: 20) + XCTAssertDiagnosed(.returnVoid, line: 4, column: 19) + } + + func testNestedFunctionTypes() { + XCTAssertFormatting( + ReturnVoidInsteadOfEmptyTuple.self, + input: + """ + typealias Nested1 = (() -> ()) -> Int + typealias Nested2 = (() -> ()) -> () + typealias Nested3 = Int -> (() -> ()) + """, + expected: + """ + typealias Nested1 = (() -> Void) -> Int + typealias Nested2 = (() -> Void) -> Void + typealias Nested3 = Int -> (() -> Void) + """, + checkForUnassertedDiagnostics: true + ) + XCTAssertDiagnosed(.returnVoid, line: 1, column: 28) + XCTAssertDiagnosed(.returnVoid, line: 2, column: 28) + XCTAssertDiagnosed(.returnVoid, line: 2, column: 35) + XCTAssertDiagnosed(.returnVoid, line: 3, column: 35) + } + + func testClosureSignatures() { + XCTAssertFormatting( + ReturnVoidInsteadOfEmptyTuple.self, + input: + """ + callWithTrailingClosure(arg) { arg -> () in body } + callWithTrailingClosure(arg) { arg -> () in + nestedCallWithTrailingClosure(arg) { arg -> () in + body + } + } + callWithTrailingClosure(arg) { (arg: () -> ()) -> Int in body } + callWithTrailingClosure(arg) { (arg: () -> ()) -> () in body } + """, + expected: + """ + callWithTrailingClosure(arg) { arg -> Void in body } + callWithTrailingClosure(arg) { arg -> Void in + nestedCallWithTrailingClosure(arg) { arg -> Void in + body + } + } + callWithTrailingClosure(arg) { (arg: () -> Void) -> Int in body } + callWithTrailingClosure(arg) { (arg: () -> Void) -> Void in body } + """, + checkForUnassertedDiagnostics: true + ) + XCTAssertDiagnosed(.returnVoid, line: 1, column: 39) + XCTAssertDiagnosed(.returnVoid, line: 2, column: 39) + XCTAssertDiagnosed(.returnVoid, line: 3, column: 47) + XCTAssertDiagnosed(.returnVoid, line: 7, column: 44) + XCTAssertDiagnosed(.returnVoid, line: 8, column: 44) + XCTAssertDiagnosed(.returnVoid, line: 8, column: 51) + } + + func testTriviaPreservation() { + XCTAssertFormatting( + ReturnVoidInsteadOfEmptyTuple.self, + input: + """ + let callback: () -> /*foo*/()/*bar*/ + let callback: (Int -> /*foo*/ () /*bar*/) -> () + """, + expected: + """ + let callback: () -> /*foo*/Void/*bar*/ + let callback: (Int -> /*foo*/ Void /*bar*/) -> Void + """, + checkForUnassertedDiagnostics: true + ) + XCTAssertDiagnosed(.returnVoid, line: 1, column: 28) + XCTAssertDiagnosed(.returnVoid, line: 2, column: 35) + XCTAssertDiagnosed(.returnVoid, line: 2, column: 52) + } + + func testEmptyTupleWithInternalCommentsIsDiagnosedButNotReplaced() { + XCTAssertFormatting( + ReturnVoidInsteadOfEmptyTuple.self, + input: + """ + let callback: () -> ( ) + let callback: () -> (\t) + let callback: () -> ( + ) + let callback: () -> ( /* please don't change me! */ ) + let callback: () -> ( /** please don't change me! */ ) + let callback: () -> ( + // don't change me either! + ) + let callback: () -> ( + /// don't change me either! + ) + let callback: () -> (\u{feff}) + + let callback: (() -> ()) -> ( /* please don't change me! */ ) + callWithTrailingClosure(arg) { (arg: () -> ()) -> ( /* no change */ ) in body } + """, + expected: + """ + let callback: () -> Void + let callback: () -> Void + let callback: () -> Void + let callback: () -> ( /* please don't change me! */ ) + let callback: () -> ( /** please don't change me! */ ) + let callback: () -> ( + // don't change me either! + ) + let callback: () -> ( + /// don't change me either! + ) + let callback: () -> (\u{feff}) + + let callback: (() -> Void) -> ( /* please don't change me! */ ) + callWithTrailingClosure(arg) { (arg: () -> Void) -> ( /* no change */ ) in body } + """, + checkForUnassertedDiagnostics: true + ) + XCTAssertDiagnosed(.returnVoid, line: 1, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 2, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 3, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 5, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 6, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 7, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 10, column: 21) + XCTAssertDiagnosed(.returnVoid, line: 15, column: 22) + XCTAssertDiagnosed(.returnVoid, line: 15, column: 29) + XCTAssertDiagnosed(.returnVoid, line: 16, column: 44) + XCTAssertDiagnosed(.returnVoid, line: 16, column: 51) } } diff --git a/Tests/SwiftFormatRulesTests/XCTestManifests.swift b/Tests/SwiftFormatRulesTests/XCTestManifests.swift index 7c57d651a..76939f230 100644 --- a/Tests/SwiftFormatRulesTests/XCTestManifests.swift +++ b/Tests/SwiftFormatRulesTests/XCTestManifests.swift @@ -290,7 +290,11 @@ extension ReturnVoidInsteadOfEmptyTupleTests { // `swift test --generate-linuxmain` // to regenerate. static let __allTests__ReturnVoidInsteadOfEmptyTupleTests = [ - ("testEmptyTupleReturns", testEmptyTupleReturns), + ("testBasic", testBasic), + ("testClosureSignatures", testClosureSignatures), + ("testEmptyTupleWithInternalCommentsIsDiagnosedButNotReplaced", testEmptyTupleWithInternalCommentsIsDiagnosedButNotReplaced), + ("testNestedFunctionTypes", testNestedFunctionTypes), + ("testTriviaPreservation", testTriviaPreservation), ] } From 400591f27d32c3e2ab994644948066257f317a00 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Wed, 1 Apr 2020 16:43:51 -0700 Subject: [PATCH 09/23] Refactor main executable into a frontend architecture. Instead of a bunch of free functions that pass their arguments around, this change now refactors the core format and lint file handling functionality into a set of "frontend" classes so that the diagnostic engine and related state can be created in one place and shared. This also significantly improves error handling. Whereas we were previously killing the process on the first sign of a source file or configuration file being invalid, we now emit an error and skip the file, allowing any remaining inputs to still be processed. Lastly, this introduces a configuration cache so that we're not re-reading the configuration from the file system for every source file we process. In most cases, when we're processing a recursive directory structure, they'll all share a configuration file at a common root, so we cache that based on its file URL and return it when requested. --- .../Frontend/ConfigurationLoader.swift | 56 +++++ .../Frontend/FormatFrontend.swift | 78 +++++++ Sources/swift-format/Frontend/Frontend.swift | 202 ++++++++++++++++++ .../swift-format/Frontend/LintFrontend.swift | 52 +++++ Sources/swift-format/Subcommands/Format.swift | 96 +-------- .../swift-format/Subcommands/LegacyMain.swift | 5 - Sources/swift-format/Subcommands/Lint.swift | 73 +------ .../Subcommands/LintFormatOptions.swift | 10 - Sources/swift-format/Utilities/Helpers.swift | 92 -------- 9 files changed, 394 insertions(+), 270 deletions(-) create mode 100644 Sources/swift-format/Frontend/ConfigurationLoader.swift create mode 100644 Sources/swift-format/Frontend/FormatFrontend.swift create mode 100644 Sources/swift-format/Frontend/Frontend.swift create mode 100644 Sources/swift-format/Frontend/LintFrontend.swift delete mode 100644 Sources/swift-format/Utilities/Helpers.swift diff --git a/Sources/swift-format/Frontend/ConfigurationLoader.swift b/Sources/swift-format/Frontend/ConfigurationLoader.swift new file mode 100644 index 000000000..473ec4254 --- /dev/null +++ b/Sources/swift-format/Frontend/ConfigurationLoader.swift @@ -0,0 +1,56 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import Foundation +import SwiftFormatConfiguration + +/// Loads formatter configurations, caching them in memory so that multiple operations in the same +/// directory do not repeatedly hit the file system. +struct ConfigurationLoader { + /// A mapping from configuration file URLs to the loaded configuration data. + private var cache = [URL: Configuration]() + + /// Returns the configuration associated with the configuration file at the given path. + /// + /// - Throws: If an error occurred loading the configuration. + mutating func configuration(atPath path: String) throws -> Configuration { + return try configuration(at: URL(fileURLWithPath: path)) + } + + /// Returns the configuration found by searching in the directory (and ancestor directories) + /// containing the given `.swift` source file. + /// + /// If no configuration file was found during the search, this method returns nil. + /// + /// - Throws: If a configuration file was found but an error occurred loading it. + mutating func configuration(forSwiftFileAtPath path: String) throws -> Configuration? { + let swiftFileURL = URL(fileURLWithPath: path) + guard let configurationFileURL = Configuration.url(forConfigurationFileApplyingTo: swiftFileURL) + else { + return nil + } + return try configuration(at: configurationFileURL) + } + + /// Returns the configuration associated with the configuration file at the given URL. + /// + /// - Throws: If an error occurred loading the configuration. + private mutating func configuration(at url: URL) throws -> Configuration { + if let cachedConfiguration = cache[url] { + return cachedConfiguration + } + + let configuration = try Configuration(contentsOf: url) + cache[url] = configuration + return configuration + } +} diff --git a/Sources/swift-format/Frontend/FormatFrontend.swift b/Sources/swift-format/Frontend/FormatFrontend.swift new file mode 100644 index 000000000..9a16f0f81 --- /dev/null +++ b/Sources/swift-format/Frontend/FormatFrontend.swift @@ -0,0 +1,78 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import Foundation +import SwiftFormat +import SwiftFormatConfiguration +import SwiftSyntax + +/// The frontend for formatting operations. +class FormatFrontend: Frontend { + /// Whether or not to format the Swift file in-place. + private let inPlace: Bool + + init(lintFormatOptions: LintFormatOptions, inPlace: Bool) { + self.inPlace = inPlace + super.init(lintFormatOptions: lintFormatOptions) + } + + override func processFile(_ fileToProcess: FileToProcess) { + // Even though `diagnosticEngine` is defined, it's use is reserved for fatal messages. Pass nil + // to the formatter to suppress other messages since they will be fixed or can't be + // automatically fixed anyway. + let formatter = SwiftFormatter( + configuration: fileToProcess.configuration, diagnosticEngine: nil) + formatter.debugOptions = debugOptions + + let path = fileToProcess.path + guard let source = fileToProcess.sourceText else { + diagnosticEngine.diagnose( + Diagnostic.Message(.error, "Unable to read source for formatting from \(path).")) + return + } + + var stdoutStream = FileHandle.standardOutput + do { + let assumingFileURL = URL(fileURLWithPath: path) + if inPlace { + var buffer = "" + try formatter.format(source: source, assumingFileURL: assumingFileURL, to: &buffer) + + let bufferData = buffer.data(using: .utf8)! // Conversion to UTF-8 cannot fail + try bufferData.write(to: assumingFileURL, options: .atomic) + } else { + try formatter.format(source: source, assumingFileURL: assumingFileURL, to: &stdoutStream) + } + } catch SwiftFormatError.fileNotReadable { + diagnosticEngine.diagnose( + Diagnostic.Message( + .error, "Unable to format \(path): file is not readable or does not exist.")) + return + } catch SwiftFormatError.fileContainsInvalidSyntax(let position) { + guard !lintFormatOptions.ignoreUnparsableFiles else { + guard !inPlace else { + // For in-place mode, nothing is expected to stdout and the file shouldn't be modified. + return + } + stdoutStream.write(source) + return + } + let location = SourceLocationConverter(file: path, source: source).location(for: position) + diagnosticEngine.diagnose( + Diagnostic.Message(.error, "file contains invalid or unrecognized Swift syntax."), + location: location) + return + } catch { + diagnosticEngine.diagnose(Diagnostic.Message(.error, "Unable to format \(path): \(error)")) + } + } +} diff --git a/Sources/swift-format/Frontend/Frontend.swift b/Sources/swift-format/Frontend/Frontend.swift new file mode 100644 index 000000000..26f085be7 --- /dev/null +++ b/Sources/swift-format/Frontend/Frontend.swift @@ -0,0 +1,202 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import Foundation +import SwiftFormat +import SwiftFormatConfiguration +import SwiftSyntax + +class Frontend { + /// Represents a file to be processed by the frontend and any file-specific options associated + /// with it. + final class FileToProcess { + /// An open file handle to the source code of the file. + private let fileHandle: FileHandle + + /// The path to the source file being processed. + /// + /// It is the responsibility of the specific frontend to make guarantees about the validity of + /// this path. For example, the formatting frontend ensures that it is a path to an existing + /// file only when doing in-place formatting (so that the file can be replaced). In other + /// situations, it may correspond to a different file than the underlying file handle (if + /// standard input is used with the `--assume-filename` flag), or it may not be a valid path at + /// all (the string `""`). + let path: String + + /// The configuration that should applied for this file. + let configuration: Configuration + + /// Returns the string contents of the file. + /// + /// The contents of the file are assumed to be UTF-8 encoded. If there is an error decoding the + /// contents, `nil` will be returned. + lazy var sourceText: String? = { + let sourceData = fileHandle.readDataToEndOfFile() + defer { fileHandle.closeFile() } + return String(data: sourceData, encoding: .utf8) + }() + + init(fileHandle: FileHandle, path: String, configuration: Configuration) { + self.fileHandle = fileHandle + self.path = path + self.configuration = configuration + } + } + + /// The diagnostic engine to which warnings and errors will be emitted. + final let diagnosticEngine: DiagnosticEngine = { + let engine = DiagnosticEngine() + let consumer = PrintingDiagnosticConsumer() + engine.addConsumer(consumer) + return engine + }() + + /// Options that apply during formatting or linting. + final let lintFormatOptions: LintFormatOptions + + /// Loads formatter configuration files. + final var configurationLoader = ConfigurationLoader() + + /// Advanced options that are useful for developing/debugging but otherwise not meant for general + /// use. + final var debugOptions: DebugOptions { + [ + lintFormatOptions.debugDisablePrettyPrint ? .disablePrettyPrint : [], + lintFormatOptions.debugDumpTokenStream ? .dumpTokenStream : [], + ] + } + + /// Indicates whether any errors were emitted during execution. + final var errorsWereEmitted: Bool { diagnosticEngine.hasErrors } + + /// Creates a new frontend with the given options. + /// + /// - Parameter lintFormatOptions: Options that apply during formatting or linting. + init(lintFormatOptions: LintFormatOptions) { + self.lintFormatOptions = lintFormatOptions + } + + /// Runs the linter or formatter over the inputs. + final func run() { + let paths = lintFormatOptions.paths + + if paths.isEmpty { + processStandardInput() + } else { + processPaths(paths) + } + } + + /// Called by the frontend to process a single file. + /// + /// Subclasses must override this method to provide the actual linting or formatting logic. + /// + /// - Parameter fileToProcess: A `FileToProcess` that contains information about the file to be + /// processed. + func processFile(_ fileToProcess: FileToProcess) { + fatalError("Must be overridden by subclasses.") + } + + /// Processes source content from standard input. + private func processStandardInput() { + guard let configuration = configuration( + atPath: lintFormatOptions.configurationPath, + orInferredFromSwiftFileAtPath: nil) + else { + // Already diagnosed in the called method. + return + } + + let fileToProcess = FileToProcess( + fileHandle: FileHandle.standardInput, + path: lintFormatOptions.assumeFilename ?? "", + configuration: configuration) + processFile(fileToProcess) + } + + /// Processes source content from a list of files and/or directories provided as paths. + private func processPaths(_ paths: [String]) { + precondition( + !paths.isEmpty, + "processPaths(_:) should only be called when paths is non-empty.") + + for path in FileIterator(paths: paths) { + guard let sourceFile = FileHandle(forReadingAtPath: path) else { + diagnosticEngine.diagnose(Diagnostic.Message(.error, "Unable to open \(path)")) + continue + } + + guard let configuration = configuration( + atPath: lintFormatOptions.configurationPath, orInferredFromSwiftFileAtPath: path) + else { + // Already diagnosed in the called method. + continue + } + + let fileToProcess = FileToProcess( + fileHandle: sourceFile, path: path, configuration: configuration) + processFile(fileToProcess) + } + } + + /// Returns the configuration that applies to the given `.swift` source file, when an explicit + /// configuration path is also perhaps provided. + /// + /// - Parameters: + /// - configurationFilePath: The path to a configuration file that will be loaded, or `nil` to + /// try to infer it from `swiftFilePath`. + /// - swiftFilePath: The path to a `.swift` file, which will be used to infer the path to the + /// configuration file if `configurationFilePath` is nil. + /// - Returns: If successful, the returned configuration is the one loaded from + /// `configurationFilePath` if it was provided, or by searching in paths inferred by + /// `swiftFilePath` if one exists, or the default configuration otherwise. If an error occurred + /// when reading the configuration, a diagnostic is emitted and `nil` is returned. + private func configuration( + atPath configurationFilePath: String?, + orInferredFromSwiftFileAtPath swiftFilePath: String? + ) -> Configuration? { + // If an explicit configuration file path was given, try to load it and fail if it cannot be + // loaded. (Do not try to fall back to a path inferred from the source file path.) + if let configurationFilePath = configurationFilePath { + do { + return try configurationLoader.configuration(atPath: configurationFilePath) + } catch { + diagnosticEngine.diagnose( + Diagnostic.Message(.error, "Unable to read configuration: \(error.localizedDescription)")) + return nil + } + } + + // If no explicit configuration file path was given but a `.swift` source file path was given, + // then try to load the configuration by inferring it based on the source file path. + if let swiftFilePath = swiftFilePath { + do { + if let configuration = + try configurationLoader.configuration(forSwiftFileAtPath: swiftFilePath) + { + return configuration + } + // Fall through to the default return at the end of the function. + } catch { + diagnosticEngine.diagnose( + Diagnostic.Message(.error, "Unable to read configuration for \(swiftFilePath): " + + "\(error.localizedDescription)")) + return nil + } + } + + // If neither path was given (for example, formatting standard input with no assumed filename) + // or if there was no configuration found by inferring it from the source file path, return the + // default configuration. + return Configuration() + } +} diff --git a/Sources/swift-format/Frontend/LintFrontend.swift b/Sources/swift-format/Frontend/LintFrontend.swift new file mode 100644 index 000000000..95a7885e3 --- /dev/null +++ b/Sources/swift-format/Frontend/LintFrontend.swift @@ -0,0 +1,52 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import Foundation +import SwiftFormat +import SwiftFormatConfiguration +import SwiftSyntax + +/// The frontend for linting operations. +class LintFrontend: Frontend { + override func processFile(_ fileToProcess: FileToProcess) { + let linter = SwiftLinter( + configuration: fileToProcess.configuration, diagnosticEngine: diagnosticEngine) + linter.debugOptions = debugOptions + + let path = fileToProcess.path + guard let source = fileToProcess.sourceText else { + diagnosticEngine.diagnose( + Diagnostic.Message( + .error, "Unable to read source for linting from \(path).")) + return + } + + do { + let assumingFileURL = URL(fileURLWithPath: path) + try linter.lint(source: source, assumingFileURL: assumingFileURL) + } catch SwiftFormatError.fileNotReadable { + diagnosticEngine.diagnose( + Diagnostic.Message( + .error, "Unable to lint \(path): file is not readable or does not exist.")) + return + } catch SwiftFormatError.fileContainsInvalidSyntax(let position) { + let location = SourceLocationConverter(file: path, source: source).location(for: position) + diagnosticEngine.diagnose( + Diagnostic.Message(.error, "file contains invalid or unrecognized Swift syntax."), + location: location) + return + } catch { + diagnosticEngine.diagnose(Diagnostic.Message(.error, "Unable to lint \(path): \(error)")) + return + } + } +} diff --git a/Sources/swift-format/Subcommands/Format.swift b/Sources/swift-format/Subcommands/Format.swift index c9d0279bc..8dfbe4ee3 100644 --- a/Sources/swift-format/Subcommands/Format.swift +++ b/Sources/swift-format/Subcommands/Format.swift @@ -11,10 +11,6 @@ //===----------------------------------------------------------------------===// import ArgumentParser -import Foundation -import SwiftFormat -import SwiftFormatConfiguration -import SwiftSyntax extension SwiftFormatCommand { /// Formats one or more files containing Swift code. @@ -41,95 +37,9 @@ extension SwiftFormatCommand { } func run() throws { - let diagnosticEngine = makeDiagnosticEngine() - - if formatOptions.paths.isEmpty { - let configuration = try loadConfiguration( - forSwiftFile: nil, configFilePath: formatOptions.configurationPath) - formatMain( - configuration: configuration, sourceFile: FileHandle.standardInput, - assumingFilename: formatOptions.assumeFilename, inPlace: false, - ignoreUnparsableFiles: formatOptions.ignoreUnparsableFiles, - debugOptions: formatOptions.debugOptions, diagnosticEngine: diagnosticEngine) - } else { - try processSources( - from: formatOptions.paths, configurationPath: formatOptions.configurationPath, - diagnosticEngine: diagnosticEngine - ) { sourceFile, path, configuration in - formatMain( - configuration: configuration, sourceFile: sourceFile, assumingFilename: path, - inPlace: inPlace, ignoreUnparsableFiles: formatOptions.ignoreUnparsableFiles, - debugOptions: formatOptions.debugOptions, diagnosticEngine: diagnosticEngine) - } - } - - try failIfDiagnosticsEmitted(diagnosticEngine) - } - } -} - -/// Runs the formatting pipeline over the provided source file. -/// -/// - Parameters: -/// - configuration: The `Configuration` that contains user-specific settings. -/// - sourceFile: A file handle from which to read the source code to be linted. -/// - assumingFilename: The filename of the source file, used in diagnostic output. -/// - inPlace: Whether or not to overwrite the current file when formatting. -/// - ignoreUnparsableFiles: Whether or not to ignore files that contain syntax errors. -/// - debugOptions: The set containing any debug options that were supplied on the command line. -/// - diagnosticEngine: A diagnostic collector that handles diagnostic messages. -/// - Returns: Zero if there were no format errors, otherwise a non-zero number. -private func formatMain( - configuration: Configuration, sourceFile: FileHandle, assumingFilename: String?, inPlace: Bool, - ignoreUnparsableFiles: Bool, debugOptions: DebugOptions, diagnosticEngine: DiagnosticEngine -) { - // Even though `diagnosticEngine` is defined, it's use is reserved for fatal messages. Pass nil - // to the formatter to suppress other messages since they will be fixed or can't be automatically - // fixed anyway. - let formatter = SwiftFormatter(configuration: configuration, diagnosticEngine: nil) - formatter.debugOptions = debugOptions - - let path = assumingFilename ?? "" - let assumingFileURL = URL(fileURLWithPath: path) - - guard let source = readSource(from: sourceFile) else { - diagnosticEngine.diagnose( - Diagnostic.Message(.error, "Unable to read source for formatting from \(path).")) - return - } - - var stdoutStream = FileHandle.standardOutput - do { - if inPlace { - var buffer = "" - try formatter.format(source: source, assumingFileURL: assumingFileURL, to: &buffer) - - let bufferData = buffer.data(using: .utf8)! // Conversion to UTF-8 cannot fail - try bufferData.write(to: assumingFileURL, options: .atomic) - } else { - try formatter.format(source: source, assumingFileURL: assumingFileURL, to: &stdoutStream) - } - } catch SwiftFormatError.fileNotReadable { - diagnosticEngine.diagnose( - Diagnostic.Message( - .error, "Unable to format \(path): file is not readable or does not exist.")) - return - } catch SwiftFormatError.fileContainsInvalidSyntax(let position) { - guard !ignoreUnparsableFiles else { - guard !inPlace else { - // For in-place mode, nothing is expected to stdout and the file shouldn't be modified. - return - } - stdoutStream.write(source) - return + let frontend = FormatFrontend(lintFormatOptions: formatOptions, inPlace: inPlace) + frontend.run() + if frontend.errorsWereEmitted { throw ExitCode.failure } } - let location = SourceLocationConverter(file: path, source: source).location(for: position) - diagnosticEngine.diagnose( - Diagnostic.Message(.error, "file contains invalid or unrecognized Swift syntax."), - location: location) - return - } catch { - diagnosticEngine.diagnose(Diagnostic.Message(.error, "Unable to format \(path): \(error)")) - return } } diff --git a/Sources/swift-format/Subcommands/LegacyMain.swift b/Sources/swift-format/Subcommands/LegacyMain.swift index 2668a646f..393f20413 100644 --- a/Sources/swift-format/Subcommands/LegacyMain.swift +++ b/Sources/swift-format/Subcommands/LegacyMain.swift @@ -11,11 +11,6 @@ //===----------------------------------------------------------------------===// import ArgumentParser -import Foundation -import SwiftFormat -import SwiftFormatConfiguration -import SwiftFormatCore -import SwiftSyntax extension SwiftFormatCommand { /// Keep the legacy `-m/--mode` flag working temporarily when no other subcommand is specified. diff --git a/Sources/swift-format/Subcommands/Lint.swift b/Sources/swift-format/Subcommands/Lint.swift index e5f31adaf..187efa451 100644 --- a/Sources/swift-format/Subcommands/Lint.swift +++ b/Sources/swift-format/Subcommands/Lint.swift @@ -11,10 +11,6 @@ //===----------------------------------------------------------------------===// import ArgumentParser -import Foundation -import SwiftFormat -import SwiftFormatConfiguration -import SwiftSyntax extension SwiftFormatCommand { /// Emits style diagnostics for one or more files containing Swift code. @@ -27,72 +23,9 @@ extension SwiftFormatCommand { var lintOptions: LintFormatOptions func run() throws { - let diagnosticEngine = makeDiagnosticEngine() - - if lintOptions.paths.isEmpty { - let configuration = try loadConfiguration( - forSwiftFile: nil, configFilePath: lintOptions.configurationPath) - lintMain( - configuration: configuration, sourceFile: FileHandle.standardInput, - assumingFilename: lintOptions.assumeFilename, debugOptions: lintOptions.debugOptions, - diagnosticEngine: diagnosticEngine) - } else { - try processSources( - from: lintOptions.paths, configurationPath: lintOptions.configurationPath, - diagnosticEngine: diagnosticEngine - ) { sourceFile, path, configuration in - lintMain( - configuration: configuration, sourceFile: sourceFile, assumingFilename: path, - debugOptions: lintOptions.debugOptions, diagnosticEngine: diagnosticEngine) - } - } - - try failIfDiagnosticsEmitted(diagnosticEngine) + let frontend = LintFrontend(lintFormatOptions: lintOptions) + frontend.run() + if frontend.errorsWereEmitted { throw ExitCode.failure } } } } - -/// Runs the linting pipeline over the provided source file. -/// -/// If there were any lint diagnostics emitted, this function returns a non-zero exit code. -/// - Parameters: -/// - configuration: The `Configuration` that contains user-specific settings. -/// - sourceFile: A file handle from which to read the source code to be linted. -/// - assumingFilename: The filename of the source file, used in diagnostic output. -/// - debugOptions: The set containing any debug options that were supplied on the command line. -/// - diagnosticEngine: A diagnostic collector that handles diagnostic messages. -/// - Returns: Zero if there were no lint errors, otherwise a non-zero number. -private func lintMain( - configuration: Configuration, sourceFile: FileHandle, assumingFilename: String?, - debugOptions: DebugOptions, diagnosticEngine: DiagnosticEngine -) { - let linter = SwiftLinter(configuration: configuration, diagnosticEngine: diagnosticEngine) - linter.debugOptions = debugOptions - let assumingFileURL = URL(fileURLWithPath: assumingFilename ?? "") - - guard let source = readSource(from: sourceFile) else { - diagnosticEngine.diagnose( - Diagnostic.Message(.error, "Unable to read source for linting from \(assumingFileURL.path).")) - return - } - - do { - try linter.lint(source: source, assumingFileURL: assumingFileURL) - } catch SwiftFormatError.fileNotReadable { - let path = assumingFileURL.path - diagnosticEngine.diagnose( - Diagnostic.Message(.error, "Unable to lint \(path): file is not readable or does not exist.")) - return - } catch SwiftFormatError.fileContainsInvalidSyntax(let position) { - let path = assumingFileURL.path - let location = SourceLocationConverter(file: path, source: source).location(for: position) - diagnosticEngine.diagnose( - Diagnostic.Message(.error, "file contains invalid or unrecognized Swift syntax."), - location: location) - return - } catch { - let path = assumingFileURL.path - diagnosticEngine.diagnose(Diagnostic.Message(.error, "Unable to lint \(path): \(error)")) - return - } -} diff --git a/Sources/swift-format/Subcommands/LintFormatOptions.swift b/Sources/swift-format/Subcommands/LintFormatOptions.swift index fd4dffb03..01faf545c 100644 --- a/Sources/swift-format/Subcommands/LintFormatOptions.swift +++ b/Sources/swift-format/Subcommands/LintFormatOptions.swift @@ -12,7 +12,6 @@ import ArgumentParser import Foundation -import SwiftFormat /// Common arguments used by the `lint` and `format` subcommands. struct LintFormatOptions: ParsableArguments { @@ -57,15 +56,6 @@ struct LintFormatOptions: ParsableArguments { @Flag(help: .hidden) var debugDisablePrettyPrint: Bool @Flag(help: .hidden) var debugDumpTokenStream: Bool - /// Advanced options that are useful for developing/debugging but otherwise not meant for general - /// use. - var debugOptions: DebugOptions { - [ - debugDisablePrettyPrint ? .disablePrettyPrint : [], - debugDumpTokenStream ? .dumpTokenStream : [], - ] - } - mutating func validate() throws { if recursive && paths.isEmpty { throw ValidationError("'--recursive' is only valid when formatting or linting files") diff --git a/Sources/swift-format/Utilities/Helpers.swift b/Sources/swift-format/Utilities/Helpers.swift deleted file mode 100644 index 1ae0933e8..000000000 --- a/Sources/swift-format/Utilities/Helpers.swift +++ /dev/null @@ -1,92 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// This source file is part of the Swift.org open source project -// -// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors -// Licensed under Apache License v2.0 with Runtime Library Exception -// -// See https://swift.org/LICENSE.txt for license information -// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors -// -//===----------------------------------------------------------------------===// - -import ArgumentParser -import Foundation -import SwiftFormat -import SwiftFormatConfiguration -import SwiftFormatCore -import SwiftSyntax - -/// Throws an error that causes the current command to exit the process with a failure exit code if -/// any of the preceding operations emitted diagnostics. -func failIfDiagnosticsEmitted(_ diagnosticEngine: DiagnosticEngine) throws { - guard diagnosticEngine.diagnostics.isEmpty else { - throw ExitCode.failure - } -} - -/// Reads from the given file handle until EOF is reached, then returns the contents as a UTF8 -/// encoded string. -func readSource(from fileHandle: FileHandle) -> String? { - let sourceData = fileHandle.readDataToEndOfFile() - return String(data: sourceData, encoding: .utf8) -} - -/// Processes the source code at the given file paths by performing a transformation, provided by a -/// closure. -/// - Parameters: -/// - paths: The file paths for the source files to process with a transformation. -/// - configurationPath: The file path to a swift-format configuration file. -/// - diagnosticEngine: A diagnostic collector that handles diagnostic messages. -/// - transform: A closure that performs a transformation on a specific source file. -func processSources( - from paths: [String], configurationPath: String?, - diagnosticEngine: DiagnosticEngine, - transform: (FileHandle, String, Configuration) -> Void -) throws { - for path in FileIterator(paths: paths) { - guard let sourceFile = FileHandle(forReadingAtPath: path) else { - diagnosticEngine.diagnose( - Diagnostic.Message(.error, "Unable to create a file handle for source from \(path).")) - return - } - let configuration = try loadConfiguration(forSwiftFile: path, configFilePath: configurationPath) - transform(sourceFile, path, configuration) - } -} - -/// Makes and returns a new configured diagnostic engine. -func makeDiagnosticEngine() -> DiagnosticEngine { - let engine = DiagnosticEngine() - let consumer = PrintingDiagnosticConsumer() - engine.addConsumer(consumer) - return engine -} - -/// Load the configuration. -func loadConfiguration(forSwiftFile swiftFilePath: String?, configFilePath: String?) - throws -> Configuration -{ - if let configFilePath = configFilePath { - return try decodedConfiguration(fromFile: URL(fileURLWithPath: configFilePath)) - } - - if let swiftFileURL = swiftFilePath.map(URL.init(fileURLWithPath:)), - let configFileURL = Configuration.url(forConfigurationFileApplyingTo: swiftFileURL) - { - return try decodedConfiguration(fromFile: configFileURL) - } - - return Configuration() -} - -/// Loads and returns a `Configuration` from the given JSON file if it is found and is valid. If the -/// file does not exist or there was an error decoding it, the program exits with a non-zero exit -/// code. -fileprivate func decodedConfiguration(fromFile url: Foundation.URL) throws -> Configuration { - do { - return try Configuration(contentsOf: url) - } catch { - throw FormatError(message: "Could not load configuration at \(url): \(error)") - } -} From ed5f980c2babe0fcaa31b01b7f9a6c39862e9c8c Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Tue, 7 Apr 2020 11:44:29 -0700 Subject: [PATCH 10/23] Prevent trailing commas from overflowing max line length. The implementation of trailing commas, using a workaround of the length calculation algorithm, resulted in edge cases where the trailing comma could overflow the max line length. This edge case happens for elements that should break internally to create additional space for the trailing comma (e.g. arrays, dictionaries, tuples). For example, an array that contains arrays could have its final element overflow the line length if the last element + correct indentation is exactly `maxLineLength`. After the trailing comma is added, the last array element *should* have broken up onto multiple lines to make space for the comma. The problem happens because the comma token was being added at the wrong syntax level, outside of the element's group, and its length wasn't propagating. Not propagating the comma's length was intentional to work around the fact that the trailing comma is only present when the comma delimited collection uses multiple lines. Unfortunately, moving the comma deeper into the element so that it can cause elements to break internally means its length has to be propagate as a normal token to also cause breaks at different syntax levels to fire too. - Array/dictionary elements won't be allowed to overflow the configured line length. - Arrays/dictionaries that could fit on 1 line without a trailing comma will be forced onto multiple lines with a trailing comma *if* they fit exactly at `maxLineLength` (without a trailing comma). --- .../SwiftFormatPrettyPrint/PrettyPrint.swift | 15 ++-- .../TokenStreamCreator.swift | 76 +++++++++++-------- .../ArrayDeclTests.swift | 67 +++++++++++++++- .../DictionaryDeclTests.swift | 74 +++++++++++++++++- .../XCTestManifests.swift | 2 + 5 files changed, 188 insertions(+), 46 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift b/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift index fa064d051..25fc1d606 100644 --- a/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift +++ b/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift @@ -651,15 +651,12 @@ public class PrettyPrinter { 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. - + // The token's length is only necessary when a comma will be printed, but it's impossible to + // know at this point whether the region-start token will be on the same line as this token. + // Without adding this length to the total, it would be possible for this comma to be + // printed in column `maxLineLength`. Unfortunately, this can cause breaks to fire + // unnecessarily when the enclosed tokens comma would fit within `maxLineLength`. + total += 1 lengths.append(1) } } diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index c73fc622e..78ae8c6dd 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -728,28 +728,33 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { override func visit(_ node: ArrayElementListSyntax) -> SyntaxVisitorContinueKind { insertTokens(.break(.same), betweenElementsOf: node) - // The syntax library can't distinguish an array initializer (where the elements are types) from - // an array literal (where the elements are the contents of the array). We never want to add a - // trailing comma in the initializer case and there's always exactly 1 element, so we never add - // a trailing comma to 1 element arrays. - if let firstElement = node.first, let lastElement = node.last, firstElement != lastElement { - before(firstElement.firstToken, tokens: .commaDelimitedRegionStart) - let endToken = - Token.commaDelimitedRegionEnd(hasTrailingComma: lastElement.trailingComma != nil) - after(lastElement.lastToken, tokens: endToken) - if let existingTrailingComma = lastElement.trailingComma { - ignoredTokens.insert(existingTrailingComma) + for element in node { + before(element.firstToken, tokens: .open) + after(element.lastToken, tokens: .close) + if let trailingComma = element.trailingComma { + closingDelimiterTokens.insert(trailingComma) + } + } + + if let lastElement = node.last { + if let trailingComma = lastElement.trailingComma { + ignoredTokens.insert(trailingComma) + } + // The syntax library can't distinguish an array initializer (where the elements are types) + // from an array literal (where the elements are the contents of the array). We never want to + // add a trailing comma in the initializer case and there's always exactly 1 element, so we + // never add a trailing comma to 1 element arrays. + if let firstElement = node.first, firstElement != lastElement { + before(firstElement.firstToken, tokens: .commaDelimitedRegionStart) + let endToken = + Token.commaDelimitedRegionEnd(hasTrailingComma: lastElement.trailingComma != nil) + after(lastElement.expression.lastToken, tokens: [endToken]) } } return .visitChildren } override func visit(_ node: ArrayElementSyntax) -> SyntaxVisitorContinueKind { - before(node.firstToken, tokens: .open) - after(node.lastToken, tokens: .close) - if let trailingComma = node.trailingComma { - closingDelimiterTokens.insert(trailingComma) - } return .visitChildren } @@ -762,17 +767,28 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { override func visit(_ node: DictionaryElementListSyntax) -> SyntaxVisitorContinueKind { insertTokens(.break(.same), betweenElementsOf: node) - // The syntax library can't distinguish a dictionary initializer (where the elements are types) - // from a dictionary literal (where the elements are the contents of the dictionary). We never - // want to add a trailing comma in the initializer case and there's always exactly 1 element, - // so we never add a trailing comma to 1 element dictionaries. - if let firstElement = node.first, let lastElement = node.last, firstElement != lastElement { - before(firstElement.firstToken, tokens: .commaDelimitedRegionStart) - let endToken = - Token.commaDelimitedRegionEnd(hasTrailingComma: lastElement.trailingComma != nil) - after(lastElement.lastToken, tokens: endToken) - if let existingTrailingComma = lastElement.trailingComma { - ignoredTokens.insert(existingTrailingComma) + for element in node { + before(element.firstToken, tokens: .open) + after(element.colon, tokens: .break) + after(element.lastToken, tokens: .close) + if let trailingComma = element.trailingComma { + closingDelimiterTokens.insert(trailingComma) + } + } + + if let lastElement = node.last { + if let trailingComma = lastElement.trailingComma { + ignoredTokens.insert(trailingComma) + } + // The syntax library can't distinguish a dictionary initializer (where the elements are + // types) from a dictionary literal (where the elements are the contents of the dictionary). + // We never want to add a trailing comma in the initializer case and there's always exactly 1 + // element, so we never add a trailing comma to 1 element dictionaries. + if let firstElement = node.first, let lastElement = node.last, firstElement != lastElement { + before(firstElement.firstToken, tokens: .commaDelimitedRegionStart) + let endToken = + Token.commaDelimitedRegionEnd(hasTrailingComma: lastElement.trailingComma != nil) + after(lastElement.lastToken, tokens: endToken) } } return .visitChildren @@ -784,12 +800,6 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { } override func visit(_ node: DictionaryElementSyntax) -> SyntaxVisitorContinueKind { - before(node.firstToken, tokens: .open) - after(node.colon, tokens: .break) - after(node.lastToken, tokens: .close) - if let trailingComma = node.trailingComma { - closingDelimiterTokens.insert(trailingComma) - } return .visitChildren } diff --git a/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift b/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift index 065bb5ddd..6ac62b475 100644 --- a/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift @@ -7,7 +7,6 @@ final class ArrayDeclTests: PrettyPrintTestCase { """ 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"] @@ -16,13 +15,13 @@ final class ArrayDeclTests: PrettyPrintTestCase { "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", ] + let a = [11111111, 2222222, 33333333, 444444] """ 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, ] @@ -43,6 +42,15 @@ final class ArrayDeclTests: PrettyPrintTestCase { ] """ + // Ideally, this array would be left on 1 line without a trailing comma. We don't know if the + // comma is required when calculating the length of array elements, so the comma's length is + // always added to last element and that 1 character causes the newlines inside of the array. + + """ + let a = [ + 11111111, 2222222, 33333333, 444444, + ] + + """ assertPrettyPrintEqual(input: input, expected: expected, linelength: 45) } @@ -134,4 +142,59 @@ final class ArrayDeclTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 32) } + + func testInnerElementBreakingFromComma() { + let input = + """ + let a = [("abc", "def", "xyz"),("this ", "string", "is long"),] + let a = [("abc", "def", "xyz"),("this ", "string", "is long")] + let a = [("this ", "string", "is long"),] + let a = [("this ", "string", "is long")] + let a = ["this ", "string", "is longer",] + let a = [("this", "str"), ("is", "lng")] + a = [("az", "by"), ("cf", "de")] + """ + + let expected = + """ + let a = [ + ("abc", "def", "xyz"), + ( + "this ", "string", "is long" + ), + ] + let a = [ + ("abc", "def", "xyz"), + ( + "this ", "string", "is long" + ), + ] + let a = [ + ("this ", "string", "is long") + ] + let a = [ + ("this ", "string", "is long") + ] + let a = [ + "this ", "string", + "is longer", + ] + let a = [ + ("this", "str"), + ("is", "lng"), + ] + + """ + // Ideally, this array would be left on 1 line without a trailing comma. We don't know if the + // comma is required when calculating the length of array elements, so the comma's length is + // always added to last element and that 1 character causes the newlines inside of the array. + + """ + a = [ + ("az", "by"), ("cf", "de"), + ] + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 32) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift b/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift index a160687dc..ef4e9cff6 100644 --- a/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift @@ -7,7 +7,6 @@ final class DictionaryDeclTests: PrettyPrintTestCase { """ let a = [1: "a", 2: "b", 3: "c",] let a: [Int: String] = [1: "a", 2: "b", 3: "c"] - let a = [10000: "abc", 20000: "def", 30000: "ghi"] let a = [10000: "abc", 20000: "def", 30000: "ghij"] let a: [Int: String] = [1: "a", 2: "b", 3: "c", 4: "d"] let a: [Int: String] = [1: "a", 2: "b", 3: "c", 4: "d", 5: "e", 6: "f", 7: "g"] @@ -16,13 +15,13 @@ final class DictionaryDeclTests: PrettyPrintTestCase { 1: "a", 2: "b", 3: "c", 4: "d", 5: "e", 6: "f", 7: "g", 8: "i", ] + let a = [10000: "abc", 20000: "def", 30000: "ghi"] """ let expected = """ let a = [1: "a", 2: "b", 3: "c"] let a: [Int: String] = [1: "a", 2: "b", 3: "c"] - let a = [10000: "abc", 20000: "def", 30000: "ghi"] let a = [ 10000: "abc", 20000: "def", 30000: "ghij", ] @@ -43,6 +42,16 @@ final class DictionaryDeclTests: PrettyPrintTestCase { ] """ + // Ideally, this dictionary would be left on 1 line without a trailing comma. We don't know if + // the comma is required when calculating the length of elements, so the comma's length is + // always added to last element and that 1 character causes the newlines inside of the + // dictionary. + + """ + let a = [ + 10000: "abc", 20000: "def", 30000: "ghi", + ] + + """ assertPrettyPrintEqual(input: input, expected: expected, linelength: 50) } @@ -175,4 +184,65 @@ final class DictionaryDeclTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 32) } + + func testInnerElementBreakingFromComma() { + let input = + """ + let a = [key1: ("abc", "def", "xyz"),key2: ("this ", "string", "is long"),] + let a = [key1: ("abc", "def", "xyz"),key2: ("this ", "string", "is long")] + let a = [key2: ("this ", "string", "is long")] + let a = [key2: ("this ", "string", "is long"),] + let a = [key2: ("this ", "string", "is long ")] + let a = [key1: ("a", "z"), key2: ("b ", "y")] + let a = [key1: ("ab", "z"), key2: ("b ", "y")] + a = [k1: ("ab", "z"), k2: ("bc", "y")] + """ + + let expected = + """ + let a = [ + key1: ("abc", "def", "xyz"), + key2: ( + "this ", "string", "is long" + ), + ] + let a = [ + key1: ("abc", "def", "xyz"), + key2: ( + "this ", "string", "is long" + ), + ] + let a = [ + key2: ("this ", "string", "is long") + ] + let a = [ + key2: ("this ", "string", "is long") + ] + let a = [ + key2: ( + "this ", "string", "is long " + ) + ] + let a = [ + key1: ("a", "z"), key2: ("b ", "y"), + ] + let a = [ + key1: ("ab", "z"), + key2: ("b ", "y"), + ] + + """ + // Ideally, this dictionary would be left on 1 line without a trailing comma. We don't know if + // the comma is required when calculating the length of elements, so the comma's length is + // always added to last element and that 1 character causes the newlines inside of the + // dictionary. + + """ + a = [ + k1: ("ab", "z"), k2: ("bc", "y"), + ] + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 38) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index 025ee9384..f0fecf6e2 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -25,6 +25,7 @@ extension ArrayDeclTests { ("testArrayOfFunctions", testArrayOfFunctions), ("testBasicArrays", testBasicArrays), ("testGroupsTrailingComma", testGroupsTrailingComma), + ("testInnerElementBreakingFromComma", testInnerElementBreakingFromComma), ("testNoTrailingCommasInTypes", testNoTrailingCommasInTypes), ("testTrailingCommaDiagnostics", testTrailingCommaDiagnostics), ("testWhitespaceOnlyDoesNotChangeTrailingComma", testWhitespaceOnlyDoesNotChangeTrailingComma), @@ -196,6 +197,7 @@ extension DictionaryDeclTests { ("testBasicDictionaries", testBasicDictionaries), ("testDiscretionaryNewlineAfterColon", testDiscretionaryNewlineAfterColon), ("testGroupsTrailingComma", testGroupsTrailingComma), + ("testInnerElementBreakingFromComma", testInnerElementBreakingFromComma), ("testNoTrailingCommasInTypes", testNoTrailingCommasInTypes), ("testTrailingCommaDiagnostics", testTrailingCommaDiagnostics), ("testWhitespaceOnlyDoesNotChangeTrailingComma", testWhitespaceOnlyDoesNotChangeTrailingComma), From 20162cd4c50596152d03d893952c17d7f01ae565 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Tue, 7 Apr 2020 12:56:18 -0700 Subject: [PATCH 11/23] Rearrange breaks so class decls won't overflow line length. The inheritance list for type and extension decls is wrapped with open/close tokens, but the relevant open-break token was nested inside of a different group. The previous token layout created the correct indentation of the tokens in the inheritance list, but meant the break between the colon and first token in the inheritance list could never fire because it was followed by a close token. This resulted in type and extension decls that overflow the column limit when the type name + `:` + first token in the inhertance list together was longer than the column limit. I fixed this by moving the open break inside of the open/close tokens surrounding the inheritance list. Normally, the open break comes before the open but that would force a break before the first token in the inheritance list if the entire list doesn't fit. That behavior wouldn't be consistent with existing behavior. Instead, placing the open break inside of the open only breaks if the first token in the inheritance list is too long. --- .../TokenStreamCreator.swift | 9 ++++---- .../ClassDeclTests.swift | 21 +++++++++++++++++++ .../ExtensionDeclTests.swift | 8 +++---- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index 78ae8c6dd..648de0c56 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -1991,10 +1991,11 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { } override func visit(_ node: TypeInheritanceClauseSyntax) -> SyntaxVisitorContinueKind { - after(node.colon, tokens: .break(.open, size: 1)) - before(node.inheritedTypeCollection.firstToken, tokens: .open) - after(node.inheritedTypeCollection.lastToken, tokens: .close) - after(node.lastToken, tokens: .break(.close, size: 0)) + // Normally, the open-break is placed before the open token. In this case, it's intentionally + // ordered differently so that the inheritance list can start on the current line and only + // breaks if the first item in the list would overflow the column limit. + before(node.inheritedTypeCollection.firstToken, tokens: .open, .break(.open, size: 1)) + after(node.inheritedTypeCollection.lastToken, tokens: .break(.close, size: 0), .close) return .visitChildren } diff --git a/Tests/SwiftFormatPrettyPrintTests/ClassDeclTests.swift b/Tests/SwiftFormatPrettyPrintTests/ClassDeclTests.swift index 3f6b5671c..bb77ef8ba 100644 --- a/Tests/SwiftFormatPrettyPrintTests/ClassDeclTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/ClassDeclTests.swift @@ -140,6 +140,15 @@ final class ClassDeclTests: PrettyPrintTestCase { let A: Int let B: Bool } + class MyClass: + SuperOne, SuperTwo, SuperThree { + let A: Int + let B: Bool + } + class MyClassWhoseNameIsVeryLong: SuperOne, SuperTwo, SuperThree { + let A: Int + let B: Bool + } """ let expected = @@ -158,6 +167,18 @@ final class ClassDeclTests: PrettyPrintTestCase { let A: Int let B: Bool } + class MyClass: + SuperOne, SuperTwo, SuperThree + { + let A: Int + let B: Bool + } + class MyClassWhoseNameIsVeryLong: + SuperOne, SuperTwo, SuperThree + { + let A: Int + let B: Bool + } """ diff --git a/Tests/SwiftFormatPrettyPrintTests/ExtensionDeclTests.swift b/Tests/SwiftFormatPrettyPrintTests/ExtensionDeclTests.swift index 15ac8055c..632ba07c1 100644 --- a/Tests/SwiftFormatPrettyPrintTests/ExtensionDeclTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/ExtensionDeclTests.swift @@ -316,8 +316,8 @@ final class ExtensionDeclTests: PrettyPrintTestCase { let expected = """ - public extension MyContainer: MyContainerProtocolOne, - MyContainerProtocolTwo, + public extension MyContainer: + MyContainerProtocolOne, MyContainerProtocolTwo, SomeoneElsesContainerProtocol, SomeFrameworkContainerProtocol where @@ -346,8 +346,8 @@ final class ExtensionDeclTests: PrettyPrintTestCase { let expected = """ - public extension MyContainer: MyContainerProtocolOne, - MyContainerProtocolTwo, + public extension MyContainer: + MyContainerProtocolOne, MyContainerProtocolTwo, SomeoneElsesContainerProtocol, SomeFrameworkContainerProtocol where From 5cca489756073ea251c7f36eea69916210d53f09 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Tue, 7 Apr 2020 13:29:33 -0700 Subject: [PATCH 12/23] Recursively apply NoEmptyTrailingClosureParentheses rule. This rule wasn't being applied recursively, which meant it wouldn't be applied to the trailing closure of a function call where parens where removed. --- .../NoEmptyTrailingClosureParentheses.swift | 21 +++++--- ...EmptyTrailingClosureParenthesesTests.swift | 51 +++++++++++++++++-- 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/Sources/SwiftFormatRules/NoEmptyTrailingClosureParentheses.swift b/Sources/SwiftFormatRules/NoEmptyTrailingClosureParentheses.swift index 9288c3a9f..2dc5889b5 100644 --- a/Sources/SwiftFormatRules/NoEmptyTrailingClosureParentheses.swift +++ b/Sources/SwiftFormatRules/NoEmptyTrailingClosureParentheses.swift @@ -22,22 +22,31 @@ import SwiftSyntax public final class NoEmptyTrailingClosureParentheses: SyntaxFormatRule { public override func visit(_ node: FunctionCallExprSyntax) -> ExprSyntax { - guard node.argumentList.count == 0 else { return ExprSyntax(node) } + guard node.argumentList.count == 0 else { return super.visit(node) } - guard node.trailingClosure != nil && node.argumentList.isEmpty && node.leftParen != nil else { - return ExprSyntax(node) + guard let trailingClosure = node.trailingClosure, + node.argumentList.isEmpty && node.leftParen != nil else + { + return super.visit(node) } guard let name = node.calledExpression.lastToken?.withoutTrivia() else { - return ExprSyntax(node) + return super.visit(node) } diagnose(.removeEmptyTrailingParentheses(name: "\(name)"), on: node) + // Need to visit `calledExpression` before creating a new node so that the location data (column + // and line numbers) is available. + guard let rewrittenCalledExpr = ExprSyntax(visit(Syntax(node.calledExpression))) else { + return super.visit(node) + } let formattedExp = replaceTrivia( - on: node.calledExpression, - token: node.calledExpression.lastToken, + on: rewrittenCalledExpr, + token: rewrittenCalledExpr.lastToken, trailingTrivia: .spaces(1)) + let formattedClosure = visit(trailingClosure).as(ClosureExprSyntax.self) let result = node.withLeftParen(nil).withRightParen(nil).withCalledExpression(formattedExp) + .withTrailingClosure(formattedClosure) return ExprSyntax(result) } } diff --git a/Tests/SwiftFormatRulesTests/NoEmptyTrailingClosureParenthesesTests.swift b/Tests/SwiftFormatRulesTests/NoEmptyTrailingClosureParenthesesTests.swift index 5dad85b83..c4aef650a 100644 --- a/Tests/SwiftFormatRulesTests/NoEmptyTrailingClosureParenthesesTests.swift +++ b/Tests/SwiftFormatRulesTests/NoEmptyTrailingClosureParenthesesTests.swift @@ -19,6 +19,21 @@ final class NoEmptyTrailingClosureParenthesesTests: LintOrFormatRuleTestCase { func myfunc(cls: MyClass) { cls.myBadClosure() { $0 } } + DispatchQueue.main.async() { + greetEnthusiastically() { "John" } + DispatchQueue.main.async() { + greetEnthusiastically() { "Willis" } + } + } + DispatchQueue.global.async(inGroup: blah) { + DispatchQueue.main.async() { + greetEnthusiastically() { "Willis" } + } + DispatchQueue.main.async { + greetEnthusiastically() { "Willis" } + } + } + foo(bar() { baz })() { blah } """, expected: """ func greetEnthusiastically(_ nameProvider: () -> String) { @@ -35,9 +50,39 @@ final class NoEmptyTrailingClosureParenthesesTests: LintOrFormatRuleTestCase { func myfunc(cls: MyClass) { cls.myBadClosure { $0 } } - """) - XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: "greetEnthusiastically")) - XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: "myBadClosure")) + DispatchQueue.main.async { + greetEnthusiastically { "John" } + DispatchQueue.main.async { + greetEnthusiastically { "Willis" } + } + } + DispatchQueue.global.async(inGroup: blah) { + DispatchQueue.main.async { + greetEnthusiastically { "Willis" } + } + DispatchQueue.main.async { + greetEnthusiastically { "Willis" } + } + } + foo(bar { baz }) { blah } + """, + checkForUnassertedDiagnostics: true) + XCTAssertDiagnosed( + .removeEmptyTrailingParentheses(name: "greetEnthusiastically"), line: 7, column: 1) + XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: "myBadClosure"), line: 13, column: 3) XCTAssertNotDiagnosed(.removeEmptyTrailingParentheses(name: "myClosure")) + XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: "async"), line: 15, column: 1) + XCTAssertDiagnosed( + .removeEmptyTrailingParentheses(name: "greetEnthusiastically"), line: 16, column: 3) + XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: "async"), line: 17, column: 3) + XCTAssertDiagnosed( + .removeEmptyTrailingParentheses(name: "greetEnthusiastically"), line: 18, column: 5) + XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: "async"), line: 22, column: 3) + XCTAssertDiagnosed( + .removeEmptyTrailingParentheses(name: "greetEnthusiastically"), line: 23, column: 5) + XCTAssertDiagnosed( + .removeEmptyTrailingParentheses(name: "greetEnthusiastically"), line: 26, column: 5) + XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: ")"), line: 29, column: 1) + XCTAssertDiagnosed(.removeEmptyTrailingParentheses(name: "bar"), line: 29, column: 5) } } From 513a2c73e016d47e2039216890cf422b9a23d5b4 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Tue, 7 Apr 2020 14:13:49 -0700 Subject: [PATCH 13/23] Fix counting of consecutive newlines. The math for calculating how many blank lines to allow didn't account for existing consecutive blank lines when the maximum blank lines configuration value was used. That meant an existing newline in the output would result in allowing max blank lines + 1 blank lines. --- .../SwiftFormatPrettyPrint/PrettyPrint.swift | 2 +- .../NewlineTests.swift | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift b/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift index 25fc1d606..477ce35f3 100644 --- a/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift +++ b/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift @@ -211,7 +211,7 @@ public class PrettyPrinter { numberToPrint = consecutiveNewlineCount == 0 ? 1 : 0 case .soft(let count, _): // We add 1 to the max blank lines because it takes 2 newlines to create the first blank line. - numberToPrint = min(count - consecutiveNewlineCount, configuration.maximumBlankLines + 1) + numberToPrint = min(count, configuration.maximumBlankLines + 1) - consecutiveNewlineCount case .hard(let count): numberToPrint = count } diff --git a/Tests/SwiftFormatPrettyPrintTests/NewlineTests.swift b/Tests/SwiftFormatPrettyPrintTests/NewlineTests.swift index d956a89ef..03f9f0090 100644 --- a/Tests/SwiftFormatPrettyPrintTests/NewlineTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/NewlineTests.swift @@ -74,4 +74,60 @@ final class NewlineTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 45) } + + func testNewlinesBetweenMembers() { + let input = + """ + + + class MyClazz { + + lazy var memberView: UIView = { + let view = UIView() + return view + }() + + + func doSomething() { + print("!") + } + + + func doSomethingElse() { + print("else!") + } + + + let constMember = 1 + + + + } + """ + + let expected = + """ + class MyClazz { + + lazy var memberView: UIView = { + let view = UIView() + return view + }() + + func doSomething() { + print("!") + } + + func doSomethingElse() { + print("else!") + } + + let constMember = 1 + + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 100) + } } From 0da09d07ebb80707c09fc6e313f94f7c869f4f54 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Wed, 8 Apr 2020 10:14:34 -0700 Subject: [PATCH 14/23] Minor updates to `@differentiable`. https://github.com/apple/swift/pull/30001 removed the logic from the parser to handle the `jvp:` and `vjp:` arguments of the attribute (but left the syntax definitions in place for the time being). Since this causes an attribute with those arguments to fail to parse, I've removed them from the tests so that those tests continue to pass under the new behavior. (The arguments were always optional, so they pass under the old behavior as well.) This also caught and fixed a bug where an attribute with only a `wrt:` and a `where` clause wasn't getting formatted correctly. --- .../TokenStreamCreator.swift | 15 +++++++++++-- .../DifferentiationAttributeTests.swift | 22 +++++-------------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index 648de0c56..8434f7107 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -2035,10 +2035,18 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { override func visit(_ node: DifferentiableAttributeArgumentsSyntax) -> SyntaxVisitorContinueKind { // This node encapsulates the entire list of arguments in a `@differentiable(...)` attribute. - after(node.diffParamsComma, tokens: .break(.same)) - var needsBreakBeforeWhereClause = false + if let diffParamsComma = node.diffParamsComma { + after(diffParamsComma, tokens: .break(.same)) + } else if node.diffParams != nil { + // If there were diff params but no comma following them, then we have "wrt: foo where ..." + // and we need a break before the where clause. + needsBreakBeforeWhereClause = true + } + + // TODO: These properties will likely go away in a future version since the parser no longer + // reads the `vjp:` and `jvp:` arguments to `@differentiable`. if let vjp = node.maybeVJP { before(vjp.firstToken, tokens: .open) after(vjp.lastToken, tokens: .close) @@ -2051,6 +2059,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { after(jvp.trailingComma, tokens: .break(.same)) needsBreakBeforeWhereClause = true } + if let whereClause = node.whereClause { if needsBreakBeforeWhereClause { before(whereClause.firstToken, tokens: .break(.same)) @@ -2066,6 +2075,8 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { { // This node encapsulates the `vjp:` or `jvp:` label and decl name in a `@differentiable` // attribute. + // TODO: This node will likely go away in a future version since the parser no longer reads the + // `vjp:` and `jvp:` arguments to `@differentiable`. after(node.colon, tokens: .break(.continue, newlines: .elective(ignoresDiscretionary: true))) return .visitChildren } diff --git a/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift b/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift index bf33ca825..864af18fd 100644 --- a/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift @@ -2,38 +2,28 @@ final class DifferentiationAttributeTests: PrettyPrintTestCase { func testDifferentiable() { let input = """ - @differentiable(wrt: x, vjp: d where T: D) + @differentiable(wrt: x where T: D) func foo(_ x: T) -> T {} - @differentiable(wrt: x, vjp: deriv where T: D) + @differentiable(wrt: x where T: Differentiable) func foo(_ x: T) -> T {} - @differentiable(wrt: x, vjp: derivativeFoo where T: Differentiable) - func foo(_ x: T) -> T {} - - @differentiable(wrt: theVariableNamedX, vjp: derivativeFoo where T: Differentiable) + @differentiable(wrt: theVariableNamedX where T: Differentiable) func foo(_ theVariableNamedX: T) -> T {} """ let expected = """ - @differentiable(wrt: x, vjp: d where T: D) + @differentiable(wrt: x where T: D) func foo(_ x: T) -> T {} @differentiable( - wrt: x, vjp: deriv where T: D - ) - func foo(_ x: T) -> T {} - - @differentiable( - wrt: x, vjp: derivativeFoo - where T: Differentiable + wrt: x where T: Differentiable ) func foo(_ x: T) -> T {} @differentiable( - wrt: theVariableNamedX, - vjp: derivativeFoo + wrt: theVariableNamedX where T: Differentiable ) func foo(_ theVariableNamedX: T) -> T {} From 8fbc3f14badc8eaade8f3a5700a6f29ee5c7f793 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Tue, 7 Apr 2020 16:05:57 -0700 Subject: [PATCH 15/23] Add breaks around conditions of while-stmt. This aligns the behavior of while-stmt and if-stmt conditions. The while-stmt conditions didn't have open-breaks, which meant there was no additional indentation nor would continuation breaks be able to stack. This can result in exceeding the max line length if a while-stmt has a label and first condition, up to its first break, that exceeds the remaining line length. --- .../TokenStreamCreator.swift | 13 +++++ .../WhileStmtTests.swift | 54 +++++++++++++++++++ .../XCTestManifests.swift | 2 + 3 files changed, 69 insertions(+) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index 8434f7107..683624124 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -526,6 +526,19 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { after(node.labelColon, tokens: .space) after(node.whileKeyword, tokens: .space) + // Add break groups, using open continuation breaks, around any conditions after the first so + // that continuations inside of the conditions can stack in addition to continuations between + // the conditions. There are no breaks around the first condition because there was historically + // not break after the while token and adding such a break would cause excessive changes to + // previously formatted code. + // This has the side effect that the label + `while` + tokens up to the first break in the first + // condition could be longer than the column limit since there are no breaks between the label + // or while token. + for condition in node.conditions.dropFirst() { + before(condition.firstToken, tokens: .break(.open(kind: .continuation), size: 0)) + after(condition.lastToken, tokens: .break(.close(mustBreak: false), size: 0)) + } + arrangeBracesAndContents(of: node.body, contentsKeyPath: \.statements) return .visitChildren diff --git a/Tests/SwiftFormatPrettyPrintTests/WhileStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/WhileStmtTests.swift index e01b6b5f1..60a9a7463 100644 --- a/Tests/SwiftFormatPrettyPrintTests/WhileStmtTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/WhileStmtTests.swift @@ -63,6 +63,11 @@ final class WhileStmtTests: PrettyPrintTestCase { let a = 123 var b = "abc" } + myLabel: while myVeryLongFirstCondition && condition2 || condition3 + { + let a = 123 + var b = "abc" + } """ let expected = @@ -83,6 +88,55 @@ final class WhileStmtTests: PrettyPrintTestCase { let a = 123 var b = "abc" } + myLabel: while myVeryLongFirstCondition + && condition2 || condition3 + { + let a = 123 + var b = "abc" + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 29) + } + + func testWhileLoopMultipleConditionElements() { + let input = + """ + while x >= 0 && y >= 0 && x < foo && y < bar, let object = foo.value(at: y), let otherObject = foo.value(at: x), isEqual(a, b) { + foo() + } + while x >= 0 && y >= 0 + && x < foo && y < bar, + let object = + foo.value(at: y), + let otherObject = foo.value(at: x), isEqual(a, b) { + foo() + } + """ + + let expected = + """ + while x >= 0 && y >= 0 + && x < foo && y < bar, + let object = foo.value( + at: y), + let otherObject = + foo.value(at: x), + isEqual(a, b) + { + foo() + } + while x >= 0 && y >= 0 + && x < foo && y < bar, + let object = + foo.value(at: y), + let otherObject = + foo.value(at: x), + isEqual(a, b) + { + foo() + } """ diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index f0fecf6e2..6463ed476 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -493,6 +493,7 @@ extension NewlineTests { static let __allTests__NewlineTests = [ ("testLeadingNewlines", testLeadingNewlines), ("testLeadingNewlinesWithComments", testLeadingNewlinesWithComments), + ("testNewlinesBetweenMembers", testNewlinesBetweenMembers), ("testTrailingNewlines", testTrailingNewlines), ("testTrailingNewlinesWithComments", testTrailingNewlinesWithComments), ] @@ -802,6 +803,7 @@ extension WhileStmtTests { static let __allTests__WhileStmtTests = [ ("testBasicWhileLoops", testBasicWhileLoops), ("testLabeledWhileLoops", testLabeledWhileLoops), + ("testWhileLoopMultipleConditionElements", testWhileLoopMultipleConditionElements), ] } From c52e0d29f43891dacb781366d36e0027425aead7 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Thu, 9 Apr 2020 14:06:10 -0700 Subject: [PATCH 16/23] Add a consistent group around if-stmts. The consistent group forces the bodies of if-stmts with else clauses to have breaks around all braces when the stmt spans multiple lines. Previously. the breaks around the braces only fired if the body didn't fit on the line. That behavior lead to hard to read if-stmts because if was unclear where conditions stopped and the body started. --- .../TokenStreamCreator.swift | 13 +++++ .../IfStmtTests.swift | 51 ++++++++++++++++++- .../XCTestManifests.swift | 1 + 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index 683624124..f029fab34 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -444,6 +444,12 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { // MARK: - Control flow statement nodes override func visit(_ node: IfStmtSyntax) -> SyntaxVisitorContinueKind { + // There may be a consistent breaking group around this node, see `CodeBlockItemSyntax`. This + // group is necessary so that breaks around and inside of the conditions aren't forced to break + // when the if-stmt spans multiple lines. + before(node.conditions.firstToken, tokens: .open) + after(node.conditions.lastToken, tokens: .close) + after(node.labelColon, tokens: .space) after(node.ifKeyword, tokens: .space) @@ -1287,6 +1293,13 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { appendFormatterIgnored(node: Syntax(node)) return .skipChildren } + + // This group applies to a top-level if-stmt so that all of the bodies will have the same + // breaking behavior. + if let ifStmt = node.item.as(IfStmtSyntax.self) { + before(ifStmt.firstToken, tokens: .open(.consistent)) + after(ifStmt.lastToken, tokens: .close) + } return .visitChildren } diff --git a/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift index d59010de4..f3bebf677 100644 --- a/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift @@ -381,7 +381,9 @@ final class IfStmtTests: PrettyPrintTestCase { SomeVeryLongTypeNameThatDefinitelyBreaks, baz: Baz ) = foo(a, b, c, d) - { return nil } + { + return nil + } """ @@ -515,4 +517,51 @@ final class IfStmtTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 50) } + + func testMultipleIfStmts() { + let input = + """ + if foo && bar { baz() } else if bar { baz() } else if foo { baz() } else { blargh() } + if foo && bar && quxxe { baz() } else if bar { baz() } else if foo { baz() } else if quxxe { baz() } else { blargh() } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz { foo() } else { bar() } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz && someOtherCondition { foo() } else { bar() } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz && someOtherCondition { foo() } + """ + + let expected = + """ + if foo && bar { baz() } else if bar { baz() } else if foo { baz() } else { blargh() } + if foo && bar && quxxe { + baz() + } else if bar { + baz() + } else if foo { + baz() + } else if quxxe { + baz() + } else { + blargh() + } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz { + foo() + } else { + bar() + } + if let foo = getmyfoo(), let bar = getmybar(), + foo.baz && bar.baz && someOtherCondition + { + foo() + } else { + bar() + } + if let foo = getmyfoo(), let bar = getmybar(), + foo.baz && bar.baz && someOtherCondition + { + foo() + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 85) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index 6463ed476..dcfa1644e 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -410,6 +410,7 @@ extension IfStmtTests { ("testIfStatement", testIfStatement), ("testLabeledIfStmt", testLabeledIfStmt), ("testMatchingPatternConditions", testMatchingPatternConditions), + ("testMultipleIfStmts", testMultipleIfStmts), ("testOptionalBindingConditions", testOptionalBindingConditions), ("testParenthesizedClauses", testParenthesizedClauses), ] From 66cac37fbe54a0abecb74eaf25c019c205de944e Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Fri, 10 Apr 2020 14:44:01 -0700 Subject: [PATCH 17/23] Disallow line breaks in completely empty array and dict exprs. There's no reason to have a newline between the square brackets when there's nothing, other than a `:` for dict exprs, between the brackets. --- .../TokenStreamCreator.swift | 17 +++++++++++++---- .../ArrayDeclTests.swift | 11 +++++++++++ .../DictionaryDeclTests.swift | 16 ++++++++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index f029fab34..e7d034e25 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -739,8 +739,10 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { } override func visit(_ node: ArrayExprSyntax) -> SyntaxVisitorContinueKind { - after(node.leftSquare, tokens: .break(.open, size: 0), .open) - before(node.rightSquare, tokens: .break(.close, size: 0), .close) + if !node.elements.isEmpty || node.rightSquare.leadingTrivia.numberOfComments > 0 { + after(node.leftSquare, tokens: .break(.open, size: 0), .open) + before(node.rightSquare, tokens: .break(.close, size: 0), .close) + } return .visitChildren } @@ -778,8 +780,15 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { } override func visit(_ node: DictionaryExprSyntax) -> SyntaxVisitorContinueKind { - after(node.leftSquare, tokens: .break(.open, size: 0), .open) - before(node.rightSquare, tokens: .break(.close, size: 0), .close) + // The node's content is either a `DictionaryElementListSyntax` or a `TokenSyntax` for a colon + // token (for an empty dictionary). + if !(node.content.as(DictionaryElementListSyntax.self)?.isEmpty ?? true) + || node.content.leadingTrivia?.numberOfComments ?? 0 > 0 + || node.rightSquare.leadingTrivia.numberOfComments > 0 + { + after(node.leftSquare, tokens: .break(.open, size: 0), .open) + before(node.rightSquare, tokens: .break(.close, size: 0), .close) + } return .visitChildren } diff --git a/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift b/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift index 6ac62b475..6961b361e 100644 --- a/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/ArrayDeclTests.swift @@ -5,6 +5,12 @@ final class ArrayDeclTests: PrettyPrintTestCase { func testBasicArrays() { let input = """ + let a = [ ] + let a = [ + ] + let a = [ + // Comment + ] let a = [1, 2, 3,] let a: [Bool] = [false, true, true, false] let a = [11111111, 2222222, 33333333, 4444444] @@ -20,6 +26,11 @@ final class ArrayDeclTests: PrettyPrintTestCase { let expected = """ + let a = [] + let a = [] + let a = [ + // Comment + ] let a = [1, 2, 3] let a: [Bool] = [false, true, true, false] let a = [ diff --git a/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift b/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift index ef4e9cff6..da913553e 100644 --- a/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/DictionaryDeclTests.swift @@ -5,6 +5,15 @@ final class DictionaryDeclTests: PrettyPrintTestCase { func testBasicDictionaries() { let input = """ + let a: [String: String] = [ : ] + let a: [String: String] = [ + : + ] + let a: [String: String] = [ + // Comment A + : + // Comment B + ] let a = [1: "a", 2: "b", 3: "c",] let a: [Int: String] = [1: "a", 2: "b", 3: "c"] let a = [10000: "abc", 20000: "def", 30000: "ghij"] @@ -20,6 +29,13 @@ final class DictionaryDeclTests: PrettyPrintTestCase { let expected = """ + let a: [String: String] = [:] + let a: [String: String] = [:] + let a: [String: String] = [ + // Comment A + : + // Comment B + ] let a = [1: "a", 2: "b", 3: "c"] let a: [Int: String] = [1: "a", 2: "b", 3: "c"] let a = [ From a4100c3f6a8dc7f8a4dea886489473f8644ff750 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Mon, 13 Apr 2020 13:17:42 -0700 Subject: [PATCH 18/23] Disallow discretionary newlines before trailing closures. The behavior was inconsistent between subscripts and function calls. They both use a same-break that ignores discretionary newlines now. The break ignores discretionary because breaking before a trailing closure's left brace uses excessive vertical whitespace without improving readability. Now the brace is usually glued to the right paren/right square bracket, except when it doesn't fit due to line length. --- .../TokenStreamCreator.swift | 8 ++- .../FunctionCallTests.swift | 49 +++++++++++++++++++ .../SubscriptExprTests.swift | 49 +++++++++++++++++++ .../XCTestManifests.swift | 2 + 4 files changed, 106 insertions(+), 2 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index e7d034e25..f97929ae1 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -855,7 +855,9 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { (node.trailingClosure != nil && !isCompactSingleFunctionCallArgument(arguments)) || mustBreakBeforeClosingDelimiter(of: node, argumentListPath: \.argumentList) - before(node.trailingClosure?.leftBrace, tokens: .break(.same)) + before( + node.trailingClosure?.leftBrace, + tokens: .break(.same, newlines: .elective(ignoresDiscretionary: true))) arrangeFunctionCallArgumentList( arguments, @@ -1048,7 +1050,9 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { node.trailingClosure != nil || mustBreakBeforeClosingDelimiter(of: node, argumentListPath: \.argumentList) - before(node.trailingClosure?.leftBrace, tokens: .space) + before( + node.trailingClosure?.leftBrace, + tokens: .break(.same, newlines: .elective(ignoresDiscretionary: true))) arrangeFunctionCallArgumentList( arguments, diff --git a/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift b/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift index 2012055c3..32d626031 100644 --- a/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift @@ -259,6 +259,55 @@ final class FunctionCallTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 20) } + func testDiscretionaryLineBreakBeforeTrailingClosure() { + let input = + """ + foo(a, b, c) + { + blah() + } + foo( + a, b, c + ) + { + blah() + } + foo(arg1, arg2, arg3, arg4, arg5, arg6, arg7) + { + blah() + } + foo(ab, arg1, arg2) { + blah() + } + """ + + let expected = + """ + foo(a, b, c) { + blah() + } + foo( + a, b, c + ) { + blah() + } + foo( + arg1, arg2, arg3, + arg4, arg5, arg6, + arg7 + ) { + blah() + } + foo(ab, arg1, arg2) + { + blah() + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 20) + } + func testGroupsTrailingComma() { let input = """ diff --git a/Tests/SwiftFormatPrettyPrintTests/SubscriptExprTests.swift b/Tests/SwiftFormatPrettyPrintTests/SubscriptExprTests.swift index a56408b09..b832120df 100644 --- a/Tests/SwiftFormatPrettyPrintTests/SubscriptExprTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/SubscriptExprTests.swift @@ -106,4 +106,53 @@ final class SubscriptExprTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 70) } + + func testDiscretionaryLineBreakBeforeTrailingClosure() { + let input = + """ + foo[a, b, c] + { + blah() + } + foo[ + a, b, c + ] + { + blah() + } + foo[arg1, arg2, arg3, arg4, arg5, arg6, arg7] + { + blah() + } + foo[ab, arg1, arg2] { + blah() + } + """ + + let expected = + """ + foo[a, b, c] { + blah() + } + foo[ + a, b, c + ] { + blah() + } + foo[ + arg1, arg2, arg3, + arg4, arg5, arg6, + arg7 + ] { + blah() + } + foo[ab, arg1, arg2] + { + blah() + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 20) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index dcfa1644e..2dfedc9da 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -298,6 +298,7 @@ extension FunctionCallTests { ("testBasicFunctionCalls_packArguments", testBasicFunctionCalls_packArguments), ("testDiscretionaryLineBreakAfterColon", testDiscretionaryLineBreakAfterColon), ("testDiscretionaryLineBreakBeforeClosingParenthesis", testDiscretionaryLineBreakBeforeClosingParenthesis), + ("testDiscretionaryLineBreakBeforeTrailingClosure", testDiscretionaryLineBreakBeforeTrailingClosure), ("testDiscretionaryLineBreaksAreSelfCorrecting", testDiscretionaryLineBreaksAreSelfCorrecting), ("testGroupsTrailingComma", testGroupsTrailingComma), ("testNestedFunctionCallExprSequences", testNestedFunctionCallExprSequences), @@ -697,6 +698,7 @@ extension SubscriptExprTests { static let __allTests__SubscriptExprTests = [ ("testBasicSubscriptGetters", testBasicSubscriptGetters), ("testBasicSubscriptSetters", testBasicSubscriptSetters), + ("testDiscretionaryLineBreakBeforeTrailingClosure", testDiscretionaryLineBreakBeforeTrailingClosure), ("testGroupsTrailingComma", testGroupsTrailingComma), ("testSubscriptGettersWithTrailingClosures", testSubscriptGettersWithTrailingClosures), ("testSubscriptSettersWithTrailingClosures", testSubscriptSettersWithTrailingClosures), From 2c29da13417212c1543b7c9ba7ad9009d51c5701 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Tue, 14 Apr 2020 12:23:07 -0700 Subject: [PATCH 19/23] Handle derivative attr without params. Using `@derivative(of:...)` without a `wrt` clause previously crashed the formatter, due to an unmatched open token. Now the close token is added when there is no `wrt` clause too. --- Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift | 3 ++- .../DifferentiationAttributeTests.swift | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index f97929ae1..5fad53919 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -2140,7 +2140,8 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { // `@transpose(...)` attribute. before(node.ofLabel, tokens: .open) after(node.colon, tokens: .break(.continue, newlines: .elective(ignoresDiscretionary: true))) - after(node.comma, tokens: .close) + // The comma after originalDeclName is optional and is only present if there are diffParams. + after(node.comma ?? node.originalDeclName.lastToken, tokens: .close) if let diffParams = node.diffParams { before(diffParams.firstToken, tokens: .break(.same), .open) diff --git a/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift b/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift index 864af18fd..5e3cf2b58 100644 --- a/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/DifferentiationAttributeTests.swift @@ -99,6 +99,9 @@ final class DifferentiationAttributeTests: PrettyPrintTestCase { #if HAS_DERIVATIVE_REGISTRATION_ATTRIBUTE let input = """ + @derivative(of: foo) + func deriv() {} + @derivative(of: foo, wrt: x) func deriv(_ x: T) {} @@ -111,6 +114,9 @@ final class DifferentiationAttributeTests: PrettyPrintTestCase { let expected = """ + @derivative(of: foo) + func deriv() {} + @derivative(of: foo, wrt: x) func deriv(_ x: T) {} From 9f863d0245c99832485732fc1b9357a471f3fde6 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Wed, 15 Apr 2020 12:55:23 -0700 Subject: [PATCH 20/23] Add a test case for stacking open-breaks. This test case exercises the case where an open break occurs adjacent to a reset break when the reset break fires. This verifies that the open breaks stack indentation when they occur on the same line, but one of the breaks is at the start of the line. --- Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift index 33b70a886..44f51097c 100644 --- a/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/SwitchStmtTests.swift @@ -83,6 +83,7 @@ final class SwitchStmtTests: PrettyPrintTestCase { case "a": print("a") case "b", "c": print("bc") case "d", "e", "f", "g", "h": print("defgh") + case someVeryLongVarName, someOtherLongVarName: foo(a: [1, 2, 3, 4, 5]) default: print("default") } """ @@ -96,6 +97,11 @@ final class SwitchStmtTests: PrettyPrintTestCase { case "d", "e", "f", "g", "h": print("defgh") + case someVeryLongVarName, + someOtherLongVarName: + foo(a: [ + 1, 2, 3, 4, 5, + ]) default: print("default") } From 06078435d9e26def8e8c46c7d12232adbe182ef1 Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Wed, 15 Apr 2020 12:54:34 -0700 Subject: [PATCH 21/23] Use real line number when moving block indentation for close breaks. --- Sources/SwiftFormatPrettyPrint/PrettyPrint.swift | 5 ++++- .../FunctionCallTests.swift | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift b/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift index 477ce35f3..d3e20117f 100644 --- a/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift +++ b/Sources/SwiftFormatPrettyPrint/PrettyPrint.swift @@ -359,11 +359,14 @@ public class PrettyPrinter { = openCloseBreakCompensatingLineNumber != matchingOpenBreak.lineNumber if matchingOpenBreak.contributesBlockIndent { + // The actual line number is used, instead of the compensating line number. When the close + // break is at the start of a new line, the block indentation isn't carried to the new line. + let currentLine = lineNumber // When two or more open breaks are encountered on the same line, only the final open // break is allowed to increase the block indent, avoiding multiple block indents. As the // open breaks on that line are closed, the new final open break must be enabled again to // add a block indent. - if matchingOpenBreak.lineNumber == openCloseBreakCompensatingLineNumber, + if matchingOpenBreak.lineNumber == currentLine, let lastActiveOpenBreak = activeOpenBreaks.last, lastActiveOpenBreak.kind == .block, !lastActiveOpenBreak.contributesBlockIndent diff --git a/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift b/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift index 32d626031..8d91cae22 100644 --- a/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/FunctionCallTests.swift @@ -135,8 +135,16 @@ final class FunctionCallTests: PrettyPrintTestCase { func testArgumentStartsWithOpenDelimiter() { let input = """ + myFunc(someArray: [ + ]) myFunc(someArray: [1000, 2000, 3000, 4000, 5000, 6000, 7000, 8000]) + myFunc(someDictionary: [ + :]) myFunc(someDictionary: ["foo": "bar", "baz": "quux", "gli": "glop"]) + myFunc(someClosure: { + }) + myFunc(someClosure: { (a, b, c) in + }) myFunc(someClosure: { foo, bar in baz(1000, 2000, 3000, 4000, 5000) }) myFunc(someArray: [1000, 2000, 3000, 4000, 5000, 6000, 7000, 8000]) { foo in bar() } myFunc(someArray: [1000, 2000, 3000, 4000, 5000, 6000, 7000, 8000]) { foo in someMuchLongerLineBreakingBarFunction() } @@ -144,13 +152,19 @@ final class FunctionCallTests: PrettyPrintTestCase { let expected = """ + myFunc(someArray: []) myFunc(someArray: [ 1000, 2000, 3000, 4000, 5000, 6000, 7000, 8000, ]) + myFunc(someDictionary: [:]) myFunc(someDictionary: [ "foo": "bar", "baz": "quux", "gli": "glop", ]) + myFunc(someClosure: { + }) + myFunc(someClosure: { (a, b, c) in + }) myFunc(someClosure: { foo, bar in baz(1000, 2000, 3000, 4000, 5000) }) From 5df9eb4690ef4fa381e903f2bfa128ac11c99efa Mon Sep 17 00:00:00 2001 From: Dylan Sturgeon Date: Thu, 16 Apr 2020 15:22:38 -0700 Subject: [PATCH 22/23] Move open group token past ifstmt's if token to avoid extra newlines. The implementation of consistent groups forces breaking inside of the group if the group starts at the beginning of a line (i.e. was immediately preceded by a break that fired). That isn't exactly what we want for if-stmt because the stmt generally starts at the beginning of a line and we want the breaks to fire only if the group is longer than the rest of the line. Moving the open token past the if `syntax` token resolves 2 known complexities with consistent groups: 1. Whether an immediately preceding break fired influences 2. The `spaceRemaining` value is only updated after printing the first `syntax` token on a new line It's a little odd to group in this way, since it logically makes more sense to group around the entire if-stmt but there aren't any breaks between the if token and the first condition so moving the open token doesn't change the length of any breaks throughout the statement. --- .../TokenStreamCreator.swift | 2 +- .../IfStmtTests.swift | 72 ++++++++++--------- 2 files changed, 39 insertions(+), 35 deletions(-) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index 5fad53919..a72188b2e 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -1310,7 +1310,7 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { // This group applies to a top-level if-stmt so that all of the bodies will have the same // breaking behavior. if let ifStmt = node.item.as(IfStmtSyntax.self) { - before(ifStmt.firstToken, tokens: .open(.consistent)) + before(ifStmt.conditions.firstToken, tokens: .open(.consistent)) after(ifStmt.lastToken, tokens: .close) } return .visitChildren diff --git a/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift b/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift index f3bebf677..5c73b5e20 100644 --- a/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/IfStmtTests.swift @@ -521,47 +521,51 @@ final class IfStmtTests: PrettyPrintTestCase { func testMultipleIfStmts() { let input = """ - if foo && bar { baz() } else if bar { baz() } else if foo { baz() } else { blargh() } - if foo && bar && quxxe { baz() } else if bar { baz() } else if foo { baz() } else if quxxe { baz() } else { blargh() } - if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz { foo() } else { bar() } - if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz && someOtherCondition { foo() } else { bar() } - if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz && someOtherCondition { foo() } + func foo() { + if foo && bar { baz() } else if bar { baz() } else if foo { baz() } else { blargh() } + if foo && bar && quxxe { baz() } else if bar { baz() } else if foo { baz() } else if quxxe { baz() } else { blargh() } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz { foo() } else { bar() } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz && someOtherCondition { foo() } else { bar() } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz && someOtherCondition { foo() } + } """ let expected = """ - if foo && bar { baz() } else if bar { baz() } else if foo { baz() } else { blargh() } - if foo && bar && quxxe { - baz() - } else if bar { - baz() - } else if foo { - baz() - } else if quxxe { - baz() - } else { - blargh() - } - if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz { - foo() - } else { - bar() - } - if let foo = getmyfoo(), let bar = getmybar(), - foo.baz && bar.baz && someOtherCondition - { - foo() - } else { - bar() - } - if let foo = getmyfoo(), let bar = getmybar(), - foo.baz && bar.baz && someOtherCondition - { - foo() + func foo() { + if foo && bar { baz() } else if bar { baz() } else if foo { baz() } else { blargh() } + if foo && bar && quxxe { + baz() + } else if bar { + baz() + } else if foo { + baz() + } else if quxxe { + baz() + } else { + blargh() + } + if let foo = getmyfoo(), let bar = getmybar(), foo.baz && bar.baz { + foo() + } else { + bar() + } + if let foo = getmyfoo(), let bar = getmybar(), + foo.baz && bar.baz && someOtherCondition + { + foo() + } else { + bar() + } + if let foo = getmyfoo(), let bar = getmybar(), + foo.baz && bar.baz && someOtherCondition + { + foo() + } } """ - assertPrettyPrintEqual(input: input, expected: expected, linelength: 85) + assertPrettyPrintEqual(input: input, expected: expected, linelength: 87) } } From e8297894eab394ffb8061ffd9aa97afbb699415e Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Fri, 17 Apr 2020 14:00:42 -0700 Subject: [PATCH 23/23] Format property wrappers correctly. --- .../TokenStreamCreator.swift | 16 +++++++ .../AttributeTests.swift | 45 +++++++++++++++++++ .../XCTestManifests.swift | 1 + 3 files changed, 62 insertions(+) diff --git a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift index a72188b2e..d717fd2d7 100644 --- a/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift +++ b/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift @@ -1391,6 +1391,22 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor { return .visitChildren } + override func visit(_ node: CustomAttributeSyntax) -> SyntaxVisitorContinueKind { + // "Custom attributes" are better known to users as "property wrappers". + before(node.firstToken, tokens: .open) + if let argumentList = node.argumentList, + let leftParen = node.leftParen, let rightParen = node.rightParen + { + arrangeFunctionCallArgumentList( + argumentList, + leftDelimiter: leftParen, + rightDelimiter: rightParen, + forcesBreakBeforeRightDelimiter: false) + } + after(node.lastToken, tokens: .close) + return .visitChildren + } + override func visit(_ node: AvailabilitySpecListSyntax) -> SyntaxVisitorContinueKind { insertTokens(.break(.same, size: 1), betweenElementsOf: node) return .visitChildren diff --git a/Tests/SwiftFormatPrettyPrintTests/AttributeTests.swift b/Tests/SwiftFormatPrettyPrintTests/AttributeTests.swift index c53c06110..9dd036ea7 100644 --- a/Tests/SwiftFormatPrettyPrintTests/AttributeTests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/AttributeTests.swift @@ -314,4 +314,49 @@ final class AttributeTests: PrettyPrintTestCase { assertPrettyPrintEqual(input: input, expected: expected, linelength: 30) } + + func testPropertyWrappers() { + // Property wrappers are `CustomAttributeSyntax` nodes (not `AttributeSyntax`) and their + // arguments are `TupleExprElementListSyntax` (like regular function call argument lists), so + // make sure that those are formatted properly. + let input = + """ + struct X { + @Wrapper var value: String + + @Wrapper ( ) var value: String + + @Wrapper (arg1:"value")var value: String + + @Wrapper (arg1:"value") + var value: String + + @Wrapper (arg1:"value",arg2:otherValue) + var value: String + } + """ + + let expected = + """ + struct X { + @Wrapper var value: String + + @Wrapper() var value: String + + @Wrapper(arg1: "value") + var value: String + + @Wrapper(arg1: "value") + var value: String + + @Wrapper( + arg1: "value", + arg2: otherValue) + var value: String + } + + """ + + assertPrettyPrintEqual(input: input, expected: expected, linelength: 32) + } } diff --git a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift index 2dfedc9da..a145ca3cc 100644 --- a/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift +++ b/Tests/SwiftFormatPrettyPrintTests/XCTestManifests.swift @@ -69,6 +69,7 @@ extension AttributeTests { ("testObjCAttributesDiscretionaryLineBreaking", testObjCAttributesDiscretionaryLineBreaking), ("testObjCAttributesPerLineBreaking", testObjCAttributesPerLineBreaking), ("testObjCBinPackedAttributes", testObjCBinPackedAttributes), + ("testPropertyWrappers", testPropertyWrappers), ] }