ci: gate go vet and gofmt -s, and fix the vet findings (MAPCO-11498)

.github/workflows/on_pr_push.yml

@@ -203,6 +203,46 @@ jobs:
         if: always()
         run: docker compose down
 
+  # Its own job, so a formatting or vet failure is named in the checks list
+  # and arrives in seconds rather than after the fixture restore and the suite.
+  static:
+    name: Static checks
+    runs-on: ubuntu-22.04
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v4
+
+      - name: Set up Go
+        uses: actions/setup-go@v5
+        with:
+          go-version-file: 'go.mod'
+          check-latest: true
+
+      # vendor/ is committed and is not `gofmt -s` clean, so `gofmt -s -l .`
+      # would report hundreds of files nobody here owns. List the tracked Go
+      # files outside it instead -- a filter on gofmt's output would have to
+      # survive grep exiting 1 on the clean run under the runner's pipefail.
+      - name: Check formatting
+        run: |
+          unformatted=$(git ls-files -z '*.go' ':!:vendor/**' | xargs -0 gofmt -s -l)
+          if [ -n "$unformatted" ]; then
+            echo "::error::these files are not gofmt -s clean:"
+            echo "$unformatted"
+            exit 1
+          fi
+
+      # The default analyzers, nothing added: the tree was brought clean against
+      # exactly these (MAPCO-11498), so a finding here is new. vet skips vendor/
+      # on its own; ./... never matches it.
+      #
+      # Twice, because vet only sees the files the build tags select. The second
+      # run sets every opt-out tag plus pprof, so the stub files that stand in
+      # for a compiled-out backend are vetted too, not only the default build.
+      - name: Vet
+        run: |
+          go vet -mod vendor ./...
+          go vet -mod vendor -tags 'noS3Cache noRedisCache noAzblobCache noGCSCache noGpkgProvider noPostgisProvider noPrometheusObserver pprof' ./...
+
   govulncheck:
     name: Run govulncheck
     runs-on: ubuntu-22.04

CONTRIBUTING.md

@@ -281,8 +281,10 @@ Optional features compile out behind `noS3Cache`, `noRedisCache`, `noAzblobCache
 
 ## Code conventions
 
-* **`gofmt -s`.** If running it produces changes in parts of the tree you are not working on, send
-  those in a separate pull request.
+* **`gofmt -s` and `go vet` are required**, and CI enforces both (see [Required checks](#required-checks)).
+  Never run `gofmt -s -w .` at the root: it rewrites `vendor/`. Format the paths you changed, and if
+  that produces changes in parts of the tree you are not working on, send those in a separate pull
+  request.
 * **Error variables** take the form `var ErrErrorName = errors.New("provider: canceled")` — the text
   all lowercase, with no punctuation at the end.
 * **Table-driven subtests keyed by name**, with a `fn := func(tc tcase) func(*testing.T)` closure.
@@ -324,13 +326,19 @@ CGO_ENABLED=0 go build -mod vendor ./...
 CGO_ENABLED=0 go test -run '^$' -mod vendor ./...   # links every test binary, runs none
 ```
 
-Two more gates worth running before you push:
+### Required checks
+
+CI fails a pull request on any of these, so run them before you push:
 
 ```bash
-gofmt -s -l . | grep -v '^vendor/'    # vendor/ is never -s clean; nothing else may appear
+git ls-files -z '*.go' ':!:vendor/**' | xargs -0 gofmt -s -l   # vendor/ is never -s clean; must print nothing
+go vet -mod vendor ./...              # the default analyzers; the tree is clean against them
 govulncheck ./...
 ```
 
+The tree is clean against `gofmt -s` and `go vet` today, so a finding from either is one your change
+introduced. Fix it rather than working around the check.
+
 ### Opt-in suites
 
 Provider- and backend-specific tests **skip unless you opt in** (`internal/ttools.ShouldSkip`), so a

cmd/internal/register/maps_test.go

@@ -14,7 +14,6 @@ import (
 
 func TestMaps(t *testing.T) {
 	type tcase struct {
-		atlas       atlas.Atlas
 		maps        []provider.Map
 		providers   []dict.Dict
 		expectedErr error
@@ -36,7 +35,10 @@ func TestMaps(t *testing.T) {
 				return
 			}
 
-			err = register.Maps(&tc.atlas, tc.maps, providers)
+			// A fresh atlas per case, not one carried in tcase: an Atlas holds a
+			// mutex, and a tcase is copied by value into fn and by range.
+			var a atlas.Atlas
+			err = register.Maps(&a, tc.maps, providers)
 			if !errors.Is(err, tc.expectedErr) {
 				t.Errorf("invalid error, expected %v got %v", tc.expectedErr, err)
 			}

cmd/shigola/cmd/tile_name_format_test.go

@@ -160,12 +160,13 @@ func TestFormatParse(t *testing.T) {
 	for k, tc := range testcases {
 
 		z, x, y, err := tc.format.Parse(tc.input)
-		if errOk(tc.err, err) {
-			continue
-		} else {
+		if !errOk(tc.err, err) {
 			t.Errorf("[%v] unexpected err, expected %v got %v", k, tc.err, err)
 			continue
 		}
+		if tc.err != nil {
+			continue
+		}
 
 		if z != tc.z || x != tc.x || y != tc.y {
 			t.Errorf("[%v] expected output (z:%v, x:%v, y:%v) got (z:%v, x:%v, y:%v)", k, tc.z, tc.x, tc.y, z, x, y)

internal/cmd/main.go

@@ -157,7 +157,9 @@ func Signal() os.Signal {
 // can be passed in as well, if no list is passed os.Interrupt, and syscall.SIGTERM is
 // assumed.
 func NewContext(signals ...os.Signal) *contextType {
-	ch := make(chan os.Signal)
+	// Buffered, as signal.Notify requires: it never blocks on a send, so a signal
+	// arriving before signalHandler is receiving would otherwise be dropped.
+	ch := make(chan os.Signal, 1)
 	if len(signals) == 0 {
 		signal.Notify(ch, os.Interrupt, syscall.SIGTERM)
 	} else {

internal/env/dict.go

@@ -78,7 +78,7 @@ func (d Dict) StringSlice(key string) (v []string, err error) {
 		var iv []interface{}
 		if iv, ok = val.([]interface{}); !ok {
 			// Could not convert to the generic type, so we don't have the correct thing.
-			return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)}
+			return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)}
 		}
 
 		v = make([]string, len(iv))
@@ -156,7 +156,7 @@ func (d Dict) BoolSlice(key string) (v []bool, err error) {
 		var iv []interface{}
 		if iv, ok = val.([]interface{}); !ok {
 			// Could not convert to the generic type, so we don't have the correct thing.
-			return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)}
+			return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)}
 		}
 
 		v = make([]bool, len(iv))
@@ -232,7 +232,7 @@ func (d Dict) IntSlice(key string) (v []int, err error) {
 		var iv []interface{}
 		if iv, ok = val.([]interface{}); !ok {
 			// Could not convert to the generic type, so we don't have the correct thing.
-			return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)}
+			return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)}
 		}
 		v = make([]int, len(iv))
 		for k := range iv {
@@ -436,7 +436,7 @@ func (d Dict) MapSlice(key string) (r []dict.Dicter, err error) {
 
 	arr, ok := v.([]map[string]interface{})
 	if !ok {
-		return r, dict.ErrKeyType{key, v, reflect.TypeOf(arr)}
+		return r, dict.ErrKeyType{Key: key, Value: v, T: reflect.TypeOf(arr)}
 	}
 
 	r = make([]dict.Dicter, len(arr))

observability/prometheus/prometheus.go

@@ -133,8 +133,8 @@ func New(config dict.Dicter) (observability.Interface, error) {
 
 func (*observer) Name() string { return Name }
 
-func (observer) Handler(string) http.Handler { return promhttp.Handler() }
-func (obs *observer) Init()                  { obs.initCall.Do(obs.init) }
+func (*observer) Handler(string) http.Handler { return promhttp.Handler() }
+func (obs *observer) Init()                   { obs.initCall.Do(obs.init) }
 func (obs *observer) init() {
 	obs.PublishBuildInfo()
 	if obs == nil || obs.pushURL == "" {