Repository navigation
feat(linters): add readerstringconv analyzer - #71891
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
Static review found no blocking issues in the diagnostic-only analyzer, regression fixtures, or registration and documentation updates.
No open issues 🎉
1 file not reviewed
pkg/linters/readerstringconv/testdata/src/readerstringconv/generated.go: Generated file
What changed in this PR
Adds readerstringconv to gh-aw’s custom Go linter suite to flag avoidable string/byte conversions when constructing readers.
Changes:
- Adds type-aware detection with suppression, file exclusions, and coverage gating.
- Adds regression fixtures for supported conversions and cases that should not report.
- Registers and documents the analyzer while deferring CI enforcement pending an audit; no automatic fixes are offered.
| File | Description |
|---|---|
pkg/linters/spec_test.go |
Adds the analyzer to specification coverage. |
pkg/linters/registry.go |
Registers the analyzer in the shared list. |
pkg/linters/README.md |
Documents behavior, safety considerations, and coverage gating. |
pkg/linters/readerstringconv/testdata/src/readerstringconv/readerstringconv.go |
Covers conversions, parentheses, aliases, shadowing, and suppression. |
pkg/linters/readerstringconv/testdata/src/readerstringconv/readerstringconv_test.go |
Verifies test-file exclusion. |
pkg/linters/readerstringconv/testdata/src/readerstringconv/generated.go |
Verifies generated-file exclusion. |
pkg/linters/readerstringconv/testdata/src/readerstringconv/dot_import.go |
Covers dot-import detection. |
pkg/linters/readerstringconv/testdata/src/readerstringconv/aliased_import.go |
Covers aliased imports. |
pkg/linters/readerstringconv/readerstringconv.go |
Implements diagnostic-only detection and reporting gates. |
pkg/linters/readerstringconv/readerstringconv_test.go |
Adds the analyzer test harness. |
pkg/linters/doc.go |
Updates the analyzer list and count. |
pkg/linters/doc_sync_test.go |
Records deferred CI enforcement. |
Review effort: Max
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
No actionable correctness, security, performance, or maintainability issues were identified in the changed lines. The analyzer's type-aware matching, suppression handling, generated-file filtering, coverage gating, registry integration, and focused test fixtures are consistent with the surrounding linter infrastructure.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~2 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (211 new lines in 📄 Draft ADR committed: The draft records the main design choice:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd to this new readerstringconv analyzer. The implementation is clean, follows the established analyzer pattern closely (mirrors stringbytesroundtrip/writebytestring), and the test coverage is thorough — aliased/dot imports, generated-file and test-file exclusions, (nolint/redacted) suppression (including :all and unrelated-linter directives), shadowed packages/conversions, named/alias types, and mutable-alias non-resolution are all exercised. Doc/spec sync (README.md, doc.go, spec_test.go, doc_sync_test.go) is correctly updated, and notYetEnforced gating with a clear audit rationale is a good call given the stated "diagnostic-only" safety caveat.
📋 Key Themes & Highlights
Key Themes
- Minor DRY nit: one inline comment suggests reusing
astutil.IsByteSliceConversion(and optionally adding a symmetricIsStringConversionhelper) instead of the manualIsType()check — non-blocking.
Positive Highlights
- ✅ Correctly handles parenthesized conversions, local alias resolution, and import aliasing via existing
astutilhelpers - ✅ Honors
(nolint/redacted), generated-file, and test-file exclusions consistent with sibling linters - ✅ Table-driven
wantassertions cover edge cases well (named types, type aliases, shadowing) - ✅ CI enforcement intentionally deferred with a documented rationale in
notYetEnforced
All local tests (readerstringconv, doc/spec sync) pass.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~43 AIC · ⌖ ~15 AIC · ⊞ 10.2K
Comment /matt to run again
| conv, ok := astutil.UnwrapParenExpr(call.Args[0]).(*ast.CallExpr) //nolint:uncheckedsliceindex // len(call.Args) == 1 is checked above. | ||
| if !ok || len(conv.Args) != 1 { | ||
| return | ||
| } |
There was a problem hiding this comment.
[/codebase-design] Nice implementation overall. Minor consistency nit: astutil.IsByteSliceConversion(pass, conv) already combines the IsType() check with IsByteSlice, and is used by writebytestring/appendbytestring. You could reuse it for the bytes.NewReader([]byte(s)) branch instead of re-deriving the IsType() check manually at line 45, and consider adding a symmetric astutil.IsStringConversion helper for the strings.NewReader(string(b)) branch so both directions follow the same pattern.
💡 Why this matters
This is purely a readability/DRY suggestion, not a correctness issue — the manual IsType() check at line 45 is functionally equivalent. Keeping conversion-detection logic centralized in astutil makes it easier for future linters (and reviewers) to reuse the same vocabulary rather than re-deriving it ad hoc.
@copilot please address this.
There was a problem hiding this comment.
Updated readerstringconv to use astutil.IsByteSliceConversion and the new symmetric astutil.IsStringConversion; both helpers now handle parenthesized conversion types. Added focused helper regression coverage. The focused tests, build, lint, and final progress gate passed. Commit: 0a88c84.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 231d1a2
|
…eaderstringconv Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Merged the latest |
|
🎉 This pull request is included in a new release. Release: |
Adds
readerstringconvto flag string/byte conversions used only to select a reader type, avoiding unnecessary copies.linters.All(), synchronizes documentation and spec coverage, and adds focused regression cases. CI enforcement remains deferred pending an audit.