Uh oh!
There was an error while loading. Please reload this page.
[linter-miner] feat(linters): add sortslice linter to flag sort.Slice/sort.SliceStable - #37888
Conversation
The sortslice linter flags calls to sort.Slice and sort.SliceStable, suggesting the type-safe slices.SortFunc and slices.SortStableFunc from the standard library slices package (available since Go 1.21). sort.Slice accepts func(i, j int) bool — the index-based callback gives no compile-time guarantee that i and j are valid indices, and the comparison parameters are untyped integers rather than elements of the slice being sorted. slices.SortFunc accepts func(a, b E) int — the callback receives the actual elements, is type-checked at compile time, and follows the modern cmp.Compare convention (negative/zero/positive). Evidence from code scanning: 47 sort.Slice and 6 sort.SliceStable calls were found in non-test Go files under pkg/ and cmd/ — every one of them is a candidate for migration to the type-safe alternative. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🧪 Test Quality Sentinel completed test quality analysis. |
✅ PR Code Quality Reviewer completed the code quality review. |
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Go analysis linter (sortslice) to the gh-aw linter suite to flag sort.Slice / sort.SliceStable usage and steer callers toward the type-safe slices.SortFunc / slices.SortStableFunc alternatives, and wires it into the cmd/linters multichecker.
Changes:
- Introduces
pkg/linters/sortsliceanalyzer that reportssort.Sliceandsort.SliceStablecall sites (withnolintsupport and test-file skipping). - Adds
analysistestcoverage and fixtures for expected diagnostics. - Registers the new analyzer in
cmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/sortslice/sortslice.go | New analyzer implementation that detects sort.Slice / sort.SliceStable calls. |
pkg/linters/sortslice/sortslice_test.go | Runs the analyzer with analysistest against testdata. |
pkg/linters/sortslice/testdata/src/sortslice/sortslice.go | Test fixtures with // want expectations for flagged and allowed cases. |
cmd/linters/main.go | Adds sortslice.Analyzer to the multichecker registration. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok { | ||
| return | ||
| } | ||
| pkgIdent, ok := sel.X.(*ast.Ident) | ||
| if !ok || pkgIdent.Name != "sort" { | ||
| return |
| rawloginlib.Analyzer, | ||
| regexpcompileinfunction.Analyzer, | ||
| ssljson.Analyzer, | ||
| seenmapbool.Analyzer, | ||
| sortslice.Analyzer, | ||
| strconvparseignorederror.Analyzer, |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (103 new lines under 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you for documenting why a custom in-house linter was chosen over an upstream equivalent. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /grill-with-docs — requesting changes on one correctness issue and missing test coverage.
📋 Key Themes & Highlights
Key Themes
- Correctness gap (blocking): Package identity is resolved by AST name (
pkgIdent.Name == "sort") rather than type resolution. This silently misses aliased imports (import s "sort") and can falsely flag unrelated local packages. The fix is a one-line change usingpass.TypesInfo.Uses— see inline comment. - Thin test coverage: The fixture covers only two positive cases. The
nolintbypass, test-file exclusion, and aliased-import paths are wired in the implementation but have zero fixture coverage. - Incomplete API surface:
sort.SliceIsSorteduses the same untyped index callback andslices.IsSortedFuncis available since Go 1.21 — worth flagging for consistency. - Minor hot-path ordering:
pos+filecheck.IsTestFileare computed for everyCallExprrather than only after confirming it is asort.*call.
Positive Highlights
- ✅ Follows the established
pkg/linters/structure faithfully — analysistest harness,nolintsupport, test-file exclusion,filecheck, correctAnalyzermetadata. - ✅ Clear, accurate diagnostic messages pointing at the right alternative.
- ✅ The PR body provides strong motivation with concrete evidence (47 + 6 call sites found).
- ✅ Clean registration in
cmd/linters/main.go.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 216.7 AIC · ⌖ 13.5 AIC
| return | ||
| } | ||
| pkgIdent, ok := sel.X.(*ast.Ident) | ||
| if !ok || pkgIdent.Name != "sort" { |
There was a problem hiding this comment.
[/tdd] Package identity is checked by AST name only (pkgIdent.Name != "sort"), which silently misses aliased imports and can produce false positives on local packages named sort.
This is a correctness gap: import s "sort" → s.Slice(...) is never flagged; a local package named "sort" with a Slice method is falsely flagged. The established pattern in this codebase (e.g. strconvparseignorederror) uses type resolution to avoid both issues.
💡 Suggested fix using type resolution
Replace the pkgIdent.Name guard with:
import (
"go/types"...
)
ifident, ok:=sel.X.(*ast.Ident); ok {
obj:=pass.TypesInfo.Uses[ident]
pkgName, ok:=obj.(*types.PkgName)
if!ok||pkgName.Imported().Path() !="sort" {
return
}
// fall through to switch on sel.Sel.Name
}This correctly flags import s "sort"; s.Slice(...) and ignores local sort packages.
| func GoodSortInts(items []int) { | ||
| sort.Ints(items) | ||
| } |
There was a problem hiding this comment.
[/tdd] The test fixture covers only the happy path. Three code paths that exist in the implementation have no test coverage: nolint directive bypass, test-file exclusion, and aliased/false-positive imports.
💡 Suggested additional fixture cases
Add these cases to exercise the escape hatches:
// Verify nolint directive suppresses the diagnosticfuncNolintSlice(items []string) {
sort.Slice(items, func(i, jint) bool { returnitems[i] <items[j] }) (nolint/redacted):sortslice
}
// Verify other sort functions are not flaggedfuncGoodSliceIs(items []string) bool {
returnsort.SliceIsSorted(items, func(i, jint) bool { returnitems[i] <items[j] })
}And add a separate *_test.go fixture file to confirm that sort.Slice in test files is not flagged.
| pos := pass.Fset.PositionFor(call.Pos(), false) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| return |
There was a problem hiding this comment.
[/zoom-out] The pos computation and filecheck.IsTestFile / nolint.HasDirective checks happen for everyCallExpr in the package — even fmt.Println(...) — not just potential sort.* calls. Move these checks after confirming the selector is sort.Slice or sort.SliceStable to avoid redundant work on the hot path.
💡 Suggested reordering
insp.Preorder(nodeFilter, func(n ast.Node) {
call, ok:=n.(*ast.CallExpr)
if!ok {
return
}
sel, ok:=call.Fun.(*ast.SelectorExpr)
if!ok {
return
}
// type-resolve first (cheap map lookup)ident, ok:=sel.X.(*ast.Ident)
if!ok {
return
}
obj:=pass.TypesInfo.Uses[ident]
pkgName, ok:=obj.(*types.PkgName)
if!ok||pkgName.Imported().Path() !="sort" {
return
}
switchsel.Sel.Name {
case"Slice", "SliceStable":
// only now pay for position + nolint lookuppos:=pass.Fset.PositionFor(call.Pos(), false)
iffilecheck.IsTestFile(pos.Filename) ||nolint.HasDirective(pos, noLintLinesByFile) {
return
}
...
}
})| case "Slice": | ||
| pass.ReportRangef(call, "sort.Slice is not type-safe; use slices.SortFunc instead") | ||
| case "SliceStable": | ||
| pass.ReportRangef(call, "sort.SliceStable is not type-safe; use slices.SortStableFunc instead") |
There was a problem hiding this comment.
[/grill-with-docs] Consider also flagging sort.SliceIsSorted — it uses the same index-based func(i, j int) bool signature and the type-safe counterpart slices.IsSortedFunc has been available since Go 1.21.
Leaving sort.SliceIsSorted unflagged creates an inconsistency: the linter guides callers toward slices.SortFunc but leaves them free to check sorted order with the equally untyped sort.SliceIsSorted.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The TestSortSlice function uses analysistest.Run with // want annotations, covering both positive (violations flagged) and negative (good code not flagged) cases. Build tag is present, no guideline violations detected.
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness/completeness bugs must be fixed before merge.
### Blocking issues summary
1. Package identity checked by identifier name only (sortslice.go:55) — critical
The linter uses pkgIdent.Name != "sort" (a syntactic string match) instead of pass.TypesInfo.Uses[pkgIdent].(*types.PkgName).Imported().Path() == "sort". This is inconsistent with every other package-qualified-call checker in this repo (e.g. strconvparseignorederror) and produces silent false negatives for aliased imports (import s "sort"; s.Slice(...)).
2. sort.SliceIsSorted not handled (sortslice.go:59–64) — high
The switch only covers Slice and SliceStable. sort.SliceIsSorted has identical type-unsafety and a direct type-safe replacement (slices.IsSortedFunc), but is silently ignored.
Non-blocking (clean up before merge)
- Dead-code
!okguard after a guaranteed type assertion (sortslice.go:37–40). - Testdata lacks coverage for aliased imports,
nolintsuppression, andSliceIsSorted.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 13.6 AIC
| return | ||
| } | ||
| pkgIdent, ok := sel.X.(*ast.Ident) | ||
| if !ok || pkgIdent.Name != "sort" { |
There was a problem hiding this comment.
Package identity is verified by identifier name only — false negatives and false positives guaranteed.
💡 Suggested fix
The check pkgIdent.Name != "sort" matches any identifier whose syntactic name is sort, regardless of what package it actually refers to. This means:
- False negative:
import s "sort"; s.Slice(...)will not be flagged. - False positive (unlikely but possible): a third-party package imported without an alias that happens to expose a
.Slice()method would be incorrectly flagged.
Every sibling linter in this repo that checks a package-qualified call resolves the package via type information. For example, strconvparseignorederror does:
obj:=pass.TypesInfo.Uses[ident]
ifpkgName, ok:=obj.(*types.PkgName); ok {
ifpkgName.Imported().Path() =="sort" {
// safe to flag
}
}Apply the same pattern here:
import"go/types"// add to importspkgIdent, ok:=sel.X.(*ast.Ident)
if!ok {
return
}
obj:=pass.TypesInfo.Uses[pkgIdent]
pkgName, ok:=obj.(*types.PkgName)
if!ok||pkgName.Imported().Path() !="sort" {
return
}This also requires adding "go/types" to the import block.
| switch sel.Sel.Name { | ||
| case "Slice": | ||
| pass.ReportRangef(call, "sort.Slice is not type-safe; use slices.SortFunc instead") | ||
| case "SliceStable": | ||
| pass.ReportRangef(call, "sort.SliceStable is not type-safe; use slices.SortStableFunc instead") | ||
| } |
There was a problem hiding this comment.
sort.SliceIsSorted is missing from the switch — the linter silently ignores a third type-unsafe API.
💡 Suggested fix
sort.SliceIsSorted(x interface{}, less func(i, j int) bool) bool has the same type-unsafety problem as sort.Slice: it takes an untyped interface{} slice and an index-based comparator. Its type-safe replacement is slices.IsSortedFunc. The linter doc string claims to flag calls "that should use the type-safe slices.SortFunc or slices.SortStableFunc" — omitting SliceIsSorted is an incomplete implementation that will leave real code unflagged.
Add to the switch:
case"SliceIsSorted":
pass.ReportRangef(call, "sort.SliceIsSorted is not type-safe; use slices.IsSortedFunc instead")Also add a corresponding // want test case in the testdata file.
| call, ok := n.(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| } |
There was a problem hiding this comment.
Dead code: the !ok branch can never execute.
💡 Suggested fix
nodeFilter is []ast.Node{(*ast.CallExpr)(nil)}, so insp.Preorder only ever calls this closure with a *ast.CallExpr. The type assertion on line 37 always succeeds; the if !ok { return } guard on lines 38–40 is unreachable and misleads readers into thinking a non-CallExpr could arrive here.
Either use a direct (panicking) assertion to make the invariant explicit:
call:=n.(*ast.CallExpr)or drop the assignment entirely and inline the cast where it is used. Remove the guard.
| func GoodSortInts(items []int) { | ||
| sort.Ints(items) | ||
| } |
There was a problem hiding this comment.
Testdata is missing coverage for aliased imports, nolint suppression, and sort.SliceIsSorted.
💡 What to add
Three gaps leave important behaviour completely unverified:
1. Aliased import (false-negative regression test)
With the current string-name-only package check, import s "sort"; s.Slice(...) is silently missed. Add a case to surface this:
import s "sort"funcBadSliceViaAlias(items []string) {
s.Slice(items, func(i, jint) bool { returnitems[i] <items[j] }) // want `sort\.Slice is not type-safe`
}Once the package-identity bug (see sortslice.go:55) is fixed using pass.TypesInfo.Uses, this // want annotation will pass.
2. nolint suppression
The linter builds a nolint index and checks it on every node, but that code path has zero test coverage:
funcSuppressedSlice(items []string) {
sort.Slice(items, func(i, jint) bool { returnitems[i] <items[j] }) (nolint/redacted):sortslice
}(No // want annotation — the diagnostic must not fire here.)
3. sort.SliceIsSorted
funcBadSliceIsSorted(items []string) {
_=sort.SliceIsSorted(items, func(i, jint) bool { returnitems[i] <items[j] }) // want `sort\.SliceIsSorted is not type-safe`
}pelikhan
commented
Jun 8, 2026
@copilot run pr-finisher skill |
Uh oh!
There was an error while loading. Please reload this page.
PR: feat(linters): add
sortslicelinter flaggingsort.Slice/sort.SliceStableSummary
Adds a new single-responsibility
golang.org/x/tools/go/analysispass,sortslice,that flags calls to
sort.Sliceandsort.SliceStableand recommends the type-safeslices.SortFunc/slices.SortStableFuncalternatives introduced in Go 1.21. Thelinter is registered in the central
multicheckerentry-point and ships withanalysistest-based fixtures and a Draft ADR.Motivation (from ADR-37888): The module targets Go 1.26.3 and a codebase scan surfaced
47
sort.Sliceand 6sort.SliceStablecalls in non-test source. This PR addsautomated prevention of new occurrences; remediation of existing calls is out of scope.
Changed Files
cmd/linters/main.gopkg/linters/sortsliceand appendssortslice.Analyzerto themultichecker.Maincallpkg/linters/sortslice/sortslice.gopkg/linters/sortslice/sortslice_test.goanalysistest-based unit testpkg/linters/sortslice/testdata/src/sortslice/sortslice.go// wantannotationsdocs/adr/37888-add-sortslice-linter.mdImplementation Detail —
sortslice.go*ast.CallExprandmatches selector expressions where the package identifier literal is
"sort"andthe method name is
"Slice"or"SliceStable". It does not resolve thesortidentifier throughpass.TypesInfo; locally-shadowedsortnames are falsepositives, and aliased imports are false negatives.
filecheck.IsTestFile) and lines carrying a// nolint:sortslicedirective (sharednolintindex) are skipped.sort.Slice→"sort.Slice is not type-safe; use slices.SortFunc instead"sort.SliceStable→"sort.SliceStable is not type-safe; use slices.SortStableFunc instead"inspect.Analyzer(same resource profile as sibling linters).Test Coverage —
sortslice_test.go/ fixtureBadSlicesort.Slice(...)// want \sort.Slice is not type-safe``BadSliceStablesort.SliceStable(...)// want \sort.SliceStable is not type-safe``GoodSortStringssort.Strings(...)GoodSortIntssort.Ints(...)Test build tag:
//go:build !integration.ADR Status
docs/adr/37888-add-sortslice-linter.mdis Draft. The file header explicitlystates it was generated by the Design Decision Gate workflow and must be reviewed,
completed, and finalised by the PR author before merge.
Key open item called out in the ADR:
Breaking Changes
None. The analyzer is additive; it emits diagnostics but does not auto-fix.
Existing
sort.Slice/sort.SliceStablecalls in the codebase (47 + 6) willtrigger failures in CI lint runs once this linter is enforced — remediation is a
separate workstream.
Checklist for Downstream Agents
sortslice.Analyzeris wired into CI lint invocation (not just registered inmain.go).go build ./...).// nolintsuppression strategy is needed to unblock CI in the interim.pass.TypesInfo.ObjectOf(pkgIdent)) to eliminate false-positive and false-negative edge cases with aliased or shadowedsortidentifiers.