realm
Add new `direct_return` opt-in rule
| 18 Messages | |
|---|---|
| :book: | Linting Aerial with this PR took 1.07s vs 1.07s on main (0% slower) |
| :book: | Linting Alamofire with this PR took 1.41s vs 1.39s on main (1% slower) |
| :book: | Linting Brave with this PR took 7.52s vs 7.49s on main (0% slower) |
| :book: | Linting DuckDuckGo with this PR took 3.13s vs 3.12s on main (0% slower) |
| :book: | Linting Firefox with this PR took 9.28s vs 9.32s on main (0% faster) |
| :book: | Linting Kickstarter with this PR took 10.09s vs 10.04s on main (0% slower) |
| :book: | Linting Moya with this PR took 0.56s vs 0.55s on main (1% slower) |
| :book: | Linting NetNewsWire with this PR took 3.09s vs 3.08s on main (0% slower) |
| :book: | Linting Nimble with this PR took 0.62s vs 0.62s on main (0% slower) |
| :book: | Linting PocketCasts with this PR took 7.5s vs 7.46s on main (0% slower) |
| :book: | Linting Quick with this PR took 0.23s vs 0.24s on main (4% faster) |
| :book: | Linting Realm with this PR took 11.9s vs 11.87s on main (0% slower) |
| :book: | Linting SourceKitten with this PR took 0.44s vs 0.44s on main (0% slower) |
| :book: | Linting Sourcery with this PR took 2.33s vs 2.3s on main (1% slower) |
| :book: | Linting Swift with this PR took 4.63s vs 4.62s on main (0% slower) |
| :book: | Linting VLC with this PR took 1.36s vs 1.37s on main (0% faster) |
| :book: | Linting Wire with this PR took 8.91s vs 8.91s on main (0% slower) |
| :book: | Linting WordPress with this PR took 11.29s vs 11.21s on main (0% slower) |
Generated by :no_entry_sign: Danger
I've checked the visible OSS findings and all are valid.
So let me merge it to gain some feedback from users.
This is great @SimplyDanny! I've turned this on for Lyft's codebase and the consensus is that folks like it.
I ran into 3 kinds of minor issues with the rule's correction logic:
1. Corrections "eats" some comments
For example this:
private func getNodes(bar: Bar) -> [(Node, Int)] {
// This is an important comment
let allNodes = bar.rootNodes
.flatMap { $0.transform1() }
.flatMap { $0.transform2() }
.map { ($0, $0.someIntProp) }
return allNodes
}
Corrects to this:
private func getNodes(bar: Bar) -> [(Node, Int)] {
return bar.rootNodes
.flatMap { $0.transform1() }
.flatMap { $0.transform2() }
.map { ($0, $0.someIntProp) }
}
Ideally it would keep the // This is an important comment comment.
2. disable:next commands aren't respected when correcting
This is probably a side-effect of 1, but this:
private func getNodes(bar: Bar) -> [(Node, Int)] {
// swiftlint:disable:next direct_return - Local var needed to help the compiler type check the expression
let allNodes = bar.rootNodes
.flatMap { $0.transform1() }
.flatMap { $0.transform2() }
.map { ($0, $0.someIntProp) }
return allNodes
}
Corrects to this:
private func getNodes(bar: Bar) -> [(Node, Int)] {
return bar.rootNodes
.flatMap { $0.transform1() }
.flatMap { $0.transform2() }
.map { ($0, $0.someIntProp) }
}
But really because of the disable comment, this shouldn't be corrected at all.
3. Some typechecking information is lost
For example this:
var values: [Foo] {
let values: [Bar] = [.first, .second, .third]
return values
}
Corrects to this:
var values: [Foo] {
return [.first, .second, .third]
}
Which may or may not typecheck.
This typechecks though:
var values: [Foo] {
return [.first, .second, .third] as [Bar]
}
This is great @SimplyDanny! I've turned this on for Lyft's codebase and the consensus is that folks like it.
Glad to hear that you like the new rule generally!
I ran into 3 kinds of minor issues with the rule's correction logic:
I've addressed these issues in #4783. Correct handling of trivia is a really unpleasant task. 😬
Correct handling of trivia is a really unpleasant task.
I agree. SwiftSyntax doesn't make it easy. I keep hoping I'm going to have a "lightbulb" moment at some point where it's going to click how to actually operate over whitespace elegantly with SwiftSyntax.
Correct handling of trivia is a really unpleasant task.
I agree. SwiftSyntax doesn't make it easy. I keep hoping I'm going to have a "lightbulb" moment at some point where it's going to click how to actually operate over whitespace elegantly with SwiftSyntax.
Haha ... same for me. 😂
It would probably make sense to have our own "trivia library" in SwiftLint. Something like "(un-)indent this list of statements" would be very useful to have, for example. But it would presumably feel like a set of heuristics that could potentially break with every new version of SwiftSyntax.
It would probably make sense to have our own "trivia library" in SwiftLint.
I wonder if this would make sense in the SwiftSyntax repo itself.
To start we could start a discussion in the Source Tooling section of the swift forums about our challenges working with trivia using SwiftSyntax and if folks have recommendations, or if folks would be interested in having a "trivia library" in SwiftSyntax.
With SwiftSyntax powering the upcoming Swift macros features, I suspect there will be increasing interest and adoption of SwiftSyntax in the coming months, so we're probably far from the only folks thinking about this.
The SwiftSyntax repo would surely be the better place where such a library should belong.
Not sure if it should start with an initial implementation, though, to have something to talk about. For experimentation, the SwiftLint repo would be a good starting point.
But asking folks in the forum first is definitely a good idea. I may prepare a post when there is some spare time left.