build: check that every //nolint names an enabled linter - #367
Open
OmarAlJarrah wants to merge 1 commit into
Open
build: check that every //nolint names an enabled linter#367OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
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.
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
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.The gate now names one golangci-lint version for both steps. The check asks golangci-lint which
linters are enabled, so its answer only describes the run above it if the two are the same build;
pinning also means a golangci-lint release can no longer redden
mainwithout a commit.//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. It goes, keeping its justification as a plain comment. #313 makes the
identical edit to that line for its own reasons, so the two merge without conflict in either order.
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.Two choices are deliberate and are written down at the code:
//nolint,//nolint:all,//nolint:, and astray trailing comma all resolve to "suppress every enabled linter". There is 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. (
nolintlint'srequire-specificcovers the sameground when it is enabled; this check does not depend on that.)
//on a line is tried, not just the one opening the comment. Telling those apart needsa Go parser, since
//also occurs inside string literals and inside 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.
Test plan
Verified against golangci-lint 2.12.2 / go1.26.4, the version the gate now pins.
Each form was planted on
pass/validate_propids_test.go:86and the file restored afterwards.1.
//nolint:forcetypeassert— a real linter that is not enabled2.
//nolint:notarealinter— no such linter3.
//nolint:unparam— an enabled linter, which must not be flagged4.
//nolintnaming nothingThe branch as it stands, with the six remaining directives all naming enabled linters:
Confirming the hole is real, not inferred from the source. With a cleaned lint cache, both
forms planted at the same line leave
golangci-lint rungreen:pass/validate_propids_test.go:86golangci-lint run//nolint:forcetypeassert0 issues., exit 0//nolint:notarealinter0 issues., exit 0No
Found unknown linterswarning appeared for either, consistent with the filter never parsing afile that has no issue in it.
Confirming the enabled set is derived, not effectively hardcoded. Editing
.golangci.ymlmovesthe verdict on directives the script never mentions:
.golangci.ymlediterrorlintfromenableload.go:482andload.go:508, the two live//nolint:errorlintforcetypeasserttoenable, planted directive restoredGrammar cases, each planted at the same line,
[R]= rejected,[A]= accepted://nolint:errorlint,forcetypeassert[R]namesforcetypeassert// nolint:notarealinter(leading space)[R]//nolint:ForceTypeAssert(mixed case)[R], lower-cased first//nolint: forcetypeassert(space after the colon)[R]//nolint:and//nolint:errorlint,[R]names no linter//nolint:all[R]names no linter//nolint:errorlint // see //nolint:bogus[R]onbogus, the documented over-report//nolintfoo:bar[A]not a directive, matching^nolint( |:|$)//nolint:gci(enabled formatter)[A]//nolint:errorlint[A]Fixing a defect the cases found.
//nolint:first passed: awk'ssplitof the empty stringyields no fields, so the name loop never ran. Fixed, and re-planting it now reddens (row above).
The CI step's
runblock was executed verbatim locally (with a darwin build in place of linux),proving the install line and the
PATHit hands the script both work — the action installsgolangci-lint at a path of its own and does not put it on
PATH, so the step installs the pinnedversion for itself:
The rest of the gate, with
golangci-lint cache cleanfirst:gofmtclean,go vet ./...clean,
golangci-lint run0 issues.,go build ./...clean,./scripts/check-coverage.shall 4942 statements covered.On the runner, where the version pin, the
envreference inwith:, and the presence ofjqare all things a local run cannot establish. The step takes about a second:
Scope
CLAUDE.md's list of gate commands gains the new one; it is written as "the same checks CI'sgatejob runs, in that order", so leaving it at five would make it wrong.
Not done here: the script assumes no tracked path contains a colon, since it splits
git grep -noutput on the first two. A path that did would be reported as an unparsable line rather than
skipped.
Closes #306