[linter-miner] feat(linters): add timenowsub linter — flag time.Now().Sub(t) → time.Since(t)#46633
Conversation
Flags time.Now().Sub(t) calls that can be simplified to time.Since(t). The time.Since function is the idiomatic Go way to compute the elapsed time since a fixed point and avoids the more verbose two-step pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds the timenowsub Go analyzer to replace time.Now().Sub(t) with time.Since(t).
Changes:
- Implements and registers the analyzer.
- Provides automatic suggested fixes.
- Adds analyzer tests and golden output.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/timenowsub/timenowsub.go |
Implements detection and fixes. |
pkg/linters/timenowsub/timenowsub_test.go |
Runs analyzer fixture tests. |
pkg/linters/timenowsub/testdata/src/timenowsub/timenowsub.go |
Defines test cases. |
pkg/linters/timenowsub/testdata/src/timenowsub/timenowsub.go.golden |
Defines expected fixes. |
cmd/linters/main.go |
Registers the analyzer. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/linters/timenowsub/timenowsub.go:91
- The fix hard-codes the qualifier
time, butisTimeNowalso recognizes aliased standard-library imports. For example,clock.Now().Sub(t)is diagnosed and rewritten totime.Since(t), which does not compile when the package is imported only asclock. Build the replacement from the qualifier in the inner selector (and add an aliased-import golden case).
NewText: []byte(fmt.Sprintf("time.Since(%s)", argText)),
pkg/linters/timenowsub/timenowsub.go:117
types.Func.FullNameuses the declaring package's name, not its import path, so a third-party package declared aspackage timewith aNowfunction is also treated as the standard library and may receive an invalidtime.Sincerewrite. Matchfn.Pkg().Path() == "time"and the function name instead, as the other standard-library linters do (for exampleinternal/astutil/astutil.go:148-150).
fn, ok := obj.(*types.Func)
if !ok {
return false
}
return fn.FullName() == "time.Now"
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
| if len(outer.Args) != 1 { | ||
| return |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — approving with no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Pattern consistency: Implementation follows the established linter structure (astutil, filecheck, nolint) — no deviations.
- Type-safe detection: Uses
fn.FullName() == "time.Now"rather than string matching on selector names, making it robust against shadowing. - Test coverage: Covers the expected positive/negative cases (
bad,badAssign,good,goodOtherSub). The golden file correctly verifies the suggested fix rewrite.
Positive Highlights
- ✅ Proper type checking via
TypesInfo.ObjectOf— won't fire on user-definedNow()methods - ✅
SuggestedFixincluded — enables automatic rewriting via gopls /go fix - ✅ Correctly skips generated files and honours
//nolint:timenowsubdirectives - ✅ Registered in
cmd/linters/main.goin alphabetical order
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 18.9 AIC · ⌖ 4.41 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review: timenowsub linter
The implementation is clean and follows established patterns in this repo:
- Correctness:
isTimeNowuses type-aware checking (fn.FullName()) — no false positives from user-definedNow()methods. - SuggestedFix: Properly replaces the full
time.Now().Sub(x)call expression withtime.Since(x). - Test coverage: Covers bad, good, and non-
time.Now()receiver cases; golden file usesRunWithSuggestedFixes. - Registration: Correctly inserted in alphabetical order in
cmd/linters/main.go. - nolint / filecheck / generated-file skipping: Properly wired.
LGTM
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17 AIC · ⌖ 4.36 AIC · ⊞ 5K
There was a problem hiding this comment.
LGTM — minor nit on golden file comments
The linter implementation is correct: type-verified detection via types.Func.FullName() == "time.Now", proper nolint/generated-file skipping, and a complete SuggestedFix with the right span (outer.Pos()→outer.End()).
🔎 One nit (non-blocking)
pkg/linters/timenowsub/testdata/src/timenowsub/timenowsub.go.golden lines 6 and 10 carry stale // want annotations copied from the source input file:
\_ = time.Since(t) // want `time\.Now\(\)\.Sub\(t\) can be simplified ...`analysistest.RunWithSuggestedFixes does a byte-for-byte comparison — it does not re-evaluate // want directives in golden files — so tests pass. But the comments are misleading: a future reader may assume these are live expectations on the fixed output. Drop them from the golden file.
🔎 Code quality review by PR Code Quality Reviewer · 26.5 AIC · ⌖ 4.41 AIC · ⊞ 5.6K
Comment /review to run again
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (172 new lines in 📄 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the merge/recompile flow. |
|
@copilot run pr-finisher skill |
…ub-95d856cd71fa7907 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR-finisher pass. Addressed the remaining timenowsub correctness issue in |
|
@copilot please run the Open review signals:
Please summarize the next concrete maintainer action after updates are pushed. Run: https://github.com/github/gh-aw/actions/runs/29702050635 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
test body