refactor: use a more straightforward return value #9519
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks for the changes. Looks like you're detecting this in other repos as well, so my question here: Did you find these using a script? If yes, are these the only occurrences in this repo? |
Yes, this is a detection program I use, but my logic may have some problems, and there will be a lot of false positives at present. Please wait for me to optimize the code and do more accurate positioning func checkFuncForRedundantReturn(fset *token.FileSet, funcDecl *ast.FuncDecl, path string) {
if funcDecl.Body == nil {
return
}
var lastIfStmt *ast.IfStmt
var lastReturnStmt *ast.ReturnStmt
var errReassigned bool = false
// 首先找到最后一个 return 语句
ast.Inspect(funcDecl.Body, func(n ast.Node) bool {
switch node := n.(type) {
case *ast.IfStmt:
if isErrorCheck(node.Cond) {
lastIfStmt = node
}
case *ast.AssignStmt:
// 检查是否有对 err 的重新赋值
for _, lhs := range node.Lhs {
if ident, ok := lhs.(*ast.Ident); ok {
if ident.Name == "err" {
// 找到了对 err 的重新赋值，并且这个赋值发生在最后一个错误检查之后
if lastIfStmt != nil && node.Pos() > lastIfStmt.Pos() {
errReassigned = true
}
}
}
}
case *ast.ReturnStmt:
lastReturnStmt = node
}
return true
})
// 如果最后一个 return 包含 err 且在 if err != nil 之后，并且没有对 err 重新赋值
if lastReturnStmt != nil && lastIfStmt != nil {
if !isInsideIfBlock(lastReturnStmt, lastIfStmt) &&
containsErrorIdent(lastReturnStmt) &&
!errReassigned { // 添加这个条件
// 检查这个 return 是否是函数的最后一个语句
if isLastStatement(funcDecl.Body, lastReturnStmt) {
pos := fset.Position(lastReturnStmt.Pos())
fmt.Printf("%s:%d: 发现函数末尾冗余的错误返回语句\n", path, pos.Line)
}
}
}
}
// 检查是否是 err != nil 的条件
func isErrorCheck(expr ast.Expr) bool {
if binaryExpr, ok := expr.(*ast.BinaryExpr); ok {
if binaryExpr.Op == token.NEQ {
if ident, ok := binaryExpr.X.(*ast.Ident); ok {
if ident.Name == "err" || ident.Name == "e" {
if nilLit, ok := binaryExpr.Y.(*ast.Ident); ok {
return nilLit.Name == "nil"
}
}
}
}
}
return false
}
// 检查返回语句是否在 if 块内
func isInsideIfBlock(returnStmt *ast.ReturnStmt, ifStmt *ast.IfStmt) bool {
start := ifStmt.Body.Pos()
end := ifStmt.Body.End()
return returnStmt.Pos() >= start && returnStmt.Pos() <= end
}
// 检查返回语句是否包含 err 标识符
func containsErrorIdent(returnStmt *ast.ReturnStmt) bool {
for _, result := range returnStmt.Results {
if ident, ok := result.(*ast.Ident); ok {
if ident.Name == "err" || ident.Name == "e" {
return true
}
}
}
return false
}
// 检查语句是否是函数体的最后一个语句
func isLastStatement(body *ast.BlockStmt, stmt ast.Stmt) bool {
if len(body.List) == 0 {
return false
}
return body.List[len(body.List)-1] == stmt
} |
|
The false postive like this: // GetClientSession loads the ClientSession with the given ID from the DB.
func (c *ClientDB) GetClientSession(id SessionID,
opts ...ClientSessionListOption) (*ClientSession, error) {
var sess *ClientSession
err := kvdb.View(c.db, func(tx kvdb.RTx) error {
sessionsBkt := tx.ReadBucket(cSessionBkt)
if sessionsBkt == nil {
return ErrUninitializedDB
}
chanIDIndexBkt := tx.ReadBucket(cChanIDIndexBkt)
if chanIDIndexBkt == nil {
return ErrUninitializedDB
}
session, err := c.getClientSession(
sessionsBkt, chanIDIndexBkt, id[:], opts...,
)
if err != nil {
return err
}
sess = session
return nil
}, func() {})
return sess, err
} |
Thanks for your patience. @Reallyfe @guggero I have found more cases like this, and I believe there won't be any more such cases. |
|
@fuyangpengqi nice code. Can you write this code to a linter ? we need it also. |
Thanks! |
Ok. Do you mean submitting code to the golanglint-ci project? I am not very familiar with the process of this project, but maybe a try. Thanks. |
Yes, I think your idea is worth to create a single linter and add it to golangci-lint, I'm also doing this kind of code clean in my job, so I need such linter. If you don't mind, we can talk more via wechat, or email or other channel 😂. |
Okay, how can I contact you? |
|
Sorry to bother, how can I trigger CI again without making a new commit? I want to check whether the error is accidental or inevitable. |
|
You can run |
|
yeah in addition you can pull the master and perform a rebase, then push it again, which will trigger the CI to run again. |
Signed-off-by: fuyangpengqi <[email protected]>
It seems that the CI error is no longer related to this submission. One of them is a timeout problem, as shown below. --- FAIL: TestLightningWallet (68.66s)
--- FAIL: TestLightningWallet/btcwallet/neutrino:reorg_wallet_balance (14.63s)
test_interface.go:2280: unable to sync wallet: timeout after 30s
FAILPlease review it again. |
Thanks! |
Change Description
use a more straightforward return value
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.