diff --git a/docs/go-cli-guide.md b/docs/go-cli-guide.md index 959a425..4c27ce6 100644 --- a/docs/go-cli-guide.md +++ b/docs/go-cli-guide.md @@ -5,8 +5,8 @@ ### RULE go-cli/cobra-not-stdlib-flag (MUST) **Owner**: go-quality-assistant -**Applies when**: a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library). -**Enforcement**: judgment (ast-grep follow-up: `import "flag"` in any `main` package + `call_expression` matching `flag.Parse` / `flag.String`. Test files exempt.) +**Applies when**: any non-test Go file imports the stdlib `flag` package or calls `flag.*` functions (Parse / String / Bool / Int / Float64 / Duration + all *Var variants + Var / Func / NewFlagSet / CommandLine). Scope is intentionally broader than "CLI binaries only" because the Why this rule prevents is **transitive `flag.init()` pollution** — a library calling `flag.String()` in its `init()` adds flags to every binary that imports it, which is the actual failure mode this rule guards against. Test files are exempt. +**Enforcement**: `rules/go/cobra-not-stdlib-flag.yml` **Why**: Stdlib `flag` uses a process-global `flag.CommandLine` FlagSet. Any transitive dependency that calls `flag.String(...)` in its `init()` adds flags to this global set — and `github.com/golang/glog` is the most common offender, adding 8+ flags (`-alsologtostderr`, `-log_dir`, `-log_backtrace_at`, `-stderrthreshold`, `-v`, `-vmodule`, …) to every binary that transitively imports it. The result: `my-tool --help` displays a wall of irrelevant glog flags before your three actual flags, and the binary accepts those flags at runtime even though no one wanted them. Cobra uses `pflag` which is isolated from `flag.CommandLine` — the global pollution can't reach it, `--help` shows only your flags, and your flag namespace stays under your control. #### Bad diff --git a/docs/go-concurrency-patterns.md b/docs/go-concurrency-patterns.md index c46ff58..85490d8 100644 --- a/docs/go-concurrency-patterns.md +++ b/docs/go-concurrency-patterns.md @@ -7,8 +7,8 @@ ### RULE go-concurrency/no-raw-go-func (MUST) **Owner**: go-architecture-assistant -**Applies when**: a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside `main.go` / top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`). -**Enforcement**: judgment (ast-grep follow-up: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter) +**Applies when**: a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`). Exempt paths: `main.go` / `cmd/**` (binary entry points where goroutine spawn-and-cancel is the canonical pattern), `pkg/cli/**` (where the canonical `Execute()` signal-listener uses `go func() { <-sigCh; cancel() }()` per the `go-cli-guide.md` pattern), and `*_test.go` / `vendor/` / `mocks/`. +**Enforcement**: `rules/go/no-raw-go-func.yml` **Why**: Raw goroutines have three failure modes the `run` package solves: (1) they leak when the parent context is cancelled but the goroutine doesn't observe it; (2) they race when the parent function returns before the goroutine writes its result; (3) error propagation requires hand-rolled channels + `sync.WaitGroup` that drift toward subtle deadlocks. `run.CancelOnFirstErrorWait` wires context cancellation, error aggregation, and synchronization in one call — every consumer learns the same primitives, refactors stay safe, and goroutine lifetimes are explicit at the type signature. #### Bad diff --git a/rules/go/cobra-not-stdlib-flag.yml b/rules/go/cobra-not-stdlib-flag.yml new file mode 100644 index 0000000..6084fa5 --- /dev/null +++ b/rules/go/cobra-not-stdlib-flag.yml @@ -0,0 +1,49 @@ +id: go-cli/cobra-not-stdlib-flag +language: go +severity: error +message: | + stdlib 'flag' package must not be used for CLI parsing. + Transitive dependencies (most famously github.com/golang/glog) register + flags via init() and pollute --help output. Use github.com/spf13/cobra + with pflag instead. + See docs/go-cli-guide.md (RULE go-cli/cobra-not-stdlib-flag). +rule: + # ast-grep 0.43.0 shape: match `import_spec` nodes whose path text + # equals "flag" — Go's tree-sitter grammar represents each import line + # (in single or grouped form) as an import_spec node with a `path` + # field whose value is the quoted package path. + any: + - kind: import_spec + has: + field: path + regex: '^"flag"$' + # Also catch flag.* call sites — covers the full primitive family + # (String/Bool/Int/Float64/Duration/Int64/Uint/Uint64) + their *Var + # variants, plus NewFlagSet (known workaround for the global pollution + # this rule prevents) and CommandLine direct access. + - pattern: flag.Parse() + - pattern: 'flag.String($$$ARGS)' + - pattern: 'flag.Bool($$$ARGS)' + - pattern: 'flag.Int($$$ARGS)' + - pattern: 'flag.Int64($$$ARGS)' + - pattern: 'flag.Uint($$$ARGS)' + - pattern: 'flag.Uint64($$$ARGS)' + - pattern: 'flag.Float64($$$ARGS)' + - pattern: 'flag.Duration($$$ARGS)' + - pattern: 'flag.StringVar($$$ARGS)' + - pattern: 'flag.BoolVar($$$ARGS)' + - pattern: 'flag.IntVar($$$ARGS)' + - pattern: 'flag.Int64Var($$$ARGS)' + - pattern: 'flag.UintVar($$$ARGS)' + - pattern: 'flag.Uint64Var($$$ARGS)' + - pattern: 'flag.Float64Var($$$ARGS)' + - pattern: 'flag.DurationVar($$$ARGS)' + - pattern: 'flag.Var($$$ARGS)' + - pattern: 'flag.Func($$$ARGS)' + - pattern: 'flag.NewFlagSet($$$ARGS)' + - pattern: 'flag.CommandLine' +ignores: + - "**/*_test.go" + - "vendor/**" + - "**/vendor/**" + - "**/mocks/**" diff --git a/rules/go/no-raw-go-func.yml b/rules/go/no-raw-go-func.yml new file mode 100644 index 0000000..649af95 --- /dev/null +++ b/rules/go/no-raw-go-func.yml @@ -0,0 +1,33 @@ +id: go-concurrency/no-raw-go-func +language: go +severity: error +message: | + Raw 'go func()' / 'go expr()' is forbidden outside main entry points. + Use github.com/bborbe/run strategies (CancelOnFirstErrorWait, All, + Sequential) — raw goroutines leak, race, and require hand-rolled + sync.WaitGroup that drift toward deadlocks. + See docs/go-concurrency-patterns.md (RULE go-concurrency/no-raw-go-func). +rule: + # ast-grep 0.43.0 shape: Go's tree-sitter grammar uses `go_statement` + # for `go expr()` statements. Direct kind match — no metavariable + # constraints needed; the path-based `ignores` carry the + # main.go / cmd/** / _test.go exemptions. + kind: go_statement +ignores: + - "main.go" + - "**/main.go" + - "cmd/**" + - "**/cmd/**" + # pkg/cli/** exempted because the canonical Execute() shape in + # go-cli-guide.md uses 'go func() { <-sigCh; cancel() }()' for + # signal-listener wiring — that's the documented correct pattern + # for that location. Subcommand RunE goroutines that don't fit + # run.* primitives are rare enough that the false-negative cost + # is lower than the false-positive cost of catching every + # signal-listener bootstrap. + - "pkg/cli/**" + - "**/pkg/cli/**" + - "**/*_test.go" + - "vendor/**" + - "**/vendor/**" + - "**/mocks/**" diff --git a/rules/index.json b/rules/index.json index c3a35d3..2746d01 100644 --- a/rules/index.json +++ b/rules/index.json @@ -190,9 +190,9 @@ }, { "anchor": "go-cli/cobra-not-stdlib-flag", - "applies_when": "a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library).", + "applies_when": "any non-test Go file imports the stdlib `flag` package or calls `flag.*` functions (Parse / String / Bool / Int / Float64 / Duration + all *Var variants + Var / Func / NewFlagSet / CommandLine). Scope is intentionally broader than \"CLI binaries only\" because the Why this rule prevents is **transitive `flag.init()` pollution** — a library calling `flag.String()` in its `init()` adds flags to every binary that imports it, which is the actual failure mode this rule guards against. Test files are exempt.", "doc_path": "docs/go-cli-guide.md", - "enforcement": "judgment (ast-grep follow-up: `import \"flag\"` in any `main` package + `call_expression` matching `flag.Parse` / `flag.String`. Test files exempt.)", + "enforcement": "`rules/go/cobra-not-stdlib-flag.yml`", "id": "go-cli/cobra-not-stdlib-flag", "level": "MUST", "owner": "go-quality-assistant" @@ -235,9 +235,9 @@ }, { "anchor": "go-concurrency/no-raw-go-func", - "applies_when": "a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside `main.go` / top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`).", + "applies_when": "a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`). Exempt paths: `main.go` / `cmd/**` (binary entry points where goroutine spawn-and-cancel is the canonical pattern), `pkg/cli/**` (where the canonical `Execute()` signal-listener uses `go func() { <-sigCh; cancel() }()` per the `go-cli-guide.md` pattern), and `*_test.go` / `vendor/` / `mocks/`.", "doc_path": "docs/go-concurrency-patterns.md", - "enforcement": "judgment (ast-grep follow-up: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter)", + "enforcement": "`rules/go/no-raw-go-func.yml`", "id": "go-concurrency/no-raw-go-func", "level": "MUST", "owner": "go-architecture-assistant"