Give stringbytesroundtrip.isExactString its own exactness logic#54722
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (29 additions across 2 files).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable correctness, performance, or maintainability issues stood out in the changed lines.
Reviewed focus
Checked the exact-string type discrimination change and the new named/alias test coverage for changed-line regressions. The implementation now makes the raw-type vs underlying-type distinction explicit, and the added fixtures cover the important behavioral split between named string types and aliases.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.12 AIC · ⌖ 6.74 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Separates exact predeclared-string detection from underlying string-type detection.
Changes:
- Implements dedicated
isExactStringlogic and documents helper contracts. - Adds named-type and predeclared-alias fixtures.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go |
Adds exactness detection logic. |
pkg/linters/stringbytesroundtrip/testdata/src/stringbytesroundtrip/stringbytesroundtrip.go |
Adds mixed named and alias coverage. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| if _, named := t.(*types.Named); named { | ||
| return false | ||
| } | ||
| return isStringType(t.Underlying()) |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs — approving with one minor doc comment suggestion.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause properly addressed:
isExactStringnow owns its own type-resolution logic instead of silently relying on caller discipline. - ✅ The
*types.Namedearly-return +t.Underlying()pattern is idiomatic and correctly handles type aliases (= string) because they are not*types.Named. - ✅ Testdata fixture covers all three meaningful branches: named outer type, named argument, and aliased predeclared string.
- ✅ PR description precisely describes the original defect, which is helpful for future archaeology.
Minor
- The
isStringTypedoc comment says "so this also matches named string types" — slightly misleading since the body only matches*types.Basic. One small clarification (see inline comment) would remove the ambiguity.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 24.8 AIC · ⌖ 9.75 AIC · ⊞ 7.6K
Comment /matt to run again
| ) | ||
| } | ||
|
|
||
| // isStringType reports whether t is a string basic type. Callers pass an |
There was a problem hiding this comment.
[/codebase-design] The phrase "so this also matches named string types" in the doc comment is slightly misleading — the body only ever matches *types.Basic. The intent is that callers pass a pre-resolved underlying type (which may originate from a named type), so the comment is about caller convention, not the function's own behaviour.
💡 Suggested wording
// isStringType reports whether t is a string basic type. Callers must pass an
// already-.Underlying()-resolved type; the function does not resolve named types
// itself.This avoids implying the function handles *types.Named directly.
@copilot please address this.
There was a problem hiding this comment.
The fix is correct. isExactString now properly distinguishes between the predeclared string type and named string types (type MyString string) by checking for *types.Named before calling isStringType. Type aliases (type aliasString = string) are not represented as *types.Named in go/types, so they correctly pass through and are treated as exact strings — both conversions can be removed. The added test cases validate all three paths.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.8 AIC · ⌖ 8.97 AIC · ⊞ 6.2K
PR Triage
|
PR TriageCategory: Non-draft, CI green (29 success), 2x APPROVED. Ready to merge. Automated triage — run 32572524009
|
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32573431024
|
…esroundtrip-no-op-alias 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>
Addressed in the latest commit. Review feedback (alias-to-named types): Failed checks: both were caused by the branch being stale rather than by the change — Commands and outcomes:
No diagnostic output changes for pre-existing inputs. |
stringbytesroundtrip.isExactStringwas a no-op alias ofisStringType, producing correct results only because callers happened to pass raw types (*types.Namedfails the*types.Basicassertion) while the gating checks passed.Underlying()-resolved types. Nothing in the name, signature, or body enforced that discipline — the same missing distinction that produced the non-compiling autofix in the siblingwritebytestringlinter (sg60a1).Changes
isExactStringimplemented independently — explicitly rejects*types.Namedand checkst.Underlying(), so it no longer depends on which type snapshot the caller passes:Documented the contract on both helpers —
isExactStringexpects the raw type and explains why the predeclared/named split matters (only predeclaredstringallows removing both conversions);isStringTypenotes it is given an already-resolved underlying type.Testdata coverage — new
badMixedNamedTypesfixture exercises both round-trip branches with a named outer conversion (myString([]byte(s))), a named argument (string([]byte(ms))), and an alias of the predeclared string (aliasString([]byte(as)), which must report the "both conversions can be removed" variant).No diagnostic output changes for existing inputs; this closes the latent gap so a future
SuggestedFixon the redundant branch can't silently reintroduce thesg60a1bug class.