From 3c64cc3a7547109d5b0d1d56220c99028dd30bdc Mon Sep 17 00:00:00 2001 From: Cyril de Catheu Date: Fri, 17 Apr 2026 17:34:24 +0200 Subject: [PATCH 1/3] feat: add close check for driver.Rows --- README.md | 12 +- main.go | 4 +- passes/chbatchclose/chbatchclose_test.go | 13 -- .../chbatchclose.go => chclose/chclose.go} | 79 +++++++----- passes/chclose/chclose_test.go | 13 ++ .../clickhouse-go/v2/lib/driver/driver.go | 13 ++ .../testdata/src/testcases/testcases.go | 116 +++++++++++++++++- 7 files changed, 197 insertions(+), 53 deletions(-) delete mode 100644 passes/chbatchclose/chbatchclose_test.go rename passes/{chbatchclose/chbatchclose.go => chclose/chclose.go} (60%) create mode 100644 passes/chclose/chclose_test.go rename passes/{chbatchclose => chclose}/testdata/src/github.com/ClickHouse/clickhouse-go/v2/lib/driver/driver.go (50%) rename passes/{chbatchclose => chclose}/testdata/src/testcases/testcases.go (67%) diff --git a/README.md b/README.md index 389b3c1..3c881ab 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ See [documentation](https://clickhouse.com/docs/integrations/go#the-clickhouse-g The linter detects 2 common implementation mistakes: - forgetting to call `rows.Err()` after calling `rows.Next()` -- forgetting to call `defer batch.Close()` when using `Batch` +- forgetting to call `defer xxx.Close()` when using `Batch` or `Rows` These implementation mistakes are often observed in codebases using the clickhouse-go driver. They result in incomplete data and hard-to-troubleshoot bugs. @@ -55,6 +55,7 @@ Incorrect code: ```go parsedRows := []... rows, _ := conn.Query(ctx, "SELECT ...") +defer rows.Close() for rows.Next() { parsedRow, err = .... if err { @@ -74,6 +75,7 @@ return parsedRows, nil ```go parsedRows := []... rows, _ := conn.Query(ctx, "SELECT ...") +defer rows.Close() for rows.Next() { parsed, err = .... if err { @@ -130,8 +132,10 @@ There are some limitations: --> in this case the linter will not be able to detect that `.Err()` may be called too late. -## 2. chbatchclose -Detect when a `github.com/ClickHouse/clickhouse-go/v2/lib/driver` `Batch` variable is used but no associated `defer batch.Close()` is called. +## 2. chclose +Detect when a `github.com/ClickHouse/clickhouse-go/v2/lib/driver`: `Batch` / `Rows` variable is used but no associated `defer xxx.Close()` is called. + +*All examples below are given for `Batch` but apply the same way for `Rows`.* Calling `defer batch.Close()` is highly recommended (see [here](https://clickhouse.com/docs/integrations/go#batch-insert)). While it is possible to not use this statement, code that do not use it often end up with subtle leaks, as the @@ -180,6 +184,8 @@ return batch.Send() ### Rule details +*All examples below are given for `Batch` but apply the same way for `Rows`.* + The linter goes through every function. If a clickhouse driver `Batch` is instantiated and is not part of the values returned by the function, a `defer batch.Close()` must be found. Also, assigning a `Batch` to the blank identifier `_` is flagged. diff --git a/main.go b/main.go index 9eb2036..a43a3ae 100644 --- a/main.go +++ b/main.go @@ -1,11 +1,11 @@ package main import ( - "github.com/ClickHouse/clickhouse-go-linter/passes/chbatchclose" + "github.com/ClickHouse/clickhouse-go-linter/passes/chclose" "github.com/ClickHouse/clickhouse-go-linter/passes/chrowserr" "golang.org/x/tools/go/analysis/multichecker" ) func main() { - multichecker.Main(chrowserr.NewAnalyzer(), chbatchclose.NewAnalyzer()) + multichecker.Main(chrowserr.NewAnalyzer(), chclose.NewAnalyzer()) } diff --git a/passes/chbatchclose/chbatchclose_test.go b/passes/chbatchclose/chbatchclose_test.go deleted file mode 100644 index 2d92ac2..0000000 --- a/passes/chbatchclose/chbatchclose_test.go +++ /dev/null @@ -1,13 +0,0 @@ -package chbatchclose_test - -import ( - "testing" - - "golang.org/x/tools/go/analysis/analysistest" - - "github.com/ClickHouse/clickhouse-go-linter/passes/chbatchclose" -) - -func TestAnalyzer(t *testing.T) { - analysistest.Run(t, analysistest.TestData(), chbatchclose.NewAnalyzer(), "testcases") -} diff --git a/passes/chbatchclose/chbatchclose.go b/passes/chclose/chclose.go similarity index 60% rename from passes/chbatchclose/chbatchclose.go rename to passes/chclose/chclose.go index 3da2c55..0e8e6c3 100644 --- a/passes/chbatchclose/chbatchclose.go +++ b/passes/chclose/chclose.go @@ -1,4 +1,4 @@ -package chbatchclose +package chclose import ( "go/ast" @@ -24,8 +24,8 @@ func NewAnalyzer() *analysis.Analyzer { debug: debug, } return &analysis.Analyzer{ - Name: "chbatchclosecheck", - Doc: "chbatchclosecheck checks whether defer batch.Close() is called on ClickHouse driver Batch variables", + Name: "chclosecheck", + Doc: "chclosecheck checks whether defer xxx.Close() is called on ClickHouse driver Batch/Rows variables", Run: a.run, Requires: []*analysis.Analyzer{inspect.Analyzer}, } @@ -57,31 +57,35 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { return nil, nil } -// batchUsage tracks whether a driver.Batch variable has a defer Close/Abort or is returned. -type batchUsage struct { +// closableTypes lists the ClickHouse driver types that must be closed defensively. +var closableTypes = []string{"Batch", "Rows"} + +// closableUsage tracks whether a driver.Batch / driver.Rows variable has a defer Close() or is returned. +type closableUsage struct { + typeName string // "Batch" or "Rows" assignPos token.Pos deferredClose bool returned bool } -func (b *batchUsage) report(varName string, pass *analysis.Pass, debug bool) { - if b.assignPos == token.NoPos { +func (u *closableUsage) report(varName string, pass *analysis.Pass, debug bool) { + if u.assignPos == token.NoPos { // no usage of Batch return } - if !b.deferredClose && !b.returned { - pass.Reportf(b.assignPos, - "clickhouse Batch %s must be closed defensively with defer %s.Close() after successful instantiation", - varName, varName) + if !u.deferredClose && !u.returned { + pass.Reportf(u.assignPos, + "clickhouse %s %s must be closed defensively with defer %s.Close() after successful instantiation", + u.typeName, varName, varName) } else if debug { - if b.deferredClose { - pass.Reportf(b.assignPos, - "clickhouse Batch %s is properly closed defensively after successful instantiation [valid]", - varName) + if u.deferredClose { + pass.Reportf(u.assignPos, + "clickhouse %s %s is properly closed defensively after successful instantiation [valid]", + u.typeName, varName) } else { - pass.Reportf(b.assignPos, - "clickhouse Batch %s is returned by the function [valid]", - varName) + pass.Reportf(u.assignPos, + "clickhouse %s %s is returned by the function [valid]", + u.typeName, varName) } } } @@ -90,7 +94,7 @@ func (b *batchUsage) report(varName string, pass *analysis.Pass, debug bool) { // It does a single-pass collection of Batch assignments, defer Close/Abort calls, and return statements. // It does not descend into nested closures (they are handled as separate units by the Preorder visitor above). func (a *analyzer) checkFunc(pass *analysis.Pass, body *ast.BlockStmt) { - usages := map[string]*batchUsage{} + usages := map[string]*closableUsage{} ast.Inspect(body, func(n ast.Node) bool { if n == nil { @@ -121,33 +125,40 @@ func (a *analyzer) checkFunc(pass *analysis.Pass, body *ast.BlockStmt) { } } -// handleAssign checks if any LHS variable in the assignment is of type driver.Batch. +// handleAssign checks if any LHS variable in the assignment is a closable ClickHouse driver type. // If a tracked variable is reassigned, it flushes/reports the previous tracking first. -func (a *analyzer) handleAssign(pass *analysis.Pass, assign *ast.AssignStmt, usages map[string]*batchUsage) { +func (a *analyzer) handleAssign(pass *analysis.Pass, assign *ast.AssignStmt, usages map[string]*closableUsage) { for _, lhs := range assign.Lhs { name := util.IdentName(lhs) if name == "" { continue } - if name == "_" && util.IsChObj(pass, lhs, "Batch") { - pass.Reportf(assign.Pos(), "clickhouse Batch assigned to blank identifier. Connection leak. clickhouse Batch must be instantiated and closed defensively with defer batch.Close() after successful instantiation") - continue - } - // if this var was already tracked, flush previous usage before re-tracking - if u, ok := usages[name]; ok { - u.report(name, pass, a.debug) - delete(usages, name) - } + for _, typeName := range closableTypes { + if !util.IsChObj(pass, lhs, typeName) { + continue + } + + if name == "_" { + pass.Reportf(assign.Pos(), "clickhouse %s assigned to blank identifier. Connection leak. clickhouse %s must be instantiated and closed defensively with defer %s.Close() after successful instantiation", + typeName, typeName, typeName) + break + } + + // if this var was already tracked, flush previous usage before re-tracking + if u, ok := usages[name]; ok { + u.report(name, pass, a.debug) + delete(usages, name) + } - if util.IsChObj(pass, lhs, "Batch") { - usages[name] = &batchUsage{assignPos: assign.Pos()} + usages[name] = &closableUsage{typeName: typeName, assignPos: assign.Pos()} + break } } } // handleDefer checks if a defer statement calls Close() or Abort() on a tracked Batch variable. -func handleDefer(deferStmt *ast.DeferStmt, usages map[string]*batchUsage) { +func handleDefer(deferStmt *ast.DeferStmt, usages map[string]*closableUsage) { call := deferStmt.Call sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { @@ -169,7 +180,7 @@ func handleDefer(deferStmt *ast.DeferStmt, usages map[string]*batchUsage) { } // handleReturn checks if any return value is a tracked Batch variable. -func handleReturn(ret *ast.ReturnStmt, usages map[string]*batchUsage) { +func handleReturn(ret *ast.ReturnStmt, usages map[string]*closableUsage) { for _, result := range ret.Results { name := util.IdentName(result) if name == "" { diff --git a/passes/chclose/chclose_test.go b/passes/chclose/chclose_test.go new file mode 100644 index 0000000..287a749 --- /dev/null +++ b/passes/chclose/chclose_test.go @@ -0,0 +1,13 @@ +package chclose_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/ClickHouse/clickhouse-go-linter/passes/chclose" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), chclose.NewAnalyzer(), "testcases") +} diff --git a/passes/chbatchclose/testdata/src/github.com/ClickHouse/clickhouse-go/v2/lib/driver/driver.go b/passes/chclose/testdata/src/github.com/ClickHouse/clickhouse-go/v2/lib/driver/driver.go similarity index 50% rename from passes/chbatchclose/testdata/src/github.com/ClickHouse/clickhouse-go/v2/lib/driver/driver.go rename to passes/chclose/testdata/src/github.com/ClickHouse/clickhouse-go/v2/lib/driver/driver.go index 6e96b25..3e0e7ad 100644 --- a/passes/chbatchclose/testdata/src/github.com/ClickHouse/clickhouse-go/v2/lib/driver/driver.go +++ b/passes/chclose/testdata/src/github.com/ClickHouse/clickhouse-go/v2/lib/driver/driver.go @@ -10,6 +10,19 @@ type Batch interface { Rows() int } +type Rows interface { + Next() bool + Scan(dest ...any) error + ScanStruct(dest any) error + // ColumnTypes() []ColumnType + Totals(dest ...any) error + Columns() []string + Close() error + Err() error + HasData() bool +} + type Conn interface { PrepareBatch(ctx any, query string, opts ...any) (Batch, error) + Query(ctx any, query string, args ...any) (Rows, error) } diff --git a/passes/chbatchclose/testdata/src/testcases/testcases.go b/passes/chclose/testdata/src/testcases/testcases.go similarity index 67% rename from passes/chbatchclose/testdata/src/testcases/testcases.go rename to passes/chclose/testdata/src/testcases/testcases.go index 7c0b951..9c05825 100644 --- a/passes/chbatchclose/testdata/src/testcases/testcases.go +++ b/passes/chclose/testdata/src/testcases/testcases.go @@ -9,6 +9,8 @@ import ( var conn driver.Conn var ctx = context.Background() +// ===== Batch tests ===== + // valid: defer Close() after PrepareBatch func validDeferClose() { batch, err := conn.PrepareBatch(ctx, "INSERT INTO t") @@ -61,7 +63,7 @@ func validFromHelper() { // invalid assignment to blank identifier. // unlikely to exist in real code base but results in a connection leak. func toBlankIdentifier() { - _, err := conn.PrepareBatch(ctx, "INSERT INTO t") // want `clickhouse Batch assigned to blank identifier. Connection leak. clickhouse Batch must be instantiated and closed defensively with defer batch\.Close\(\) after successful instantiation` + _, err := conn.PrepareBatch(ctx, "INSERT INTO t") // want `clickhouse Batch assigned to blank identifier. Connection leak. clickhouse Batch must be instantiated and closed defensively with defer Batch\.Close\(\) after successful instantiation` if err != nil { return } @@ -209,3 +211,115 @@ func invalidNotDefensive() error { } return nil } + +// ===== Rows tests ===== + +// valid: defer Close() after Query +func rowsValidDeferClose() { + rows, err := conn.Query(ctx, "SELECT 1") + if err != nil { + return + } + defer rows.Close() + for rows.Next() { + var v int + _ = rows.Scan(&v) + } +} + +// valid: rows returned to caller +func rowsValidReturn() (driver.Rows, error) { + rows, err := conn.Query(ctx, "SELECT 1") + if err != nil { + return nil, err + } + return rows, nil +} + +// valid: rows returned to caller "without instantiation" +func helperReturningRows() (driver.Rows, error) { + return conn.Query(ctx, "SELECT 1") +} + +// valid: defer Close() after Rows instantiated from helper method +func rowsValidFromHelper() { + rows, err := helperReturningRows() + if err != nil { + return + } + defer rows.Close() + for rows.Next() { + var v int + _ = rows.Scan(&v) + } +} + +// invalid: no defer, no return +func rowsInvalidNoDeferNoReturn() { + rows, err := conn.Query(ctx, "SELECT 1") //want `clickhouse Rows rows must be closed defensively with defer rows\.Close\(\) after successful instantiation` + if err != nil { + return + } + for rows.Next() { + var v int + _ = rows.Scan(&v) + } +} + +// invalid assignment to blank identifier +func rowsToBlankIdentifier() { + _, err := conn.Query(ctx, "SELECT 1") // want `clickhouse Rows assigned to blank identifier. Connection leak. clickhouse Rows must be instantiated and closed defensively with defer Rows\.Close\(\) after successful instantiation` + if err != nil { + return + } +} + +// invalid: rows from helper function, no defer, no return +func rowsInvalidFromHelper() { + rows, err := helperReturningRows() //want `clickhouse Rows rows must be closed defensively with defer rows\.Close\(\) after successful instantiation` + if err != nil { + return + } + for rows.Next() { + var v int + _ = rows.Scan(&v) + } +} + +// reassignment: first has no defer (invalid), second has defer (valid) +func rowsReassignInvalidValid() { + rows, err := conn.Query(ctx, "SELECT 1") //want `clickhouse Rows rows must be closed defensively with defer rows\.Close\(\) after successful instantiation` + if err != nil { + return + } + _ = rows.Err() + + rows, err = conn.Query(ctx, "SELECT 2") + if err != nil { + return + } + defer rows.Close() +} + +// reassignment: first has defer (valid), second has no defer (invalid) +func rowsReassignValidInvalid() { + rows, err := conn.Query(ctx, "SELECT 1") + if err != nil { + return + } + defer rows.Close() + + rows, err = conn.Query(ctx, "SELECT 2") //want `clickhouse Rows rows must be closed defensively with defer rows\.Close\(\) after successful instantiation` + if err != nil { + return + } +} + +// closures are not supported +func rowsDeferCloseIsInClosure() { + rows, err := conn.Query(ctx, "SELECT 1") //want `clickhouse Rows rows must be closed defensively with defer rows\.Close\(\) after successful instantiation` + if err != nil { + return + } + defer func() { rows.Close() }() +} From c44c0e2c889e2564310766c1974fe08db19dd866 Mon Sep 17 00:00:00 2001 From: Cyril de Catheu Date: Fri, 17 Apr 2026 18:05:04 +0200 Subject: [PATCH 2/3] fix false positive on wrapped rows/batch --- passes/chclose/chclose.go | 20 ++++--- .../testdata/src/testcases/testcases.go | 52 +++++++++++++++++++ 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/passes/chclose/chclose.go b/passes/chclose/chclose.go index 0e8e6c3..031a653 100644 --- a/passes/chclose/chclose.go +++ b/passes/chclose/chclose.go @@ -179,15 +179,19 @@ func handleDefer(deferStmt *ast.DeferStmt, usages map[string]*closableUsage) { } } -// handleReturn checks if any return value is a tracked Batch variable. +// handleReturn checks if any return expression contains a tracked variable, +// including when wrapped in a function call or struct literal (e.g. return NewWrapper(rows), nil). func handleReturn(ret *ast.ReturnStmt, usages map[string]*closableUsage) { for _, result := range ret.Results { - name := util.IdentName(result) - if name == "" { - continue - } - if u, exists := usages[name]; exists { - u.returned = true - } + ast.Inspect(result, func(n ast.Node) bool { + id, ok := n.(*ast.Ident) + if !ok { + return true + } + if u, exists := usages[id.Name]; exists { + u.returned = true + } + return true + }) } } diff --git a/passes/chclose/testdata/src/testcases/testcases.go b/passes/chclose/testdata/src/testcases/testcases.go index 9c05825..6fe530e 100644 --- a/passes/chclose/testdata/src/testcases/testcases.go +++ b/passes/chclose/testdata/src/testcases/testcases.go @@ -49,6 +49,32 @@ func helperReturningBatch() (driver.Batch, error) { return conn.PrepareBatch(ctx, "INSERT INTO t") } +type batchWrapper struct { + driver.Batch +} + +func wrapBatch(b driver.Batch) *batchWrapper { + return &batchWrapper{b} +} + +// valid: batch returned wrapped in a function call +func validReturnWrappedInCall() (*batchWrapper, error) { + batch, err := conn.PrepareBatch(ctx, "INSERT INTO t") + if err != nil { + return nil, err + } + return wrapBatch(batch), nil +} + +// valid: batch returned wrapped in a struct literal +func validReturnWrappedInStruct() (*batchWrapper, error) { + batch, err := conn.PrepareBatch(ctx, "INSERT INTO t") + if err != nil { + return nil, err + } + return &batchWrapper{batch}, nil +} + // valid: defer Close() after Batch is instantiated from helper method func validFromHelper() { batch, err := helperReturningBatch() @@ -241,6 +267,32 @@ func helperReturningRows() (driver.Rows, error) { return conn.Query(ctx, "SELECT 1") } +type rowsWrapper struct { + driver.Rows +} + +func wrapRows(r driver.Rows) *rowsWrapper { + return &rowsWrapper{r} +} + +// valid: rows returned wrapped in a function call +func rowsValidReturnWrappedInCall() (*rowsWrapper, error) { + rows, err := conn.Query(ctx, "SELECT 1") + if err != nil { + return nil, err + } + return wrapRows(rows), nil +} + +// valid: rows returned wrapped in a struct literal +func rowsValidReturnWrappedInStruct() (*rowsWrapper, error) { + rows, err := conn.Query(ctx, "SELECT 1") + if err != nil { + return nil, err + } + return &rowsWrapper{rows}, nil +} + // valid: defer Close() after Rows instantiated from helper method func rowsValidFromHelper() { rows, err := helperReturningRows() From e5d60cde3dc7a623b03a75e5326632c36834c242 Mon Sep 17 00:00:00 2001 From: Cyril de Catheu Date: Wed, 6 May 2026 12:02:39 +0200 Subject: [PATCH 3/3] wip --- README.md | 12 ++- passes/chclose/chclose.go | 90 ++++++++++++++++++- .../testdata/src/testcases/testcases.go | 36 ++++++-- 3 files changed, 125 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 3c881ab..b8dbe16 100644 --- a/README.md +++ b/README.md @@ -189,12 +189,16 @@ return batch.Send() The linter goes through every function. If a clickhouse driver `Batch` is instantiated and is not part of the values returned by the function, a `defer batch.Close()` must be found. Also, assigning a `Batch` to the blank identifier `_` is flagged. -Variable re-assignments and intertwined variable are supported. See [testcases.go](passes/chbatchclose/testdata/src/testcases/testcases.go). +Variable re-assignments and intertwined variable are supported. See [testcases.go](passes/chclose/testdata/src/testcases/testcases.go). There are some limitations: -- except for looking into defer blocks;, the linter does not cross function block boundaries. If a `Batch` variable is instantiated and `batch.Close()` is called in a - closure inside the defer call, the linter will not be able to associate the `batch.Close()` to the variable. - (note: in most cases such pattern is a bad idea). See `deferCloseIsInClosure` test case. +- linting of structs wrapping or embedding a `Batch` is not supported +- tracking across function calls is not supported: + - if a `Batch` variable is instantiated and returned by a function call `return logAndReturn(batch)`, + the linter cannot know the batch is part of the values returned. + - If a `Batch` variable is instantiated and `batch.Close()` is called in a + closure inside the defer call, the linter will not be able to associate the `batch.Close()` to the variable. + (note: in most cases such pattern is a bad idea). See `deferCloseIsInClosure` test case. - `defer batch.Close()` must be called after checking that the `PrepareBatch` call returned no error. incorrect: ``` diff --git a/passes/chclose/chclose.go b/passes/chclose/chclose.go index 031a653..b2c2f70 100644 --- a/passes/chclose/chclose.go +++ b/passes/chclose/chclose.go @@ -119,12 +119,89 @@ func (a *analyzer) checkFunc(pass *analysis.Pass, body *ast.BlockStmt) { return true }) - // remaining usages that were not flushed + // for violations, check if the tracked var was stored in a struct literal whose result was returned for varName, u := range usages { + if !u.deferredClose && !u.returned && isStoredInStructAndReturned(body, varName) { + u.returned = true + } u.report(varName, pass, a.debug) } } +// isStoredInStructAndReturned checks if varName appears as a value in a composite literal +// assigned to some variable, and that variable is later returned. +// This handles: l := &Wrapper{rows}; return l, nil +func isStoredInStructAndReturned(body *ast.BlockStmt, varName string) bool { + // step 1: find assignments where varName is inside a struct literal on the RHS + derivedVars := map[string]bool{} + ast.Inspect(body, func(n ast.Node) bool { + assign, ok := n.(*ast.AssignStmt) + if !ok { + return true + } + for i, rhs := range assign.Rhs { + if !compositeLiteralContains(rhs, varName) { + continue + } + if i < len(assign.Lhs) { + if lhsName := util.IdentName(assign.Lhs[i]); lhsName != "" { + derivedVars[lhsName] = true + } + } + } + return true + }) + if len(derivedVars) == 0 { + return false + } + + // step 2: check if any derived variable is returned + found := false + ast.Inspect(body, func(n ast.Node) bool { + if found { + return false + } + ret, ok := n.(*ast.ReturnStmt) + if !ok { + return true + } + for _, result := range ret.Results { + if name := util.IdentName(result); name != "" && derivedVars[name] { + found = true + return false + } + } + return true + }) + return found +} + +// compositeLiteralContains checks if an expression is (or contains via unary &) a composite literal +// that references varName as a value. +func compositeLiteralContains(expr ast.Expr, varName string) bool { + // unwrap &Wrapper{...} + if unary, ok := expr.(*ast.UnaryExpr); ok { + expr = unary.X + } + lit, ok := expr.(*ast.CompositeLit) + if !ok { + return false + } + for _, elt := range lit.Elts { + switch e := elt.(type) { + case *ast.Ident: + if e.Name == varName { + return true + } + case *ast.KeyValueExpr: + if id, ok := e.Value.(*ast.Ident); ok && id.Name == varName { + return true + } + } + } + return false +} + // handleAssign checks if any LHS variable in the assignment is a closable ClickHouse driver type. // If a tracked variable is reassigned, it flushes/reports the previous tracking first. func (a *analyzer) handleAssign(pass *analysis.Pass, assign *ast.AssignStmt, usages map[string]*closableUsage) { @@ -179,11 +256,18 @@ func handleDefer(deferStmt *ast.DeferStmt, usages map[string]*closableUsage) { } } -// handleReturn checks if any return expression contains a tracked variable, -// including when wrapped in a function call or struct literal (e.g. return NewWrapper(rows), nil). +// handleReturn checks if any return expression contains a tracked variable. +// It recognizes direct returns (return rows, nil) and struct literal wrapping (return &Wrapper{rows}, nil). +// Function calls in return expressions are NOT walked into (we cannot distinguish +// wrapping (ownership transfer) from consuming (just reads)). func handleReturn(ret *ast.ReturnStmt, usages map[string]*closableUsage) { for _, result := range ret.Results { ast.Inspect(result, func(n ast.Node) bool { + switch n.(type) { + case *ast.CallExpr: + // don't descend into function calls — opaque ownership semantics + return false + } id, ok := n.(*ast.Ident) if !ok { return true diff --git a/passes/chclose/testdata/src/testcases/testcases.go b/passes/chclose/testdata/src/testcases/testcases.go index 6fe530e..8f16ccb 100644 --- a/passes/chclose/testdata/src/testcases/testcases.go +++ b/passes/chclose/testdata/src/testcases/testcases.go @@ -57,9 +57,10 @@ func wrapBatch(b driver.Batch) *batchWrapper { return &batchWrapper{b} } -// valid: batch returned wrapped in a function call -func validReturnWrappedInCall() (*batchWrapper, error) { - batch, err := conn.PrepareBatch(ctx, "INSERT INTO t") +// known false positive: batch returned wrapped in a function call. +// we cannot statically distinguish wrapping (ownership transfer) from consuming. +func falsePositiveReturnWrappedInCall() (*batchWrapper, error) { + batch, err := conn.PrepareBatch(ctx, "INSERT INTO t") //want `clickhouse Batch batch must be closed defensively with defer batch\.Close\(\) after successful instantiation` if err != nil { return nil, err } @@ -275,9 +276,10 @@ func wrapRows(r driver.Rows) *rowsWrapper { return &rowsWrapper{r} } -// valid: rows returned wrapped in a function call -func rowsValidReturnWrappedInCall() (*rowsWrapper, error) { - rows, err := conn.Query(ctx, "SELECT 1") +// known false positive: rows returned wrapped in a function call. +// we cannot statically distinguish wrapping (ownership transfer) from consuming. +func rowsFalsePositiveReturnWrappedInCall() (*rowsWrapper, error) { + rows, err := conn.Query(ctx, "SELECT 1") //want `clickhouse Rows rows must be closed defensively with defer rows\.Close\(\) after successful instantiation` if err != nil { return nil, err } @@ -293,6 +295,28 @@ func rowsValidReturnWrappedInStruct() (*rowsWrapper, error) { return &rowsWrapper{rows}, nil } +// known false positive: rows wrapped via function call into intermediate variable then returned. +// we cannot statically distinguish wrapping (ownership transfer) from consuming (just reads rows). +func rowsFalsePositiveWrappedViaFuncCall() (*rowsWrapper, error) { + rows, err := conn.Query(ctx, "SELECT 1") //want `clickhouse Rows rows must be closed defensively with defer rows\.Close\(\) after successful instantiation` + if err != nil { + return nil, err + } + l := wrapRows(rows) + return l, nil +} + +// valid: rows stored in struct literal via intermediate variable then returned. +// struct literals are unambiguous — rows is a field in the returned struct, so ownership is transferred. +func rowsValidWrappedViaStructLiteral() (*rowsWrapper, error) { + rows, err := conn.Query(ctx, "SELECT 1") + if err != nil { + return nil, err + } + l := &rowsWrapper{rows} + return l, nil +} + // valid: defer Close() after Rows instantiated from helper method func rowsValidFromHelper() { rows, err := helperReturningRows()