diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 862abce0109..25924f8f1c5 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -39,6 +39,7 @@ /wormchain/devnet/ @evan-gray /wormchain/devnet/txverifier @djb15 @johnsaigle @mdulin2 @pleasew8t /wormchain/ts-sdk/ @evan-gray @kev1n-peters @panoel +/linters/ @djb15 @johnsaigle @mdulin2 @pleasew8t @bemic # Protobuf for node diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ee4d306aa2b..b97d286a6e0 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -306,10 +306,8 @@ jobs: run: go install golang.org/x/tools/cmd/goimports@v0.8.0 - name: Formatting checks run: ./scripts/lint.sh -l -g format - - name: Install linters - run: curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.12.2 - name: Run linters - run: make generate && golangci-lint --version && ./scripts/lint.sh -g lint + run: make generate && ./scripts/lint.sh -g lint - name: Ensure generated proto matches run: | rm -rf node/pkg/proto @@ -377,6 +375,19 @@ jobs: - name: Check coverage against baseline run: ./coverage-check + # Run unit tests for the custom Go linters + linter-tests: + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 + with: + go-version: "1.25.10" + - name: Run custom linter tests + run: make -C linters test + # Run Rust lints and tests rust-lint-and-tests: runs-on: ubuntu-24.04 diff --git a/.gitignore b/.gitignore index e711c3f5226..36b73faba47 100644 --- a/.gitignore +++ b/.gitignore @@ -33,4 +33,4 @@ sui/examples/wrapped_coin /coverage-check # Only used for internal testing *.testnet.yaml -coverage.txt +coverage.txt \ No newline at end of file diff --git a/.golangci.yml b/.golangci.yml index ad67134f82d..c7613993f80 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,6 +7,8 @@ linters: - bidichk # Ensure that the body is closed on HTTP and websocket conns - bodyclose + # Blocking channel sends + # - channelcheck - contextcheck - depguard # Duplicate word usage, such as 'and and' in a comment. @@ -206,6 +208,18 @@ linters: # # Default: false check-exported: true + custom: + channelcheck: + type: "module" + description: Static analysis for go channel issues + settings: + CheckBlockingSends: true + CheckUnbufferedChannels: false + CheckEmptyDefault: true + # msgC sends are intentionally blocking because we don't want to drop observations + IgnoreChannelsByName: + - msgC + - msgChan exclusions: generated: lax presets: @@ -231,6 +245,7 @@ linters: path: node/hack/ # Ignore test files for these tools. - linters: + - channelcheck - contextcheck - dupWord - exhaustruct @@ -408,4 +423,4 @@ formatters: paths: - third_party$ - builtin$ - - examples$ + - examples$ \ No newline at end of file diff --git a/Makefile b/Makefile index bb5d031552d..b3c460fa356 100755 --- a/Makefile +++ b/Makefile @@ -53,9 +53,9 @@ $(BIN)/guardiand: dirs generate github.com/certusone/wormhole/node .PHONY: test-coverage -## Run tests with coverage for node and sdk (matches CI) +## Run tests with coverage for node, and sdk test-coverage: - @echo "Running tests with coverage for node and sdk..." + @echo "Running tests with coverage for node, and sdk..." @set -o pipefail && (cd node && go test -count=1 -v -timeout 5m -race -cover ./...) 2>&1 | tee coverage.txt @set -o pipefail && (cd sdk && go test -count=1 -v -timeout 5m -race -cover ./...) 2>&1 | tee -a coverage.txt diff --git a/cspell-custom-words.txt b/cspell-custom-words.txt index 7cb57281011..39a9dae69f0 100644 --- a/cspell-custom-words.txt +++ b/cspell-custom-words.txt @@ -38,6 +38,8 @@ certusone chainid ChainID Chainlink +channelcheck +channellint Coinspect collateralization colour @@ -71,6 +73,7 @@ Finalizer fogo Fogo fogoshim +footguns forgeable FQTs frontends @@ -142,8 +145,10 @@ monad Monad moonscan moretags +multichecker Neodyme nhooyr +nolint obsv Obsv OP_CHECKMULTISIG @@ -222,6 +227,7 @@ superminority tendermint Tendermint terrad +testdata tokenbridge tokenfactory traceback @@ -233,6 +239,8 @@ txverifier uatom uluna unbond +Unbuffered +unbuffered Uncompromised undelegated undercollateralization diff --git a/linters/.custom-gcl.yml b/linters/.custom-gcl.yml new file mode 100644 index 00000000000..d62aa6aa3c9 --- /dev/null +++ b/linters/.custom-gcl.yml @@ -0,0 +1,6 @@ +version: v2.12.2 +name: wormhole-golangci-lint +destination: ./bin +plugins: + - module: github.com/wormhole-foundation/wormhole/linters/rules/channelcheck + path: ./rules/channelcheck diff --git a/linters/.gitignore b/linters/.gitignore new file mode 100644 index 00000000000..359b8d230ff --- /dev/null +++ b/linters/.gitignore @@ -0,0 +1,7 @@ +# Built binaries. +/bin/ + +# Stray root-level builds, e.g. from a bare `go build ./...` (the Makefile +# targets put these under bin/). +/wormhole-lint +/wormhole-golangci-lint diff --git a/linters/Makefile b/linters/Makefile new file mode 100644 index 00000000000..e9244d899ba --- /dev/null +++ b/linters/Makefile @@ -0,0 +1,36 @@ +# Pin by commit hash rather than tag, so a re-pointed/malleable tag can't change +# what we build. Keep the trailing comment's tag in sync with the hash and with +# the `version:` field in .custom-gcl.yml. +GOLANGCI_LINT_HASH ?= c0d3ddc9cf3faa61a4e378e879ece580256d76e5 # v2.12.2 + +BIN_DIR := bin + +.PHONY: build build-golangci-lint install-golangci-lint test clean + +# Standalone multichecker over every rules/.Analyzer. +build: + go build -o $(BIN_DIR)/wormhole-lint ./cmd/wormhole-lint + +# Pinned upstream golangci-lint, dropped into $(BIN_DIR). Used as the driver +# for `golangci-lint custom`; keep its version aligned with the `version:` +# field in .custom-gcl.yml. +install-golangci-lint: + GOOS= GOARCH= GOBIN=$(abspath $(BIN_DIR)) go install \ + github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_HASH) + +# Custom golangci-lint with our module plugins baked in. Driven by +# .custom-gcl.yml in this directory (golangci-lint custom reads it from the +# cwd); output goes to ./bin per `destination:`. +build-golangci-lint: install-golangci-lint + $(BIN_DIR)/golangci-lint custom + +test: + go test -v ./... + @for mod in $$(find rules -name go.mod); do \ + dir=$$(dirname $$mod); \ + echo "==> testing $$dir"; \ + (cd $$dir && go test -v ./...) || exit $$?; \ + done + +clean: + rm -rf $(BIN_DIR) diff --git a/linters/README.md b/linters/README.md new file mode 100644 index 00000000000..2484cd8eb48 --- /dev/null +++ b/linters/README.md @@ -0,0 +1,125 @@ +# Golang CI Lints + +Custom Go linters used on Wormhole CI. Each linter is a [golangci-lint +module plugin](https://golangci-lint.run/plugins/module-plugins/) and lives +as its own Go module under `rules//`. + +Prefer to use the `release` builds. + +Currently supported linters: + +| Name | Purpose | Features | +| -------------- | -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | +| `channelcheck` | Flag channel usage patterns that can deadlock or block. | • Blocking channel sends outside a `select`
• Unbuffered channel creation (`make(chan T)`)
• Empty `default:` that silently drops a send (advisory, off by default)
• Ignore specific channels | + + +## Build & test +``` +make build # bin/wormhole-lint +make build-golangci-lint # bin/wormhole-golangci-lint +make test # root + each rules/ module +``` + +`make build-golangci-lint` first installs the pinned upstream +`golangci-lint` into `bin/` (per `GOLANGCI_LINT_HASH`), then runs +`golangci-lint custom`. + +## Use + +Standalone: + +``` +bin/wormhole-lint ./... +``` + +Via the custom golangci-lint: + +``` +bin/wormhole-golangci-lint run --timeout=10m ./... +``` + +## Enable a plugin in `.golangci.yml` + +Module plugins are addressed under `linters.settings.custom.` and +enabled in `linters.enable` by the plugin's registered name. Example for +`channelcheck`: + +```yaml +version: "2" +linters: + enable: + - channelcheck + settings: + custom: + channelcheck: + type: module + description: reports channel blocking issues + settings: + blocking: true + unbuffered: false + bufferMax: 0 + emptyDefault: false + ignoreChannelsByName: [errC] +``` + +`channelcheck` settings: + +| Setting | Type | Default | Description | +| ---------------------- | ---------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------- | +| `blocking` | `bool` | `true` | Flag blocking sends that have no escape (timer, `default:`, or `ctx.Done()`), including sends outside any `select`. | +| `unbuffered` | `bool` | `false` | Flag unbuffered channel creation (`make(chan T)` / `make(chan T, 0)`). | +| `bufferMax` | `uint` | `0` | Flag channel buffers larger than this. `0` disables the check. | +| `emptyDefault` | `bool` | `false` | Advisory: flag an empty `default:` in a `select` with a send, since it silently drops the send. Off by default — it fires on the idiomatic non-blocking/coalescing-send pattern, so enable it only where dropping should always be logged or documented. | +| `ignoreChannelsByName` | `[]string` | `[]` | Channel/field names whose direct sends are exempt from the blocking-send, empty-default, and `ctx.Done()` checks. | + +## Development + +### Adding a new linter + +1. Scaffold the module: + ``` + mkdir -p rules/ && cd rules/ + go mod init github.com/wormhole-foundation/wormhole/linters/rules/ + ``` +2. Implement `.go` following the channelcheck reference + (`rules/channelcheck/channelcheck.go`): + - Export an `Analyzer` of type `*analysis.Analyzer`. + - Define a `Settings` struct and a `New(any) (register.LinterPlugin, error)` + constructor that decodes settings via + `register.DecodeSettings[Settings]`. + - Implement `BuildAnalyzers()` and `GetLoadMode()` on your plugin type. + - In `init()`, call `register.Plugin("", New)` so + `golangci-lint custom` picks it up. +3. Add tests + fixtures under `rules//testdata/` following + `rules/channelcheck/channelcheck_test.go`. +4. Wire it into the root module so the aggregator can import it: + - In root `go.mod`, add + `require github.com/wormhole-foundation/wormhole/linters/rules/ v0.0.0` and + `replace github.com/wormhole-foundation/wormhole/linters/rules/ => ./rules/`. + - In `cmd/wormhole-lint/main.go`, add the import and append + `.Analyzer` to the `multichecker.Main` call. +5. Wire it into `.custom-gcl.yml` so the custom golangci-lint picks it up: + ```yaml + plugins: + - module: github.com/wormhole-foundation/wormhole/linters/rules/ + path: ./rules/ + ``` +6. `make test && make build && make build-golangci-lint` to verify. +7. Add linter to `.golangci.yml`. +8. Fix linter errors in the monorepo with legitimate changes or a `nolint` comment. + +### Layout + +``` +.custom-gcl.yml # plugin manifest for `golangci-lint custom` +Makefile +go.mod # root module: cmd/* + replaces for each rules/ +cmd/ + wormhole-lint/ # multichecker aggregator binary +rules/ + channelcheck/ # standalone Go module per linter + channelcheck.go + channelcheck_test.go + go.mod + testdata/ +``` diff --git a/linters/cmd/wormhole-lint/main.go b/linters/cmd/wormhole-lint/main.go new file mode 100644 index 00000000000..81dfeba8615 --- /dev/null +++ b/linters/cmd/wormhole-lint/main.go @@ -0,0 +1,16 @@ +// Command wormhole-lint runs every custom linter under rules/ as a single +// standalone binary. Each rules// exports an Analyzer; add new +// linters by appending one import + one Analyzer below. +package main + +import ( + "golang.org/x/tools/go/analysis/multichecker" + + "github.com/wormhole-foundation/wormhole/linters/rules/channelcheck" +) + +func main() { + multichecker.Main( + channelcheck.Analyzer, + ) +} diff --git a/linters/go.mod b/linters/go.mod new file mode 100644 index 00000000000..bdff541867b --- /dev/null +++ b/linters/go.mod @@ -0,0 +1,16 @@ +module github.com/wormhole-foundation/wormhole/linters + +go 1.25.10 + +require ( + github.com/wormhole-foundation/wormhole/linters/rules/channelcheck v0.0.0 + golang.org/x/tools v0.30.0 +) + +require ( + github.com/golangci/plugin-module-register v0.1.1 // indirect + golang.org/x/mod v0.23.0 // indirect + golang.org/x/sync v0.11.0 // indirect +) + +replace github.com/wormhole-foundation/wormhole/linters/rules/channelcheck => ./rules/channelcheck diff --git a/linters/go.sum b/linters/go.sum new file mode 100644 index 00000000000..5ac45bec971 --- /dev/null +++ b/linters/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= +github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM= +golang.org/x/mod v0.23.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.30.0 h1:BgcpHewrV5AUp2G9MebG4XPFI1E2W41zU1SaqVA9vJY= +golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= diff --git a/linters/rules/channelcheck/README.md b/linters/rules/channelcheck/README.md new file mode 100644 index 00000000000..f481f692eb2 --- /dev/null +++ b/linters/rules/channelcheck/README.md @@ -0,0 +1,24 @@ +## Channel Check + +Channels are a great feature of Golang but have several footguns that can lead to deadlocks. In particular, if the receiving channel stops processing the messages, a *non-blocking* channel send would fail to continue. In certain mission-critical sections of code, this could lead to a complete deadlock. + +This linter currently has three features: +- Identify blocking sends +- Identify non-buffered channel creation +- Identify buffered channel size exceeds maximum size checks + +Many of these will lead to false positives or situations where we *want* a blocking channel send. In these cases, `nolint:channelcheck` is easy to add (assuming this is integrated directly with golangci-lint). Regardless, having this issue pointed out automatically is a good way to fix bugs. + +## Configuration + +Each option can be set two ways: as a golangci-lint module setting (under +`settings.custom.channelcheck.settings:`, using the **Setting** name) or as a +standalone analyzer flag (when running the `wormhole-lint` binary, using the +**Flag** name). + +| Setting | Flag | Type | Default | Description | +| ------------------------- | ------------ | ---------- | ------- | ----------------------------------------------------------------------------------------------- | +| `CheckBlockingSends` | `blocking` | bool | `true` | Flag blocking sends that lack a `default`/timeout/ticker escape in their enclosing `select`. | +| `CheckUnbufferedChannels` | `unbuffered` | bool | `false` | Flag creation of unbuffered channels (`make(chan T)`). | +| `CheckBufferAmount` | `bufferMax` | uint64 | `0` | Flag buffered channels whose size exceeds this max. `0` disables the check. | +| `IgnoreChannelsByName` | *(none)* | []string | `[]` | Channel/field names whose direct sends are exempt from the blocking-send check (e.g. `errC`). Settings-only; no standalone flag. | \ No newline at end of file diff --git a/linters/rules/channelcheck/channelcheck.go b/linters/rules/channelcheck/channelcheck.go new file mode 100644 index 00000000000..b10209ff38d --- /dev/null +++ b/linters/rules/channelcheck/channelcheck.go @@ -0,0 +1,488 @@ +package channelcheck + +/* + Strategy: Identify proper SendStmt's early. If we don't see it within a Select, then it's not being used correctly. + Or, we simply MISS it, making it a false positive which is fine. We'd rather fail open and get the user to + use nolints than miss a potential bug altogether. +*/ +import ( + "bytes" + "flag" + "fmt" + "go/ast" + "go/constant" + "go/printer" + "go/token" + "go/types" + + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +// ChannelCheckPlugin is the golangci-lint module-plugin entry point. Its +// configuration lives in the package-level settings var (populated by New), +// which is the single source of truth — shared with the standalone +// cmd/wormhole-lint multichecker, which populates the same var via flags. +type ChannelCheckPlugin struct{} + +// Settings holds the configuration for the channelcheck linter. +type Settings struct { + CheckUnbufferedChannels bool // Enable/disable checking for unbuffered channel creation. + CheckBufferAmount uint64 // The amount that can be in a buffer. 0 means don't do this check. + CheckBlockingSends bool // Enable/disable checking for blocking sends without default/timeout. + CheckEmptyDefault bool // Enable/disable the (advisory) empty-default-drops-the-send check. + IgnoreChannelsByName []string // Channel/field names whose direct sends are exempt from the blocking-send check. + + // ignoreChannelNames is the lookup form of IgnoreChannelsByName, built + // from the slice during configuration. + ignoreChannelNames map[string]bool +} + +// EscapeKind classifies a non-send CommClause in a select, describing what +// kind of escape valve (if any) it provides for a sibling send. +type EscapeKind int + +const ( + EscapeNone EscapeKind = iota + EscapeTimer // receive on <-chan time.Time + EscapeDefaultWithCode // default: with non-empty body + EscapeEmptyDefault // default: with empty body + EscapeContextDone // receive on a call to (context.Context).Done() + EscapeOther // any other clause in the statement +) + +// SelectAnalysis is the result of inspecting a select statement. +type SelectAnalysis struct { + Sends []*ast.SendStmt // SendStmts that are direct CommClause heads + Escapes []EscapeKind // one entry per non-send clause (excludes EscapeNone) + EmptyDefaultPos token.Pos // position of an empty default clause, or NoPos if none + ContextDonePos token.Pos // position of a <-ctx.Done() clause, or NoPos if none +} + +var Analyzer = &analysis.Analyzer{ + Name: "channelcheck", + Doc: "reports channel blocking issues", + Run: run, + Flags: flagSet, +} + +// Flags for the analyzer +var flagSet flag.FlagSet + +// Global structure to store the variables in +var settings Settings + +func New(settingsNew any) (register.LinterPlugin, error) { + s, err := register.DecodeSettings[Settings](settingsNew) + if err != nil { + return nil, err + } + // Assign the whole struct so a future Settings field can't be silently + // dropped, then derive the lookup map from the decoded slice. + settings = s + settings.ignoreChannelNames = buildIgnoreSet(s.IgnoreChannelsByName) + + return &ChannelCheckPlugin{}, nil +} + +func buildIgnoreSet(names []string) map[string]bool { + if len(names) == 0 { + return nil + } + out := make(map[string]bool, len(names)) + for _, n := range names { + out[n] = true + } + return out +} + +func (f *ChannelCheckPlugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + // Reuse the package-level Analyzer (the same one the standalone + // cmd/wormhole-lint multichecker runs) instead of defining a second one. + return []*analysis.Analyzer{Analyzer}, nil +} + +// Initialize the flags from the golangci-lint +func init() { + + flagSet.BoolVar(&settings.CheckUnbufferedChannels, "unbuffered", false, "Check for unbuffered channel creation") + flagSet.BoolVar(&settings.CheckBlockingSends, "blocking", true, "Check for blocking sends without default/timeout") + flagSet.BoolVar(&settings.CheckEmptyDefault, "emptyDefault", false, "Advise when an empty default case silently drops a send") + flagSet.Uint64Var(&settings.CheckBufferAmount, "bufferMax", 0, "Check for maximum length of channel buffer being exceeded") + Analyzer.Flags = flagSet + register.Plugin("channelcheck", New) +} + +func (f *ChannelCheckPlugin) GetLoadMode() string { + return register.LoadModeTypesInfo +} + +func run(pass *analysis.Pass) (interface{}, error) { + + for _, file := range pass.Files { + var seenPositions = make(map[token.Pos]bool) + + ast.Inspect(file, func(node ast.Node) bool { + switch n := node.(type) { + // Fails open by design. Will + case *ast.SelectStmt: // Select statement for channel matching + + if !settings.CheckBlockingSends { + break + } + selectAnalysis := processSelect(pass, n) + + /* + A send is considered safe (i.e., the SendStmt-level diagnostic is + suppressed) if its enclosing select has at least one of: + - Timer, DefaultWithCode, or EmptyDefault — real backpressure relief + - ctx.Done() — shutdown-safe; we suppress the bare blocking-send + diagnostic and emit a more specific ctx.Done() finding instead + (only when ctx.Done() is the lone escape — if a Timer/Default is + also present, that's already doing the real work). + + EscapeOther alone does NOT make the send safe. + */ + sendsAreSafe := false + foundContext := false + for _, escapeKind := range selectAnalysis.Escapes { + switch escapeKind { + case EscapeTimer, EscapeDefaultWithCode, EscapeEmptyDefault: + sendsAreSafe = true + case EscapeContextDone: + foundContext = true + case EscapeNone, EscapeOther: + // Neither marks the send safe: EscapeNone shouldn't appear + // here (Escapes excludes it) and EscapeOther alone provides + // no backpressure relief. + } + } + + if sendsAreSafe || foundContext { + for _, send := range selectAnalysis.Sends { + seenPositions[send.Pos()] = true + } + } + + // Collect the sends whose channels are NOT in the user's ignore list. + // These are the anchor points for the empty-default and ctx.Done() + // diagnostics so that a //nolint:channelcheck on the consciously- + // blocking send line suppresses them. + var trackedSends []*ast.SendStmt + for _, send := range selectAnalysis.Sends { + if name, named := sendChanName(send); named && settings.ignoreChannelNames[name] { + continue + } + trackedSends = append(trackedSends, send) + } + + // Empty default case. Only meaningful when there's a tracked send + // in the select (a receive-only select with an empty default has no + // backpressure concern). Anchor at each tracked send so users can + // suppress with a //nolint next to the blocking send. + if settings.CheckEmptyDefault && selectAnalysis.EmptyDefaultPos != token.NoPos { + for _, send := range trackedSends { + pass.Reportf(send.Pos(), + "empty default in channel select silently drops the send; log it or document why dropping is intended") + } + } + + // ctx.Done() was the only thing found alongside the send. Flag it. + // Only fire when there's a send in the select — a receive-only select + // with ctx.Done() is the canonical "wait or shutdown" idiom and has + // no backpressure concern. Anchor the diagnostic at each tracked send so that + // a //nolint:channelcheck next to the consciously-blocking send works. + if !sendsAreSafe && foundContext && selectAnalysis.ContextDonePos != token.NoPos { + for _, send := range trackedSends { + pass.Reportf(send.Pos(), + "ctx.Done() in channel select not backfill safe. Consider adding a timer, or default statement.") + } + } + + // Most of the work is done in the previous case statement. + case *ast.SendStmt: + + if !settings.CheckBlockingSends { + break + } + + // If the SendStmt was NOT found within a Select clause, then add a linter error. + tokenID := n.Pos() + if _, ok := seenPositions[tokenID]; !ok { + if name, named := sendChanName(n); named && settings.ignoreChannelNames[name] { + return true + } + pass.Reportf(tokenID, "Blocking send. Add timer, ticker, or default case: %q", render(pass.Fset, n)) + } + return true + case *ast.CallExpr: + // Channel creation that's unbuffered + didCreateChannelWithoutBuffering, bufferAmount := checkChannelCreation(pass, n) + if didCreateChannelWithoutBuffering && settings.CheckUnbufferedChannels { + pass.Reportf(n.Pos(), "unbuffered channel creation detected - consider specifying buffer size %q", render(pass.Fset, n)) + } + + if settings.CheckBufferAmount > 0 && bufferAmount > 0 && bufferAmount > settings.CheckBufferAmount { + pass.Reportf(n.Pos(), "channel buffer size exceeds the specified limit %q", render(pass.Fset, n)) + } + return true + + default: + return true // Continue traversing for other node types + } + + return true + }) + } + + return nil, nil +} + +/* +Walks a select statement and classifies each CommClause. + +Sends that are the direct Comm of a CommClause are recorded in SendPositions. +Every other clause is classified via classifyClause and appended to Escapes. +An empty default body is additionally recorded by position so the caller can +emit a separate finding for it. + +NOTE: A send is always a candidate for a finding unless its enclosing select has a +recognized escape (Timer, DefaultWithCode, or EmptyDefault). Fails open by design. +*/ +func processSelect(pass *analysis.Pass, selectStmt *ast.SelectStmt) SelectAnalysis { + var analysis SelectAnalysis + for _, clause := range selectStmt.Body.List { + commClause, ok := clause.(*ast.CommClause) + if !ok { + continue // Skip if not a CommClause (e.g., a declaration inside the select) + } + if sendNode, isSend := commClause.Comm.(*ast.SendStmt); isSend { + analysis.Sends = append(analysis.Sends, sendNode) + continue + } + clauseKind := classifyClause(pass, commClause) + if clauseKind == EscapeNone { + continue + } + analysis.Escapes = append(analysis.Escapes, clauseKind) + if clauseKind == EscapeEmptyDefault { + analysis.EmptyDefaultPos = commClause.Pos() + } + if clauseKind == EscapeContextDone { + analysis.ContextDonePos = commClause.Pos() + } + } + return analysis +} + +// classifyClause returns the EscapeKind for a non-send CommClause. +// Returns EscapeNone for SendStmt clauses (they aren't escapes themselves). +func classifyClause(pass *analysis.Pass, commClause *ast.CommClause) EscapeKind { + if commClause.Comm == nil { + if len(commClause.Body) == 0 { + return EscapeEmptyDefault + } + return EscapeDefaultWithCode + } + if _, isSend := commClause.Comm.(*ast.SendStmt); isSend { + return EscapeNone + } + channelExpr := extractRecvChannel(commClause.Comm) + if channelExpr == nil { + return EscapeOther + } + if isContextDoneCall(pass, channelExpr) { + return EscapeContextDone + } + elementType := recvElementType(pass, channelExpr) + if elementType != nil && isNamedType(elementType, "time", "Time") { + return EscapeTimer + } + return EscapeOther +} + +// isContextDoneCall reports whether expr is a call to (context.Context).Done(). +func isContextDoneCall(pass *analysis.Pass, expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + if selector.Sel.Name != "Done" { + return false + } + if pass.TypesInfo == nil { + return false + } + methodObj, ok := pass.TypesInfo.Uses[selector.Sel].(*types.Func) + if !ok { + return false + } + methodPkg := methodObj.Pkg() + return methodPkg != nil && methodPkg.Path() == "context" +} + +// extractRecvChannel returns the channel expression of a receive embedded in +// any of the three CommClause shapes: +// +// case <-ch: (*ast.ExprStmt wrapping *ast.UnaryExpr) +// case v := <-ch: (*ast.AssignStmt, one RHS) +// case v, ok := <-ch: (*ast.AssignStmt, one RHS, two LHS) +// +// Returns nil if the statement is not a receive. +func extractRecvChannel(commStmt ast.Stmt) ast.Expr { + var receiveExpr ast.Expr + switch typedStmt := commStmt.(type) { + case *ast.ExprStmt: + receiveExpr = typedStmt.X + case *ast.AssignStmt: + if len(typedStmt.Rhs) != 1 { + return nil + } + receiveExpr = typedStmt.Rhs[0] + default: + return nil + } + unaryExpr, ok := receiveExpr.(*ast.UnaryExpr) + if !ok || unaryExpr.Op != token.ARROW { + return nil + } + return unaryExpr.X +} + +// recvElementType returns the element type of the channel being received from, +// or nil if type info is unavailable or the expression is not a channel. +func recvElementType(pass *analysis.Pass, channelExpr ast.Expr) types.Type { + if pass.TypesInfo == nil { + return nil + } + channelExprType := pass.TypesInfo.TypeOf(channelExpr) + if channelExprType == nil { + return nil + } + channelType, ok := channelExprType.Underlying().(*types.Chan) + if !ok { + return nil + } + return channelType.Elem() +} + +// isNamedType reports whether t is the named type pkgPath.name (e.g. "time", "Time"). +func isNamedType(candidateType types.Type, pkgPath, typeName string) bool { + namedType, ok := candidateType.(*types.Named) + if !ok { + return false + } + typeObj := namedType.Obj() + if typeObj == nil || typeObj.Pkg() == nil { + return false + } + return typeObj.Pkg().Path() == pkgPath && typeObj.Name() == typeName +} + +// makeChanBufferedArgs is the argument count of a buffered channel make: +// make(chan T, size) — the channel type plus the buffer-size expression. +const makeChanBufferedArgs = 2 + +func checkChannelCreation(pass *analysis.Pass, node *ast.CallExpr) (bool, uint64) { + fun, ok := node.Fun.(*ast.Ident) + if !ok || fun == nil || fun.Name != "make" { + return false, 0 + } + + if len(node.Args) > 0 { + if _, ok := node.Args[0].(*ast.ChanType); ok { // It's a channel + if len(node.Args) == 1 { + return true, 0 // Unbuffered channel + } + + if len(node.Args) == makeChanBufferedArgs { + // Evaluate the buffer size expression + bufferSize, err := evalBufferSize(pass, node.Args[1]) + if err != nil { + // Has a buffer arg but the size is not statically determinable + // (e.g. a runtime variable). Don't flag it as unbuffered and + // don't check against the max — the size is just unknown. + return false, 0 + } + + // make(chan T, 0) is semantically identical to make(chan T) — + // both produce an unbuffered channel with synchronous rendezvous. + if bufferSize == 0 { + return true, 0 + } + return false, bufferSize + } + } + } + + return false, 0 +} + +/* +Evaluates the buffer size expression in a make(chan T, N) call. Returns the +constant value of N if it is statically determinable — which covers integer +literals (`make(chan T, 100)`), named constants (`const N = 100; make(chan T, N)`), +and constant arithmetic (`make(chan T, 2*N+1)`). + +Returns an error for runtime values (`var n = 100; make(chan T, n)`) which +cannot be evaluated at lint time. +*/ +func evalBufferSize(pass *analysis.Pass, expr ast.Expr) (uint64, error) { + if pass.TypesInfo == nil { + return 0, fmt.Errorf("type info unavailable") + } + typeAndValue, ok := pass.TypesInfo.Types[expr] + if !ok || typeAndValue.Value == nil { + return 0, fmt.Errorf("buffer size is not a constant expression") + } + intValue := constant.ToInt(typeAndValue.Value) + if intValue.Kind() != constant.Int { + return 0, fmt.Errorf("buffer size is not an integer: %v", typeAndValue.Value.Kind()) + } + bufferSize, exact := constant.Uint64Val(intValue) + if !exact { + return 0, fmt.Errorf("buffer size is too large or negative") + } + return bufferSize, nil +} + +// render returns the pretty-print of the given node +func render(fset *token.FileSet, x interface{}) string { + var buf bytes.Buffer + if err := printer.Fprint(&buf, fset, x); err != nil { + panic(err) + } + return buf.String() +} + +// sendChanName returns the receiving channel's variable name for a send +// statement (the channel on the left of `<-`). The second return value is +// false when the Chan expression has no meaningful single name — e.g. +// `channels[0] <- x`, `chFunc() <- x`, or a type assertion `iface.(chan T) <- x`. +// For those, callers should fall back to render(fset, sendStmt.Chan). +// +// Supported shapes: +// +// ch <- x → "ch" (*ast.Ident) +// s.eventCh <- x → "eventCh" (*ast.SelectorExpr; returns the field name) +// (ch) <- x → "ch" (*ast.ParenExpr; unwrapped recursively) +func sendChanName(sendStmt *ast.SendStmt) (string, bool) { + chanExpr := sendStmt.Chan + for { + switch typedExpr := chanExpr.(type) { + case *ast.Ident: + return typedExpr.Name, true + case *ast.SelectorExpr: + return typedExpr.Sel.Name, true + case *ast.ParenExpr: + chanExpr = typedExpr.X + continue + default: + return "", false + } + } +} diff --git a/linters/rules/channelcheck/channelcheck_test.go b/linters/rules/channelcheck/channelcheck_test.go new file mode 100644 index 00000000000..50c57a4ee6b --- /dev/null +++ b/linters/rules/channelcheck/channelcheck_test.go @@ -0,0 +1,137 @@ +package channelcheck + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +// runFixture runs the Analyzer against testdata/src/ under a transient +// settings override. It restores the previous settings on return so tests do +// not interfere with each other. +func runFixture(t *testing.T, pkg string, override Settings) { + t.Helper() + + saved := settings + defer func() { settings = saved }() + + // Default matches the flag defaults (blocking on, everything else off). + settings = Settings{CheckBlockingSends: true} + + settings.CheckUnbufferedChannels = override.CheckUnbufferedChannels + settings.CheckBlockingSends = override.CheckBlockingSends + settings.CheckEmptyDefault = override.CheckEmptyDefault + settings.CheckBufferAmount = override.CheckBufferAmount + settings.IgnoreChannelsByName = override.IgnoreChannelsByName + settings.ignoreChannelNames = buildIgnoreSet(override.IgnoreChannelsByName) + + analysistest.Run(t, analysistest.TestData(), Analyzer, pkg) +} + +func defaultSettings() Settings { + return Settings{CheckBlockingSends: true} +} + +func TestBlockingSend(t *testing.T) { + runFixture(t, "blocking_send", defaultSettings()) +} + +func TestEmptyDefault(t *testing.T) { + runFixture(t, "empty_default", Settings{CheckBlockingSends: true, CheckEmptyDefault: true}) +} + +func TestCtxDoneOnly(t *testing.T) { + runFixture(t, "ctx_done_only", defaultSettings()) +} + +func TestTimerSafe(t *testing.T) { + runFixture(t, "timer_safe", defaultSettings()) +} + +func TestDefaultSafe(t *testing.T) { + runFixture(t, "default_safe", defaultSettings()) +} + +func TestUnbufferedChannel(t *testing.T) { + runFixture(t, "unbuffered_chan", Settings{ + CheckUnbufferedChannels: true, + CheckBlockingSends: false, + }) +} + +func TestBufferTooLarge(t *testing.T) { + runFixture(t, "buffer_too_large", Settings{ + CheckBlockingSends: false, + CheckBufferAmount: 10, + }) +} + +func TestBlockingDisabled(t *testing.T) { + runFixture(t, "blocking_disabled", Settings{CheckBlockingSends: false}) +} + +func TestBlockingDisabledSuppressesEmptyDefault(t *testing.T) { + runFixture(t, "blocking_disabled_empty_default", Settings{CheckBlockingSends: false}) +} + +func TestBlockingDisabledSuppressesCtxDone(t *testing.T) { + runFixture(t, "blocking_disabled_ctx_done", Settings{CheckBlockingSends: false}) +} + +func TestUnbufferedDisabledByDefault(t *testing.T) { + // Default config has CheckUnbufferedChannels=false. unbuffered_chan.go's + // want marker only fires when the check is on; with the check off we need + // a fixture without a // want marker. + runFixture(t, "unbuffered_disabled", defaultSettings()) +} + +func TestEscapeOtherAlone(t *testing.T) { + runFixture(t, "escape_other_alone", defaultSettings()) +} + +func TestDerivedCtxDone(t *testing.T) { + runFixture(t, "derived_ctx_done", defaultSettings()) +} + +func TestFakeDoneCounter(t *testing.T) { + runFixture(t, "fake_done_counter", defaultSettings()) +} + +func TestSendInCaseBody(t *testing.T) { + runFixture(t, "send_in_case_body", defaultSettings()) +} + +func TestIgnoreChannelsByName(t *testing.T) { + runFixture(t, "ignore_by_name", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestIgnoreSuppressesEmptyDefault(t *testing.T) { + runFixture(t, "ignore_empty_default", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestIgnoreSuppressesCtxDone(t *testing.T) { + runFixture(t, "ignore_ctx_done", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestIgnorePartialStillFires(t *testing.T) { + runFixture(t, "ignore_partial", Settings{ + CheckBlockingSends: true, + IgnoreChannelsByName: []string{"ignoreMe"}, + }) +} + +func TestBufferMaxDisabledByDefault(t *testing.T) { + // Default has CheckBufferAmount=0 which disables the check. The + // buffer_too_large fixture's // want marker requires the check on, so a + // no-want fixture is used here. + runFixture(t, "buffer_disabled", Settings{CheckBlockingSends: false}) +} diff --git a/linters/rules/channelcheck/go.mod b/linters/rules/channelcheck/go.mod new file mode 100644 index 00000000000..4e5308adb7c --- /dev/null +++ b/linters/rules/channelcheck/go.mod @@ -0,0 +1,11 @@ +module github.com/wormhole-foundation/wormhole/linters/rules/channelcheck + +go 1.25.10 + +require golang.org/x/tools v0.30.0 + +require ( + github.com/golangci/plugin-module-register v0.1.1 // indirect + golang.org/x/mod v0.23.0 // indirect + golang.org/x/sync v0.11.0 // indirect +) diff --git a/linters/rules/channelcheck/go.sum b/linters/rules/channelcheck/go.sum new file mode 100644 index 00000000000..5ac45bec971 --- /dev/null +++ b/linters/rules/channelcheck/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= +github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM= +golang.org/x/mod v0.23.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.30.0 h1:BgcpHewrV5AUp2G9MebG4XPFI1E2W41zU1SaqVA9vJY= +golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= diff --git a/linters/rules/channelcheck/testdata/src/blocking_disabled/blocking_disabled.go b/linters/rules/channelcheck/testdata/src/blocking_disabled/blocking_disabled.go new file mode 100644 index 00000000000..8cd4b8c890b --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_disabled/blocking_disabled.go @@ -0,0 +1,6 @@ +package fixture + +func blockingSend() { + c := make(chan int, 1) + c <- 1 +} diff --git a/linters/rules/channelcheck/testdata/src/blocking_disabled_ctx_done/blocking_disabled_ctx_done.go b/linters/rules/channelcheck/testdata/src/blocking_disabled_ctx_done/blocking_disabled_ctx_done.go new file mode 100644 index 00000000000..c470e24a128 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_disabled_ctx_done/blocking_disabled_ctx_done.go @@ -0,0 +1,11 @@ +package fixture + +import "context" + +func ctxDoneOnly(ctx context.Context) { + c := make(chan int, 1) + select { + case c <- 1: + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/blocking_disabled_empty_default/blocking_disabled_empty_default.go b/linters/rules/channelcheck/testdata/src/blocking_disabled_empty_default/blocking_disabled_empty_default.go new file mode 100644 index 00000000000..24ee0a91fa1 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_disabled_empty_default/blocking_disabled_empty_default.go @@ -0,0 +1,9 @@ +package fixture + +func emptyDefault() { + c := make(chan int, 1) + select { + case c <- 1: + default: + } +} diff --git a/linters/rules/channelcheck/testdata/src/blocking_send/blocking_send.go b/linters/rules/channelcheck/testdata/src/blocking_send/blocking_send.go new file mode 100644 index 00000000000..c2299b749dd --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/blocking_send/blocking_send.go @@ -0,0 +1,7 @@ +package fixture + +// Single match +func blockingSend() { + c := make(chan int, 1) + c <- 1 // want `Blocking send` +} diff --git a/linters/rules/channelcheck/testdata/src/buffer_disabled/buffer_disabled.go b/linters/rules/channelcheck/testdata/src/buffer_disabled/buffer_disabled.go new file mode 100644 index 00000000000..4be2f5bb2ee --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/buffer_disabled/buffer_disabled.go @@ -0,0 +1,5 @@ +package fixture + +func tooLarge() { + _ = make(chan int, 100) +} diff --git a/linters/rules/channelcheck/testdata/src/buffer_too_large/buffer_too_large.go b/linters/rules/channelcheck/testdata/src/buffer_too_large/buffer_too_large.go new file mode 100644 index 00000000000..52393e7c421 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/buffer_too_large/buffer_too_large.go @@ -0,0 +1,6 @@ +package fixture + +// Single match for buffer too large +func tooLarge() { + _ = make(chan int, 100) // want `buffer size exceeds` +} diff --git a/linters/rules/channelcheck/testdata/src/ctx_done_only/ctx_done_only.go b/linters/rules/channelcheck/testdata/src/ctx_done_only/ctx_done_only.go new file mode 100644 index 00000000000..3df21df308f --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ctx_done_only/ctx_done_only.go @@ -0,0 +1,12 @@ +package fixture + +import "context" + +// Single match for having a 'Done()' without anything else that was useful. +func ctxDoneOnly(ctx context.Context) { + c := make(chan int, 1) + select { + case c <- 1: // want `ctx\.Done\(\)` + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/default_safe/default_safe.go b/linters/rules/channelcheck/testdata/src/default_safe/default_safe.go new file mode 100644 index 00000000000..fbc70229e03 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/default_safe/default_safe.go @@ -0,0 +1,14 @@ +// OK +package fixture + +import "fmt" + +// Channel send with default and a println. This is "safe" according to the code. +func defaultSafe() { + c := make(chan int, 1) + select { + case c <- 1: + default: + fmt.Println("dropped") + } +} diff --git a/linters/rules/channelcheck/testdata/src/derived_ctx_done/derived_ctx_done.go b/linters/rules/channelcheck/testdata/src/derived_ctx_done/derived_ctx_done.go new file mode 100644 index 00000000000..d8e40340f68 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/derived_ctx_done/derived_ctx_done.go @@ -0,0 +1,20 @@ +package fixture + +import ( + "context" + "time" +) + +// A derived context (timeoutCtx) — its Done() method still resolves to the +// "context" package, so the rule must still recognize it as EscapeContextDone +// and emit the ctx.Done() diagnostic. +func derivedCtxDone(ctx context.Context) { + timeoutCtx, cancel := context.WithTimeout(ctx, time.Second) + defer cancel() + + c := make(chan int, 1) + select { + case c <- 1: // want `ctx\.Done\(\)` + case <-timeoutCtx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/empty_default/empty_default.go b/linters/rules/channelcheck/testdata/src/empty_default/empty_default.go new file mode 100644 index 00000000000..926dd4774d9 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/empty_default/empty_default.go @@ -0,0 +1,10 @@ +package fixture + +// Match - empty default case +func emptyDefault() { + c := make(chan int, 1) + select { + case c <- 1: // want `empty default in channel select silently drops the send` + default: + } +} diff --git a/linters/rules/channelcheck/testdata/src/escape_other_alone/escape_other_alone.go b/linters/rules/channelcheck/testdata/src/escape_other_alone/escape_other_alone.go new file mode 100644 index 00000000000..e74ef7254f8 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/escape_other_alone/escape_other_alone.go @@ -0,0 +1,11 @@ +package fixture + +// Match for no default or timeout +func escapeOtherAlone() { + c := make(chan int, 1) + other := make(chan struct{}, 1) + select { + case c <- 1: // want `Blocking send` + case <-other: + } +} diff --git a/linters/rules/channelcheck/testdata/src/fake_done_counter/fake_done_counter.go b/linters/rules/channelcheck/testdata/src/fake_done_counter/fake_done_counter.go new file mode 100644 index 00000000000..38b27891106 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/fake_done_counter/fake_done_counter.go @@ -0,0 +1,19 @@ +package fixture + +// Non-match. Only the ctx.Done() function should trigger this. +type fakeDone struct { + done chan struct{} +} + +func (f *fakeDone) Done() <-chan struct{} { + return f.done +} + +func fakeDoneCounter() { + c := make(chan int, 1) + f := &fakeDone{done: make(chan struct{}, 1)} + select { + case c <- 1: // want `Blocking send` + case <-f.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_by_name/ignore_by_name.go b/linters/rules/channelcheck/testdata/src/ignore_by_name/ignore_by_name.go new file mode 100644 index 00000000000..8ad8d986194 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_by_name/ignore_by_name.go @@ -0,0 +1,17 @@ +package fixture + +type holder struct { + ignoreMe chan int +} + +// ignoreMe and h.ignoreMe are not matched based on ignore channel rules. +// tracked is still matched. +func ignoreByName() { + ignoreMe := make(chan int, 1) + tracked := make(chan int, 1) + h := &holder{ignoreMe: make(chan int, 1)} + + ignoreMe <- 1 // ignored by name + h.ignoreMe <- 2 // ignored by selector field name + tracked <- 3 // want `Blocking send` +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_ctx_done/ignore_ctx_done.go b/linters/rules/channelcheck/testdata/src/ignore_ctx_done/ignore_ctx_done.go new file mode 100644 index 00000000000..3ae29d47c2b --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_ctx_done/ignore_ctx_done.go @@ -0,0 +1,11 @@ +package fixture + +import "context" + +func ignoreCtxDone(ctx context.Context) { + ignoreMe := make(chan int, 1) + select { + case ignoreMe <- 1: + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_empty_default/ignore_empty_default.go b/linters/rules/channelcheck/testdata/src/ignore_empty_default/ignore_empty_default.go new file mode 100644 index 00000000000..b612543254a --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_empty_default/ignore_empty_default.go @@ -0,0 +1,9 @@ +package fixture + +func ignoreEmptyDefault() { + ignoreMe := make(chan int, 1) + select { + case ignoreMe <- 1: + default: + } +} diff --git a/linters/rules/channelcheck/testdata/src/ignore_partial/ignore_partial.go b/linters/rules/channelcheck/testdata/src/ignore_partial/ignore_partial.go new file mode 100644 index 00000000000..49864694f06 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/ignore_partial/ignore_partial.go @@ -0,0 +1,15 @@ +package fixture + +import "context" + +// Mixed sends in one select: one ignored, one tracked. The select must still +// produce the ctx.Done() diagnostic because not every send is ignored. +func ignorePartial(ctx context.Context) { + ignoreMe := make(chan int, 1) + tracked := make(chan int, 1) + select { + case ignoreMe <- 1: + case tracked <- 2: // want `ctx\.Done\(\)` + case <-ctx.Done(): + } +} diff --git a/linters/rules/channelcheck/testdata/src/send_in_case_body/send_in_case_body.go b/linters/rules/channelcheck/testdata/src/send_in_case_body/send_in_case_body.go new file mode 100644 index 00000000000..93743da0d53 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/send_in_case_body/send_in_case_body.go @@ -0,0 +1,18 @@ +package fixture + +import "time" + +// A send inside a case BODY is unrelated to the select's escape mechanism — +// it executes synchronously when that branch is chosen and blocks just like +// any other bare send. Even with a real timer escape on the select, the rule +// must still flag the in-body sends. +func sendInCaseBody() { + c := make(chan int, 1) + other := make(chan int, 1) + select { + case <-time.After(time.Second): + c <- 1 // want `Blocking send` + case <-other: + c <- 2 // want `Blocking send` + } +} diff --git a/linters/rules/channelcheck/testdata/src/timer_safe/timer_safe.go b/linters/rules/channelcheck/testdata/src/timer_safe/timer_safe.go new file mode 100644 index 00000000000..fa04cd687bb --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/timer_safe/timer_safe.go @@ -0,0 +1,12 @@ +// OK +package fixture + +import "time" + +func timerSafe() { + c := make(chan int, 1) + select { + case c <- 1: + case <-time.After(time.Second): + } +} diff --git a/linters/rules/channelcheck/testdata/src/unbuffered_chan/unbuffered_chan.go b/linters/rules/channelcheck/testdata/src/unbuffered_chan/unbuffered_chan.go new file mode 100644 index 00000000000..dea6fd1066d --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/unbuffered_chan/unbuffered_chan.go @@ -0,0 +1,5 @@ +package fixture + +func unbuffered() { + _ = make(chan int) // want `unbuffered channel` +} diff --git a/linters/rules/channelcheck/testdata/src/unbuffered_disabled/unbuffered_disabled.go b/linters/rules/channelcheck/testdata/src/unbuffered_disabled/unbuffered_disabled.go new file mode 100644 index 00000000000..5075016ec15 --- /dev/null +++ b/linters/rules/channelcheck/testdata/src/unbuffered_disabled/unbuffered_disabled.go @@ -0,0 +1,5 @@ +package fixture + +func unbuffered() { + _ = make(chan int) +} diff --git a/linters/rules/example_linter/example.go b/linters/rules/example_linter/example.go new file mode 100644 index 00000000000..08388a6b5c0 --- /dev/null +++ b/linters/rules/example_linter/example.go @@ -0,0 +1,66 @@ +// Package example is a no-op reference linter. It demonstrates the minimum +// structure needed to plug a new analyzer into both the wormhole-lint +// aggregator (cmd/wormhole-lint) and the custom golangci-lint binary +// (.custom-gcl.yml). Copy this package as a starting point when adding a +// real linter under rules//. +package example + +import ( + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +// Settings is the per-invocation configuration block passed in via +// golangci-lint's `linters.settings.custom.example.settings:` map. Fields +// here become user-facing tunables; an empty struct is fine for a no-op. +type Settings struct{} + +// Analyzer is the standalone entrypoint consumed by cmd/wormhole-lint. +// Keep Name unique across all rules// analyzers — golangci-lint +// addresses the plugin by this name in .golangci.yml. +var Analyzer = &analysis.Analyzer{ + Name: "example", + Doc: "no-op reference linter; never reports diagnostics", + Run: run, +} + +func run(_ *analysis.Pass) (any, error) { + // Intentionally empty: this linter never flags anything. A real linter + // walks pass.Files and calls pass.Reportf / pass.Report on offenders. + return nil, nil +} + +// Plugin is the type returned to golangci-lint's module-plugin loader. One +// plugin can expose multiple analyzers via BuildAnalyzers. +type Plugin struct { + settings Settings +} + +// New is the constructor registered with golangci-lint. It receives the +// raw settings map from .golangci.yml and decodes it into Settings. +func New(raw any) (register.LinterPlugin, error) { + s, err := register.DecodeSettings[Settings](raw) + if err != nil { + return nil, err + } + return &Plugin{settings: s}, nil +} + +// BuildAnalyzers returns the analyzers this plugin contributes. Add more +// entries here if your linter ships multiple checks. +func (p *Plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{Analyzer}, nil +} + +// GetLoadMode tells golangci-lint how much type information to load. +// LoadModeSyntax is enough for AST-only checks; LoadModeTypesInfo is +// required when you need full type resolution (recommended default). +func (p *Plugin) GetLoadMode() string { + return register.LoadModeTypesInfo +} + +func init() { + // The string passed here is the name used in .golangci.yml under + // linters.enable and linters.settings.custom. + register.Plugin("example", New) +} diff --git a/linters/rules/example_linter/example_test.go b/linters/rules/example_linter/example_test.go new file mode 100644 index 00000000000..63dcbb4e700 --- /dev/null +++ b/linters/rules/example_linter/example_test.go @@ -0,0 +1,13 @@ +package example + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestExample(t *testing.T) { + // The fixture contains no `// want "..."` comments — the linter is a + // no-op, so analysistest should see zero diagnostics. + analysistest.Run(t, analysistest.TestData(), Analyzer, "./...") +} diff --git a/linters/rules/example_linter/go.mod b/linters/rules/example_linter/go.mod new file mode 100644 index 00000000000..163f7ebc698 --- /dev/null +++ b/linters/rules/example_linter/go.mod @@ -0,0 +1,13 @@ +module github.com/wormhole-foundation/wormhole/linters/rules/example_linter + +go 1.25.10 + +require ( + github.com/golangci/plugin-module-register v0.1.1 + golang.org/x/tools v0.30.0 +) + +require ( + golang.org/x/mod v0.23.0 // indirect + golang.org/x/sync v0.11.0 // indirect +) diff --git a/linters/rules/example_linter/go.sum b/linters/rules/example_linter/go.sum new file mode 100644 index 00000000000..5ac45bec971 --- /dev/null +++ b/linters/rules/example_linter/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= +github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM= +golang.org/x/mod v0.23.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.30.0 h1:BgcpHewrV5AUp2G9MebG4XPFI1E2W41zU1SaqVA9vJY= +golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= diff --git a/linters/rules/example_linter/testdata/example.go b/linters/rules/example_linter/testdata/example.go new file mode 100644 index 00000000000..02f55ef4b9d --- /dev/null +++ b/linters/rules/example_linter/testdata/example.go @@ -0,0 +1,9 @@ +package testdata + +// Fixture for the example linter. It contains no offenders because the +// example linter never flags anything; this file exists only to give +// `go test` something to load when running `analysistest`. + +func Hello() string { + return "hello" +} diff --git a/node/Makefile b/node/Makefile index 7fcf7cde8c0..e8eba1525ea 100644 --- a/node/Makefile +++ b/node/Makefile @@ -1,6 +1,7 @@ .PHONY: lint lint: - golangci-lint run -c ../.golangci.yml ./... +# Lints spelling and Go via the custom wormhole-golangci-lint + bash ../scripts/lint.sh lint .PHONY: test test: diff --git a/node/pkg/common/scissors.go b/node/pkg/common/scissors.go index aab8b61febe..f5cc06ba570 100644 --- a/node/pkg/common/scissors.go +++ b/node/pkg/common/scissors.go @@ -67,7 +67,7 @@ func StartRunnable(ctx context.Context, errC chan error, catchPanics bool, name } // We don't want this to hang if the listener has already gone away. select { - case errC <- err: + case errC <- err: // Note on channel capacity: intentional best-effort: silently drop if the listener has already gone away default: } ScissorsPanicsCaught.WithLabelValues(name).Inc() @@ -85,7 +85,7 @@ func startRunnable(ctx context.Context, errC chan error, name string, runnable s if err != nil { // We don't want this to hang if the listener has already gone away. select { - case errC <- err: + case errC <- err: // Note on channel capacity: intentional best-effort: silently drop if the listener has already gone away default: } ScissorsErrorsCaught.WithLabelValues(name).Inc() diff --git a/node/pkg/p2p/p2p.go b/node/pkg/p2p/p2p.go index 7f8ba41071b..53fa9fa77d1 100644 --- a/node/pkg/p2p/p2p.go +++ b/node/pkg/p2p/p2p.go @@ -1370,7 +1370,7 @@ func Run(params *RunParams) func(ctx context.Context) error { for { envelope, err := managerSubscription.Next(ctx) // Note: sub.Next(ctx) will return an error once ctx is canceled if err != nil { - errC <- fmt.Errorf("failed to receive pubsub message on manager topic: %w", err) //nolint:channelcheck // The runnable will exit anyway + errC <- fmt.Errorf("failed to receive pubsub message on manager topic: %w", err) // Note on channel capacity: The runnable will exit anyway return } diff --git a/node/pkg/watchers/evm/connectors/common.go b/node/pkg/watchers/evm/connectors/common.go index c5607800428..7b5bc5cbd7a 100644 --- a/node/pkg/watchers/evm/connectors/common.go +++ b/node/pkg/watchers/evm/connectors/common.go @@ -109,7 +109,7 @@ func (sub *PollSubscription) Unsubscribe() { // non-blocking: unsubDone is buffered and only ever needs a single value. func (sub *PollSubscription) signalUnsubscribed() { select { - case sub.unsubDone <- struct{}{}: //nolint:channelcheck // Buffered; single signal. + case sub.unsubDone <- struct{}{}: // Note on channel capacity: Buffered; single signal. default: } } diff --git a/node/pkg/watchers/solana/tx_for_addr.go b/node/pkg/watchers/solana/tx_for_addr.go index e9e84ba0d0b..af475ea02c3 100644 --- a/node/pkg/watchers/solana/tx_for_addr.go +++ b/node/pkg/watchers/solana/tx_for_addr.go @@ -39,7 +39,7 @@ func (s *SolanaWatcher) transactionProcessor(ctx context.Context) error { s.pollPrevWormholeSignature, err = s.getPrevWormholeSignature() if err != nil { s.logger.Error("failed to get the last wormhole signature on start up", zap.Error(err)) - s.errC <- err + s.errC <- err // Note on channel capacity: Error channel. Should never be stuck for long periods of time. return err } } @@ -58,7 +58,7 @@ func (s *SolanaWatcher) transactionProcessor(ctx context.Context) error { err := s.processNewTransactions() if err != nil { s.logger.Error("failed to get transactions", zap.Error(err)) - s.errC <- err + s.errC <- err // Note on channel capacity: Error channel. Should never be stuck for long periods of time. return err } } diff --git a/node/pkg/watchers/xrpl/watcher.go b/node/pkg/watchers/xrpl/watcher.go index 9a772ddf254..88c9056196f 100644 --- a/node/pkg/watchers/xrpl/watcher.go +++ b/node/pkg/watchers/xrpl/watcher.go @@ -257,7 +257,7 @@ func (w *Watcher) Run(ctx context.Context) error { if msg != nil { msg.IsReobservation = true - w.msgChan <- msg + w.msgChan <- msg // Note on channel capacity: We never want to drop messages. watchers.ReobservationsByChain.WithLabelValues("xrpl", "std").Inc() } } @@ -351,7 +351,7 @@ func (w *Watcher) processTransaction(tx *streamtypes.TransactionStream) error { } // Send to processor - w.msgChan <- msg + w.msgChan <- msg // Note on channel capacity: We never want to drop messages. xrplMessagesConfirmed.Inc() w.logger.Info("message observed", msg.ZapFields()...) diff --git a/scripts/Dockerfile.lint b/scripts/Dockerfile.lint index a66f32d0648..231de7e1628 100644 --- a/scripts/Dockerfile.lint +++ b/scripts/Dockerfile.lint @@ -1,13 +1,21 @@ # syntax=docker.io/docker/dockerfile:1.3@sha256:42399d4635eddd7a9b8a24be879d2f9a930d0ed040a61324cfdf59ef1357b3b2 FROM docker.io/golang:1.25.10-bookworm@sha256:154bd7001b6eb339e88c964442c0ad6ed5e53f09844cc818a41ce4ecb3ce3b43 +# make: drives the custom linter build via linters/Makefile. +RUN apt-get update && apt-get install -y --no-install-recommends make \ + && rm -rf /var/lib/apt/lists/* + RUN useradd -u 1000 -U -m -d /home/lint lint USER 1000 WORKDIR /home/lint -# install goimports +# install goimports (used by `format`) RUN go install golang.org/x/tools/cmd/goimports@latest -# install golangci-lint -RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | \ - sh -s -- -b $(go env GOPATH)/bin v1.52.2 +# Build the custom wormhole-golangci-lint (golangci-lint with our linters/ module +# plugins) from source into the image and put it on PATH, so `-c lint` runs it +# without writing to the read-only repo mount. Rebuilt when anything under +# linters/ changes (Docker layer cache). +COPY --chown=1000:1000 linters/ /home/lint/linters/ +RUN make -C /home/lint/linters clean && make -C /home/lint/linters build-golangci-lint +ENV PATH="/home/lint/linters/bin:${PATH}" diff --git a/scripts/lint.sh b/scripts/lint.sh index da82a8b4b2e..ef569475ce3 100755 --- a/scripts/lint.sh +++ b/scripts/lint.sh @@ -6,6 +6,14 @@ set -eo pipefail -o nounset ROOT="$(dirname "$(dirname "$(realpath "$0")")")" DOCKERFILE="$ROOT/scripts/Dockerfile.lint" +# The custom golangci-lint (with our linters/ module plugins baked in) is built +# from source under linters/ via `make -C linters build-golangci-lint`. The +# golangci-lint version is owned by linters/ (GOLANGCI_LINT_HASH in +# linters/Makefile, whose tag comment must match `version:` in +# linters/.custom-gcl.yml). The Go build cache makes rebuilds a near-no-op after +# the first build, so we just rebuild every run rather than caching the binary. +LINTERS_DIR="$ROOT/linters" + VALID_COMMANDS=("lint" "format") SELF_ARGS_WITHOUT_DOCKER="" @@ -43,7 +51,7 @@ format(){ fi # Use -exec because of pitfall #1 in http://mywiki.wooledge.org/BashPitfalls - GOFMT_OUTPUT="$(find "./sdk" "./node" "./wormchain" -type f -name '*.go' -not -path '*.pb.go' -print0 | xargs -r -0 goimports $GOIMPORTS_ARGS 2>&1)" + GOFMT_OUTPUT="$(find "./sdk" "./node" "./wormchain" "./linters" -type f -name '*.go' -not -path '*.pb.go' -print0 | xargs -r -0 goimports $GOIMPORTS_ARGS 2>&1)" if [ -n "$GOFMT_OUTPUT" ]; then if [ "$GITHUB_ACTION" == "true" ]; then @@ -54,6 +62,22 @@ format(){ fi } +ensure_wormhole_golangci_lint() { + # In the -c docker image the custom linter is already built from source and + # placed on PATH; run that instead of rebuilding (the repo mount is read-only). + if command -v wormhole-golangci-lint >/dev/null 2>&1; then + command -v wormhole-golangci-lint + return + fi + + # Build the custom golangci-lint. The Go build cache makes this a near-no-op + # once it's been built once, so we don't cache the binary ourselves. + # (stderr only — stdout is the binary path.) + echo "Building wormhole-golangci-lint..." >&2 + make -C "$LINTERS_DIR" build-golangci-lint >&2 + echo "$LINTERS_DIR/bin/wormhole-golangci-lint" +} + lint(){ # === Spell check if ! command -v cspell >/dev/null 2>&1; then @@ -61,19 +85,26 @@ lint(){ else cspell "*/**.*md" fi - - # === Go linting - # Check for dependencies - if ! command -v golangci-lint >/dev/null 2>&1; then - printf "%s\n" "Require golangci-lint. You can run this command in a docker container instead with '-c' and not worry about it or install it: https://golangci-lint.run/usage/install/" - fi - - # Do the actual linting! - cd "$ROOT"/node - golangci-lint run --timeout=10m $GOLANGCI_LINT_ARGS ./... - cd "${ROOT}/sdk" - golangci-lint run --timeout=10m $GOLANGCI_LINT_ARGS ./... + # === Go linting (custom wormhole-golangci-lint) + local LINT_BIN + LINT_BIN="$(ensure_wormhole_golangci_lint)" + + # Lint node, sdk, and linters/ is the custom linter's own code; its + # rules// are separate modules (mirroring `make -C linters test`), so + # lint each one too. Run in a subshell so a failure still halts under + # `set -e` without leaking the working directory between modules. + # NOTE this does NOT lint the wormchain directory. + lint_module() { + ( cd "$1" && "$LINT_BIN" run --timeout=10m $GOLANGCI_LINT_ARGS ./... ) + } + + lint_module "$ROOT/node" + lint_module "$ROOT/sdk" + lint_module "$ROOT/linters" + while IFS= read -r gomod; do + lint_module "$(dirname "$gomod")" + done < <(find "$ROOT/linters/rules" -name go.mod) } DOCKER="false"