chore: retire stale nolint directives and guard against new ones - #313
Merged
Conversation
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
Three of the seven
//nolintdirectives in the tree suppressed nothing. A directive that hasoutlived its finding is worse than no comment at all: it reads as a live constraint on the code
under it, and the gate stays green either way, so the only way to tell a real suppression from a
dead one is to delete it and look.
Deleted, each confirmed inert by deletion +
golangci-lint run(table below):pass/validate_propids_test.go:86—//nolint:forcetypeassert.forcetypeasserthas never beenin the enabled set in
.golangci.yml, so this never suppressed anything. The justification itcarried ("validDoc builds it") is still worth having, so it stays as a plain comment.
compilers/openapi/internal/load/load.go:83—//nolint:unparamonLoad.unparamdoes notcheck exported functions unless
check-exportedis set, which it is not.compilers/openapi/internal/load/load.go:482—//nolint:errorlintonjoinedParts. Theassertion below it is to an anonymous
interface{ Unwrap() []error }, whicherrorlintallows asthe standard idiom for splitting a join. Only the directive is gone; the paragraph explaining why
the code hand-asserts rather than calling
errors.Asis the real content and is kept.The four that remain all suppress a finding that reappears the moment they are removed, and all four
already state why, so none needed a rationale added.
To stop the class from re-accumulating,
nolintlintis now enabled withallow-unused: false,require-specific: true, andrequire-explanation: true— an inert, bare, or unexplained directivenow fails the gate. It does not close the hole completely, and the config says so at the setting:
allow-unusedonly reaches directives naming an enabled linter, so one naming a disabled ornonexistent linter is still reported by nothing. That is exactly the shape of the
forcetypeassertdirective above, and it is filed as #306 rather than fixed here.Separately,
applyExclusivetook two adjacent positional bools, so its call sites readapplyExclusive(c, s, true, exclusiveBoolean)with nothing at the call site saying whattruemeant. The min/max selector becomes a named
boundSidetype, which also retires theisMin boolthreaded through
reconcileBound,inclusiveIsTighter,setExclusiveFlag, andsetExclusiveBound.Behaviour-neutral; no golden churn.
Scope
This closes #88 partially, and it is worth being precise about which part. The issue's item 1 (eight
//nolint:unuseddirectives) and itslowerDiscriminatordead-parameter item were alreadydischarged by #97 — zero
nolint:unusedremain andlowerDiscriminatorno longer takes theparameter described. What was still live is
applyExclusive, fixed here, and three new directivesof the same kind the issue was opened for, also fixed here.
The issue's remaining item —
nullUnionCollapsereturning itshintparameter unchanged — isdeliberately left. That signature is where #281 is doing its work, and nothing currently guards the
hint at that site (passing a literal
"MUTANT"in its place leaves the whole suite green), somoving the parameter here would settle a naming question that belongs to #281.
Test plan
scripts/check-coverage.shand the rest of the gate pass; coverage stays at 100%.Every
//nolintin the tree was classified by execution, not by reading: delete the directive, rungolangci-lint run, restore. The cache matters — a warm cache reports0 issues.for a file whosedirective was just deleted, which is indistinguishable from a stale directive, so every run below
was preceded by
golangci-lint cache clean.pass/validate_propids_test.go:86forcetypeassert0 issues.load.go:83unparam0 issues.load.go:482errorlint0 issues.load.go:508errorlintload.go:510:15: type switch on error will fail on wrapped errorsinternal/harness/path_test.go:66staticcheckpath_test.go:66:30: SA1012: do not pass a nil Contextinternal/harness/harness_test.go:29staticcheckharness_test.go:29:21: SA1012: do not pass a nil Contextcompilers/openapi/openapi.go:70gocriticopenapi.go:70:20: appendAssign: append result not assigned to the same sliceDeleting the
unparamdirective also requires deleting the now-orphaned//line above it;removing only the directive leaves
load.go:82:1: File is not properly formatted (gci).The new
nolintlintconfig was checked by planting each defect it claims to catch and confirmingthe gate reddens:
unparamdirective, restoreddirective ... is unused for linter "unparam"errorlintdirective, restoreddirective ... is unused for linter "errorlint"//nolintshould mention specific linter//nolint:gocriticwith no explanationshould provide explanationforcetypeassertdirective, restoredFor the signature change,
mainand this branch were both built and run over a spec exercisingevery branch of the min/max split — both dialects, exclusive-tighter, inclusive-tighter, equal
magnitudes, and the wrong-form refusal. Emitted IR and the full diagnostic list are byte-identical.
That comparison is not blind: inverting the polarity of the new selector
(
if side == minBound→maxBound) makes both the IR and the diagnostics differ, and reddensTestConstraints_ExclusiveNumeric31and theconstraintsandnumeric-precisionconformance cases.Swapping the two constants' numeric values, by contrast, changes nothing — correctly, since only the
names are ever compared.
Closes #88