Repository navigation
[sergo] Sergo Report: Paren-Unwrap-Gap Perf-Linter Audit - 2026-10-31 #73413
Closed
Replies: 1 comment
|
This discussion has been marked as outdated by Sergo - Serena Go Expert. A newer discussion is available at Discussion #73698. |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Executive Summary
R113. The custom Go analyzer registry stayed stable at 85 analyzers (no new linter since R108). Confirmed, via direct file read rather than trusting PR/issue state, that R112 readerstringconv fix genuinely merged this time. Delivered one tested pull request fixing a new location-variant of the long-running
paren_unwrap_gapbug class across 4 never-before-audited coverage-gated perf linters.Tool and Registry Status
serena --helpexact match to cache).grep -c Analyzer,$ pkg/linters/registry.go).Strategy: 50/50 Cached + New
Cached (50%): Reconciled the 3 open
sergo-labeled issues. Most importantly, directly re-read all 3 cited production files (pkg/cli/secret_set_command.go,pkg/workflow/work_queue_memory.go,pkg/cli/experiments_fetch.go) rather than trusting the prior run own memory claim - confirmed all 3 now usebytes.NewReader(...)directly, so R112 PR genuinely landed on main this time (the first fully-confirmed merge in this lineage after R111 phantom success, caused by a GitHub App workflow-scope push rejection).New (50%): The
internal/coveragepackage gates 15 perf linters behind a sharedcoverage.ShouldApplythreshold wrapper. Prior runs only ever audited that shared gating logic (position-alignment, line/column blindness, suffix-match nondeterminism). This run instead audited 4 of those 15 linters own, independent AST-matching code for the first time:appendbytestring,writebytestring,bytescomparestring,stringbytesroundtrip.Findings
All 4 share the exact same established
paren_unwrap_gapshape (bare.(*ast.CallExpr)type assertion with noastutil.UnwrapParenExprcall first), but in a location never seen before in this codebase - a function-call-argument or binary-operand position, rather than the usual assignment/statement position:pkg/linters/appendbytestring/appendbytestring.go:72- missesappend(b, ([]byte(s))...).pkg/linters/writebytestring/writebytestring.go:95- missesw.Write(([]byte(s))).pkg/linters/bytescomparestring/bytescomparestring.go:223(extractByteSliceStringConv) - missesstring(a) == (string(b)).pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go:73(roundTripCalls) - missesstring(([]byte(s)))and[]byte((string(b))).stringsjoinonewas also checked and found immune: its ownCallExprnode filter re-visits the inner call directly regardless of an enclosingParenExpr, since parens do not hide nested AST nodes fromast.Inspect. The 4 buggy linters break this immunity because they do a manual type assertion on a sub-expression position outside the node-filter-driven traversal.No live production call site was found with this exact parenthesized shape; same filing bar used throughout this codebase history for latent/forward-looking fixes of this class. All 4 are presently gated behind the dormant
GH_AW_LINT_COVERAGE_PROFILEthreshold, so this closes a correctness gap ahead of eventual enforcement.Delivered Pull Request
Branch
sergo/paren-unwrap-gap-perf-linters-r113: wraps each bare assertion inastutil.UnwrapParenExpr(...), matching the idiom already used in 10+ other linters for this exact class. Added one new paren-case test per linter, with updated.goldenfixtures for the 3 usinganalysistest.RunWithSuggestedFixes.go test/go build/go version/makewere all denied outright in this sandbox run, so validation usedserena get_diagnostics_for_file(zero compile errors on all 8 touched files) andserena find_referencing_symbols(confirmed each modified helper has exactly one call site). Golden fixtures were hand-traced against an existing analogous fixture in the same package as a format reference. This is documented explicitly in the PR body as a sandbox limitation - a maintainer should still rungo test ./pkg/linters/...before merge.Reconcile
#71997(readerstringconv CI-enforcement half): still issue-only, blocked by the standingworkflow_permission_blockrule (any PR touching.github/workflows/*.ymlfails to push here).#72308(manualpathconcat): reconfirmed genuinely fixed in code; stays open on GitHub since no close tool exists in this workflow.#72309(2 linters never added to cgo.yml): still blocked, same CI-config-only constraint.Metrics
Historical Context
paren_unwrap_gapis the longest-running pattern class in sergo history, first identified at R69 and now confirmed or fixed over 20 times across dozens of individually-authored linters and 2 shared frameworks (resourcetracker,astutilhelpers). This run extends it to a new sub-shape (call-argument/operand position) rather than the usual statement position.Recommendations
go test ./pkg/linters/...to confirm the new PR before merging; this sandbox could not execute it this run.sergocould be granted workflow-write credentials for a future run, since 2 of the 3 remaining open issues (#71997,#72309) can never ship as pull requests under the current GitHub App permissions.Next Run Focus
appendoneelement,stringsconcatloop,bytesbufferstringfor the same argument-positionparen_unwrap_gapsub-shape.References:
All reactions