diff --git a/skills/constable/SKILL.md b/skills/constable/SKILL.md index c415843..56e9ed8 100644 --- a/skills/constable/SKILL.md +++ b/skills/constable/SKILL.md @@ -1,158 +1,219 @@ --- name: constable -description: Use constable to check that annotated Go functions and methods do not mutate caller-visible argument or receiver state. +description: Use constable to check Go packages with its analyzers and to repair each diagnostic that it reports. --- # Go Constable -`constable` is a Go static analysis tool for opt-in non-mutation checks. +`constable` is a static analysis tool for Go. +It runs a set of analyzers on each package and prints one diagnostic per line. +Use it to check a change, and to hold a repository rule in a hook or in CI. -Use it when a function or method should read its inputs without changing state that the caller can observe. -The check does not guarantee that the function is pure or free of all side effects. +## Run it -## Installation and invocation - -Install the released command with: +Run it from the module root, ```sh -go install github.com/nnutter/constable/cmd/constable@latest +constable ./... ``` -Run it against the packages in a module: +Each diagnostic has the form `file:line:col: message` and goes to stderr. +Exit status `0` means no diagnostic, `3` means at least one diagnostic, and `1` means constable failed. +An invocation that finds no diagnostic succeeds silently. -```sh -constable ./... +Further flags: + +- `-json` prints JSON and always exits `0`. +- `-c N` prints N lines of source around each reported line. +- `-fix` writes the fixes, and `-fix -diff` prints them as a diff and writes nothing. +- `-=false` turns off one analyzer, for example `-complexity=false`. +- `-flags` lists every flag, and `help ` describes one analyzer. + +A file that carries the generated-code comment `// Code generated ... DO NOT EDIT.` is skipped, because the next run of the generator overwrites it. +Change such a file through its generator. +`-include-generated` reports in it anyway. + +An exit status `1` with `analysis skipped due to errors in package` means the package does not build. +Fix the build error first, then run constable again. + +## Repair each diagnostic + +### nonmutating + +```text +//constable:nonmutating function mutates pointer parameter p +//constable:nonmutating function mutates parameter s +//constable:nonmutating function deletes from map parameter m +//constable:nonmutating method mutates receiver r ``` -The command reports diagnostics and exits unsuccessfully when an annotated function violates the contract. -An invocation with no diagnostics succeeds silently. +The statement writes to data that the caller owns. +The non-mutating contract section below lists the writes that count. +The message names the origin: `pointer parameter` for a write through a pointer, `parameter` for a slice, a map, or another reference, `receiver` for a method receiver, and `deletes` for `delete` on a map. +Repair it in this order: -## Annotating Go code +1. Stop the write. + Copy the data and write to the copy, or return a new value for the caller to assign. +1. Delete the directive when the function must change caller-visible state, because the contract is wrong for that function. -Put the exact directive `//constable:nonmutating` immediately before the function or method declaration. -There must not be a space between `//` and `constable:`. +`p = nil` on its own reports nothing, but the caller's data still changes when the code also writes through `p`. +Do not hide a diagnostic by assigning a new value to the parameter. +Prefer the copy or the new value. -```go -//constable:nonmutating -func ReadValue(p *int) int { - return *p -} +### methodical + +```text +method Read of type Buffer should be in same file as type definition +method Append of type Buffer should be sorted before method Read +method Len of type Buffer should be grouped with method Read ``` -The directive applies to functions and methods only. -Do not add it to a function that is intentionally allowed to mutate caller-visible state. +Run the fix, -## Mutations that must be avoided +```sh +constable -fix -diff ./... # read the change +constable -fix ./... # write the change +``` + +The fix moves every method of the reported type directly after the type declaration. +It orders those methods by name, with a generic method after the non-generic methods. +It adds the imports that a moved method needs and removes the imports that the old file no longer uses. + +Move a method by hand when the fix skips it. +The fix skips a generated file, a file with build constraints or Go directives, a file that uses cgo, a file whose import block holds comments, a file with comments that no declaration owns, and a declaration that does not start its own line. +The fix writes only a file that Git tracks and that has no changes, so `git checkout` recovers the file. -The analyzer reports direct mutations through pointer, slice, and map parameters. -It also reports mutations through pointer receivers and through reference-like fields of value receivers. +### predicates + +```text +use predicate function instead of "b" in compound conditional +``` + +The quoted text is the operand to replace. +Move that operand into a function that returns `bool`, and call the function in its place, ```go -//constable:nonmutating -func WriteValue(p *int) { - *p = 1 // reported -} +func isB() bool { return b } -//constable:nonmutating -func WriteSlice(values []int) { - values[0] = 1 // reported -} +if a && isB() { // was: if a && b { +``` -//constable:nonmutating -func WriteMap(values map[string]int) { - values["key"] = 1 // reported -} +A variable does not satisfy the analyzer. +Only a call does. -//constable:nonmutating -func DeleteMapEntry(values map[string]int) { - delete(values, "key") // reported -} +### testify -type Buffer struct { - data []byte -} +```text +use testify/assert instead of testing.Error +use testify/require instead of testing.Fatal +``` -//constable:nonmutating -func (b Buffer) ClearFirstByte() { - b.data[0] = 0 // reported: b.data shares caller-visible backing storage -} +Replace the `testing` call with the testify call, and add the import. +`assert` does not stop the test, so it replaces `t.Error`, `t.Errorf`, and `t.Fail`. `require` stops the test, so it replaces `t.Fatal`, `t.Fatalf`, and `t.FailNow`. +Prefer a specific assertion, such as `assert.Equal` or `require.NoError`, because it prints the values that differ. + +### complexity + +```text +cyclomatic complexity 12 exceeds limit 10 (function Busy) ``` -Aliases of parameters are also checked when the alias is assigned to a local variable. -Nested reference-like values, such as a slice of slices, are considered caller-visible when mutated. +Extract branches into smaller functions until the count is at or below the limit. +Extract a branch when you can give the new function a name from the problem domain, such as `isOverdue` or `hasOpenInvoice`. +That name states the fact that the condition tests, so the reader sees the rule instead of the steps. +A name that only restates the steps, such as `checkFlagsAndReturnTwo`, does not help the reader. +When only that kind of name fits, the branch is not a good candidate for extraction. +Split the function somewhere else. +When the function is one coherent rule, raise the limit instead. +The count starts at 1 and adds 1 for each `if`, `for`, `range`, `case`, and `select` clause, `&&`, and `||`. +A closure adds to the function that holds it, and a `default` clause counts. +`-complexity.limit=15` raises the limit for the whole run. +Say why in the change when you use it. -## Code that is allowed +### errcheck, sumtype, testifylint, and nilaway -Reassigning a parameter does not mutate the value held by the caller. -Mutating a local variable or a pointer to a local variable is also allowed. -A value receiver may mutate its scalar fields because those fields belong to the receiver copy. +These analyzers come from other modules. +Their messages, flags, and repairs are theirs. -```go -//constable:nonmutating -func ReassignParameter(p *int) { - p = nil // allowed -} +| Analyzer | Message | Repair | +| --- | --- | --- | +| `errcheck` | `unchecked error` | Handle the error. Assign it to `_` only when the code discards it on purpose. | +| `sumtype` | `missing cases for Square` | Add the case for the variant, or add a `default` clause and set `-sumtype.default-signifies-exhaustive`. | +| `testifylint` | text that names a testify check | Follow the message. The check name states the misuse, such as `expected-actual`, `formatter`, or `require-error`. | +| `nilaway` | `Potential nil panic detected` | Remove the nil flow: check the value, return early, or initialize it where it is declared. | -//constable:nonmutating -func MutateLocal() int { - value := 0 - value++ // allowed - return value -} +The embedded analyzers have more flags than this document lists. +Their flags keep the analyzer name as a prefix, such as `-errcheck.blank` or `-testifylint.disable-all`. -type Count struct { - n int -} +NilAway reads every package that it loads by default, which costs seconds and gigabytes. +constable limits it to the main modules of the packages that it analyzes. +Set `-nilaway_config.include-pkgs=example.com/my/module` to choose the packages, or `-nilaway.include-all-pkgs` to analyze the dependencies and the standard library too. +Suppress a report that the code cannot fix with a `//nolint:nilaway` comment on the reported statement. + +## The non-mutating contract + +`//constable:nonmutating` states that the function or method does not change state that the caller can observe through its arguments or its receiver. +The check does not prove that the function is pure or free of side effects. + +Put the exact directive immediately before the declaration, with no space between `//` and `constable:`. +```go //constable:nonmutating -func (c Count) Increment() Count { - c.n++ // allowed: c is a value-receiver copy - return c +func ReadValue(p *int) int { + return *p } ``` -## Predicates +The directive applies to functions and methods only. + +### Reported mutations -The predicates analyzer reports `&&` and `||` operands, except the first, that are not predicate-function calls. -The first operand always executes, so coverage observes short-circuiting through the rest. +- A write through a pointer parameter or a pointer receiver, such as `*p = 1`. +- A write to a slice element, a map entry, or a field that the parameter reaches. +- `delete` on a map parameter. +- A write to a reference-like field of a value receiver, such as a slice field. +- The same writes through an alias that a local variable holds. ```go -func F(a, b bool) { - if a && b { // reported: use a && isB() - } -} +type Buffer struct{ data []byte } -func isB() bool { return false } - -func G(a bool) { - if a && isB() { // allowed - } +//constable:nonmutating +func (b Buffer) ClearFirstByte() { + b.data[0] = 0 // b.data shares the caller's backing array } ``` -The check covers `if` and `for` conditions and bool assignments such as `c := a && b`, including `!a`, field and map access, and comparisons. -Operands that are already calls, compound expressions, or `true`/`false`/`nil` are allowed. +Nested references count too, such as a write to an element of a slice of slices. + +### Allowed code -## Agent workflow +- A new value for the parameter itself, such as `p = nil`. +- A write to a local variable, or to memory that the function allocates. +- A scalar field write on a value receiver, because that field belongs to the receiver copy. + +```go +type Count struct{ n int } -1. Identify functions and methods whose API contract should not mutate caller-owned data. -1. Add `//constable:nonmutating` immediately before each eligible declaration. -1. Run `constable ./...` from the Go module root. -1. For every diagnostic, either remove the caller-visible mutation or remove the annotation if the contract is incorrect. -1. Re-run the analyzer and the package tests before finalizing the change. +//constable:nonmutating +func (c Count) Increment() Count { + c.n++ // c is the receiver copy + return c +} +``` -Prefer returning a new value or making a defensive copy when the implementation needs to transform caller-provided data. -Do not silence a diagnostic by merely reassigning a parameter while continuing to mutate data reachable through it. +## Workflow -## Scope and limitations +1. Identify the functions and methods whose contract must not change caller-owned data. +1. Add `//constable:nonmutating` immediately before each of them. +1. Run `constable ./...`. +1. Repair each diagnostic with the advice above. +1. Re-run constable and the package tests before you finalize the change. -The check is focused on direct, intra-procedural mutations represented by assignments, increments, slice or map indexing, and `delete`. -It is not a general immutability checker. -It does not by itself guarantee that a function has no I/O, global state changes, goroutines, reflection, unsafe operations, or other side effects. -Calls to helper functions, interface methods, function values, and other indirect mutation paths may not be modeled. -The analyzer currently recognizes only the `nonmutating` directive. +## Scope and limits -The analyzers skip files that carry the standard generated-code comment, `// Code generated ... DO NOT EDIT.`. -Edit such a file through its generator, not by hand. +The non-mutating check finds direct writes in one function body: assignment, `++`, `--`, index writes, field writes, and `delete`. +It does not model a call to a helper function, a write through `append`, or an indirect path such as an interface method or a function value. +It is not a general immutability checker, and it does not by itself guarantee that a function has no I/O, global state changes, goroutines, reflection, or unsafe operations. +The analyzer recognizes only the `nonmutating` directive. -When behavior is uncertain, inspect the diagnostic and the implementation rather than assuming that a clean run proves complete immutability. +When a run is clean, inspect the implementation before you claim that it is fully non-mutating.