Skip to content

Commit

Permalink
Add noExplicitOwnership rule
Browse files Browse the repository at this point in the history
  • Loading branch information
calda authored and nicklockwood committed Sep 29, 2023
1 parent 6265935 commit 06ab67c
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 28 deletions.
16 changes: 16 additions & 0 deletions Rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
* [docComments](#docComments)
* [isEmpty](#isEmpty)
* [markTypes](#markTypes)
* [noExplicitOwnership](#noExplicitOwnership)
* [organizeDeclarations](#organizeDeclarations)
* [preferForLoop](#preferForLoop)
* [sortSwitchCases](#sortSwitchCases)
Expand Down Expand Up @@ -1153,6 +1154,21 @@ Option | Description
</details>
<br/>

## noExplicitOwnership

Don't use explicit ownership modifiers (borrowing / consuming).

<details>
<summary>Examples</summary>

```diff
- borrowing func foo(_ bar: consuming Bar) { ... }
+ func foo(_ bar: Bar) { ... }
```

</details>
<br/>

## numberFormatting

Use consistent grouping for numeric literals. Groups will be separated by `_`
Expand Down
7 changes: 7 additions & 0 deletions Sources/Examples.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1729,4 +1729,11 @@ private struct Examples {
.forEach { print($0) }
```
"""

let noExplicitOwnership = """
```diff
- borrowing func foo(_ bar: consuming Bar) { ... }
+ func foo(_ bar: Bar) { ... }
```
"""
}
33 changes: 24 additions & 9 deletions Sources/ParsingHelpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1202,8 +1202,10 @@ extension Formatter {
/// - `(...) -> ...`
/// - `...?`
/// - `...!`
func parseType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange<Int>) {
let baseType = parseNonOptionalType(at: startOfTypeIndex)
/// - `borrowing ...`
/// - `consuming ...`
func parseType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange<Int>)? {
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),
Expand All @@ -1216,7 +1218,7 @@ extension Formatter {
return baseType
}

private func parseNonOptionalType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange<Int>) {
private func parseNonOptionalType(at startOfTypeIndex: Int) -> (name: String, range: ClosedRange<Int>)? {
// Parse types of the form `[...]`
if tokens[startOfTypeIndex] == .startOfScope("["),
let endOfScope = endOfScope(at: startOfTypeIndex)
Expand All @@ -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)
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 }
Expand All @@ -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
}
Expand Down
42 changes: 39 additions & 3 deletions Sources/Rules.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down Expand Up @@ -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)
}
}
}
}
46 changes: 35 additions & 11 deletions Tests/ParsingHelpersTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<Bar, Baaz> = .init()
"""))
XCTAssertEqual(formatter.parseType(at: 5).name, "Foo<Bar, Baaz>")
XCTAssertEqual(formatter.parseType(at: 5)?.name, "Foo<Bar, Baaz>")
}

func testParseOptionalGenericType() {
let formatter = Formatter(tokenize("""
let foo: Foo<Bar, Baaz>? = .init()
"""))
XCTAssertEqual(formatter.parseType(at: 5).name, "Foo<Bar, Baaz>?")
XCTAssertEqual(formatter.parseType(at: 5)?.name, "Foo<Bar, Baaz>?")
}

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() {
Expand Down
2 changes: 1 addition & 1 deletion Tests/RulesTests+Organization.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
62 changes: 58 additions & 4 deletions Tests/RulesTests+Redundancy.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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)
}
}

0 comments on commit 06ab67c

Please sign in to comment.