-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Lint/Format] Add a rule to omit return
from functions, closures, subscripts, and variables
#596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Mainly style comments, to try to align with other changes I've been making in the code base.
let decl = super.visit(node) | ||
|
||
// func <name>() -> <Type> { return ... } | ||
if var funcDecl = decl.as(FunctionDeclSyntax.self), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Here and elsewhere, can you make these kinds of conditions guard
s instead so that the "interesting" part of the function is at the bottom and unnested?
// func <name>() -> <Type> { return ... } | ||
if var funcDecl = decl.as(FunctionDeclSyntax.self), | ||
let body = funcDecl.body, | ||
let `return` = containsSingleReturn(body.statements) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it reads a little better if these variables were named returnStmt
, subscriptDecl
, instead of escaping just the keyword. `return`
is a little ambiguous because it's not clear if it's referring to the statement or the keyword token.
|
||
diagnose(.omitReturnStatement, on: `return`, severity: .refactoring) | ||
|
||
return .init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point soon I'll be working on a modernization pass of the code base that starts doing regular mutation on nodes. Can you change this to just mutate a copy of the node and return that? For example,
var newBlock = accessorBlock
accessorBlock.accessors = .accessors(accessors.with(\.[getterAt], getter)
return newBlock
Likewise for the other branch below. This makes it easier to scan quickly and see what part of the node is being changed and what parts are staying the same.
/// Format: `func <name>() { return ... }` constructs will be replaced with | ||
/// equivalent `func <name>() { ... }` constructs. | ||
@_spi(Rules) | ||
public final class OmitReturns: SyntaxFormatRule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use OmitSingleExpressionReturns
as the rule name instead, so it's a little more self-descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more confusing to me, maybe OmitExplicitReturns
? Verbose version is OmitExplicitReturnsFromSingleExpressionBodies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OmitExplicitReturns
sounds good to me!
|
||
extension Finding.Message { | ||
public static let omitReturnStatement: Finding.Message = | ||
"`return` can be omitted because body consists of a single expression" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"`return` can be omitted because body consists of a single expression" | |
"remove 'return' because the body is a single expression" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(As a style choice, we use single quotes instead of backticks, since that matched Swift compiler error messages at the time. However, it also looks like diagnostics have been introduced since then that use backticks...)
return nil | ||
} | ||
|
||
private func unwrapReturnStmt(_ `return`: ReturnStmtSyntax) -> CodeBlockItemListSyntax { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rewrapReturnedExpression
for the name of this method? It's doing more than just unwrapping it, it's also wrapping it back up in a code block item list.
Thank you, I'll try to address this today! |
@allevato I think I addressed all of the style/naming comments, please let me know if I missed something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
This is an opt-in rule that would warn about a possible refactoring in lint mode and drop returns from single-expression bodies of functions, closures, subscripts and computed variables/properties.