[fuad-daoud]

Summary

In short: No pipeline stage may keep mutable package-level state (invariant 5), yet a stage writing a package variable passed vet, lint and the tests, and only the race detector could catch it, when a run happened to reach it. This adds a globalwrites analyzer to make vet that refuses such a write in ir/, pass, compilers/ and emitters/ outside init.

Adds the globalwrites analyzer to internal/lintrules, run by make vet through cmd/morphic-vet. In a package under ir/, pass, compilers/ or emitters/, production code may not write a package-level variable, its own or another package's, outside init. A stage holds no mutable state (invariant 5), which engine.Engine's concurrent use rests on, and neither reassign, vet nor the tests saw a counter kept in one. Closes #1446. Related, left open: #1316 (exported mutable keyword slices): the rule refuses a write to them from any package that can import them, but a callee such as slices.Sort still can, and that issue asks that they stop being handed out.

What it refuses, directly or through a local map, slice or pointer that holds the variable (named as the target of :=, =, a var or a range clause, or by a return into a named result): an assignment, a compound assignment, ++ and --, a write to an element or field, taking the address, a method call ((*T).M(v) included) or method value whose receiver is a pointer, map or slice, and delete, clear, copy or append on it. One receiver is listed and stays legal, *encoding/base64.Encoding, which is read-only; an unlisted receiver draws until it is listed, *regexp.Regexp included, since its Longest and UnmarshalText write. At 0d09a96d no production package under the roots imports regexp, and the module's only call sites on the listed receiver are compilers/openapi/internal/schema/enum.go:248 and :278.

Where. The body of every function and method but init. In a declaration at package level or in init, which run once, each outermost function literal, each go statement and each selector not called there, as a method value is, since any of them can outlive initialization. What lies beneath such a selector is checked with it, so a method called there draws though it runs once. A statement written in init itself stays legal, and a stored method value or a go statement does not (storedInit = ledger.Add and go ledger.Add() draw; ledger.Add() and defer ledger.Add() do not, as both run inside init; defer func() { ledger.Add() }() draws, being a function literal, though it too runs inside init). gochecknoinits refuses an init function in production code as such, but not what a statement in one writes, so that verdict is this rule's alone. Aliases are found over a whole declaration, so a local bound in init and written by a literal init stores draws (m := props; hook = func() { m["n"]++ }), and so does go q.Add() through its alias.

The cost. A selector is checked on its syntax, not on whether it runs again, so var v = ledger.Self().n draws although Self runs once, and so does the same expression in init. The remedy is the rule's own: a method with a value receiver, or state kept in a local. A local is read without regard to order or path: one that holds the variable on any path holds it throughout, so if x { return names }; m = map[string]int{}; m["a"] = 1 draws at the last line, and the remedy is a separate local.

Out of reach. The rule reports writes it can see syntactically: a direct write, an increment, a pointer-receiver, map or slice method, and code init stores or starts. Any other way code bound at initialization can run later and write is out of reach by design, and a further form of that kind is a stated limit, not a defect. No issue tracks these. It reads one package's syntax and no callee's body, so:

• A variable passed to a call or returned to a caller (slices.Sort(v), helper(v)), whose callee may write it. A named result a return binds to the variable is not this case: a write through it in the function's own body draws.
• A method called at package level or in init that keeps its receiver, as one returning a closure does (var f = ledger.Bind()).
• A method on a struct or array value or an interface, though it can write through a map, slice or pointer the value holds.
• An alias held in a struct, array or interface, in an element of a slice or map or behind a pointer (a literal, s[0] = v or *p = v puts it there), or reached through a conversion, a type assertion or a function result; an alias across functions or packages.
• A write through reflect or unsafe after the address is taken, and a channel send or close.
• Code outside the roots, test files included.

Before and after

flowchart LR
  subgraph before["Before"]
    direction TB
    b1["a counter in pass, written from a stage"] --> b2["make vet, lint and go test: clean"]
    b1 --> b3["race detector: only if two tests<br/>overlap on that path in that run"]
  end
  subgraph after["After"]
    direction TB
    a1["a counter in pass, written from a stage"] --> a2["make vet: globalwrites refuses it<br/>on the pull request, every time"]
    a3["make coverage's race detector"] -. "still races what this cannot see,<br/>if a test overlaps it" .-> a2
  end
  before ~~~ after

Loading

What it brings: invariant 5 is enforced at the line that breaks it, on every pull request, without running a test. This is the static half of the narrower race run #1311 proposes, and it holds whether or not that lands: the common way to break the invariant no longer depends on a race detector run reaching that path.

Test plan

• go test ./internal/lintrules ./cmd/morphic-vet at 100.0% coverage. The plants under testdata/mod hold the verdicts, the scope edges and the legal reads of the rule's forms, and these of the forms its comments state it cannot see: a channel send or close, a conversion, a type assertion, a callee given the variable by a call, a function's result, a method at package level that keeps its receiver, a method on a struct value holding a map, and an alias held in a literal, in an element, in a copied struct or array or behind a pointer. Widening the rule past one of those, as its plant spells it, reddens TestAnalyzers_DrawExactlyThePlantedFindings. Not every stated limit or legal form has a plant: a method on an array value holding a map, a variable passed to a go statement or to reflection, and &, delete and the other writing builtins written in init itself all draw nothing at f21dc849, and a change that made one of them draw would leave that test green. Of 120 single-site mutants of globals.go on 8d083c61, planted one at a time, 4 did not compile, 5 panicked the run, 102 failed that test and 9 survived. One survivor is a fixed cap of four passes on the fixed point, the hops ChainAlone plants: a five-hop chain tells it apart, and a plant that long would only let a cap of five survive in its place. The others are the classes no input tells apart: one alias map shared across declarations, whose locals are each one declaration's; a function literal walked from its parent, whose nodes are the same; the guards on := targets; a nil-package guard the type checker leaves no input for; the fixed point's early exit; a target beyond a comma-ok pair; and ranging over a basic type, whose key and value hold no reference. A mutant outside those 120, among them one that only moves where a finding lands, since the test reads a finding's line and not its column, is not covered by this count.
• go vet -vettool=<tool> -globalwrites ./ir/... ./pass/... ./compilers/... prints nothing: the module is clean.
• lint: refuse writes to package-level variables in pipeline packages #1446's probe (var danglingProps int and danglingProps++ in checkPropIDRefs's loop) fails make vet; with new(atomic.Int64) and .Add(1), with a counter written by a literal an init stores, with a local bound in init that such a literal writes through, with a method value init stores (danglingBump = danglingCount.Add, a new(atomic.Int64)) that the loop calls, or with a method started by go in init, it fails too.
• CI on this head, main at 2a0996a6 merged in, runs make gate. On f21dc849, go build ./..., go test on internal/lintrules, cmd/morphic-vet and internal/archtest, make vet, make lint, make lint-planted and skills_lint.py each pass.