From 06ab67c9c1930c3eca9e303a0fc9e177b7a5218c Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Sun, 27 Aug 2023 08:35:37 -0700 Subject: [PATCH] Add noExplicitOwnership rule --- Rules.md | 16 ++++++++ Sources/Examples.swift | 7 ++++ Sources/ParsingHelpers.swift | 33 ++++++++++----- Sources/Rules.swift | 42 +++++++++++++++++-- Tests/ParsingHelpersTests.swift | 46 ++++++++++++++++----- Tests/RulesTests+Organization.swift | 2 +- Tests/RulesTests+Redundancy.swift | 62 +++++++++++++++++++++++++++-- 7 files changed, 180 insertions(+), 28 deletions(-) diff --git a/Rules.md b/Rules.md index 2c25af1e2..89bb1c1b2 100644 --- a/Rules.md +++ b/Rules.md @@ -94,6 +94,7 @@ * [docComments](#docComments) * [isEmpty](#isEmpty) * [markTypes](#markTypes) +* [noExplicitOwnership](#noExplicitOwnership) * [organizeDeclarations](#organizeDeclarations) * [preferForLoop](#preferForLoop) * [sortSwitchCases](#sortSwitchCases) @@ -1153,6 +1154,21 @@ Option | Description
+## noExplicitOwnership + +Don't use explicit ownership modifiers (borrowing / consuming). + +
+Examples + +```diff +- borrowing func foo(_ bar: consuming Bar) { ... } ++ func foo(_ bar: Bar) { ... } +``` + +
+
+ ## numberFormatting Use consistent grouping for numeric literals. Groups will be separated by `_` diff --git a/Sources/Examples.swift b/Sources/Examples.swift index 95e91e70c..e49a9a0ed 100644 --- a/Sources/Examples.swift +++ b/Sources/Examples.swift @@ -1729,4 +1729,11 @@ private struct Examples { .forEach { print($0) } ``` """ + + let noExplicitOwnership = """ + ```diff + - borrowing func foo(_ bar: consuming Bar) { ... } + + func foo(_ bar: Bar) { ... } + ``` + """ } diff --git a/Sources/ParsingHelpers.swift b/Sources/ParsingHelpers.swift index c7a0a376b..211a84f29 100644 --- a/Sources/ParsingHelpers.swift +++ b/Sources/ParsingHelpers.swift @@ -1202,8 +1202,10 @@ extension Formatter { /// - `(...) -> ...` /// - `...?` /// - `...!` - func parseType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange) { - let baseType = parseNonOptionalType(at: startOfTypeIndex) + /// - `borrowing ...` + /// - `consuming ...` + func parseType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange)? { + guard let baseType = parseNonOptionalType(at: startOfTypeIndex) else { return nil } // Any type can be optional, so check for a trailing `?` or `!` if let nextToken = index(of: .nonSpaceOrCommentOrLinebreak, after: baseType.range.upperBound), @@ -1216,7 +1218,7 @@ extension Formatter { return baseType } - private func parseNonOptionalType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange) { + private func parseNonOptionalType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange)? { // Parse types of the form `[...]` if tokens[startOfTypeIndex] == .startOfScope("["), let endOfScope = endOfScope(at: startOfTypeIndex) @@ -1232,9 +1234,9 @@ extension Formatter { // Parse types of the form `(...) -> ...` if let closureReturnIndex = index(of: .nonSpaceOrCommentOrLinebreak, after: endOfScope), tokens[closureReturnIndex] == .operator("->", .infix), - let returnTypeIndex = index(of: .nonSpaceOrCommentOrLinebreak, after: closureReturnIndex) + let returnTypeIndex = index(of: .nonSpaceOrCommentOrLinebreak, after: closureReturnIndex), + let returnTypeRange = parseType(at: returnTypeIndex)?.range { - let returnTypeRange = parseType(at: returnTypeIndex).range let typeRange = startOfTypeIndex ... returnTypeRange.upperBound return (name: tokens[typeRange].string, range: typeRange) } @@ -1253,8 +1255,21 @@ extension Formatter { return (name: tokens[typeRange].string, range: typeRange) } + // Parse types of the form `borrowing ...` and `consuming ...` + if ["borrowing", "consuming"].contains(tokens[startOfTypeIndex].string), + let nextToken = index(of: .nonSpaceOrCommentOrLinebreak, after: startOfTypeIndex), + let followingType = parseType(at: nextToken) + { + let typeRange = startOfTypeIndex ... followingType.range.upperBound + return (name: tokens[typeRange].string, range: typeRange) + } + // Otherwise this is just a single identifier - return (name: tokens[startOfTypeIndex].string, range: startOfTypeIndex ... startOfTypeIndex) + if tokens[startOfTypeIndex].isIdentifier || tokens[startOfTypeIndex].isKeyword { + return (name: tokens[startOfTypeIndex].string, range: startOfTypeIndex ... startOfTypeIndex) + } + + return nil } struct ImportRange: Comparable { @@ -2266,7 +2281,7 @@ extension Formatter { // https://docs.swift.org/swift-book/ReferenceManual/Types.html#grammar_protocol-composition-type let firstIdentifierIndex = index(of: .nonSpaceOrCommentOrLinebreak, after: equalsIndex), tokens[firstIdentifierIndex].isIdentifier, - case var lastTypeEndIndex = parseType(at: firstIdentifierIndex).range.upperBound, + var lastTypeEndIndex = parseType(at: firstIdentifierIndex)?.range.upperBound, let firstAndIndex = index(of: .nonSpaceOrCommentOrLinebreak, after: lastTypeEndIndex), tokens[firstAndIndex] == .operator("&", .infix) else { return nil } @@ -2278,9 +2293,9 @@ extension Formatter { while let nextAndIndex = index(of: .nonSpaceOrCommentOrLinebreak, after: lastTypeEndIndex), tokens[nextAndIndex] == .operator("&", .infix), let nextIdentifierIndex = index(of: .nonSpaceOrCommentOrLinebreak, after: nextAndIndex), - tokens[nextIdentifierIndex].isIdentifier + tokens[nextIdentifierIndex].isIdentifier, + let endOfType = parseType(at: nextIdentifierIndex)?.range.upperBound { - let endOfType = parseType(at: nextIdentifierIndex).range.upperBound andTokenIndices.append(nextAndIndex) lastTypeEndIndex = endOfType } diff --git a/Sources/Rules.swift b/Sources/Rules.swift index f0f245771..e1fb9400c 100644 --- a/Sources/Rules.swift +++ b/Sources/Rules.swift @@ -6825,9 +6825,8 @@ public struct _FormatRules { let startOfTypeIndex = formatter.index(of: .nonSpaceOrCommentOrLinebreak, after: colonIndex) else { return } - let (typeName, typeRange) = formatter.parseType(at: startOfTypeIndex) - - guard let startOfConditional = formatter.index(of: .nonSpaceOrCommentOrLinebreak, after: typeRange.upperBound), + guard let (typeName, typeRange) = formatter.parseType(at: startOfTypeIndex), + let startOfConditional = formatter.index(of: .nonSpaceOrCommentOrLinebreak, after: typeRange.upperBound), let conditionalBranches = formatter.conditionalBranches(at: startOfConditional) else { return } @@ -7397,4 +7396,41 @@ public struct _FormatRules { } } } + + public let noExplicitOwnership = FormatRule( + help: "Don't use explicit ownership modifiers (borrowing / consuming).", + disabledByDefault: true + ) { formatter in + formatter.forEachToken { keywordIndex, token in + guard ["borrowing", "consuming"].contains(token.string), + let nextTokenIndex = formatter.index(of: .nonSpaceOrLinebreak, after: keywordIndex) + else { return } + + // Use of `borrowing` and `consuming` as ownership modifiers + // immediately precede a valid type, or the `func` keyword. + // You could also simply use these names as a property, + // like `let borrowing = foo` or `func myFunc(borrowing foo: Foo)`. + // As a simple heuristic to detect the difference, attempt to parse the + // following tokens as a type, and require that it doesn't start with lower-case letter. + let isValidOwnershipModifier: Bool + + if formatter.tokens[nextTokenIndex] == .keyword("func") { + isValidOwnershipModifier = true + } + + else if formatter.parseType(at: nextTokenIndex) != nil, + formatter.tokens[nextTokenIndex].string.first?.isLowercase == false + { + isValidOwnershipModifier = true + } + + else { + isValidOwnershipModifier = false + } + + if isValidOwnershipModifier { + formatter.removeTokens(in: keywordIndex ..< nextTokenIndex) + } + } + } } diff --git a/Tests/ParsingHelpersTests.swift b/Tests/ParsingHelpersTests.swift index 87497dd42..e4b6e492e 100644 --- a/Tests/ParsingHelpersTests.swift +++ b/Tests/ParsingHelpersTests.swift @@ -1652,77 +1652,101 @@ class ParsingHelpersTests: XCTestCase { let formatter = Formatter(tokenize(""" let foo: Foo = .init() """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "Foo") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "Foo") } func testParseOptionalType() { let formatter = Formatter(tokenize(""" let foo: Foo? = .init() """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "Foo?") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "Foo?") } func testParseIOUType() { let formatter = Formatter(tokenize(""" let foo: Foo! = .init() """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "Foo!") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "Foo!") } func testParseGenericType() { let formatter = Formatter(tokenize(""" let foo: Foo = .init() """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "Foo") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "Foo") } func testParseOptionalGenericType() { let formatter = Formatter(tokenize(""" let foo: Foo? = .init() """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "Foo?") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "Foo?") } func testParseDictionaryType() { let formatter = Formatter(tokenize(""" let foo: [Foo: Bar] = [:] """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "[Foo: Bar]") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "[Foo: Bar]") } func testParseOptionalDictionaryType() { let formatter = Formatter(tokenize(""" let foo: [Foo: Bar]? = [:] """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "[Foo: Bar]?") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "[Foo: Bar]?") } func testParseTupleType() { let formatter = Formatter(tokenize(""" let foo: (Foo, Bar) = (Foo(), Bar()) """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "(Foo, Bar)") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "(Foo, Bar)") } func testParseClosureType() { let formatter = Formatter(tokenize(""" let foo: (Foo, Bar) -> (Foo, Bar) = { foo, bar in (foo, bar) } """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "(Foo, Bar) -> (Foo, Bar)") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "(Foo, Bar) -> (Foo, Bar)") + } + + func testParseClosureTypeWithOwnership() { + let formatter = Formatter(tokenize(""" + let foo: (consuming Foo, borrowing Bar) -> (Foo, Bar) = { foo, bar in (foo, bar) } + """)) + XCTAssertEqual(formatter.parseType(at: 5)?.name, "(consuming Foo, borrowing Bar) -> (Foo, Bar)") } func testParseOptionalReturningClosureType() { let formatter = Formatter(tokenize(""" let foo: (Foo, Bar) -> (Foo, Bar)? = { foo, bar in (foo, bar) } """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "(Foo, Bar) -> (Foo, Bar)?") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "(Foo, Bar) -> (Foo, Bar)?") } func testParseOptionalClosureType() { let formatter = Formatter(tokenize(""" let foo: ((Foo, Bar) -> (Foo, Bar)?)? = { foo, bar in (foo, bar) } """)) - XCTAssertEqual(formatter.parseType(at: 5).name, "((Foo, Bar) -> (Foo, Bar)?)?") + XCTAssertEqual(formatter.parseType(at: 5)?.name, "((Foo, Bar) -> (Foo, Bar)?)?") + } + + func testParseOptionalClosureTypeWithOwnership() { + let formatter = Formatter(tokenize(""" + let foo: ((consuming Foo, borrowing Bar) -> (Foo, Bar)?)? = { foo, bar in (foo, bar) } + """)) + XCTAssertEqual(formatter.parseType(at: 5)?.name, "((consuming Foo, borrowing Bar) -> (Foo, Bar)?)?") + } + + func testParseInvalidType() { + let formatter = Formatter(tokenize(""" + let foo = { foo, bar in (foo, bar) } + """)) + XCTAssertEqual(formatter.parseType(at: 4)?.name, nil) + XCTAssertEqual(formatter.parseType(at: 5)?.name, nil) + XCTAssertEqual(formatter.parseType(at: 6)?.name, nil) + XCTAssertEqual(formatter.parseType(at: 7)?.name, nil) } func testEndOfDeclaration() { diff --git a/Tests/RulesTests+Organization.swift b/Tests/RulesTests+Organization.swift index c105857f8..3b7543669 100644 --- a/Tests/RulesTests+Organization.swift +++ b/Tests/RulesTests+Organization.swift @@ -3165,7 +3165,7 @@ class OrganizationTests: RulesTests { let input = "consuming public func close()" let output = "public consuming func close()" let options = FormatOptions(modifierOrder: ["public", "consuming"]) - testFormatting(for: input, output, rule: FormatRules.modifierOrder, options: options) + testFormatting(for: input, output, rule: FormatRules.modifierOrder, options: options, exclude: ["noExplicitOwnership"]) } func testNoConfusePostfixIdentifierWithKeyword() { diff --git a/Tests/RulesTests+Redundancy.swift b/Tests/RulesTests+Redundancy.swift index 7f8948343..2e1d78cf8 100644 --- a/Tests/RulesTests+Redundancy.swift +++ b/Tests/RulesTests+Redundancy.swift @@ -7568,7 +7568,7 @@ class RedundancyTests: RulesTests { file.close() } """ - testFormatting(for: input, rule: FormatRules.unusedArguments) + testFormatting(for: input, rule: FormatRules.unusedArguments, exclude: ["noExplicitOwnership"]) } func testUsedConsumingBorrowingArguments() { @@ -7578,7 +7578,7 @@ class RedundancyTests: RulesTests { borrow(b) } """ - testFormatting(for: input, rule: FormatRules.unusedArguments) + testFormatting(for: input, rule: FormatRules.unusedArguments, exclude: ["noExplicitOwnership"]) } func testUnusedConsumingArgument() { @@ -7592,7 +7592,7 @@ class RedundancyTests: RulesTests { print("no-op") } """ - testFormatting(for: input, output, rule: FormatRules.unusedArguments) + testFormatting(for: input, output, rule: FormatRules.unusedArguments, exclude: ["noExplicitOwnership"]) } func testUnusedConsumingBorrowingArguments() { @@ -7606,7 +7606,7 @@ class RedundancyTests: RulesTests { print("no-op") } """ - testFormatting(for: input, output, rule: FormatRules.unusedArguments) + testFormatting(for: input, output, rule: FormatRules.unusedArguments, exclude: ["noExplicitOwnership"]) } func testFunctionArgumentUsedInGuardNotRemoved() { @@ -8295,4 +8295,58 @@ class RedundancyTests: RulesTests { testFormatting(for: input, output, rule: FormatRules.redundantInternal, exclude: ["redundantExtensionACL"]) } + + // MARK: - noExplicitOwnership + + func testRemovesOwnershipKeywordsFromFunc() { + let input = """ + consuming func myMethod(consuming foo: consuming Foo, borrowing bars: borrowing [Bar]) {} + borrowing func myMethod(consuming foo: consuming Foo, borrowing bars: borrowing [Bar]) {} + """ + + let output = """ + func myMethod(consuming foo: Foo, borrowing bars: [Bar]) {} + func myMethod(consuming foo: Foo, borrowing bars: [Bar]) {} + """ + + testFormatting(for: input, output, rule: FormatRules.noExplicitOwnership, exclude: ["unusedArguments"]) + } + + func testRemovesOwnershipKeywordsFromClosure() { + let input = """ + foos.map { (foo: consuming Foo) in + foo.bar + } + + foos.map { (foo: borrowing Foo) in + foo.bar + } + """ + + let output = """ + foos.map { (foo: Foo) in + foo.bar + } + + foos.map { (foo: Foo) in + foo.bar + } + """ + + testFormatting(for: input, output, rule: FormatRules.noExplicitOwnership, exclude: ["unusedArguments"]) + } + + func testRemovesOwnershipKeywordsFromType() { + let input = """ + let consuming: (consuming Foo) -> Bar + let borrowing: (borrowing Foo) -> Bar + """ + + let output = """ + let consuming: (Foo) -> Bar + let borrowing: (Foo) -> Bar + """ + + testFormatting(for: input, output, rule: FormatRules.noExplicitOwnership) + } }