diff --git a/internal/engine.go b/internal/engine.go index 6338aca..bea830f 100644 --- a/internal/engine.go +++ b/internal/engine.go @@ -52,6 +52,7 @@ var allRuleConstructors = ruleMap{ "gno-mod-tidy": NewMissingModPackageRule, "slice-bounds-check": NewSliceBoundCheckRule, "const-error-declaration": NewConstErrorDeclarationRule, + "error-check": NewErrCheckRule, } func (e *Engine) applyRules(rules map[string]tt.ConfigRule) { diff --git a/internal/lints/err_check.go b/internal/lints/err_check.go new file mode 100644 index 0000000..35ca514 --- /dev/null +++ b/internal/lints/err_check.go @@ -0,0 +1,186 @@ +package lints + +import ( + "go/ast" + "go/token" + "strings" + + tt "github.com/gnolang/tlin/internal/types" +) + +func DetectErrCheck(filename string, node *ast.File, fset *token.FileSet, severity tt.Severity) ([]tt.Issue, error) { + v := &errCheckVisitor{ + filename: filename, + fset: fset, + severity: severity, + imports: make(map[string]bool), + errorVars: make(map[string]bool), + errorChecks: make(map[string]bool), + } + + // 1st pass: collect error variable declarations and checks + ast.Inspect(node, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.AssignStmt: + v.collectErrorVars(x) + case *ast.IfStmt: + v.collectErrorChecks(x) + } + return true + }) + + // 2nd pass: check for unchecked errors + ast.Walk(v, node) + return v.issues, nil +} + +type errCheckVisitor struct { + filename string + fset *token.FileSet + severity tt.Severity + issues []tt.Issue + imports map[string]bool + errorVars map[string]bool // tracks declared error variables + errorChecks map[string]bool // tracks which error vars have been checked + currentFunc *ast.FuncDecl // tracks current function being analyzed +} + +func (v *errCheckVisitor) collectErrorVars(assign *ast.AssignStmt) { + for _, lhs := range assign.Lhs { + if ident, ok := lhs.(*ast.Ident); ok { + if ident.Name == "err" || strings.HasSuffix(ident.Name, "Error") { + v.errorVars[ident.Name] = true + } + } + } +} + +func (v *errCheckVisitor) collectErrorChecks(ifStmt *ast.IfStmt) { + // check `err != nil` pattern + if binExpr, ok := ifStmt.Cond.(*ast.BinaryExpr); ok { + if ident, ok := binExpr.X.(*ast.Ident); ok { + if v.errorVars[ident.Name] { + v.errorChecks[ident.Name] = true + } + } + } +} + +func (v *errCheckVisitor) Visit(node ast.Node) ast.Visitor { + if node == nil { + return v + } + + switch n := node.(type) { + case *ast.FuncDecl: + v.currentFunc = n + defer func() { v.currentFunc = nil }() + case *ast.AssignStmt: + v.checkAssignment(n) + case *ast.ExprStmt: + v.checkExprStmt(n) + } + + return v +} + +func (v *errCheckVisitor) checkAssignment(assign *ast.AssignStmt) { + // multi-return cases + if len(assign.Rhs) == 1 && len(assign.Lhs) > 1 { + if call, ok := assign.Rhs[0].(*ast.CallExpr); ok { + v.checkMultiValueAssign(assign.Lhs, call) + } + return + } + + // single assignments + for i, rhs := range assign.Rhs { + if i >= len(assign.Lhs) { + continue + } + if call, ok := rhs.(*ast.CallExpr); ok && v.isLikelyErrorReturn(call) { + lhs := assign.Lhs[i] + if isBlankIdentifier(lhs) { + v.addIssue( + assign.Pos(), assign.End(), + "error return value is ignored with blank identifier", + "consider handling the error value", + ) + } + } + } +} + +func (v *errCheckVisitor) checkMultiValueAssign(lhs []ast.Expr, call *ast.CallExpr) { + if !v.isLikelyErrorReturn(call) { + return + } + + // check if the last return value (typically error) is properly handled + if len(lhs) >= 2 { + lastExpr := lhs[len(lhs)-1] + if isBlankIdentifier(lastExpr) { + v.addIssue( + call.Pos(), call.End(), + "error return value is ignored with blank identifier", + "consider handling the error value", + ) + } + } +} + +func (v *errCheckVisitor) checkExprStmt(expr *ast.ExprStmt) { + if call, ok := expr.X.(*ast.CallExpr); ok && v.isLikelyErrorReturn(call) { + // ignore certain method calls that are commonly used without error checking + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + if sel.Sel.Name == "Close" || sel.Sel.Name == "Print" || sel.Sel.Name == "Println" { + return + } + } + + v.addIssue( + expr.Pos(), expr.End(), + "error-returning function call's result is ignored", + "add error handling or explicitly assign the error", + ) + } +} + +func (v *errCheckVisitor) isLikelyErrorReturn(call *ast.CallExpr) bool { + switch fun := call.Fun.(type) { + case *ast.Ident: + // common functions that don't return errors + switch fun.Name { + case "print", "println", "len", "cap", "append", "recover", "panic": + return false + } + case *ast.SelectorExpr: + // common error-returning methods + switch fun.Sel.Name { + case "Error", "String", "GoString": + return false + case "Close", "Write", "Read", "Scan", "Parse", "Open", "Create", + "Delete", "Update", "Exec", "Query": + return true + } + } + return true +} + +func isBlankIdentifier(expr ast.Expr) bool { + ident, ok := expr.(*ast.Ident) + return ok && ident.Name == "_" +} + +func (v *errCheckVisitor) addIssue(start, end token.Pos, msg, note string) { + issue := tt.Issue{ + Rule: "error-check", + Filename: v.filename, + Start: v.fset.Position(start), + End: v.fset.Position(end), + Message: msg, + Note: note, + Severity: v.severity, + } + v.issues = append(v.issues, issue) +} diff --git a/internal/lints/err_check_test.go b/internal/lints/err_check_test.go new file mode 100644 index 0000000..64cc115 --- /dev/null +++ b/internal/lints/err_check_test.go @@ -0,0 +1,154 @@ +package lints + +import ( + "os" + "path/filepath" + "testing" + + "github.com/gnolang/tlin/internal/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDetectErrCheck(t *testing.T) { + t.Parallel() + tests := []struct { + name string + code string + expected int + messages []string + }{ + { + name: "Simple error ignored", + code: ` +package main +import "os" +func main() { + f, _ := os.Open("file.txt") + defer f.Close() +}`, + expected: 1, + messages: []string{ + "error return value is ignored with blank identifier", + }, + }, + { + name: "Simple error ignored 2", + code: ` +package main + +import "errors" + +func foo() (int, error) { + return 0, errors.New("test") +} + +func main() { + v, _ := foo() + println(v) +}`, + expected: 1, + messages: []string{ + "error return value is ignored with blank identifier", + }, + }, + { + name: "Error not checked after assignment", + code: ` +package main +import "os" +func main() { + var err error + f, err := os.Open("file.txt") + f.Write([]byte("test")) + defer f.Close() +}`, + expected: 1, + messages: []string{ + "error-returning function call's result is ignored", + }, + }, + { + name: "Proper error handling", + code: ` +package main + +import "errors" + +func foo() (int, error) { + return 0, errors.New("test") +} + +func main() { + v, err := foo() + if err != nil { + panic(err) + } + println(v) +}`, + expected: 0, + messages: []string{}, + }, + { + name: "Multiple error ignoring patterns", + code: ` +package main +import ( + "os" + "encoding/json" +) +func main() { + f, _ := os.Open("file.txt") + json.Unmarshal([]byte("{}"), &struct{}{}) + f.Write([]byte("test")) +}`, + expected: 3, + messages: []string{ + "error return value is ignored with blank identifier", + "error-returning function call's result is ignored", + "error-returning function call's result is ignored", + }, + }, + { + name: "Non-error returning functions", + code: ` +package main +func main() { + println("test") + x := len("test") + _ = cap([]int{1,2,3}) +}`, + expected: 0, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + tmpDir, err := os.MkdirTemp("", "lint-test") + require.NoError(t, err) + defer os.RemoveAll(tmpDir) + + tmpfile := filepath.Join(tmpDir, "test.go") + err = os.WriteFile(tmpfile, []byte(tt.code), 0o644) + require.NoError(t, err) + + node, fset, err := ParseFile(tmpfile, nil) + require.NoError(t, err) + + issues, err := DetectErrCheck(tmpfile, node, fset, types.SeverityError) + require.NoError(t, err) + + assert.Equal(t, tt.expected, len(issues), + "Number of detected issues doesn't match expected") + + if len(issues) > 0 { + for i, issue := range issues { + assert.Equal(t, "error-check", issue.Rule) + assert.Contains(t, issue.Message, tt.messages[i]) + } + } + }) + } +} diff --git a/internal/rule_set.go b/internal/rule_set.go index edb4650..c8c9216 100644 --- a/internal/rule_set.go +++ b/internal/rule_set.go @@ -365,6 +365,32 @@ func (r *ConstErrorDeclarationRule) Severity() tt.Severity { return r.severity } +type ErrCheckRule struct { + severity tt.Severity +} + +func NewErrCheckRule() LintRule { + return &ErrCheckRule{ + severity: tt.SeverityWarning, + } +} + +func (r *ErrCheckRule) Check(filename string, node *ast.File, fset *token.FileSet) ([]tt.Issue, error) { + return lints.DetectErrCheck(filename, node, fset, r.severity) +} + +func (r *ErrCheckRule) Name() string { + return "error-check" +} + +func (r *ErrCheckRule) Severity() tt.Severity { + return r.severity +} + +func (r *ErrCheckRule) SetSeverity(severity tt.Severity) { + r.severity = severity +} + // ----------------------------------------------------------------------------- // Regex related rules diff --git a/testdata/error-check/ec0.gno b/testdata/error-check/ec0.gno new file mode 100644 index 0000000..6f48a2e --- /dev/null +++ b/testdata/error-check/ec0.gno @@ -0,0 +1,12 @@ +package main + +import "errors" + +func foo() (int, error) { + return 0, errors.New("test") +} + +func main() { + v, _ := foo() + println(v) +} diff --git a/testdata/error-check/ec1.gno b/testdata/error-check/ec1.gno new file mode 100644 index 0000000..81d75e0 --- /dev/null +++ b/testdata/error-check/ec1.gno @@ -0,0 +1,15 @@ +package main + +import "errors" + +func foo() (int, error) { + return 0, errors.New("test") +} + +func main() { + v, err := foo() + if err != nil { + panic(err) + } + println(v) +} diff --git a/testdata/pkg/pkg1.gno b/testdata/pkg/pkg1.gno index 6bc2f8f..af614dc 100644 --- a/testdata/pkg/pkg1.gno +++ b/testdata/pkg/pkg1.gno @@ -5,5 +5,6 @@ import ( ) func main() { + //nolint:error-check strings.Contains("foo", "o") } \ No newline at end of file