Summary
timenowsub only simplifies time.Now().Sub(t) → time.Since(t) when the argument t is an *ast.Ident or a parenthesized ident. Its isSafeSinceArg guard (pkg/linters/timenowsub/timenowsub.go:131-140) rejects every other expression form, including side-effect-free ones such as selector expressions (s.start, p.startTime, pkg.Deadline). These are the most common real-world shapes for a stored start time, so the linter misses them — a false negative from an over-narrow pattern set (same class as #40581 lenstringzero and #40244 errstringmatch).
Evidence
func isSafeSinceArg(expr ast.Expr) bool {
switch e := expr.(type) {
case *ast.Ident:
return true
case *ast.ParenExpr:
return isSafeSinceArg(e.X)
default:
return false // <- selectors, index exprs all fall here
}
}The testdata goodCallExprArg (time.Now().Sub(loadStart())) documents the intent: exclude args whose evaluation has side effects or could observe the clock, because time.Since(t) evaluates t before the internal Now(), whereas time.Now().Sub(t) evaluates the receiver Now() first. Excluding calls is correct. But the guard also excludes selector/index reads, which are pure.
Why selector args are safe to simplify
time.Since(t) is defined as return Now().Sub(t). The only behavioral difference between time.Now().Sub(x.field) and time.Since(x.field) is the relative evaluation order of x.field and time.Now(). A field/selector read (x.field, p.field, pkg.Var) has no side effects and cannot influence or observe the monotonic clock, so reordering it around a side-effect-free Now() is unobservable. (A nil-pointer deref would panic identically in both orderings, since Now() has no side effects.) Therefore *ast.SelectorExpr is unambiguously safe to accept.
Impact
Latent / low: this is a non-CI-enforced simplification linter, and there are currently zero time.Now().Sub( sites in pkg/ (verified — so no immediate production miss). But the guard silently caps coverage below the linter's own stated purpose, so real future time.Now().Sub(s.start) idioms — the canonical elapsed-time pattern — will go unflagged.
Recommendation
Extend isSafeSinceArg to accept *ast.SelectorExpr (field/package-var reads). Optionally also *ast.IndexExpr when the index is itself side-effect-free (e.g. an ident/const), though selector coverage captures the common case:
case *ast.SelectorExpr:
return true // field / pkg-var read: pure, evaluation-order-invariantAdd a bad-case in testdata (time.Now().Sub(s.start) with golden time.Since(s.start)), keeping the existing goodCallExprArg exclusion for calls.
Validation checklist
Effort
Small (one case arm + one testdata pair + golden line).
Sergo R61 — audit of the two post-R60 linters lacking prior issues. timenowsub autofix, type-resolution (ObjectOf/PkgName), nolint+generated-file wiring, and RWSF verification are otherwise clean; appendoneelement audited fully clean (autofix provably compiles: the spread literal's element type must equal the slice element type, so append(s, x) is always well-typed).
Generated by 🤖 Sergo - Serena Go Expert · 243.2 AIC · ⌖ 11 AIC · ⊞ 5.8K · ◷
Summary
timenowsubonly simplifiestime.Now().Sub(t)→time.Since(t)when the argumenttis an*ast.Identor a parenthesized ident. ItsisSafeSinceArgguard (pkg/linters/timenowsub/timenowsub.go:131-140) rejects every other expression form, including side-effect-free ones such as selector expressions (s.start,p.startTime,pkg.Deadline). These are the most common real-world shapes for a stored start time, so the linter misses them — a false negative from an over-narrow pattern set (same class as #40581 lenstringzero and #40244 errstringmatch).Evidence
The testdata
goodCallExprArg(time.Now().Sub(loadStart())) documents the intent: exclude args whose evaluation has side effects or could observe the clock, becausetime.Since(t)evaluatestbefore the internalNow(), whereastime.Now().Sub(t)evaluates the receiverNow()first. Excluding calls is correct. But the guard also excludes selector/index reads, which are pure.Why selector args are safe to simplify
time.Since(t)is defined asreturn Now().Sub(t). The only behavioral difference betweentime.Now().Sub(x.field)andtime.Since(x.field)is the relative evaluation order ofx.fieldandtime.Now(). A field/selector read (x.field,p.field,pkg.Var) has no side effects and cannot influence or observe the monotonic clock, so reordering it around a side-effect-freeNow()is unobservable. (A nil-pointer deref would panic identically in both orderings, sinceNow()has no side effects.) Therefore*ast.SelectorExpris unambiguously safe to accept.Impact
Latent / low: this is a non-CI-enforced simplification linter, and there are currently zero
time.Now().Sub(sites inpkg/(verified — so no immediate production miss). But the guard silently caps coverage below the linter's own stated purpose, so real futuretime.Now().Sub(s.start)idioms — the canonical elapsed-time pattern — will go unflagged.Recommendation
Extend
isSafeSinceArgto accept*ast.SelectorExpr(field/package-var reads). Optionally also*ast.IndexExprwhen the index is itself side-effect-free (e.g. an ident/const), though selector coverage captures the common case:Add a
bad-case in testdata (time.Now().Sub(s.start)with goldentime.Since(s.start)), keeping the existinggoodCallExprArgexclusion for calls.Validation checklist
time.Now().Sub(x.field)is flagged and autofixed totime.Since(x.field)time.Now().Sub(f())remains unflagged (call side effects preserved)bad/golden pair added;RunWithSuggestedFixesstill greentimequalifier/alias (already correct viaqualifier)Effort
Small (one case arm + one testdata pair + golden line).
Sergo R61 — audit of the two post-R60 linters lacking prior issues.
timenowsubautofix, type-resolution (ObjectOf/PkgName), nolint+generated-file wiring, and RWSF verification are otherwise clean;appendoneelementaudited fully clean (autofix provably compiles: the spread literal's element type must equal the slice element type, soappend(s, x)is always well-typed).