Document a repair for each diagnostic in the constable skill

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.
+- `-<analyzer>=false` turns off one analyzer, for example `-complexity=false`.
+- `-flags` lists every flag, and `help <analyzer>` 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.