build: check that every //nolint names an enabled linter - #367
Merged
Conversation
nolintlint reports a directive that suppresses nothing only when the linter it names is enabled: the nolint filter drops nolintlint's "unused directive" issue outright for a linter that is off, so a directive naming a disabled linter -- or one that does not exist -- suppresses nothing, fails nothing, and goes on reading as a live constraint on the code beneath it. scripts/check-nolint-linters.sh cross-checks the names in every //nolint against the set golangci-lint reports as enabled, and the gate runs it after the lint step. The enabled set is asked of golangci-lint rather than copied from .golangci.yml, so enabling or dropping a linter needs no edit there. The gate now names one golangci-lint version for both steps: the check's answer only describes the run above it if the two are the same build. //nolint:forcetypeassert on pass/validate_propids_test.go:86 was of exactly the kind the check rejects -- forcetypeassert has never been enabled here -- so it goes, keeping its justification as a plain comment.
…ter-names # Conflicts: # CLAUDE.md
Four things could make the check report a verdict about something other than
this tree, or describe a directive as doing the opposite of what it does.
The enabled set was asked of golangci-lint in whatever directory the caller
stood in, while the directives were read with `git -C "$repo_root" grep`. Run
from anywhere holding a different config, the check reported every real
directive here as naming a linter that is not running. Both list calls now run
in the repo the script lives in.
`jq -r '.Enabled[].name'` cannot iterate the null that golangci-lint emits when
a list is empty, and jq reports only its last input's status: with both files in
one call, a null linters list exited 0 and silently shrank the enabled set to
the formatters, so every directive read as disabled. A null formatters list — an
ordinary config with no formatters — killed the script outright with a raw jq
error. Each list is now read on its own with a `// []` default, and the existing
"no enabled linters" guard sits between them, where it can finally fire; before,
neither it nor its awk counterpart could be reached in either case.
Three of the four forms the check called blanket suppressions are not. Measured
against golangci-lint 2.12.2: `//nolint:` suppresses nothing at all, a stray
comma suppresses only the names beside it, and `//nolint:allfoo` does blanket —
because extractInlineRangeFromComment tests HasPrefix("nolint:all") before it
splits, which the check did not mirror. A directive that suppresses nothing was
being reported as suppressing everything, sending a reader looking for
over-suppression when the finding beneath it is live.
`scan` resumed two characters past each "//", which re-entered the same run of
slashes and read one `////nolint:x` as two directives, reporting it twice and
inflating both counts. It now resumes past the whole run.
Also refuses a tree in which a tracked path holds a colon rather than misparsing
`git grep -n` output, which folded the line number into the scanned text and
reported the finding at a location without one.
The gate script re-implements extractInlineRangeFromComment in awk, and every verdict it reaches rests on that grammar agreeing with golangci-lint's. Nothing held it to that. The tree carries four directives, all of one shape, so the committed corpus exercises almost none of the reader: a name that is merely disabled, an empty name, a blanket suppression spelled four different ways — none of them appear, and a drift in any of them would be nobody's job to notice. Twenty-four cases cover the grammar and the shapes around it that decide whether the answer describes this repo: which config the enabled set comes from, a null Enabled list, and a path git grep -n output cannot be split on. Each is followed by a mutation that must turn it red, so a case that catches nothing fails here rather than reading as coverage. Two cases pin the case-sensitivity split that is easiest to get wrong. golangci-lint tests the "all" prefix on the raw text but compares the split-out names lower-cased, so `//nolint:allfoo` blankets while `//nolint:AllFoo` is merely an unknown name, and `//nolint:ALL` blankets by the second test after failing the first. The checks that keep a case from passing vacuously are the ones that matter most: a case naming a fixture nothing built, a mutation naming a case the table does not carry, and an anchor appearing zero or many times all fail loudly. A mutation whose anchor no longer appears changes nothing, and a target that "survives" that is indistinguishable from a case that tests nothing.
The nolint step installed golangci-lint a second time, on the grounds that the action "runs golangci-lint from a path of its own and does not put it on PATH". It does put it on PATH: the bundle the runner executes at v8.0.0 calls core.addPath(path.dirname(binPath)) in its entrypoint, which prepends that directory to PATH for every later step in the job. Running the install block locally proved the install works; it could not observe what the action exported, which is why the claim stood. Dropping the second install makes the check ask the same build that produced the findings above it, rather than a separately downloaded copy that merely carries the same version string, and takes a curl-piped-to-sh out of the job. The version pin moves to the action's own input, where the one step that consumes it is. Adds the grammar verifier as its own step ahead of the check it drives, matching how the coverage counter sits ahead of the coverage gate.
CLAUDE.md and README.md each introduce their command block as the checks CI's gate job runs, so a step that is not in them makes the promise false. README.md was missing the directive check as well as the grammar verifier; its three older disagreements with the gate are separate and are filed as #415. .golangci.yml described the gap nolintlint leaves as "GitHub #306", which this change closes — leaving a reader at a resolved issue rather than at the script that now covers it.
`sort -u` folds together whatever the locale calls equal, and outside C that reaches past exact bytes — names differing only in case or punctuation can collapse into one, and the name that lost would then read as a linter golangci-lint is not running. Pinned to C, matching the failure listing in check-coverage.sh. The grammar fixtures also stop inheriting the caller's git configuration. A global commit.gpgsign with no key on the runner would fail the fixture commit, and the suite would read as a broken script rather than as a fixture that was never built.
check_case bound the case name and never looked at it; the repo field is what selects the fixture.
A stray comma leaves the names beside it working; only "//nolint:" on its own suppresses nothing at all. The comment claimed the latter of both.
go/scanner drops a trailing CR from a // comment before the nolint filter ever sees it, so on a CRLF file golangci-lint honours a bare //nolint — measured, the finding under it disappears. The grammar here was looking at "nolint\r", which matches neither anchor, and missed the directive entirely. That is an under-report, the one direction this check is not allowed to fail in, and the gate's gofmt step rejecting CRLF first does not make it right for a check meant to stand on its own. Covered by a CRLF fixture and the mutation that removes the strip. Two guards no planted directive can reach are now driven directly: the tool check, which runs before the script has read anything, and the awk-side empty check, which only fires when awk cannot read the file the shell just found non-empty. Both were reachable and neither was tested; removing either now turns its check red. The comment on the enabled-set sort claimed collation could fold two names into one. It cannot for any name golangci-lint ships: glibc separates "gocritic", "go-critic" and "GoCritic" under en_US.UTF-8. The pin stays as insurance; the comment now says which it is. Also drops the rule that skipped an empty line in the git grep output. No such line can occur, and skipping it silently disagreed with the rule below it, which treats any line that is not path:line:text as a broken invariant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nolintlintfails on a//nolintthat suppresses nothing, but only when the linter it names isenabled. golangci-lint's nolint filter drops nolintlint's "unused directive" issue outright for
a linter that is off —
shouldPassIssueinpkg/result/processors/nolint_filter.go, under thecomment "don't expect disabled linters to cover their nolint statements". So a directive naming a
disabled linter, or one that does not exist at all, suppresses nothing and fails nothing, while
still reading as a live constraint on the code beneath it.
Nothing else in the gate closes that. golangci-lint has a
Found unknown linters in //nolint directiveswarning, but it reaches even less than it looks: the filter parses a file only when ithas an issue in that file to filter, so a directive in a clean file is never read at all, and the
warning is printed by a run that exits 0 either way. Planting
//nolint:notarealinteron a cleanfile produces no warning and
0 issues.scripts/check-nolint-linters.shcross-checks the names in every//nolintagainst the setgolangci-lint reports as enabled, and the gate runs it as one more step after
lint. The enabledset comes from
golangci-lint linters --jsonplusgolangci-lint formatters --json, not from acopy of
.golangci.yml, so enabling or dropping a linter never needs an edit in the script.Formatters are in the set because
runreports their findings under their own name (File is not properly formatted (gci)), which makes//nolint:gcia real directive.Both list calls run in the repo the script lives in, not in whatever directory the caller stood in.
golangci-lint discovers
.golangci.ymlrelative to its own working directory, so an unanchoredcall would answer for a config with nothing to do with the tree
git grepis reading.scripts/verify-nolint-grammar.shdrives the check over every grammar form and mutates it to proveeach case bites; the gate runs it ahead of the check, the way the coverage counter sits ahead of
the coverage gate.
//nolint:forcetypeassertatpass/validate_propids_test.go:86was of exactly the kind the checkrejects:
forcetypeasserthas never been in the enabled set here, so it never suppressed anythingfrom the day it was written. #313 has since made that same edit on
mainfor its own reasons,so the line no longer appears in this diff.
Grammar, and where the check deliberately differs from golangci-lint
The parsing mirrors
extractInlineRangeFromComment: strip leading/and spaces (so// nolint:xcounts), require
nolintfollowed by a space, a colon, or the end, cut a trailing// reason,split on commas, trim and lower-case each name.
//nolint:a,b,cis checked name by name.A directive that names no linter fails. It has no name to cross-check, which also makes it the one
way to write a suppression this check cannot see through, so it fails rather than passing silently.
The failure says which kind it is, because the two are opposites and the gate is the only thing
that will tell a reader them apart:
//nolint,//nolint:all//nolint:allfoo,//nolint:ALL//nolint://nolint:errorlint,errorlintonlyThe
allrows are not a typo.extractInlineRangeFromCommenttestsHasPrefix(text, "nolint:all")on the raw text before it splits, and compares the split-out names lower-cased — so
//nolint:allfooblankets,//nolint:AllFoois merely an unknown name, and//nolint:ALLblankets by the second test after failing the first. The check mirrors all three.
One difference is deliberate: every
//on a line is tried, not just the one opening thecomment. Telling those apart needs a Go parser, since
//also occurs inside string literals andinside prose quoting a directive. Trying all of them over-reports — a directive spelled inside a
string literal is reported although golangci-lint would never see it — and never under-reports,
which is the direction a check written because another check missed something has to fail in.
Block comments need no such care:
TrimLeft(text, "/ ")stops at the*, so golangci-lint ignores/*nolint:x*/too, and both agree.A trailing CR is dropped before the grammar runs, because
go/scannerdrops it before the nolintfilter reads the comment. Without that, a CRLF file leaves a bare
//nolintreading asnolint\r,which matches no anchor — measured, golangci-lint honours that directive and the finding under it
disappears, so missing it would be the one under-report the design does not allow. The gate's
gofmtstep rejects a CRLF file first, but this check does not lean on a step above it.git grep -noutput is split on its first two colons, which a path holding one would derail. Sucha tree is refused rather than misparsed: the split would keep the file and silently drop the line
number.
Test plan
Verified against golangci-lint 2.12.2 / go1.26.4, the version the gate pins.
Ground truth first. A throwaway module with a real
errorlintviolation, measuring what eachdirective actually suppresses rather than what the source suggests it should:
errorlintstill reported?nolintlintsays//nolint:errorlint // r//nolint:notarealinter//nolint:forcetypeassert//nolint://nolint:errorlint,//nolint:allfoo/*nolint:errorlint*/The three bold rows are what this PR exists for: a directive that suppresses nothing, reported by
nothing.
//nolint:is reported by nothing at all, not even a format complaint.The check, against the same forms.
scripts/verify-nolint-grammar.shis that table plus theconfiguration shapes, run as 24 cases against fixture repos with a known enabled set, each followed
by a mutation that must turn it red:
Confirming the verifier cannot pass vacuously. Four ways a case could look like coverage
without being any, each planted and each caught:
main→mian)FAIL: disabled: no fixture repo named mian was builtFAIL: no-cap: names a case the table does not carryFAIL: no-cap: its anchor appears 0 times ..., want 1FAIL: no-lowercase: its anchor appears 5 times ..., want 1Confirming the enabled set is derived, not effectively hardcoded. Editing
.golangci.ymlmovesthe verdict on directives the script never mentions: dropping
errorlintfromenablereddens thelive
//nolint:errorlint, and addingforcetypeassertturns a planted one green.The branch as it stands, with every remaining directive naming an enabled linter:
The rest of the gate, run in order:
gofmtclean,go vet ./...clean,golangci-lint run0 issues., both verifiersall checks passed,go build ./...clean, and./scripts/check-coverage.shall 6132 statements covered.Scope
CLAUDE.mdandREADME.mdeach introduce their command block as the checks CI'sgatejob runs,so both gain the two new steps. README's three older disagreements with the gate —
gofmt -l .,a
go test ./...step the gate does not have, and a per-package coverage claim the script does notmake — are not this change's and are filed as #415.
.golangci.yml's note on the nolintlint gap pointed at #306, which this closes; it now points atthe script instead.
Closes #306