chore: use upstream channel safety and slice concatenation cops

rubocop-go now ships these checks, so drop our local copies and wire
the shared constructors in instead. Same behavior, less code to carry.

Assisted-By: docker-agent
This commit is contained in:
David Gageot committed 2026-10-16 22:21:28 +02:00
1 parent cc52bd767c
commit dcbf5a58fc
18 files changed
+230 -2220

No files matched your search

+7 -2
View File
@@ -105,15 +105,20 @@ Key conventions:
## Lint rules
`task lint` runs the shared and project-specific cops selected in `lint/main.go`.
Reusable checks come from [rubocop-go](https://github.com/dgageot/rubocop-go/blob/202b67b66167808e75285d0666f880a2bd4b1144/docs/shared-cops.md)
Reusable checks come from [rubocop-go](https://github.com/dgageot/rubocop-go/blob/f5ca63a6433aa9ed2ea9e251986b0d728965dbb9/docs/shared-cops.md)
(pinned in `go.mod`); project-specific checks and frozen-config exclusions stay
in `lint/`. Cop IDs and `//rubocop:disable` annotations are unchanged. Add shared checks by their
constructors, not by enabling the entire upstream catalog. 34 of the 36 shared
constructors, not by enabling the entire upstream catalog. 40 of the 42 shared
opt-in cops are currently selected. `Lint/ContextFirstParameter` and
`Lint/NoContextField` remain disabled: existing public constructor signatures and
intentionally owned lifecycle or telemetry contexts would require suppressions or
unrelated refactoring.
`CancellationLoop`, `ClosedChannelLoop`, `ChannelStateCheck`,
`CancellationSafeResultSend`, `NonblockingChannelReplacement`, and `SlicesConcat`
use the upstream implementations and synthetic tests. Their frozen-config
exclusions, registration tests, and application regressions remain here.
`Lint/ChannelDirection` suggests directional raw-channel parameters and results.
It follows requirements through locals, fields, direct calls, returns, and closure
captures, including internal/external tests. Closing requires send capability;
+1 -1
View File
@@ -33,7 +33,7 @@ require (
github.com/clipperhouse/uax29/v2 v2.7.0
github.com/coder/acp-go-sdk v0.13.5
github.com/creack/pty v1.1.24
github.com/dgageot/rubocop-go v1.0.1-0.20261009115150-202b67b66167
github.com/dgageot/rubocop-go v1.0.1-0.20261016190213-f5ca63a6433a
github.com/docker/aijson v0.1.0
github.com/docker/cli v29.9.1+incompatible
github.com/docker/go-units v0.5.0
+2 -2
View File
@@ -154,8 +154,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM=
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/dgageot/rubocop-go v1.0.1-0.20261009115150-202b67b66167 h1:rdZvdv690da37V6udEtybQNTrJ7eZ8jercvtrlOBLxc=
github.com/dgageot/rubocop-go v1.0.1-0.20261009115150-202b67b66167/go.mod h1:szP4Puq4DW5fbuTd3EZQH/NsN1VqMHPSI5JcKKZN21Y=
github.com/dgageot/rubocop-go v1.0.1-0.20261016190213-f5ca63a6433a h1:NDj6pISWml9ArsOIYYifJrPBFVkYaNN8PZHMmx/KnCU=
github.com/dgageot/rubocop-go v1.0.1-0.20261016190213-f5ca63a6433a/go.mod h1:szP4Puq4DW5fbuTd3EZQH/NsN1VqMHPSI5JcKKZN21Y=
github.com/dgageot/ultraviolet v0.0.0-20260313154905-9451997d56b6 h1:88fWkkjwzuI4tRTqadbJIbA9O+gO67oyu+2OpHHuuT8=
github.com/dgageot/ultraviolet v0.0.0-20260313154905-9451997d56b6/go.mod h1:SQpCTRNBtzJkwku5ye4S3HEuthAlGy2n9VXZnWkEW98=
github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk=
-441
View File
@@ -1,441 +0,0 @@
package main
import (
"go/ast"
"go/constant"
"go/token"
"go/types"
"maps"
"github.com/dgageot/rubocop-go/cop"
"github.com/dgageot/rubocop-go/prog"
)
// CancellationSafeResultSend checks straight-line setup of local unbuffered
// result channels with one inline producer and one returning context select.
// Escapes, extra receivers, and buffered channels are outside this analysis.
var CancellationSafeResultSend = prog.FromFile(cop.New(cop.Meta{
Name: "Lint/CancellationSafeResultSend",
Description: "Do not strand result senders when callers return on cancellation",
Severity: cop.Warning,
}, checkCancellationSafeResultSend, cop.WithTypes(), cop.WithScope(outsideFrozenConfig)))
func checkCancellationSafeResultSend(p *cop.Pass) {
if p.Info == nil || p.IsTestFile() || ast.IsGenerated(p.File) {
return
}
ast.Inspect(p.File, func(n ast.Node) bool {
switch fn := n.(type) {
case *ast.FuncDecl:
checkResultSendBody(p, fn.Body)
case *ast.FuncLit:
checkResultSendBody(p, fn.Body)
}
return true
})
}
type resultChannel struct {
object types.Object
pos token.Pos
}
func checkResultSendBody(p *cop.Pass, body *ast.BlockStmt) {
if body == nil {
return
}
aliases := make(map[types.Object]types.Object)
allowed := make(map[*ast.Ident]bool)
var channels []resultChannel
var workers []*ast.GoStmt
var selects []*ast.SelectStmt
bind := func(id *ast.Ident, rhs ast.Expr) {
obj := p.Info.Defs[id]
if obj == nil {
return
}
if call, ok := ast.Unparen(rhs).(*ast.CallExpr); ok && p.CalleeObject(call) == types.Universe.Lookup("make") && len(call.Args) >= 1 {
if typ := p.Info.TypeOf(call); typ != nil {
if _, ok := typ.Underlying().(*types.Chan); ok && (len(call.Args) == 1 || len(call.Args) == 2 && resultZero(p, call.Args[1])) {
aliases[obj] = obj
channels = append(channels, resultChannel{obj, id.Pos()})
}
}
} else if source := streamObject(p, rhs); source != nil {
if original := aliases[source]; original != nil {
source = original
}
aliases[obj] = source
allowed[ast.Unparen(rhs).(*ast.Ident)] = true
}
}
for _, stmt := range body.List {
switch stmt := stmt.(type) {
case *ast.AssignStmt:
if len(stmt.Lhs) == len(stmt.Rhs) && stmt.Tok == token.DEFINE {
for i, lhs := range stmt.Lhs {
if id, ok := lhs.(*ast.Ident); ok {
bind(id, stmt.Rhs[i])
}
}
}
case *ast.DeclStmt:
if decl, ok := stmt.Decl.(*ast.GenDecl); ok && decl.Tok == token.VAR {
for _, spec := range decl.Specs {
value := spec.(*ast.ValueSpec)
if len(value.Names) == len(value.Values) {
for i, id := range value.Names {
bind(id, value.Values[i])
}
}
}
}
case *ast.GoStmt:
if fn, ok := stmt.Call.Fun.(*ast.FuncLit); ok {
workers = append(workers, stmt)
var params []*ast.Ident
for _, field := range fn.Type.Params.List {
params = append(params, field.Names...)
}
if len(params) == len(stmt.Call.Args) {
for i, arg := range stmt.Call.Args {
if source := streamObject(p, arg); source != nil {
if original := aliases[source]; original != nil {
source = original
}
aliases[p.Info.Defs[params[i]]] = source
allowed[ast.Unparen(arg).(*ast.Ident)] = true
}
}
}
}
case *ast.SelectStmt:
selects = append(selects, stmt)
}
}
if len(channels) == 0 || len(workers) == 0 || len(selects) == 0 {
return
}
parents := make(map[ast.Node]ast.Node)
var stack []ast.Node
ast.Inspect(body, func(n ast.Node) bool {
if n == nil {
stack = stack[:len(stack)-1]
return false
}
if len(stack) > 0 {
parents[n] = stack[len(stack)-1]
}
stack = append(stack, n)
return true
})
contexts := resultContextUses(p, body)
for _, ch := range channels {
for _, sel := range selects {
cancellations, known := resultCancellations(p, sel, true, contexts)
if !known || len(cancellations) == 0 || sel.Pos() < ch.pos {
continue
}
checkResultChannel(p, body, ch, sel, cancellations, contexts, workers, aliases, allowed, parents)
}
}
}
func resultZero(p *cop.Pass, expr ast.Expr) bool {
v := p.Info.Types[expr].Value
return v != nil && v.Kind() == constant.Int && constant.Sign(v) == 0
}
type resultContext struct {
object types.Object
}
func resultCancellations(p *cop.Pass, sel *ast.SelectStmt, returning bool, contexts map[*ast.Ident]resultContext) (map[resultContext]bool, bool) {
found := make(map[resultContext]bool)
for _, stmt := range sel.Body.List {
clause := stmt.(*ast.CommClause)
var recv *ast.UnaryExpr
switch comm := clause.Comm.(type) {
case *ast.ExprStmt:
recv, _ = ast.Unparen(comm.X).(*ast.UnaryExpr)
case *ast.AssignStmt:
if len(comm.Rhs) == 1 {
recv, _ = ast.Unparen(comm.Rhs[0]).(*ast.UnaryExpr)
}
}
if recv == nil || recv.Op != token.ARROW {
continue
}
call, ok := ast.Unparen(recv.X).(*ast.CallExpr)
if !ok || len(call.Args) != 0 {
continue
}
method, ok := p.CalleeObject(call).(*types.Func)
if !ok || method.Pkg() == nil || method.Pkg().Path() != "context" || method.Name() != "Done" {
continue
}
expr, ok := ast.Unparen(call.Fun).(*ast.SelectorExpr)
if !ok {
continue
}
if returning {
if len(clause.Body) != 1 {
continue
}
if _, ok := clause.Body[0].(*ast.ReturnStmt); !ok {
continue
}
}
id, ok := ast.Unparen(expr.X).(*ast.Ident)
if !ok || contexts[id].object == nil {
return nil, false
}
found[contexts[id]] = true
}
return found, true
}
func checkResultChannel(p *cop.Pass, body *ast.BlockStmt, ch resultChannel, consumer *ast.SelectStmt, cancellations map[resultContext]bool, contexts map[*ast.Ident]resultContext, workers []*ast.GoStmt, aliases map[types.Object]types.Object, allowed map[*ast.Ident]bool, parents map[ast.Node]ast.Node) {
uses := make(map[*ast.Ident]bool)
var sends []*ast.SendStmt
var producer *ast.FuncLit
receives, uncertain := 0, false
mark := func(expr ast.Expr) bool {
id, ok := ast.Unparen(expr).(*ast.Ident)
if !ok || aliases[p.Info.ObjectOf(id)] != ch.object {
return false
}
uses[id] = true
return true
}
ast.Inspect(body, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.SendStmt:
if !mark(n.Chan) {
break
}
var fn *ast.FuncLit
for parent := parents[n]; parent != nil; parent = parents[parent] {
if fn, _ = parent.(*ast.FuncLit); fn != nil {
break
}
}
inline := false
for _, worker := range workers {
if worker.Call.Fun == fn && worker.Pos() > ch.pos && worker.Pos() < consumer.Pos() {
inline = true
break
}
}
if !inline || producer != nil && producer != fn {
uncertain = true
}
producer = fn
if clause, ok := parents[n].(*ast.CommClause); ok && clause.Comm == n {
if sel, ok := parents[parents[clause]].(*ast.SelectStmt); ok {
exits, known := resultCancellations(p, sel, false, contexts)
if !known {
uncertain = true
}
protected := known
for ctx := range cancellations {
protected = protected && exits[ctx]
}
if protected {
break
}
for _, stmt := range sel.Body.List {
if stmt.(*ast.CommClause).Comm == nil {
return true
}
}
}
}
sends = append(sends, n)
case *ast.UnaryExpr:
if n.Op != token.ARROW || !mark(n.X) {
break
}
receives++
comm := parents[n]
if _, ok := comm.(*ast.ParenExpr); ok {
comm = parents[comm]
}
clause, ok := parents[comm].(*ast.CommClause)
if !ok || clause.Comm != comm || parents[parents[clause]] != consumer {
uncertain = true
}
case *ast.CallExpr:
if p.CalleeObject(n) == types.Universe.Lookup("close") && len(n.Args) == 1 {
mark(n.Args[0])
}
}
return true
})
for id := range cop.Nodes[*ast.Ident](body) {
if aliases[p.Info.Uses[id]] == ch.object && !uses[id] && !allowed[id] {
uncertain = true
}
}
if !uncertain && receives == 1 {
for _, send := range sends {
p.Report(send, "result send can block after the caller returns on context cancellation; select on cancellation or use a buffered one-shot result channel")
}
}
}
// Snapshot straight-line assignments; goroutine arguments copy values, captures do not.
func resultContextUses(p *cop.Pass, body *ast.BlockStmt) map[*ast.Ident]resultContext {
uses := make(map[*ast.Ident]resultContext)
type environment map[types.Object]resultContext
var value func(environment, ast.Expr) resultContext
value = func(env environment, expr ast.Expr) resultContext {
if expr == nil {
return resultContext{}
}
if call, ok := ast.Unparen(expr).(*ast.CallExpr); ok {
if len(call.Args) == 1 && p.Info.Types[call.Fun].IsType() && types.Identical(p.Info.TypeOf(call), p.Info.TypeOf(call.Args[0])) {
return value(env, call.Args[0])
}
if fn := p.CalleeObject(call); fn != nil && fn.Pkg() != nil && fn.Pkg().Path() == "context" && fn.Name() == "WithValue" && len(call.Args) == 3 {
return value(env, call.Args[0])
}
}
obj := streamObject(p, expr)
if obj == nil {
return resultContext{}
}
if v, ok := env[obj]; ok {
return v
}
return resultContext{object: obj}
}
record := func(node ast.Node, env environment) {
ast.Inspect(node, func(n ast.Node) bool {
if _, ok := n.(*ast.FuncLit); ok {
return false
}
if id, ok := n.(*ast.Ident); ok && p.Info.Uses[id] != nil {
uses[id] = value(env, id)
}
return true
})
}
written := func(node ast.Node) map[types.Object]bool {
objects := make(map[types.Object]bool)
mark := func(expr ast.Expr) {
if obj := streamObject(p, expr); obj != nil {
objects[obj] = true
}
}
ast.Inspect(node, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.AssignStmt:
for _, lhs := range n.Lhs {
mark(lhs)
}
case *ast.RangeStmt:
if n.Key != nil {
mark(n.Key)
}
if n.Value != nil {
mark(n.Value)
}
case *ast.UnaryExpr:
if n.Op == token.AND {
mark(n.X)
}
}
return true
})
return objects
}
var scan func(*ast.BlockStmt, environment)
scan = func(block *ast.BlockStmt, env environment) {
escaped := make(map[types.Object]bool)
for i, stmt := range block.List {
ast.Inspect(stmt, func(n ast.Node) bool {
if call, ok := n.(*ast.CallExpr); ok {
if fn, ok := ast.Unparen(call.Fun).(*ast.FuncLit); ok {
for obj := range written(fn.Body) {
env[obj] = resultContext{}
}
}
}
if _, ok := n.(*ast.FuncLit); ok {
return false
}
if address, ok := n.(*ast.UnaryExpr); ok && address.Op == token.AND {
if obj := streamObject(p, address.X); obj != nil {
escaped[obj] = true
}
}
return true
})
for obj := range escaped {
env[obj] = resultContext{}
}
switch stmt := stmt.(type) {
case *ast.AssignStmt:
record(stmt, env)
values := make([]resultContext, len(stmt.Lhs))
for j := range stmt.Lhs {
if len(stmt.Lhs) == len(stmt.Rhs) {
values[j] = value(env, stmt.Rhs[j])
}
}
for j, lhs := range stmt.Lhs {
if obj := streamObject(p, lhs); obj != nil {
env[obj] = values[j]
}
}
case *ast.DeclStmt:
record(stmt, env)
if decl, ok := stmt.Decl.(*ast.GenDecl); ok && decl.Tok == token.VAR {
for _, spec := range decl.Specs {
v := spec.(*ast.ValueSpec)
for j, id := range v.Names {
ctx := resultContext{}
if len(v.Values) == len(v.Names) {
ctx = value(env, v.Values[j])
}
env[p.Info.Defs[id]] = ctx
}
}
}
case *ast.GoStmt:
fn, ok := ast.Unparen(stmt.Call.Fun).(*ast.FuncLit)
if !ok {
continue
}
child := maps.Clone(env)
for _, next := range block.List[i+1:] {
for obj := range written(next) {
child[obj] = resultContext{}
}
}
var params []*ast.Ident
for _, field := range fn.Type.Params.List {
params = append(params, field.Names...)
}
if len(params) == len(stmt.Call.Args) {
for j, arg := range stmt.Call.Args {
child[p.Info.Defs[params[j]]] = value(env, arg)
}
}
scan(fn.Body, child)
default:
// Branch/loop writes and address escapes cannot be resolved locally.
for obj := range written(stmt) {
env[obj] = resultContext{}
}
record(stmt, env)
}
for obj := range escaped {
env[obj] = resultContext{}
}
}
}
scan(body, make(environment))
return uses
}
+1 -120
View File
@@ -1,10 +1,7 @@
package main
import (
"fmt"
"os"
"path/filepath"
"strings"
"testing"
"github.com/dgageot/rubocop-go/coptest"
@@ -12,126 +9,10 @@ import (
"github.com/stretchr/testify/require"
)
func TestCancellationSafeResultSend(t *testing.T) {
t.Parallel()
tests := []struct {
name, body string
want int
}{
{"unguarded send", `ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"explicit zero capacity", `ch := make(chan int, 1-1); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"var declaration", `var ch = make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"channel alias", `ch := make(chan int); out := ch; go func(){ out <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"receive alias", `ch := make(chan int); out := ch; go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-out: }`, 1},
{"goroutine argument", `ch := make(chan int); go func(out chan<- int){ out <- 1 }(ch); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"parentheses", `ch := make(chan int); go func(){ (ch) <- 1 }(); select { case <-(ctx.Done()): return; case <-((ch)): }`, 1},
{"multiple result channels", `ch := make(chan int); errs := make(chan error); go func(){ defer close(ch); defer close(errs); if yes { ch <- 1 } else { errs <- nil } }(); select { case <-ctx.Done(): return; case <-ch: case <-errs: }`, 2},
{"shadowed unrelated local", `ch := make(chan int); go func(){ { ch := make(chan int, 1); ch <- 2 }; ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"named channel", `type result chan int; ch := make(result); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"buffered one shot", `ch := make(chan int, 1); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"cancellation aware send", `ch := make(chan int); go func(){ select { case ch <- 1: case <-ctx.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"cancellation aware context alias", `ch := make(chan int); alias := ctx; go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"cancellation aware context argument", `ch := make(chan int); go func(ctx c.Context){ select { case ch <- 1: case <-ctx.Done(): } }(ctx); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"shadowed context does not guard", `ch := make(chan int); go func(ctx c.Context){ select { case ch <- 1: case <-ctx.Done(): } }(other); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"shadowed receive in goroutine", `ch := make(chan int); go func(){ ch := make(chan int, 1); ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"extra receiver through alias", `ch := make(chan int); alias := ch; go func(){ ch <- 1 }(); go func(){ <-alias }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"escaped through alias", `ch := make(chan int); alias := ch; consume(alias); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"range drain", `ch := make(chan int); defer func(){ for range ch {} }(); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"nonblocking send", `ch := make(chan int); go func(){ select { case ch <- 1: default: } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"other context does not guard send", `ch := make(chan int); go func(){ select { case ch <- 1: case <-other.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"extra receiver", `ch := make(chan int); go func(){ ch <- 1 }(); go func(){ <-ch }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"extra caller receive", `ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }; <-ch`, 0},
{"deferred drain", `ch := make(chan int); defer func(){ <-ch }(); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"escaped to helper", `ch := make(chan int); consume(ch); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"escaped into container", `ch := make(chan int); _ = []chan int{ch}; go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"address escapes", `ch := make(chan int); _ = &ch; go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"reassigned channel", `ch := make(chan int); go func(){ ch <- 1 }(); ch = make(chan int); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"reassigned alias", `ch := make(chan int); out := ch; go func(){ out <- 1 }(); out = make(chan int); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"multiple producers", `ch := make(chan int); go func(){ ch <- 1 }(); go func(){ ch <- 2 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"nested nonproducer closure", `ch := make(chan int); go func(){ func(){ ch <- 1 }() }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"no cancellation", `ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ch: }`, 0},
{"cancellation without return", `ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): case <-ch: }; <-ch`, 0},
{"unrelated done method", `ch := make(chan int); go func(){ ch <- 1 }(); select { case <-fake.Done(): return; case <-ch: }`, 0},
{"shadowed receiver channel", `ch := make(chan int); go func(){ ch <- 1 }(); { ch := make(chan int, 1); select { case <-ctx.Done(): return; case <-ch: } }`, 0},
{"shadowed make", `make := func() chan int { return nil }; ch := make(); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"parameter channel", `go func(){ external <- 1 }(); select { case <-ctx.Done(): return; case <-external: }`, 0},
{"second consumer cancellation", `ch := make(chan int); go func(){ select { case ch <- 1: case <-ctx.Done(): } }(); select { case <-ctx.Done(): return; case <-other.Done(): return; case <-ch: }`, 1},
{"second producer cancellation", `ch := make(chan int); go func(){ select { case ch <- 1: case <-other.Done(): case <-ctx.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"all consumer cancellations guarded", `ch := make(chan int); go func(){ select { case ch <- 1: case <-other.Done(): case <-ctx.Done(): } }(); select { case <-ctx.Done(): return; case <-other.Done(): return; case <-ch: }`, 0},
{"duplicate cancellation alias", `alias := ctx; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-alias.Done(): return; case <-ch: }`, 0},
{"guard alias reassigned away", `alias := ctx; alias = other; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"guard alias reassigned to consumer", `alias := other; alias = ctx; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"argument snapshots context before reassignment", `ch := make(chan int); go func(c c.Context){ select { case ch <- 1: case <-c.Done(): } }(ctx); ctx = other; select { case <-ctx.Done(): return; case <-ch: }`, 1},
{"separate alias retains copied context", `alias := ctx; ch := make(chan int); go func(c c.Context){ select { case ch <- 1: case <-c.Done(): } }(alias); alias = other; select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"multiple consumer exits only one report", `ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-other.Done(): return; case <-ch: }`, 1},
{"uncertain branch alias skipped", `alias := ctx; if yes { alias = other }; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"identity context conversion", `alias := c.Context(ctx); ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"WithValue shares cancellation", `alias := c.WithValue(ctx, "key", "value"); ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"invoked closure mutates alias", `alias := other; func(){ alias = ctx }(); ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"derived context unknown", `alias, cancel := c.WithCancel(ctx); defer cancel(); ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"address escapes through assignment", `alias := other; ptr := &alias; *ptr = ctx; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"address escapes through declaration", `alias := other; var ptr = &alias; *ptr = ctx; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"address escapes through assigned call", `alias := other; _ = replace(&alias, ctx); ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"invoked closure in assignment", `alias := other; unused := func() int { alias = ctx; return 0 }(); _ = unused; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"invoked closure in declaration", `alias := other; var unused = func() int { alias = ctx; return 0 }(); _ = unused; ch := make(chan int); go func(){ select { case ch <- 1: case <-alias.Done(): } }(); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"invoked closure in argument", `alias := other; ch := make(chan int); go func(_ int){ select { case ch <- 1: case <-alias.Done(): } }(func() int { alias = ctx; return 0 }()); select { case <-ctx.Done(): return; case <-ch: }`, 0},
{"caller closure", `_ = func(){ ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: } }`, 1},
}
files := coptest.ProgramFiles{}
for i, tc := range tests {
files[fmt.Sprintf("case%d/sample.go", i)] = `package p
import c "context"
type fakeContext struct{}
func (fakeContext) Done() <-chan struct{} { return nil }
func consume(chan int) {}
func replace(dst *c.Context, ctx c.Context) bool { *dst = ctx; return true }
func f(ctx, other c.Context, external chan int, yes bool, fake fakeContext) { ` + tc.body + ` }
`
}
offenses := coptest.RunProgram(t, CancellationSafeResultSend, files)
got := make(map[string]int)
for _, o := range offenses {
assert.Equal(t, "Lint/CancellationSafeResultSend", o.CopName)
assert.Contains(t, o.Message, "context cancellation")
got[filepath.Base(filepath.Dir(o.Pos.Filename))]++
}
for i, tc := range tests {
assert.Equal(t, tc.want, got[fmt.Sprintf("case%d", i)], tc.name)
}
}
func TestCancellationSafeResultSendScope(t *testing.T) {
t.Parallel()
const src = `package p
import "context"
func f(ctx context.Context) { ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: } }
`
offenses := coptest.RunProgram(t, CancellationSafeResultSend, coptest.ProgramFiles{
"pkg/config/v0/sample.go": src,
"pkg/config/v15/sample.go": src,
"pkg/config/latest/sample.go": src,
"generated/sample.go": "// Code generated by fixture; DO NOT EDIT.\n" + src,
"tests/sample.go": "package p\n",
"tests/sample_test.go": src,
})
require.Len(t, offenses, 1)
assert.True(t, strings.HasSuffix(filepath.ToSlash(offenses[0].Pos.Filename), "/pkg/config/latest/sample.go"))
}
func TestCancellationSafeResultSendReadLine(t *testing.T) {
t.Parallel()
src, err := os.ReadFile("../pkg/input/readline.go")
require.NoError(t, err)
offenses := coptest.RunProgram(t, CancellationSafeResultSend, coptest.ProgramFiles{"input/readline.go": string(src)})
offenses := coptest.RunProgram(t, registeredSharedCop(t, programCops, "Lint/CancellationSafeResultSend"), coptest.ProgramFiles{"input/readline.go": string(src)})
assert.Empty(t, offenses)
}
func TestCancellationSafeResultSendSuppressions(t *testing.T) {
testCorrectnessCopSuppressions(t, CancellationSafeResultSend, `package sample
import "context"
func f(ctx context.Context) { ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: } } // offense
`)
}
-376
View File
@@ -1,376 +0,0 @@
package main
import (
"go/ast"
"go/token"
"go/types"
"github.com/dgageot/rubocop-go/cop"
"github.com/dgageot/rubocop-go/prog"
"golang.org/x/tools/go/cfg"
)
var CancellationLoop = withTypedFiles(func(opts ...cop.FuncOption) *prog.Func {
return prog.FromFile(newCancellationLoop(opts...))
})
var ClosedChannelLoop = withTypedFiles(func(opts ...cop.FuncOption) *prog.Func {
return prog.FromFile(newClosedChannelLoop(opts...))
})
func newCancellationLoop(opts ...cop.FuncOption) *cop.Func {
return cop.New(cop.Meta{
Name: "Lint/CancellationLoop", Description: "leave polling loops after context cancellation",
Severity: cop.Warning,
}, func(p *cop.Pass) { checkChannelLoops(p, true) }, append([]cop.FuncOption{cop.WithTypes(), cop.WithScope(outsideFrozenConfig)}, opts...)...)
}
func newClosedChannelLoop(opts ...cop.FuncOption) *cop.Func {
return cop.New(cop.Meta{
Name: "Lint/ClosedChannelLoop", Description: "stop polling a channel after observing closure",
Severity: cop.Warning,
}, func(p *cop.Pass) { checkChannelLoops(p, false) }, append([]cop.FuncOption{cop.WithTypes(), cop.WithScope(outsideFrozenConfig)}, opts...)...)
}
type channelLoopCandidate struct {
comm ast.Node
clause *ast.CommClause
ch ast.Expr
ok types.Object
}
func checkChannelLoops(p *cop.Pass, cancellation bool) {
if p.Info == nil || ast.IsGenerated(p.File) {
return
}
ast.Inspect(p.File, func(n ast.Node) bool {
var body *ast.BlockStmt
switch n := n.(type) {
case *ast.FuncDecl:
body = n.Body
case *ast.FuncLit:
body = n.Body
default:
return true
}
if body == nil {
return true
}
candidates := channelLoopCandidates(p, body, cancellation)
if len(candidates) == 0 {
return true
}
graph := cfg.New(body, func(call *ast.CallExpr) bool { return p.CalleeObject(call) != types.Universe.Lookup("panic") })
flow := newChannelLoopFlow(graph, body)
for _, candidate := range candidates {
if !channelLoopRepeats(p, flow, candidate) {
continue
}
if cancellation {
p.Report(candidate.comm, "cancellation can return to the same select; return or break the enclosing loop, not just the select")
} else {
p.Report(candidate.comm, "closed channel can be polled again; leave the loop or disable the channel after !ok")
}
}
return true
})
}
// Restrict warnings to unconditional loops; conditional loops may exit through state updates.
func channelLoopCandidates(p *cop.Pass, body *ast.BlockStmt, cancellation bool) []channelLoopCandidate {
var candidates []channelLoopCandidate
var stack []ast.Node
ast.Inspect(body, func(n ast.Node) bool {
if n == nil {
stack = stack[:len(stack)-1]
return false
}
if _, nested := n.(*ast.FuncLit); nested {
return false
}
stack = append(stack, n)
var recv *ast.UnaryExpr
var result types.Object
switch n := n.(type) {
case *ast.ExprStmt:
recv, _ = ast.Unparen(n.X).(*ast.UnaryExpr)
case *ast.AssignStmt:
if len(n.Rhs) == 1 && (len(n.Lhs) == 2 || cancellation && len(n.Lhs) == 1) {
recv, _ = ast.Unparen(n.Rhs[0]).(*ast.UnaryExpr)
if len(n.Lhs) == 2 {
if id, ok := n.Lhs[1].(*ast.Ident); ok && id.Name != "_" {
result = p.Info.ObjectOf(id)
}
}
}
case *ast.ValueSpec:
if len(n.Values) == 1 && len(n.Names) == 2 {
recv, _ = ast.Unparen(n.Values[0]).(*ast.UnaryExpr)
if n.Names[1].Name != "_" {
result = p.Info.ObjectOf(n.Names[1])
}
}
}
if recv == nil || recv.Op != token.ARROW || channelType(p.Info.TypeOf(recv.X)) == nil {
return true
}
var clause *ast.CommClause
inLoop := false
for i := len(stack) - 2; i >= 0; i-- {
switch ancestor := stack[i].(type) {
case *ast.CommClause:
if ancestor.Comm == n {
clause = ancestor
}
case *ast.ForStmt:
inLoop = ancestor.Cond == nil
goto foundLoop
case *ast.RangeStmt:
goto foundLoop
}
}
foundLoop:
if !inLoop {
return true
}
if cancellation {
call, ok := ast.Unparen(recv.X).(*ast.CallExpr)
if !ok || clause == nil {
return true
}
obj := p.CalleeObject(call)
if obj == nil || obj.Pkg() == nil || obj.Pkg().Path() != "context" || obj.Name() != "Done" {
return true
}
} else if result == nil || !channelLoopSame(p, recv.X, recv.X) {
return true
}
if cancellation {
call := ast.Unparen(recv.X).(*ast.CallExpr)
sel, ok := ast.Unparen(call.Fun).(*ast.SelectorExpr)
if !ok || !channelLoopSame(p, sel.X, sel.X) {
return true
}
result = nil
}
candidates = append(candidates, channelLoopCandidate{comm: n, clause: clause, ch: recv.X, ok: result})
return true
})
return candidates
}
type channelLoopPoint struct {
block *cfg.Block
index int
closed bool
}
type channelLoopFlow struct {
graph *cfg.CFG
conditions map[ast.Expr]ast.Expr
selectWrites map[ast.Node]bool
}
func newChannelLoopFlow(graph *cfg.CFG, body *ast.BlockStmt) channelLoopFlow {
flow := channelLoopFlow{graph: graph, conditions: make(map[ast.Expr]ast.Expr), selectWrites: make(map[ast.Node]bool)}
ast.Inspect(body, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.FuncLit:
return false
case *ast.IfStmt:
flow.conditions[n.Cond] = nil
case *ast.SwitchStmt:
for _, clause := range n.Body.List {
for _, expr := range clause.(*ast.CaseClause).List {
flow.conditions[expr] = n.Tag
}
}
case *ast.CommClause:
if assign, ok := n.Comm.(*ast.AssignStmt); ok {
flow.selectWrites[assign] = true
}
}
return true
})
return flow
}
func channelLoopRepeats(p *cop.Pass, flow channelLoopFlow, candidate channelLoopCandidate) bool {
var start channelLoopPoint
for _, block := range flow.graph.Blocks {
if candidate.clause != nil && block.Kind == cfg.KindSelectCaseBody && block.Stmt == candidate.clause {
start = channelLoopPoint{block: block}
break
}
if candidate.clause == nil {
for i, node := range block.Nodes {
if node == candidate.comm {
start = channelLoopPoint{block: block, index: i + 1}
}
}
}
}
if start.block == nil || !start.block.Live {
return false
}
start.closed = candidate.ok == nil
queue := []channelLoopPoint{start}
seen := make(map[channelLoopPoint]bool)
for len(queue) > 0 {
point := queue[0]
queue = queue[1:]
if seen[point] {
continue
}
seen[point] = true
if clause, ok := point.block.Stmt.(*ast.CommClause); ok && point.block.Kind == cfg.KindSelectCaseBody && clause != candidate.clause {
if channelLoopMutation(p, clause.Comm, candidate) {
continue
}
}
blocked := false
for _, node := range point.block.Nodes[point.index:] {
if node == candidate.comm {
if point.closed {
return true
}
blocked = true
break
}
if !flow.selectWrites[node] && channelLoopMutation(p, node, candidate) {
blocked = true
break
}
}
if blocked {
continue
}
for edge, next := range point.block.Succs {
if candidate.ok != nil && len(point.block.Succs) == 2 && len(point.block.Nodes) > 0 {
expr, _ := point.block.Nodes[len(point.block.Nodes)-1].(ast.Expr)
if value, known := flow.condition(p, expr, candidate.ok); known {
point.closed = true
if value != (edge == 0) {
continue
}
}
}
queue = append(queue, channelLoopPoint{block: next, closed: point.closed})
}
}
return false
}
func (flow channelLoopFlow) condition(p *cop.Pass, expr ast.Expr, result types.Object) (bool, bool) {
tag, found := flow.conditions[expr]
if !found {
return false, false
}
usesResult := false
for _, root := range []ast.Expr{expr, tag} {
if root == nil {
continue
}
ast.Inspect(root, func(n ast.Node) bool {
if id, ok := n.(*ast.Ident); ok && p.Info.ObjectOf(id) == result {
usesResult = true
}
return true
})
}
if !usesResult {
return false, false
}
value, known := channelLoopBool(p, expr, result)
if tag == nil {
return value, known
}
want, tagKnown := channelLoopBool(p, tag, result)
return value == want, known && tagKnown
}
func channelLoopMutation(p *cop.Pass, node ast.Node, candidate channelLoopCandidate) bool {
var lhsList []ast.Expr
switch node := node.(type) {
case *ast.AssignStmt:
lhsList = node.Lhs
case *ast.ValueSpec:
for _, name := range node.Names {
lhsList = append(lhsList, name)
}
default:
return false
}
for _, lhs := range lhsList {
if id, ok := lhs.(*ast.Ident); ok && candidate.ok != nil && p.Info.ObjectOf(id) == candidate.ok {
return true
}
expr := ast.Unparen(candidate.ch)
if call, ok := expr.(*ast.CallExpr); ok {
if sel, ok := ast.Unparen(call.Fun).(*ast.SelectorExpr); ok {
expr = sel.X
}
}
for {
if channelLoopSame(p, lhs, expr) {
return true
}
sel, ok := ast.Unparen(expr).(*ast.SelectorExpr)
if !ok {
break
}
expr = sel.X
}
}
return false
}
func channelLoopSame(p *cop.Pass, x, y ast.Expr) bool {
switch x := ast.Unparen(x).(type) {
case *ast.Ident:
y, ok := ast.Unparen(y).(*ast.Ident)
return ok && p.Info.ObjectOf(x) != nil && p.Info.ObjectOf(x) == p.Info.ObjectOf(y)
case *ast.SelectorExpr:
y, ok := ast.Unparen(y).(*ast.SelectorExpr)
return ok && p.Info.ObjectOf(x.Sel) != nil && p.Info.ObjectOf(x.Sel) == p.Info.ObjectOf(y.Sel) && channelLoopSame(p, x.X, y.X)
}
return false
}
// Evaluate simple closure tests with the comma-ok result known to be false.
func channelLoopBool(p *cop.Pass, expr ast.Expr, result types.Object) (bool, bool) {
if expr == nil {
return false, false
}
switch expr := ast.Unparen(expr).(type) {
case *ast.Ident:
if p.Info.ObjectOf(expr) == types.Universe.Lookup("true") {
return true, true
}
if p.Info.ObjectOf(expr) == types.Universe.Lookup("false") {
return false, true
}
if p.Info.ObjectOf(expr) == result {
return false, true
}
case *ast.UnaryExpr:
if expr.Op == token.NOT {
value, known := channelLoopBool(p, expr.X, result)
return !value, known
}
case *ast.BinaryExpr:
if expr.Op == token.EQL || expr.Op == token.NEQ {
for _, pair := range [][2]ast.Expr{{expr.X, expr.Y}, {expr.Y, expr.X}} {
id, ok := ast.Unparen(pair[1]).(*ast.Ident)
if !ok || (p.Info.ObjectOf(id) != types.Universe.Lookup("false") && p.Info.ObjectOf(id) != types.Universe.Lookup("true")) {
continue
}
if value, known := channelLoopBool(p, pair[0], result); known {
equal := value == (id.Name == "true")
return equal == (expr.Op == token.EQL), true
}
}
}
}
return false, false
}
-145
View File
@@ -1,145 +0,0 @@
package main
import (
"bytes"
"fmt"
"os"
"path/filepath"
"strconv"
"testing"
"github.com/dgageot/rubocop-go/config"
"github.com/dgageot/rubocop-go/coptest"
"github.com/dgageot/rubocop-go/prog"
"github.com/dgageot/rubocop-go/runner"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestCancellationLoop(t *testing.T) {
t.Parallel()
tests := []struct {
name, body string
want int
}{
{"cancel single assignment", `for { select { case done:=<-ctx.Done(): _=done; continue; case <-ch: } }`, 1},
{"cancel comma ok", `for { select { case _,ok:=<-ctx.Done(): _=ok; break; case <-ch: } }`, 1},
{"parenthesized context", `for { select { case <-(ctx.Done()): ctx=other; case <-ch: } }`, 0},
{"parenthesized method", `for { select { case <-(ctx.Done)(): ctx=other; case <-ch: } }`, 0},
{"recreated context", `for { var ctx,cancel=context.WithCancel(ctx); select { case <-ctx.Done(): cancel(); break; case <-ch: }; cancel() }`, 0},
{"break select", `for { select { case <-ctx.Done(): break; case <-ch: } }`, 1},
{"continue loop", `for { select { case <-ctx.Done(): continue; case <-ch: } }`, 1},
{"fallthrough", `for { select { case <-ctx.Done(): println("cancelled"); case <-ch: } }`, 1},
{"nested break", `for { select { case <-ctx.Done(): if yes { break }; case <-ch: } }`, 1},
{"safe return", `for { select { case <-ctx.Done(): return; case <-ch: } }`, 0},
{"safe labelled break", `outer: for { select { case <-ctx.Done(): break outer; case <-ch: } }`, 0},
{"safe after select", `for { select { case <-ctx.Done(): break; case <-ch: }; return }`, 0},
{"safe conditional after select", `for { select { case <-ctx.Done(): break; case <-ch: }; if yes { return } }`, 1},
{"conditional loop", `for yes { select { case <-ctx.Done(): yes=false; case <-ch: } }`, 0},
{"replace context", `for { select { case <-ctx.Done(): ctx = other; case <-ch: } }`, 0},
{"panic", `for { select { case <-ctx.Done(): panic("stop"); case <-ch: } }`, 0},
{"select once", `select { case <-ctx.Done(): break; case <-ch: }`, 0},
{"nested closure", `go func(){ for { select { case <-ctx.Done(): break; case <-ch: } } }()`, 1},
{"closure not loop", `for { go func(){ select { case <-ctx.Done(): break; case <-ch: } }(); return }`, 0},
{"range loop", `for range ch { select { case <-ctx.Done(): break; default: } }`, 0},
{"shadowed context", `{ ctx := fake{}; for { select { case <-ctx.Done(): break; case <-ch: } } }`, 0},
{"nested loop exits", `for { select { case <-ctx.Done(): for { break }; return; case <-ch: } }`, 0},
}
files := coptest.ProgramFiles{}
for i, tc := range tests {
files[fmt.Sprintf("case%d/sample.go", i)] = `package p
import "context"
type fake struct{}
func(fake) Done() <-chan struct{} { return nil }
func f(ctx,other context.Context, ch chan int, yes bool) { _=ctx; _=other; ` + tc.body + ` }`
}
offenses := coptest.RunProgram(t, CancellationLoop, files)
counts := make(map[string]int)
for _, offense := range offenses {
assert.Equal(t, "Lint/CancellationLoop", offense.CopName)
counts[filepath.Base(filepath.Dir(offense.Pos.Filename))]++
}
for i, tc := range tests {
assert.Equal(t, tc.want, counts["case"+strconv.Itoa(i)], tc.name)
}
}
func TestClosedChannelLoop(t *testing.T) {
t.Parallel()
tests := []struct {
name, body string
want int
}{
{"var receive", `for { var _,ok = <-ch; if !ok { continue } }`, 1},
{"var receive return", `for { var _,ok = <-ch; if !ok { return } }`, 0},
{"switch false safe", `for { _,ok:=<-ch; switch false { case ok: return; default: continue } }`, 0},
{"switch ok unsafe", `for { _,ok:=<-ch; switch ok { case false: continue; default: return } }`, 1},
{"switch ok safe", `for { _,ok:=<-ch; switch ok { case false: return; default: continue } }`, 0},
{"switch false unsafe", `for { _,ok:=<-ch; switch false { case ok: continue; default: return } }`, 1},
{"nested select default", `for { _,ok:=<-ch; if !ok { select { case ch=<-replacements: return; default: continue } } }`, 1},
{"nested select ok write", `for { _,ok:=<-ch; if !ok { select { case ok=<-flags: return; default: continue } } }`, 1},
{"recreated channel var", `for { var ch=make(chan int); close(ch); _,ok:=<-ch; if !ok { continue } }`, 0},
{"recreated channel short", `for { ch:=make(chan int); close(ch); _,ok:=<-ch; if !ok { continue } }`, 0},
{"continue", `for { select { case _,ok:=<-ch: if !ok { continue }; default: } }`, 1},
{"break select", `for { select { case _,ok:=<-ch: if !ok { break }; default: } }`, 1},
{"fallthrough", `for { select { case _,ok:=<-ch: if !ok { println("closed") }; default: } }`, 1},
{"direct receive", `for { _,ok:=<-ch; if !ok { continue } }`, 1},
{"return", `for { select { case _,ok:=<-ch: if !ok { return }; default: } }`, 0},
{"labelled break", `outer: for { select { case _,ok:=<-ch: if !ok { break outer }; default: } }`, 0},
{"direct break", `for { _,ok:=<-ch; if !ok { break } }`, 0},
{"disable", `for { select { case _,ok:=<-ch: if !ok { ch=nil; continue }; default: } }`, 0},
{"replace", `for { select { case _,ok:=<-ch: if !ok { ch=other }; default: } }`, 0},
{"unrelated channel nil", `for { select { case _,ok:=<-ch: if !ok { other=nil; continue }; default: } }`, 1},
{"shadowed nil", `for { select { case _,ok:=<-ch: if !ok { ch:=other; _=ch; continue }; default: } }`, 1},
{"positive check", `for { select { case _,ok:=<-ch: if ok { println("item") } else { continue }; default: } }`, 1},
{"positive check return", `for { select { case _,ok:=<-ch: if ok { continue } else { return }; default: } }`, 0},
{"equals false", `for { select { case _,ok:=<-ch: if ok==false { continue }; default: } }`, 1},
{"not equals true", `for { select { case _,ok:=<-ch: if true!=ok { continue }; default: } }`, 1},
{"unchecked", `for { select { case _,ok:=<-ch: _=ok; default: } }`, 0},
{"single receive", `for { select { case <-ch: continue; default: } }`, 0},
{"conditional loop", `for yes { _,ok:=<-ch; if !ok { yes=false } }`, 0},
{"after select break", `for { select { case _,ok:=<-ch: if !ok { break }; default: }; break }`, 0},
{"closure", `go func(){ for { _,ok:=<-ch; if !ok { continue } } }()`, 1},
{"panic", `for { _,ok:=<-ch; if !ok { panic("closed") } }`, 0},
{"ok overwritten", `for { _,ok:=<-ch; ok=yes; if !ok { continue } }`, 0},
{"nil only on one path", `for { _,ok:=<-ch; if !ok { if yes { ch=nil }; continue } }`, 1},
}
files := coptest.ProgramFiles{}
for i, tc := range tests {
files[fmt.Sprintf("case%d/sample.go", i)] = `package p
func f(ch,other chan int, yes bool, replacements chan chan int, flags chan bool) { _=other; ` + tc.body + ` }`
}
offenses := coptest.RunProgram(t, ClosedChannelLoop, files)
counts := make(map[string]int)
for _, offense := range offenses {
assert.Equal(t, "Lint/ClosedChannelLoop", offense.CopName)
counts[filepath.Base(filepath.Dir(offense.Pos.Filename))]++
}
for i, tc := range tests {
assert.Equal(t, tc.want, counts["case"+strconv.Itoa(i)], tc.name)
}
}
func TestChannelLoopRunner(t *testing.T) {
dir := t.TempDir()
t.Chdir(dir)
const body = `package p
func f(ch chan int) { for { _,ok:=<-ch; if !ok { continue } } }
`
for name, content := range map[string]string{
"go.mod": "module example.test\n\ngo 1.27\n",
"regular/sample.go": body,
"tests/sample_test.go": body,
"pkg/config/v0/sample.go": body,
"generated/sample.go": "// Code generated by fixture; DO NOT EDIT.\n" + body,
"suppressed/sample.go": "//rubocop:disable-file Lint/ClosedChannelLoop\n" + body,
} {
require.NoError(t, os.MkdirAll(filepath.Dir(name), 0o700))
require.NoError(t, os.WriteFile(name, []byte(content), 0o600))
}
var output bytes.Buffer
r := runner.New(nil, config.DefaultConfig(), &output).WithProgramCops([]prog.Cop{ClosedChannelLoop})
count, err := r.Run([]string{"."})
require.NoError(t, err)
assert.Equal(t, 2, count, output.String())
}
-377
View File
@@ -1,377 +0,0 @@
package main
import (
"go/ast"
"go/constant"
"go/token"
"go/types"
"slices"
"github.com/dgageot/rubocop-go/cop"
"github.com/dgageot/rubocop-go/prog"
)
var ChannelStateCheck = withTypedFiles(func(opts ...cop.FuncOption) *prog.Func {
return prog.FromFile(newChannelStateCheck(opts...))
})
func newChannelStateCheck(opts ...cop.FuncOption) *cop.Func {
return cop.New(cop.Meta{
Name: "Lint/ChannelStateCheck",
Description: "Channel length and capacity checks do not reserve a value or capacity",
Severity: cop.Warning,
}, checkChannelStateCheck, append([]cop.FuncOption{cop.WithTypes(), cop.WithScope(outsideFrozenConfig)}, opts...)...)
}
type stateCheckGuard struct {
channel ast.Expr
send bool
}
type stateCheckVisitor struct {
pass *cop.Pass
nonblocking map[ast.Node]bool
}
func checkChannelStateCheck(p *cop.Pass) {
if p.Info == nil || ast.IsGenerated(p.File) {
return
}
v := stateCheckVisitor{pass: p, nonblocking: make(map[ast.Node]bool)}
v.walk(p.File, nil)
}
func (v *stateCheckVisitor) walk(node ast.Node, guards []stateCheckGuard) {
if node == nil {
return
}
ast.Inspect(node, func(node ast.Node) bool {
switch node := node.(type) {
case *ast.FuncLit:
// An outer check says nothing about when a closure will run.
v.walk(node.Body, nil)
return false
case *ast.BlockStmt:
v.statements(node.List, guards)
return false
case *ast.CaseClause:
for _, expr := range node.List {
v.walk(expr, guards)
}
v.statements(node.Body, guards)
return false
case *ast.CommClause:
v.walk(node.Comm, guards)
v.statements(node.Body, v.afterWrites(node.Comm, guards))
return false
case *ast.BinaryExpr:
v.walk(node.X, guards)
rightGuards := v.afterWrites(node.X, guards)
if node.Op == token.LAND || node.Op == token.LOR {
rightGuards = slices.Concat(rightGuards, v.conditions(node.X, node.Op == token.LAND))
}
v.walk(node.Y, rightGuards)
return false
case *ast.IfStmt:
v.walk(node.Init, guards)
guards = v.afterWrites(node.Init, guards)
v.walk(node.Cond, guards)
v.walk(node.Body, slices.Concat(guards, v.conditions(node.Cond, true)))
v.walk(node.Else, slices.Concat(guards, v.conditions(node.Cond, false)))
return false
case *ast.ForStmt, *ast.RangeStmt:
// A loop's writes can invalidate an outer check on later iterations.
loopGuards := v.afterWrites(node, guards)
switch node := node.(type) {
case *ast.ForStmt:
v.walk(node.Init, loopGuards)
v.walk(node.Cond, loopGuards)
bodyGuards := slices.Concat(loopGuards, v.conditions(node.Cond, true))
v.walk(node.Body, bodyGuards)
v.walk(node.Post, v.afterWrites(node.Body, bodyGuards))
case *ast.RangeStmt:
v.walk(node.X, guards)
v.walk(node.Key, loopGuards)
v.walk(node.Value, loopGuards)
v.walk(node.Body, loopGuards)
}
return false
case *ast.SwitchStmt:
v.walk(node.Init, guards)
guards = v.afterWrites(node.Init, guards)
v.walk(node.Tag, guards)
if node.Tag != nil {
v.walk(node.Body, guards)
return false
}
var falling []stateCheckGuard
falls := false
for _, stmt := range node.Body.List {
clause := stmt.(*ast.CaseClause)
for _, expr := range clause.List {
v.walk(expr, guards)
}
bodyGuards := guards
if len(clause.List) == 1 {
bodyGuards = slices.Concat(guards, v.conditions(clause.List[0], true))
}
if falls {
if clause.List == nil {
bodyGuards = falling
} else {
bodyGuards = v.commonGuards(bodyGuards, falling)
}
}
v.statements(clause.Body, bodyGuards)
falls = false
if len(clause.Body) > 0 {
branch, ok := clause.Body[len(clause.Body)-1].(*ast.BranchStmt)
falls = ok && branch.Tok == token.FALLTHROUGH
}
falling = v.afterWrites(clause, bodyGuards)
}
return false
case *ast.TypeSwitchStmt:
v.walk(node.Init, guards)
guards = v.afterWrites(node.Init, guards)
v.walk(node.Assign, guards)
v.walk(node.Body, v.afterWrites(node.Assign, guards))
return false
case *ast.SelectStmt:
nonblocking := slices.ContainsFunc(node.Body.List, func(stmt ast.Stmt) bool {
return stmt.(*ast.CommClause).Comm == nil
})
if nonblocking {
for _, stmt := range node.Body.List {
switch comm := stmt.(*ast.CommClause).Comm.(type) {
case *ast.SendStmt:
v.nonblocking[comm] = true
case *ast.ExprStmt:
v.nonblocking[ast.Unparen(comm.X)] = true
case *ast.AssignStmt:
for _, rhs := range comm.Rhs {
v.nonblocking[ast.Unparen(rhs)] = true
}
}
}
}
case *ast.SendStmt:
v.operation(node, node.Chan, true, guards)
case *ast.UnaryExpr:
if node.Op == token.ARROW {
v.operation(node, node.X, false, guards)
}
}
return true
})
}
func (v *stateCheckVisitor) statements(stmts []ast.Stmt, guards []stateCheckGuard) {
for _, stmt := range stmts {
if _, labelled := stmt.(*ast.LabeledStmt); labelled {
guards = nil
}
v.walk(stmt, guards)
guards = v.afterWrites(stmt, guards)
if branch, ok := stmt.(*ast.BranchStmt); ok && branch.Tok == token.GOTO {
return
}
if stmt, ok := stmt.(*ast.IfStmt); ok && stmt.Else == nil && stateCheckExits(stmt.Body) {
guards = slices.Concat(guards, v.conditions(stmt.Cond, false))
}
}
}
func (v *stateCheckVisitor) commonGuards(a, b []stateCheckGuard) []stateCheckGuard {
var common []stateCheckGuard
for _, x := range a {
if slices.ContainsFunc(b, func(y stateCheckGuard) bool { return x.send == y.send && v.sameChannel(x.channel, y.channel) }) {
common = append(common, x)
}
}
return common
}
func stateCheckExits(block *ast.BlockStmt) bool {
if len(block.List) == 0 {
return false
}
switch stmt := block.List[len(block.List)-1].(type) {
case *ast.ReturnStmt:
return true
case *ast.BranchStmt:
return stmt.Tok == token.CONTINUE || stmt.Tok == token.BREAK
}
return false
}
func (v *stateCheckVisitor) operation(node ast.Node, channel ast.Expr, send bool, guards []stateCheckGuard) {
if v.nonblocking[node] {
return
}
for _, guard := range guards {
if guard.send == send && v.sameChannel(guard.channel, channel) {
v.pass.Report(node, "channel state check does not reserve capacity or a value; use select or explicit synchronization instead of guarding a potentially blocking channel operation with len/cap")
return
}
}
}
func (v *stateCheckVisitor) conditions(expr ast.Expr, truth bool) []stateCheckGuard {
if expr == nil {
return nil
}
switch expr := ast.Unparen(expr).(type) {
case *ast.UnaryExpr:
if expr.Op == token.NOT {
return v.conditions(expr.X, !truth)
}
case *ast.BinaryExpr:
if (expr.Op == token.LAND && truth) || (expr.Op == token.LOR && !truth) {
return slices.Concat(v.conditions(expr.X, truth), v.conditions(expr.Y, truth))
}
op, left, right := expr.Op, expr.X, expr.Y
channel := v.builtinChannel(left, "len")
if channel == nil {
channel = v.builtinChannel(right, "len")
right = left
op = stateCheckReverse(op)
}
if channel == nil {
return nil
}
if !truth {
op = stateCheckNegate(op)
}
if capacity := v.builtinChannel(right, "cap"); capacity != nil && v.sameChannel(channel, capacity) {
if op == token.LSS || op == token.NEQ {
return []stateCheckGuard{{channel: channel, send: true}}
}
return nil
}
value := v.pass.Info.Types[ast.Unparen(right)].Value
if value == nil || value.Kind() != constant.Int {
return nil
}
zero, one := constant.MakeInt64(0), constant.MakeInt64(1)
if (op == token.GTR && constant.Compare(value, token.GEQ, zero)) ||
(op == token.GEQ && constant.Compare(value, token.GEQ, one)) ||
(op == token.EQL && constant.Compare(value, token.GEQ, one)) ||
(op == token.NEQ && constant.Sign(value) == 0) {
return []stateCheckGuard{{channel: channel}}
}
}
return nil
}
func stateCheckReverse(op token.Token) token.Token {
switch op {
case token.LSS:
return token.GTR
case token.LEQ:
return token.GEQ
case token.GTR:
return token.LSS
case token.GEQ:
return token.LEQ
}
return op
}
func stateCheckNegate(op token.Token) token.Token {
switch op {
case token.LSS:
return token.GEQ
case token.LEQ:
return token.GTR
case token.GTR:
return token.LEQ
case token.GEQ:
return token.LSS
case token.EQL:
return token.NEQ
case token.NEQ:
return token.EQL
}
return token.ILLEGAL
}
func (v *stateCheckVisitor) builtinChannel(expr ast.Expr, name string) ast.Expr {
call, ok := ast.Unparen(expr).(*ast.CallExpr)
if !ok || len(call.Args) != 1 || v.pass.CalleeObject(call) != types.Universe.Lookup(name) {
return nil
}
channel := ast.Unparen(call.Args[0])
typ := v.pass.Info.TypeOf(channel)
if typ == nil {
return nil
}
if _, ok := typ.Underlying().(*types.Chan); !ok || !v.sameChannel(channel, channel) {
return nil
}
return channel
}
// Match resolved storage expressions, not spelling or repeated calls.
func (v *stateCheckVisitor) sameChannel(a, b ast.Expr) bool {
a, b = ast.Unparen(a), ast.Unparen(b)
switch a := a.(type) {
case *ast.Ident:
b, ok := b.(*ast.Ident)
return ok && v.pass.Info.ObjectOf(a) != nil && v.pass.Info.ObjectOf(a) == v.pass.Info.ObjectOf(b)
case *ast.SelectorExpr:
b, ok := b.(*ast.SelectorExpr)
return ok && v.pass.Info.ObjectOf(a.Sel) != nil && v.pass.Info.ObjectOf(a.Sel) == v.pass.Info.ObjectOf(b.Sel) && v.sameChannel(a.X, b.X)
case *ast.StarExpr:
b, ok := b.(*ast.StarExpr)
return ok && v.sameChannel(a.X, b.X)
}
return false
}
func (v *stateCheckVisitor) afterWrites(node ast.Node, guards []stateCheckGuard) []stateCheckGuard {
if node == nil || len(guards) == 0 {
return guards
}
var writes []ast.Expr
ast.Inspect(node, func(node ast.Node) bool {
switch node := node.(type) {
case *ast.FuncLit:
return false
case *ast.AssignStmt:
writes = append(writes, node.Lhs...)
case *ast.IncDecStmt:
writes = append(writes, node.X)
case *ast.RangeStmt:
if node.Tok == token.ASSIGN {
if node.Key != nil {
writes = append(writes, node.Key)
}
if node.Value != nil {
writes = append(writes, node.Value)
}
}
}
return true
})
if len(writes) == 0 {
return guards
}
var retained []stateCheckGuard
for _, guard := range guards {
changed := slices.ContainsFunc(writes, func(write ast.Expr) bool {
found := false
ast.Inspect(guard.channel, func(node ast.Node) bool {
if expr, ok := node.(ast.Expr); ok && v.sameChannel(expr, write) {
found = true
}
return !found
})
return found
})
if !changed {
retained = append(retained, guard)
}
}
return retained
}
-180
View File
@@ -1,180 +0,0 @@
package main
import (
"bytes"
"os"
"path/filepath"
"strings"
"testing"
"github.com/dgageot/rubocop-go/config"
"github.com/dgageot/rubocop-go/cop"
"github.com/dgageot/rubocop-go/coptest"
"github.com/dgageot/rubocop-go/prog"
"github.com/dgageot/rubocop-go/runner"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestChannelStateCheck(t *testing.T) {
t.Parallel()
for _, tc := range []struct {
name, src string
want int
}{
{"for post guard", `func f(ch chan int) { for ;len(ch)>0;<-ch {} }`, 1},
{"for post reassigned", `func f(ch,other chan int) { for ;len(ch)>0;<-ch { ch=other } }`, 0},
{"goto skips guard", `func f(ch chan int) { goto read; if len(ch)==0 { return }; read: <-ch }`, 0},
{"fallthrough guard", `func f(ch chan int) { switch { case len(ch)>0: fallthrough; default: <-ch } }`, 1},
{"fallthrough mutation", `func f(ch,other chan int, ready bool) { if len(ch)>0 { switch { case ready: ch=other; fallthrough; default: <-ch } } }`, 0},
{"for guard", `func f(ch chan int) { for len(ch)>0 { <-ch } }`, 1},
{"short circuit", `func f(ch chan int) { if len(ch)>0 && <-ch>0 {} }`, 1},
{"short circuit or", `func f(ch chan int) { if len(ch)==0 || <-ch>0 {} }`, 1},
{"switch guard", `func f(ch chan int) { switch { case len(ch)>0: <-ch } }`, 1},
{"short circuit unsafe alternative", `func f(ch chan int, ready bool) { if ready || len(ch)>0 && <-ch>0 {} }`, 1},
{"send", `func f(ch chan int) { if len(ch) < cap(ch) { ch <- 1 } }`, 1},
{"else if", `func f(ch chan int, ready bool) { if len(ch) == 0 {} else if ready { <-ch } }`, 1},
{"if initializer", `func f(ch chan int) { if ch := ch; len(ch) > 0 { <-ch } }`, 1},
{"loop", `func f(ch chan int) { if len(ch) > 0 { for { <-ch; break } } }`, 1},
{"switch clause", `func f(ch chan int) { if len(ch) > 0 { switch { default: <-ch } } }`, 1},
{"loop own guard", `func f(ch, other chan int) { for { if len(ch) > 0 { <-ch }; ch = other } }`, 1},
{"same name independent scopes", `func f(ch chan int) { if len(ch) > 0 { <-ch }; { ch := make(chan int); <-ch } }`, 1},
{"partial types", `var _ = missing; func f(ch chan int) { if len(ch) > 0 { <-ch } }`, 1},
{"receive", `func f(ch chan int) { if len(ch) > 0 { <-ch } }`, 1},
{"receive assignment", `func f(ch chan int) { if len(ch) > 0 { v, ok := <-ch; _, _ = v, ok } }`, 1},
{"receive expression", `func f(ch chan int) int { if len(ch) > 0 { return 1 + <-ch }; return 0 }`, 1},
{"reversed send", `func f(ch chan int) { if cap(ch) > len(ch) { ch <- 1 } }`, 1},
{"reversed receive", `func f(ch chan int) { if 0 < len(ch) { <-ch } }`, 1},
{"not empty", `func f(ch chan int) { if len(ch) != 0 { <-ch } }`, 1},
{"not full", `func f(ch chan int) { if len(ch) != cap(ch) { ch <- 1 } }`, 1},
{"minimum receive", `func f(ch chan int) { if len(ch) >= 1 { <-ch } }`, 1},
{"exact receive", `func f(ch chan int) { if len(ch) == 2 { <-ch } }`, 1},
{"constant receive", `const n = 1; func f(ch chan int) { if len(ch) >= n { <-ch } }`, 1},
{"negated receive", `func f(ch chan int) { if !(len(ch) == 0) { <-ch } }`, 1},
{"send else", `func f(ch chan int) { if len(ch) == cap(ch) {} else { ch <- 1 } }`, 1},
{"send reversed else", `func f(ch chan int) { if cap(ch) <= len(ch) {} else { ch <- 1 } }`, 1},
{"receive else", `func f(ch chan int) { if len(ch) == 0 {} else { <-ch } }`, 1},
{"receive reversed else", `func f(ch chan int) { if 0 >= len(ch) {} else { <-ch } }`, 1},
{"receive return guard", `func f(ch chan int) { if len(ch) == 0 { return }; <-ch }`, 1},
{"send return guard", `func f(ch chan int) { if len(ch) >= cap(ch) { return }; ch <- 1 }`, 1},
{"continue guard", `func f(ch chan int) { for { if len(ch) == 0 { continue }; <-ch } }`, 1},
{"break guard", `func f(ch chan int) { for { if len(ch) == 0 { break }; <-ch } }`, 1},
{"conjunction", `func f(ch chan int, ready bool) { if ready && len(ch) > 0 { <-ch } }`, 1},
{"else disjunction", `func f(ch chan int, ready bool) { if !ready || len(ch) == 0 {} else { <-ch } }`, 1},
{"parentheses", `func f(ch chan int) { if (len((ch))) < (cap((ch))) { (ch) <- 1 } }`, 1},
{"named channel", `type queue chan int; func f(ch queue) { if len(ch) > 0 { <-ch } }`, 1},
{"directional channels", `func f(in <-chan int, out chan<- int) { if len(in) > 0 { <-in }; if len(out) < cap(out) { out <- 1 } }`, 2},
{"field", `type q struct { ch chan int }; func f(q *q) { if len(q.ch) > 0 { <-q.ch } }`, 1},
{"pointer", `func f(ch *chan int) { if len(*ch) > 0 { <-*ch } }`, 1},
{"nested if", `func f(ch chan int, ready bool) { if len(ch) > 0 { if ready { <-ch } } }`, 1},
{"multiple operations", `func f(ch chan int) { if len(ch) > 0 { <-ch; <-ch } }`, 2},
{"duplicate guards", `func f(ch chan int) { if len(ch) > 0 { if len(ch) > 0 { <-ch } } }`, 1},
{"closure own guard", `func f(ch chan int) { go func() { if len(ch) > 0 { <-ch } }() }`, 1},
{"blocking select receive", `func f(ch chan int) { if len(ch) > 0 { select { case <-ch: } } }`, 1},
{"blocking select send", `func f(ch chan int) { if len(ch) < cap(ch) { select { case ch <- 1: } } }`, 1},
{"select body", `func f(ch chan int) { if len(ch) > 0 { select { default: <-ch } } }`, 1},
{"select send value blocks", `func f(ch chan int) { if len(ch) > 0 { select { case ch <- <-ch: default: } } }`, 1},
{"select channel expression blocks", `func f(ch chan chan int) { if len(ch) > 0 { select { case <- (<-ch): default: } } }`, 1},
{"goroutine argument", `func consume(int) {}; func f(ch chan int) { if len(ch) > 0 { go consume(<-ch) } }`, 1},
{"deferred argument", `func consume(int) {}; func f(ch chan int) { if len(ch) > 0 { defer consume(<-ch) } }`, 1},
{"slices", `func f(xs []int, ch chan int) { if len(xs) < cap(xs) { ch <- 1 }; if len(xs) > 0 { <-ch } }`, 0},
{"maps", `func f(xs map[int]int, ch chan int) { if len(xs) > 0 { <-ch } }`, 0},
{"different channel", `func f(a, b chan int) { if len(a) < cap(a) { b <- 1 }; if len(a) > 0 { <-b } }`, 0},
{"different capacity", `func f(a, b chan int) { if len(a) < cap(b) { a <- 1 } }`, 0},
{"different receiver", `type q struct { ch chan int }; func f(a, b *q) { if len(a.ch) > 0 { <-b.ch } }`, 0},
{"shadowed len", `func f(ch chan int) { len := func(chan int) int { return 1 }; if len(ch) > 0 { <-ch } }`, 0},
{"shadowed cap", `func f(ch chan int) { cap := func(chan int) int { return 1 }; if len(ch) < cap(ch) { ch <- 1 } }`, 0},
{"shadowed channel", `func f(ch chan int) { if len(ch) > 0 { ch := make(chan int); <-ch } }`, 0},
{"shadowed receiver", `type q struct { ch chan int }; func f(v *q) { if len(v.ch) > 0 { v := &q{}; <-v.ch } }`, 0},
{"reassigned channel", `func f(ch, other chan int) { if len(ch) > 0 { ch = other; <-ch } }`, 0},
{"reassigned field", `type q struct { ch chan int }; func f(v *q, ch chan int) { if len(v.ch) > 0 { v.ch = ch; <-v.ch } }`, 0},
{"reassigned receiver", `type q struct { ch chan int }; func f(v, other *q) { if len(v.ch) > 0 { v = other; <-v.ch } }`, 0},
{"channel alias", `func f(ch chan int) { other := ch; if len(ch) > 0 { <-other } }`, 0},
{"nested goroutine", `func f(ch chan int) { if len(ch) > 0 { go func() { <-ch }() } }`, 0},
{"nested function", `func f(ch chan int) { if len(ch) > 0 { fn := func() { <-ch }; fn() } }`, 0},
{"immediate function", `func f(ch chan int) { if len(ch) > 0 { func() { <-ch }() } }`, 0},
{"deferred function", `func f(ch chan int) { if len(ch) > 0 { defer func() { <-ch }() } }`, 0},
{"nonblocking receive", `func f(ch chan int) { if len(ch) > 0 { select { case <-ch: default: } } }`, 0},
{"nonblocking receive assignment", `func f(ch chan int) { if len(ch) > 0 { select { case v, ok := <-ch: _, _ = v, ok; default: } } }`, 0},
{"nonblocking send", `func f(ch chan int) { if len(ch) < cap(ch) { select { case ch <- 1: default: } } }`, 0},
{"full does not guard send", `func f(ch chan int) { if len(ch) == cap(ch) { ch <- 1 } }`, 0},
{"empty does not guard receive", `func f(ch chan int) { if len(ch) == 0 { <-ch } }`, 0},
{"possibly empty", `func f(ch chan int) { if len(ch) >= 0 { <-ch } }`, 0},
{"possibly full", `func f(ch chan int) { if len(ch) <= cap(ch) { ch <- 1 } }`, 0},
{"unknown receive bound", `func f(ch chan int, n int) { if len(ch) > n { <-ch } }`, 0},
{"disjunction", `func f(ch chan int, ready bool) { if ready || len(ch) > 0 { <-ch } }`, 0},
{"else conjunction", `func f(ch chan int, ready bool) { if ready && len(ch) == 0 {} else { <-ch } }`, 0},
{"unrelated subsequent receive", `func f(ch chan int) { if len(ch) > 0 {}; <-ch }`, 0},
{"conditional return", `func f(ch chan int, ready bool) { if len(ch) == 0 { if ready { return } }; <-ch }`, 0},
{"repeated call", `func f(get func() chan int) { if len(get()) > 0 { <-get() } }`, 0},
{"select receive reassignment", `func f(ch chan int, outer chan chan int) { if len(ch) > 0 { select { case ch = <-outer: <-ch; default: } } }`, 0},
{"if initializer reassignment", `func f(ch, other chan int) { if len(ch) > 0 { if ch = other; true { <-ch } } }`, 0},
{"switch initializer reassignment", `func f(ch, other chan int) { if len(ch) > 0 { switch ch = other; { default: <-ch } } }`, 0},
{"loop reassignment", `func f(ch, other chan int) { if len(ch) > 0 { for { <-ch; ch = other } } }`, 0},
{"loop initializer reassignment", `func f(ch, other chan int) { if len(ch) > 0 { for ch = other; ; { <-ch } } }`, 0},
{"range reassignment", `func f(ch chan int, chs []chan int) { if len(ch) > 0 { for _, ch = range chs { <-ch } } }`, 0},
{"pointer reassignment", `func f(ch *chan int, other chan int) { if len(*ch) > 0 { *ch = other; <-*ch } }`, 0},
{"indexed channels", `func f(chs []chan int) { if len(chs[0]) > 0 { <-chs[0] } }`, 0},
} {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
offenses := coptest.RunTyped(t, newChannelStateCheck(), "package p\n"+tc.src)
require.Len(t, offenses, tc.want)
for _, offense := range offenses {
assert.Equal(t, "Lint/ChannelStateCheck", offense.CopName)
assert.Contains(t, offense.Message, "does not reserve")
assert.Contains(t, offense.Message, "select or explicit synchronization")
}
})
}
}
func TestChannelStateCheckScope(t *testing.T) {
t.Parallel()
const src = "package p\nfunc f(ch chan int) { if len(ch) > 0 { <-ch } }"
files := coptest.ProgramFiles{
"pkg/config/v0/sample.go": src,
"pkg/config/v15/sample.go": src,
"pkg/config/latest/sample.go": src,
"generated/sample.go": "// Code generated by fixture; DO NOT EDIT.\n" + src,
}
offenses := coptest.RunProgram(t, prog.FromFile(newChannelStateCheck()), files)
require.Len(t, offenses, 1)
assert.True(t, strings.HasSuffix(filepath.ToSlash(offenses[0].Pos.Filename), "/pkg/config/latest/sample.go"))
assert.Empty(t, coptest.Run(t, newChannelStateCheck(), src))
}
func TestChannelStateCheckRunner(t *testing.T) {
dir := t.TempDir()
t.Chdir(dir)
const src = "package p\nfunc f(ch chan int) { if len(ch) > 0 { <-ch } }\n"
for name, content := range map[string]string{
"go.mod": "module example.test\n\ngo 1.27\n",
"reported/sample.go": src,
"inline/sample.go": strings.ReplaceAll(src, " }\n", " } //rubocop:disable Lint/ChannelStateCheck\n"),
"suppressed/sample.go": "//rubocop:disable-file Lint/ChannelStateCheck\n" + src,
"tests/sample.go": "package p\n",
"tests/sample_test.go": src,
"external/sample.go": "package p\n",
"external/sample_test.go": strings.Replace(src, "package p", "package p_test", 1),
"testonly/sample_test.go": src,
"generated/sample_test.go": "// Code generated by fixture; DO NOT EDIT.\n" + src,
"pkg/config/v1/sample_test.go": src,
} {
require.NoError(t, os.MkdirAll(filepath.Dir(name), 0o700))
require.NoError(t, os.WriteFile(name, []byte(content), 0o600))
}
var output bytes.Buffer
// The production wrapper extracts this file cop from a program-cop constructor.
wrapped := withTypedFiles(func(opts ...cop.FuncOption) *prog.Func { return prog.FromFile(newChannelStateCheck(opts...)) })
r := runner.New(nil, config.DefaultConfig(), &output).WithProgramCops([]prog.Cop{wrapped})
count, err := r.Run([]string{"."})
require.NoError(t, err)
assert.Equal(t, 4, count, output.String())
for _, name := range []string{"reported/sample.go", "tests/sample_test.go", "external/sample_test.go", "testonly/sample_test.go"} {
assert.Contains(t, filepath.ToSlash(output.String()), name)
content, err := os.ReadFile(name)
require.NoError(t, err)
assert.Contains(t, string(content), "if len(ch) > 0 { <-ch }")
}
}
+6 -6
View File
@@ -75,14 +75,14 @@ var programCops = []prog.Cop{
rubocops.NewLintCutPrefix(cop.WithScope(outsideFrozenConfig)),
rubocops.NewLintCutSuffix(cop.WithScope(outsideFrozenConfig)),
rubocops.NewLintFieldsSeqLookup(),
SlicesConcat,
rubocops.NewLintSlicesConcat(cop.WithScope(outsideFrozenConfig)),
ChannelDirection,
CancellationLoop,
ChannelStateCheck,
ClosedChannelLoop,
rubocops.NewLintCancellationLoop(cop.WithScope(outsideFrozenConfig)),
rubocops.NewLintChannelStateCheck(cop.WithScope(outsideFrozenConfig)),
rubocops.NewLintClosedChannelLoop(cop.WithScope(outsideFrozenConfig)),
SessionStateAccessors,
CancellationSafeResultSend,
NonblockingChannelReplacement,
rubocops.NewLintCancellationSafeResultSend(cop.WithScope(outsideFrozenConfig)),
rubocops.NewLintNonblockingChannelReplacement(cop.WithScope(outsideFrozenConfig)),
BoundedCommandPipeLifetime,
SilentLimitedRead,
rubocops.NewLintStreamCloseSafety(),
+173
View File
@@ -0,0 +1,173 @@
package main
import (
"bytes"
"encoding/json"
"os"
"path/filepath"
"strings"
"testing"
"github.com/dgageot/rubocop-go/config"
"github.com/dgageot/rubocop-go/coptest"
"github.com/dgageot/rubocop-go/prog"
"github.com/dgageot/rubocop-go/runner"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestMigratedChannelCopsRunner(t *testing.T) {
dir := t.TempDir()
t.Chdir(dir)
const src = `package p
import "context"
func concat(a, b []int) []int { return append(append([]int(nil), a...), b...) } // offense
func cancellation(ctx context.Context, ch chan int) { for { select { case <-ctx.Done(): continue; case <-ch: } } } // offense
func closed(ch chan int) { for { _, ok := <-ch; if !ok { continue } } } // offense
func state(ch chan int) { if len(ch) > 0 { <-ch } } // offense
func result(ctx context.Context) { ch := make(chan int); go func(){ ch <- 1 }(); select { case <-ctx.Done(): return; case <-ch: } } // offense
func replacement(ch chan int) { select { case ch <- 1: default: <-ch; ch <- 1 } } // offense
`
checks := []struct {
name string
testAware bool
}{
{"SlicesConcat", false},
{"CancellationLoop", true},
{"ClosedChannelLoop", true},
{"ChannelStateCheck", true},
{"CancellationSafeResultSend", false},
{"NonblockingChannelReplacement", false},
}
var selected []prog.Cop
var ids []string
cfg := config.DefaultConfig()
for _, check := range checks {
id := "Lint/" + check.name
selected = append(selected, registeredSharedCop(t, programCops, id))
ids = append(ids, id)
cfg.Cops[id] = config.CopConfig{Severity: "warning"}
for _, c := range cops {
assert.NotEqual(t, id, c.Name(), "must only be registered as a program cop")
}
count := 0
for _, c := range programCops {
if c.Name() == id {
count++
}
}
assert.Equal(t, 1, count, id)
}
var want []string
files := make(map[string]string)
for _, fixture := range []struct {
dir, prefix string
excluded bool
}{
{dir: "reported"},
{dir: "pkg/config/v0", excluded: true},
{dir: "pkg/config/v15", excluded: true},
{dir: "nested/pkg/config/v2", excluded: true},
{dir: "pkg/config/latest"},
{dir: "pkg/config/version"},
{dir: "pkg/config/v1beta"},
{dir: "notpkg/config/v0"},
{dir: "generated", prefix: "// Code generated by fixture; DO NOT EDIT.\n", excluded: true},
{dir: "inactive", prefix: "//go:build lint_fixture_disabled\n\n", excluded: true},
{dir: "inline", excluded: true},
{dir: "suppressed", prefix: "//rubocop:disable-file " + strings.Join(ids, ", ") + "\n", excluded: true},
{dir: "external"},
{dir: "testonly"},
{dir: "illtyped"},
{dir: "illtypedexternal"},
{dir: "illtypedonly"},
} {
for _, filename := range []string{"sample.go", "sample_test.go"} {
isTest := strings.HasSuffix(filename, "_test.go")
content := src
testsOnly := fixture.dir == "external" || fixture.dir == "testonly" || strings.HasPrefix(fixture.dir, "illtyped")
if !isTest && testsOnly {
if fixture.dir == "testonly" || fixture.dir == "illtypedonly" {
continue
}
content = "package p\n"
}
if isTest {
content = strings.ReplaceAll(content, "\nfunc ", "\nfunc test")
if fixture.dir == "external" || fixture.dir == "illtypedexternal" {
content = strings.Replace(content, "package p", "package p_test", 1)
}
if strings.HasPrefix(fixture.dir, "illtyped") {
content += "\nvar _ = missing\n"
}
}
if fixture.dir == "inline" {
content = strings.ReplaceAll(content, "// offense", "//rubocop:disable "+strings.Join(ids, ", "))
}
path := fixture.dir + "/" + filename
files[path] = fixture.prefix + content
if fixture.excluded || (!isTest && testsOnly) {
continue
}
for _, check := range checks {
if !isTest || check.testAware {
want = append(want, "Lint/"+check.name+":"+path)
}
}
}
}
for path, content := range files {
require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700))
require.NoError(t, os.WriteFile(path, []byte(content), 0o600))
}
for _, version := range []string{"1.21", "1.22", "1.27"} {
t.Run(version, func(t *testing.T) {
require.NoError(t, os.WriteFile("go.mod", []byte("module example.test\n\ngo "+version+"\n"), 0o600))
var output bytes.Buffer
r := runner.New(nil, cfg, &output).WithProgramCops(selected)
r.Reporter = runner.NewJSONReporter(&output)
count, err := r.Run([]string{"."})
require.NoError(t, err)
var expected []string
for _, offense := range want {
if version != "1.21" || !strings.HasPrefix(offense, "Lint/SlicesConcat:") {
expected = append(expected, offense)
}
}
assert.Equal(t, len(expected), count, output.String())
var report struct {
Offenses []struct{ Cop, File, Severity string }
}
require.NoError(t, json.Unmarshal(output.Bytes(), &report))
var got []string
for _, offense := range report.Offenses {
assert.Equal(t, "warning", offense.Severity)
path, err := filepath.Rel(dir, offense.File)
require.NoError(t, err)
got = append(got, offense.Cop+":"+filepath.ToSlash(path))
}
assert.ElementsMatch(t, expected, got)
for path, content := range files {
actual, err := os.ReadFile(path)
require.NoError(t, err)
assert.Equal(t, content, string(actual), path)
}
})
}
}
func TestSharedStreamCloseSafetySliceBounds(t *testing.T) {
t.Parallel()
check := registeredSharedCop(t, programCops, "Lint/StreamCloseSafety")
offenses := coptest.RunProgram(t, check, coptest.ProgramFiles{
"stream.go": `package p
import "sync"
type stream struct { mu sync.Mutex; data []byte }
func (s *stream) Next() []byte { s.mu.Lock(); defer s.mu.Unlock(); return s.data[:] }
func (s *stream) Close() { s.mu.Lock(); defer s.mu.Unlock(); s.data = nil }
`,
})
assert.Empty(t, offenses)
}
-113
View File
@@ -1,113 +0,0 @@
package main
import (
"go/ast"
"go/token"
"go/types"
"github.com/dgageot/rubocop-go/cop"
"github.com/dgageot/rubocop-go/prog"
)
// NonblockingChannelReplacement checks adjacent drain/resend fallbacks, not
// arbitrary queue protocols or channel expressions with calls or indexes.
var NonblockingChannelReplacement = prog.FromFile(cop.New(cop.Meta{
Name: "Lint/NonblockingChannelReplacement",
Description: "Keep channel replacement fallbacks nonblocking",
Severity: cop.Warning,
}, checkNonblockingChannelReplacement, cop.WithTypes(), cop.WithScope(outsideFrozenConfig)))
func checkNonblockingChannelReplacement(p *cop.Pass) {
if p.Info == nil || p.IsTestFile() || ast.IsGenerated(p.File) {
return
}
for sel := range cop.Nodes[*ast.SelectStmt](p.File) {
if len(sel.Body.List) != 2 {
continue
}
var send *ast.SendStmt
var fallback *ast.CommClause
for _, stmt := range sel.Body.List {
clause := stmt.(*ast.CommClause)
if clause.Comm == nil {
fallback = clause
} else {
send, _ = clause.Comm.(*ast.SendStmt)
}
}
if send == nil || fallback == nil || len(fallback.Body) != 2 {
continue
}
stable := true
ast.Inspect(send.Value, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.CallExpr:
stable = false
case *ast.UnaryExpr:
stable = stable && n.Op != token.ARROW
}
return stable
})
if !stable {
continue
}
recv := replacementReceive(p, fallback.Body[0])
retry, ok := fallback.Body[1].(*ast.SendStmt)
if recv == nil || !ok || !sameReplacementChannel(p, send.Chan, recv.X) || !sameReplacementChannel(p, send.Chan, retry.Chan) {
continue
}
p.Report(recv, "channel replacement can block if another receiver drains it after the select; keep the fallback drain and resend nonblocking")
}
}
func replacementReceive(p *cop.Pass, stmt ast.Stmt) *ast.UnaryExpr {
var expr ast.Expr
switch stmt := stmt.(type) {
case *ast.ExprStmt:
expr = stmt.X
case *ast.AssignStmt:
for _, lhs := range stmt.Lhs {
id, ok := lhs.(*ast.Ident)
if !ok || id.Name != "_" && (stmt.Tok != token.DEFINE || p.Info.Defs[id] == nil) {
return nil
}
}
if len(stmt.Rhs) == 1 {
expr = stmt.Rhs[0]
}
}
if expr != nil {
if recv, ok := ast.Unparen(expr).(*ast.UnaryExpr); ok && recv.Op == token.ARROW {
return recv
}
}
return nil
}
func sameReplacementChannel(p *cop.Pass, left, right ast.Expr) bool {
typ := p.Info.TypeOf(left)
if typ == nil {
return false
}
if _, ok := typ.Underlying().(*types.Chan); !ok {
return false
}
return sameReplacementExpr(p, left, right)
}
func sameReplacementExpr(p *cop.Pass, left, right ast.Expr) bool {
switch left := ast.Unparen(left).(type) {
case *ast.Ident:
right, ok := ast.Unparen(right).(*ast.Ident)
obj, local := p.Info.ObjectOf(left).(*types.Var)
return ok && local && obj.Parent() != nil && obj.Pkg() != nil && obj.Parent() != obj.Pkg().Scope() && obj == p.Info.ObjectOf(right)
case *ast.SelectorExpr:
right, ok := ast.Unparen(right).(*ast.SelectorExpr)
if !ok {
return false
}
l, r := p.Info.Selections[left], p.Info.Selections[right]
return l != nil && r != nil && l.Kind() == types.FieldVal && r.Kind() == types.FieldVal && l.Obj() == r.Obj() && sameReplacementExpr(p, left.X, right.X)
}
return false
}
+1 -76
View File
@@ -1,9 +1,7 @@
package main
import (
"fmt"
"os"
"path/filepath"
"strings"
"testing"
@@ -12,73 +10,6 @@ import (
"github.com/stretchr/testify/require"
)
func TestNonblockingChannelReplacement(t *testing.T) {
t.Parallel()
tests := []struct {
name, src string
want int
}{
{"local", `func f(c chan int) { select { case c <- 1: default: <-c; c <- 1 } }`, 1},
{"local allocation", `func f() { c := make(chan int, 1); select { case c <- 1: default: <-c; c <- 1 } }`, 1},
{"stable selector", `type watcher struct { changes chan string }; func (w *watcher) refresh(branch string) { select { case w.changes <- branch: default: <-w.changes; w.changes <- branch } }`, 1},
{"nested selector", `type watcher struct { state struct { changes chan int } }; func f(w *watcher) { select { case w.state.changes <- 1: default: <-w.state.changes; w.state.changes <- 1 } }`, 1},
{"assigned receive", `func f(c chan int) { select { case c <- 1: default: _ = <-c; c <- 1 } }`, 1},
{"comma ok receive", `func f(c chan int) { select { case c <- 1: default: _, _ = <-c; c <- 1 } }`, 1},
{"declared receive", `func f(c chan int) { select { case c <- 1: default: old := <-c; c <- old } }`, 1},
{"receive reassigns channel", `type ch chan ch; func f(c ch) { select { case c <- c: default: c = <-c; c <- c } }`, 0},
{"receive reassigns selector", `type w struct { c chan *w }; func f(a *w) { select { case a.c <- a: default: a = <-a.c; a.c <- a } }`, 0},
{"parentheses", `func f(c chan int) { select { case (c) <- 1: default: <-((c)); (c) <- 1 } }`, 1},
{"default first", `func f(c chan int) { select { default: <-c; c <- 1; case c <- 1: } }`, 1},
{"shadowed unrelated local", `func f(c chan int) { { c := make(chan int, 1); select { case c <- 1: default: <-c; c <- 1 } } }`, 1},
{"no replacement", `func f(c chan int) { select { case c <- 1: default: } }`, 0},
{"no default", `func f(c, other chan int) { select { case c <- 1: case <-other: <-c; c <- 1 } }`, 0},
{"nonblocking drain", `func f(c chan int) { select { case c <- 1: default: select { case <-c: default: }; c <- 1 } }`, 0},
{"nonblocking resend", `func f(c chan int) { select { case c <- 1: default: <-c; select { case c <- 1: default: } } }`, 0},
{"unrelated drain", `func f(c, other chan int) { select { case c <- 1: default: <-other; c <- 1 } }`, 0},
{"unrelated resend", `func f(c, other chan int) { select { case c <- 1: default: <-c; other <- 1 } }`, 0},
{"different selector base", `type w struct { c chan int }; func f(a, b *w) { select { case a.c <- 1: default: <-b.c; a.c <- 1 } }`, 0},
{"different field", `type w struct { a, b chan int }; func f(w *w) { select { case w.a <- 1: default: <-w.b; w.a <- 1 } }`, 0},
{"shadowed channel", `func f(c chan int, other chan chan int) { select { case c <- 1: default: c := <-other; c <- 1 } }`, 0},
{"shadowed selector base", `type w struct { c chan *w }; func f(a *w) { select { case a.c <- a: default: a := <-a.c; a.c <- a } }`, 0},
{"call expression", `func f(get func() chan int) { select { case get() <- 1: default: <-get(); get() <- 1 } }`, 0},
{"mutating send value", `type w struct { c chan int }; func f(w *w, mutate func() int) { select { case w.c <- mutate(): default: <-w.c; w.c <- 1 } }`, 0},
{"receiving send value", `func f(c chan int) { select { case c <- <-c: default: <-c; c <- 1 } }`, 0},
{"indexed channel", `func f(cs []chan int) { select { case cs[0] <- 1: default: <-cs[0]; cs[0] <- 1 } }`, 0},
{"package global", `var c chan int; func f() { select { case c <- 1: default: <-c; c <- 1 } }`, 0},
{"intervening work", `func f(c chan int, work func()) { select { case c <- 1: default: <-c; work(); c <- 1 } }`, 0},
{"additional select arm", `func f(c, done chan int) { select { case c <- 1: case <-done: default: <-c; c <- 1 } }`, 0},
}
files := coptest.ProgramFiles{}
for i, tc := range tests {
files[fmt.Sprintf("case%d/sample.go", i)] = "package p\n" + tc.src
}
offenses := coptest.RunProgram(t, NonblockingChannelReplacement, files)
got := make(map[string]int)
for _, o := range offenses {
assert.Equal(t, "Lint/NonblockingChannelReplacement", o.CopName)
assert.Contains(t, o.Message, "another receiver")
got[filepath.Base(filepath.Dir(o.Pos.Filename))]++
}
for i, tc := range tests {
assert.Equal(t, tc.want, got[fmt.Sprintf("case%d", i)], tc.name)
}
}
func TestNonblockingChannelReplacementScope(t *testing.T) {
t.Parallel()
const src = "package p\nfunc f(c chan int) { select { case c <- 1: default: <-c; c <- 1 } }\n"
offenses := coptest.RunProgram(t, NonblockingChannelReplacement, coptest.ProgramFiles{
"pkg/config/v0/sample.go": src,
"pkg/config/v15/sample.go": src,
"pkg/config/latest/sample.go": src,
"generated/sample.go": "// Code generated by fixture; DO NOT EDIT.\n" + src,
"tests/sample.go": "package p\n",
"tests/sample_test.go": src,
})
require.Len(t, offenses, 1)
assert.True(t, strings.HasSuffix(filepath.ToSlash(offenses[0].Pos.Filename), "/pkg/config/latest/sample.go"))
}
func TestNonblockingChannelReplacementWatcher(t *testing.T) {
t.Parallel()
src, err := os.ReadFile("../pkg/gitbranch/watcher.go")
@@ -86,7 +17,7 @@ func TestNonblockingChannelReplacementWatcher(t *testing.T) {
// Keep the actual method while avoiding the unrelated fsnotify dependency.
_, method, ok := strings.Cut(string(src), "func (w *Watcher) refresh()")
require.True(t, ok)
offenses := coptest.RunProgram(t, NonblockingChannelReplacement, coptest.ProgramFiles{
offenses := coptest.RunProgram(t, registeredSharedCop(t, programCops, "Lint/NonblockingChannelReplacement"), coptest.ProgramFiles{
"gitbranch/watcher.go": `package gitbranch
import "sync"
type Watcher struct { mu sync.Mutex; dir, current string; changes chan string }
@@ -95,9 +26,3 @@ func (w *Watcher) refresh()` + method,
})
assert.Empty(t, offenses)
}
func TestNonblockingChannelReplacementSuppressions(t *testing.T) {
testCorrectnessCopSuppressions(t, NonblockingChannelReplacement, `package sample
func f(c chan int) { select { case c <- 1: default: <-c; c <- 1 } } // offense
`)
}
+5 -4
View File
@@ -44,7 +44,7 @@ var OTelTracerName = &cop.Func{
}
arg := call.Args[0]
name, ok := tracerName(p, arg)
if !ok || name == expected || isAppName(p.Info, arg) {
if !ok || name == expected || isAppName(p, arg) {
return
}
if name == "cagent" {
@@ -75,13 +75,14 @@ func packageImportPath(pkgPath string) string {
return modulePath + "/" + strings.TrimPrefix(pkgPath, "./")
}
func isAppName(info *types.Info, expr ast.Expr) bool {
c, ok := constObject(info, expr)
func isAppName(p *cop.Pass, expr ast.Expr) bool {
c, ok := constObject(p.Info, expr)
if !ok || c.Name() != "AppName" || c.Pkg() == nil {
return false
}
pkg := c.Pkg().Path()
return pkg == versionPkgPath || pkg == cmdRootPkgPath
return pkg == versionPkgPath || pkg == cmdRootPkgPath ||
c.Pkg() == p.Package && p.FileUnder("cmd/root")
}
func constStringValue(info *types.Info, expr ast.Expr) (string, bool) {
+33
View File
@@ -1,9 +1,15 @@
package main
import (
"bytes"
"os"
"path/filepath"
"testing"
"github.com/dgageot/rubocop-go/config"
"github.com/dgageot/rubocop-go/cop"
"github.com/dgageot/rubocop-go/coptest"
"github.com/dgageot/rubocop-go/runner"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@@ -55,3 +61,30 @@ func f() { _ = otel.Tracer("wrong") }
require.Len(t, offenses, 1)
assert.Contains(t, offenses[0].Message, `got "wrong"`)
}
func TestOTelTracerNameRunnerAllowsRootAppName(t *testing.T) {
dir := t.TempDir()
t.Chdir(dir)
const src = `package root
import "go.opentelemetry.io/otel"
const AppName = "docker-agent"
func f() { _ = otel.Tracer(AppName) }
`
for name, content := range map[string]string{
"go.mod": "module github.com/docker/docker-agent\n\ngo 1.27\nrequire go.opentelemetry.io/otel v0.0.0\nreplace go.opentelemetry.io/otel => ./otel\n",
"otel/go.mod": "module go.opentelemetry.io/otel\n\ngo 1.27\n",
"otel/tracer.go": "package otel\nfunc Tracer(string) any { return nil }\n",
"cmd/root/tracer.go": src,
"pkg/other/tracer.go": src,
} {
require.NoError(t, os.MkdirAll(filepath.Dir(name), 0o700))
require.NoError(t, os.WriteFile(name, []byte(content), 0o600))
}
var output bytes.Buffer
r := runner.New([]cop.Cop{OTelTracerName}, config.DefaultConfig(), &output)
count, err := r.Run([]string{"."})
require.NoError(t, err)
assert.Equal(t, 1, count, output.String())
assert.Contains(t, filepath.ToSlash(output.String()), "pkg/other/tracer.go")
assert.NotContains(t, filepath.ToSlash(output.String()), "cmd/root/tracer.go")
}
-236
View File
@@ -1,236 +0,0 @@
package main
import (
"go/ast"
"go/constant"
"go/token"
"go/types"
"github.com/dgageot/rubocop-go/cop"
"github.com/dgageot/rubocop-go/prog"
)
// SlicesConcat finds fresh-slice append chains, not appends that may reuse caller storage.
var SlicesConcat = prog.FromFile(cop.New(cop.Meta{
Name: "Lint/SlicesConcat",
Description: "Use slices.Concat to concatenate slices into fresh storage",
Severity: cop.Convention,
}, checkSlicesConcat, cop.WithTypes(), cop.WithMinStdlibVersion("go1.22"), cop.WithScope(outsideFrozenConfig)))
func checkSlicesConcat(p *cop.Pass) {
if p.Info == nil || ast.IsGenerated(p.File) {
return
}
seen := make(map[*ast.CallExpr]bool)
checkStatements := func(stmts []ast.Stmt) {
for i, stmt := range stmts {
id, init := concatLocal(stmt)
if id == nil || p.Info.Defs[id] == nil {
continue
}
obj := p.Info.Defs[id]
typ := obj.Type()
var inputs []ast.Expr
var calls []*ast.CallExpr
if init != nil {
var ok bool
inputs, calls, ok = concatChain(p, init)
if !ok || !types.Identical(p.Info.TypeOf(init), typ) {
continue
}
}
blocked := false
for _, next := range stmts[i+1:] {
assign, ok := next.(*ast.AssignStmt)
if !ok || assign.Tok != token.ASSIGN || len(assign.Lhs) != 1 || len(assign.Rhs) != 1 || !concatObject(p, assign.Lhs[0], obj) {
break
}
call, ok := ast.Unparen(assign.Rhs[0]).(*ast.CallExpr)
if !ok || !concatAppend(p, call) || !concatObject(p, call.Args[0], obj) || !concatInput(p, call.Args[1], typ) || concatUses(p, call.Args[1], obj) {
blocked = true
break
}
inputs = append(inputs, call.Args[1])
calls = append(calls, call)
}
if !blocked && len(inputs) >= 2 {
reportConcat(p, stmt)
}
if blocked || len(inputs) >= 2 {
for _, call := range calls {
seen[call] = true
}
}
}
}
ast.Inspect(p.File, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.BlockStmt:
checkStatements(n.List)
case *ast.CaseClause:
checkStatements(n.Body)
case *ast.CommClause:
checkStatements(n.Body)
case *ast.CallExpr:
if p.CalleeObject(n) == types.Universe.Lookup("append") && !n.Ellipsis.IsValid() {
markConcatAppends(p, n, seen)
}
if seen[n] {
return true
}
inputs, calls, ok := concatChain(p, n)
if ok && len(inputs) >= 2 {
reportConcat(p, n)
for _, call := range calls {
seen[call] = true
}
}
}
return true
})
}
func markConcatAppends(p *cop.Pass, expr ast.Expr, seen map[*ast.CallExpr]bool) {
for {
call, ok := ast.Unparen(expr).(*ast.CallExpr)
if !ok || p.CalleeObject(call) != types.Universe.Lookup("append") || len(call.Args) == 0 {
return
}
seen[call] = true
expr = call.Args[0]
}
}
func reportConcat(p *cop.Pass, n ast.Node) {
p.Report(n, "use slices.Concat to combine slices into fresh storage; preserve nil/empty and capacity-sensitive behavior")
}
func concatLocal(stmt ast.Stmt) (*ast.Ident, ast.Expr) {
switch stmt := stmt.(type) {
case *ast.AssignStmt:
if stmt.Tok == token.DEFINE && len(stmt.Lhs) == 1 && len(stmt.Rhs) == 1 {
id, _ := stmt.Lhs[0].(*ast.Ident)
return id, stmt.Rhs[0]
}
case *ast.DeclStmt:
decl, ok := stmt.Decl.(*ast.GenDecl)
if !ok || decl.Tok != token.VAR || len(decl.Specs) != 1 {
break
}
spec := decl.Specs[0].(*ast.ValueSpec)
if len(spec.Names) == 1 {
if len(spec.Values) == 1 {
return spec.Names[0], spec.Values[0]
}
return spec.Names[0], nil
}
}
return nil, nil
}
func concatChain(p *cop.Pass, expr ast.Expr) (inputs []ast.Expr, calls []*ast.CallExpr, ok bool) {
expr = ast.Unparen(expr)
if typ := p.Info.TypeOf(expr); typ == nil {
return nil, nil, false
} else if _, ok := typ.Underlying().(*types.Slice); !ok {
return nil, nil, false
}
switch expr := expr.(type) {
case *ast.CompositeLit:
return nil, nil, len(expr.Elts) == 0
case *ast.CallExpr:
if fn, ok := p.CalleeObject(expr).(*types.Func); ok && fn.Pkg() != nil && fn.Pkg().Path() == "slices" && fn.Name() == "Clone" && len(expr.Args) == 1 && concatInput(p, expr.Args[0], p.Info.TypeOf(expr)) {
return []ast.Expr{expr.Args[0]}, nil, true
}
switch {
case concatAppend(p, expr):
inputs, calls, ok = concatChain(p, expr.Args[0])
if !ok || !concatInput(p, expr.Args[1], p.Info.TypeOf(expr)) {
return nil, nil, false
}
return append(inputs, expr.Args[1]), append(calls, expr), true
case p.CalleeObject(expr) == types.Universe.Lookup("make"):
if len(expr.Args) < 2 || !concatZero(p, expr.Args[1]) {
return nil, nil, false
}
return nil, nil, len(expr.Args) == 2 || concatCapacity(p, expr.Args[2])
case len(expr.Args) == 1 && p.Info.Types[expr.Fun].IsType():
id, ok := ast.Unparen(expr.Args[0]).(*ast.Ident)
return nil, nil, ok && p.Info.Uses[id] == types.Universe.Lookup("nil")
}
}
return nil, nil, false
}
func concatAppend(p *cop.Pass, call *ast.CallExpr) bool {
return p.CalleeObject(call) == types.Universe.Lookup("append") && call.Ellipsis.IsValid() && len(call.Args) == 2
}
func concatInput(p *cop.Pass, expr ast.Expr, typ types.Type) bool {
return types.Identical(p.Info.TypeOf(expr), typ) && concatStable(p, expr)
}
// Calls could mutate an earlier input before Concat copies it, unlike chained append.
func concatStable(p *cop.Pass, expr ast.Expr) bool {
if expr == nil {
return true
}
switch expr := ast.Unparen(expr).(type) {
case *ast.Ident, *ast.BasicLit:
return true
case *ast.SelectorExpr:
return concatStable(p, expr.X)
case *ast.SliceExpr:
return concatStable(p, expr.X) && concatStable(p, expr.Low) && concatStable(p, expr.High) && concatStable(p, expr.Max)
case *ast.IndexExpr:
return concatStable(p, expr.X) && concatStable(p, expr.Index)
case *ast.BinaryExpr:
return concatStable(p, expr.X) && concatStable(p, expr.Y)
case *ast.CallExpr:
obj := p.CalleeObject(expr)
return (obj == types.Universe.Lookup("len") || obj == types.Universe.Lookup("cap")) && len(expr.Args) == 1 && concatStable(p, expr.Args[0])
}
return false
}
func concatCapacity(p *cop.Pass, expr ast.Expr) bool {
switch expr := ast.Unparen(expr).(type) {
case *ast.Ident, *ast.BasicLit, *ast.SelectorExpr:
return concatStable(p, expr)
case *ast.BinaryExpr:
return (expr.Op == token.ADD || expr.Op == token.SUB) && concatCapacity(p, expr.X) && concatCapacity(p, expr.Y)
case *ast.CallExpr:
obj := p.CalleeObject(expr)
return (obj == types.Universe.Lookup("len") || obj == types.Universe.Lookup("cap")) && len(expr.Args) == 1 && concatCapacityBase(expr.Args[0])
}
return false
}
func concatCapacityBase(expr ast.Expr) bool {
switch expr := ast.Unparen(expr).(type) {
case *ast.Ident:
return true
case *ast.SelectorExpr:
return concatCapacityBase(expr.X)
}
return false
}
func concatZero(p *cop.Pass, expr ast.Expr) bool {
v := p.Info.Types[expr].Value
return v != nil && v.Kind() == constant.Int && constant.Sign(v) == 0
}
func concatObject(p *cop.Pass, expr ast.Expr, obj types.Object) bool {
id, ok := ast.Unparen(expr).(*ast.Ident)
return ok && p.Info.Uses[id] == obj
}
func concatUses(p *cop.Pass, expr ast.Expr, obj types.Object) bool {
for id := range cop.Nodes[*ast.Ident](expr) {
if p.Info.Uses[id] == obj {
return true
}
}
return false
}
-140
View File
@@ -1,140 +0,0 @@
package main
import (
"bytes"
"fmt"
"os"
"path/filepath"
"strings"
"testing"
"github.com/dgageot/rubocop-go/config"
"github.com/dgageot/rubocop-go/coptest"
"github.com/dgageot/rubocop-go/prog"
"github.com/dgageot/rubocop-go/runner"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestSlicesConcat(t *testing.T) {
t.Parallel()
tests := []struct {
name, src string
want int
}{
{"nested nil", `func f(a, b []int) []int { return append(append([]int(nil), a...), b...) }`, 1},
{"nested empty", `func f(a, b, c []int) []int { return append(append(append([]int{}, a...), b...), c...) }`, 1},
{"nested make", `func f(a, b []int) []int { return append(append(make([]int, 0, len(a)+len(b)), a...), b...) }`, 1},
{"nil local", `func f(a, b []int) []int { var out []int; out = append(out, a...); out = append(out, b...); return out }`, 1},
{"typed nil local", `func f(a, b []int) []int { var out = []int(nil); out = append(out, a...); out = append(out, b...); return out }`, 1},
{"empty local", `func f(a, b []int) []int { out := []int{}; out = append(out, a...); out = append(out, b...); return out }`, 1},
{"make local", `func f(a, b []int) []int { out := make([]int, 0, len(a)+len(b)); out = append(out, a...); out = append(out, b...); return out }`, 1},
{"copied local", `func f(a, b []int) []int { out := append([]int(nil), a...); out = append(out, b...); return out }`, 1},
{"named slice", `type ints []int; func f(a, b ints) ints { out := ints(nil); out = append(out, a...); out = append(out, b...); return out }`, 1},
{"selectors and subslices", `type data struct { a, b []int }; func f(d data, i int) []int { out := make([]int, 0, len(d.a)-i+len(d.b)); out = append(out, d.a[:i]...); out = append(out, d.b[i:]...); return out }`, 1},
{"parentheses", `func f(a, b []int) []int { return append((append(([]int(nil)), (a)...)), (b)...) }`, 1},
{"clone", `import "slices"; func f(a, b []int) []int { return append(slices.Clone(a), b...) }`, 1},
{"aliased clone", `import s "slices"; func f(a, b []int) []int { out := s.Clone(a); out = append(out, b...); return out }`, 1},
{"switch clause", `func f(a, b []int) []int { switch { default: var out []int; out = append(out, a...); out = append(out, b...); return out } }`, 1},
{"select clause", `func f(a, b []int) []int { select { default: var out []int; out = append(out, a...); out = append(out, b...); return out } }`, 1},
{"one report", `func f(a, b, c []int) []int { out := append(append([]int(nil), a...), b...); out = append(out, c...); return out }`, 1},
{"existing slice", `func f(a, b []int) []int { return append(a, b...) }`, 0},
{"existing nested slice", `func f(a, b, c []int) []int { return append(append(a, b...), c...) }`, 0},
{"existing local", `func f(a, b, c []int) []int { out := a; out = append(out, b...); out = append(out, c...); return out }`, 0},
{"one copy", `func f(a []int) []int { return append([]int(nil), a...) }`, 0},
{"one local copy", `func f(a []int) []int { var out []int; out = append(out, a...); return out }`, 0},
{"scalar append", `func f(a []int) []int { return append(append([]int(nil), a...), 1) }`, 0},
{"prepend literal", `func f(a []int) []int { return append([]int{1}, a...) }`, 0},
{"nonzero make", `func f(a, b []int) []int { return append(append(make([]int, 1), a...), b...) }`, 0},
{"string append", `func f(a []byte, b string) []byte { return append(append([]byte(nil), a...), b...) }`, 0},
{"different named types", `type ints []int; func f(a ints, b []int) ints { return append(append(ints(nil), a...), b...) }`, 0},
{"shadowed append", `func f(a, b []int) []int { append := func(a []int, b ...int) []int { return a }; return append(append([]int(nil), a...), b...) }`, 0},
{"shadowed make", `func f(a, b []int) []int { make := func(int, int) []int { return a }; out := make(0, 0); out = append(out, a...); out = append(out, b...); return out }`, 0},
{"shadowed nil", `func f(a, b []int) []int { nil := a; return append(append([]int(nil), a...), b...) }`, 0},
{"fake clone", `type helper struct{}; func (helper) Clone(a []int) []int { return a }; func f(a, b []int) []int { slices := helper{}; return append(slices.Clone(a), b...) }`, 0},
{"tail call", `func f(a []int, mutate func() []int) []int { return append(append([]int(nil), a...), mutate()...) }`, 0},
{"head call", `func f(b []int, mutate func() []int) []int { return append(append([]int(nil), mutate()...), b...) }`, 0},
{"index call", `func f(a [][]int, b []int, index func() int) []int { return append(append([]int(nil), a[index()]...), b...) }`, 0},
{"receive", `func f(a []int, ch chan []int) []int { return append(append([]int(nil), a...), (<-ch)...) }`, 0},
{"capacity call", `func f(a, b []int, size func() int) []int { out := make([]int, 0, size()); out = append(out, a...); out = append(out, b...); return out }`, 0},
{"intervening call", `func f(a, b []int, mutate func()) []int { var out []int; out = append(out, a...); mutate(); out = append(out, b...); return out }`, 0},
{"intervening alias", `func f(a, b []int) []int { var out []int; out = append(out, a...); alias := out; out = append(out, b...); return alias }`, 0},
{"self append", `func f(a []int) []int { var out []int; out = append(out, a...); out = append(out, out...); return out }`, 0},
{"self length", `func f(a, b []int) []int { var out []int; out = append(out, a...); out = append(out, b[:len(out)]...); return out }`, 0},
{"shadowed local", `func f(a, b []int) []int { var out []int; out = append(out, a...); { out := append(out, b...); _ = out }; return out }`, 0},
{"loop accumulation", `func f(inputs [][]int) []int { var out []int; for _, a := range inputs { out = append(out, a...) }; return out }`, 0},
{"conditional append", `func f(a, b []int, yes bool) []int { var out []int; out = append(out, a...); if yes { out = append(out, b...) }; return out }`, 0},
{"explicit clone conversion", `import "slices"; type ints []int; func f(a ints, b []int) any { return append(slices.Clone[[]int](a), b...) }`, 0},
{"converted local", `import "slices"; type A []int; type B []int; func f(a A, b B) B { var out B = slices.Clone[[]int](a); out = append(out, b...); return out }`, 0},
{"indexed capacity", `func f(a, b []int, sizes []int) []int { out := make([]int, 0, sizes[0]); out = append(out, a...); out = append(out, b...); return out }`, 0},
{"divided capacity", `func f(a, b []int, n, d int) []int { out := make([]int, 0, n/d); out = append(out, a...); out = append(out, b...); return out }`, 0},
{"scalar tail", `func f(a, b []int) []int { out := make([]int, 0, len(a)+len(b)+1); out = append(out, a...); out = append(out, b...); out = append(out, 1); return out }`, 0},
{"nested scalar tail", `func f(a, b []int) []int { return append(append(append(make([]int, 0, len(a)+len(b)+1), a...), b...), 1) }`, 0},
{"nested initializer scalar tail", `func f(a, b []int) []int { out := append(append(make([]int, 0, len(a)+len(b)+1), a...), b...); out = append(out, 1); return out }`, 0},
{"indexed capacity length", `func f(a, b []int, groups [][]int, i int) []int { out := make([]int, 0, len(groups[i])); out = append(out, a...); out = append(out, b...); return out }`, 0},
{"sliced capacity length", `func f(a, b, sizes []int, n, d int) []int { out := make([]int, 0, len(sizes[:n/d])); out = append(out, a...); out = append(out, b...); return out }`, 0},
{"already concat", `import "slices"; func f(a, b []int) []int { return slices.Concat(a, b) }`, 0},
}
files := coptest.ProgramFiles{}
for i, tc := range tests {
name := fmt.Sprintf("case%d/sample.go", i)
files[name] = "package p\n" + tc.src
}
offenses := coptest.RunProgram(t, SlicesConcat, files)
got := make(map[string]int)
for _, o := range offenses {
assert.Equal(t, "Lint/SlicesConcat", o.CopName)
assert.Contains(t, o.Message, "slices.Concat")
name := filepath.ToSlash(o.Pos.Filename)
got[filepath.Base(filepath.Dir(name))+"/sample.go"]++
}
for i, tc := range tests {
name := fmt.Sprintf("case%d/sample.go", i)
assert.Equal(t, tc.want, got[name], tc.name)
}
}
func TestSlicesConcatScope(t *testing.T) {
t.Parallel()
const src = "package p\nfunc f(a, b []int) []int { return append(append([]int(nil), a...), b...) }"
files := coptest.ProgramFiles{
"pkg/config/v0/sample.go": src,
"pkg/config/v15/sample.go": src,
"pkg/config/latest/sample.go": src,
"generated/sample.go": "// Code generated by fixture; DO NOT EDIT.\n" + src,
}
offenses := coptest.RunProgram(t, SlicesConcat, files)
require.Len(t, offenses, 1)
assert.True(t, strings.HasSuffix(filepath.ToSlash(offenses[0].Pos.Filename), "/pkg/config/latest/sample.go"))
}
func TestSlicesConcatOldGoVersion(t *testing.T) {
t.Parallel()
assert.Empty(t, coptest.RunProgram(t, SlicesConcat, coptest.ProgramFiles{
"go.mod": "module example.test\n\ngo 1.21\n",
"sample.go": "package p\nfunc f(a, b []int) []int { return append(append([]int(nil), a...), b...) }",
}))
}
func TestSlicesConcatRunner(t *testing.T) {
dir := t.TempDir()
t.Chdir(dir)
const src = "package p\nfunc f(a, b []int) []int { return append(append([]int(nil), a...), b...) }\n"
for name, content := range map[string]string{
"go.mod": "module example.test\n\ngo 1.22\n",
"inline/sample.go": strings.ReplaceAll(src, " }\n", " } //rubocop:disable Lint/SlicesConcat\n"),
"suppressed/sample.go": "//rubocop:disable-file Lint/SlicesConcat\n" + src,
"reported/sample.go": src,
"tests/sample.go": "package p\n",
"tests/sample_test.go": src,
} {
require.NoError(t, os.MkdirAll(filepath.Dir(name), 0o700))
require.NoError(t, os.WriteFile(name, []byte(content), 0o600))
}
var output bytes.Buffer
r := runner.New(nil, config.DefaultConfig(), &output).WithProgramCops([]prog.Cop{SlicesConcat})
count, err := r.Run([]string{"."})
require.NoError(t, err)
assert.Equal(t, 1, count, output.String())
assert.Contains(t, output.String(), filepath.Join("reported", "sample.go"))
}
+1 -1
View File
@@ -50,7 +50,7 @@ func NewCallbackServer(ctx context.Context) (*CallbackServer, error) {
// Use port 0 to let the OS pick a random available port.
func NewCallbackServerOnPort(ctx context.Context, port int) (*CallbackServer, error) {
var lc net.ListenConfig
listener, err := lc.Listen(ctx, "tcp", fmt.Sprintf("127.0.0.1:%d", port))
listener, err := lc.Listen(ctx, "tcp", fmt.Sprintf("127.0.0.1:%d", port)) //rubocop:disable Lint/ConstructorNetworkIO // reserve the redirect port before starting OAuth
if err != nil {
return nil, fmt.Errorf("failed to find available port: %w", err)
}