feat: add structify — params-struct rewriter with whole-program caller updates - #20
Merged
Conversation
…truct, callers included Companion to fieldalign for the codegen pipeline (codegen -> structify -> fieldalign): functions with more than -max-params inputs get a generated XxxParams struct built from the parameter names, the signature becomes func F(ctx, arg XxxParams), the body's parameter uses become arg.Field, and EVERY call site across the loaded packages is rewritten in place. A leading context.Context stays positional. Caller rewrites are pure zero-width insertions around the existing argument expressions, so they compose with rewrites happening inside those arguments (including calls to other structified functions in the same pass). Signature changes are only safe when every reference is visible and rewritable, so functions are conservatively skipped (diagnosed, not rewritten) when they are variadic, generic, have blank params, are used as function values, satisfy an interface, are reassigned with :=, have callers in generated files, or callers that can't name the struct. Suppressions: //lint:ignore structify (funcparamlint spelling accepted) and structify:ignore. structify.Analyzer provides funcparamlint-compatible diagnostics for multichecker integration; cmd/structify does the whole-program rewrite. Tests cover the rewrite goldens plus every skip gate, and re-typecheck each rewritten module to prove the refactor compiles; validated against the Ditto backend: 264 functions / 217 files rewritten, builds clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new ChangesStructify Analyzer and Rewrite Tool
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as structify CLI
participant Packages as go/packages
participant Planner as structify.Plan
participant Applier as structify.Apply
User->>CLI: run structify [-fix] [patterns]
CLI->>Packages: packages.Load(patterns)
Packages-->>CLI: loaded packages
CLI->>Planner: Plan(pkgs, cfg)
Planner-->>CLI: Result{Rewritten, Skipped, Edits}
alt report mode
CLI-->>User: print rewritten/skipped and exit 1 if candidates exist
else fix mode
CLI->>Applier: Apply(source, edits)
Applier-->>CLI: rewritten source
CLI->>CLI: format and write files
CLI-->>User: print rewrite summary
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ng param Embedding a context in a struct field is an anti-pattern (context docs); the convention is func F(ctx, arg Params). A misplaced ctx now skips the function with a reason — reorder ctx first by hand, then structify keeps it positional. Found via ditto-backend's codemode dispatch(vm, ctx, ...). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
structify/analyzer.go (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated default threshold constant.
analyzerMaxParams = 4duplicates the default returned byConfig.maxParams()in structify.go (also 4). If the tool's default threshold changes in one place, the analyzer's diagnostic threshold silently diverges from the rewrite tool's candidate threshold.♻️ Suggested consolidation
-const analyzerMaxParams = 4 +// analyzerMaxParams mirrors Config{}.maxParams()'s default so the +// diagnostic threshold and the rewrite threshold never drift apart. +var analyzerMaxParams = Config{}.maxParams()Also applies to: 27-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@structify/analyzer.go` at line 25, The analyzer’s default max-params threshold is duplicated, with analyzerMaxParams in analyzer.go matching Config.maxParams() in structify.go. Consolidate this into a single shared source of truth and have the analyzer use that shared default so the diagnostic threshold and rewrite candidate threshold stay aligned; update the analyzer logic and any related helpers in analyzer.go to reference the shared config value instead of the local constant.structify/suppress.go (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial
readFileis unrelated to suppression parsing.Minor organizational nit — this trivial
os.ReadFilewrapper (consumed by the planner in structify.go per the linked context snippet) doesn't belong conceptually in a suppression-parsing file; could live alongside its consumer instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@structify/suppress.go` at line 67, The trivial readFile wrapper is misplaced in the suppression parsing area and should live with its actual consumer instead. Move readFile out of the suppression-related file and place it alongside the planner code in structify.go (or the closest consumer-owned location), then update any references so the planner continues to call the same helper without changing behavior.structify/structify.go (1)
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
walkErris declared and checked but never assigned.
walkErris read at Line 208 but nothing inside theforEachPkgclosure ever sets it, so this error path is currently dead code that misleadingly suggests errors from the indexing pass are handled.Also applies to: 208-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@structify/structify.go` at line 158, The `walkErr` path in `structify` is dead code because `forEachPkg` never assigns to the `walkErr` variable before it is checked later. Update the indexing flow in `structify` and its `forEachPkg` callback so any package-walk/indexing error is actually captured into `walkErr`, or remove the unused variable and its later check if errors are already handled elsewhere. Focus on the `structify` function and the `forEachPkg` closure to keep the error handling consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@structify/cmd/structify/main.go`:
- Around line 77-90: The `-fix` path in `main.go` writes each rewritten file
immediately inside the `res.Edits` loop, which can leave a partially updated
tree if a later file fails formatting or writing. Change the
`structify.Apply`/`format.Source` flow to a two-pass approach: first read,
rewrite, and format every file and keep the results in memory while validating
all succeed; only after that, write the buffered outputs back to disk in a
second pass. Use the existing `res.Edits`, `structify.Apply`, and
`format.Source` flow to locate the rewrite logic.
In `@structify/structify.go`:
- Around line 257-259: The candidate ordering in structify.go is using
target.Pos.String() for sort comparison, which is lexicographic and can misorder
positions within the same file. Update the sort in the candidate ordering logic
around sort.Slice(cands, ...) to compare the underlying token.Position fields
numerically (file, then line, then column) via the target.Pos values instead of
their String() output so the processing order of candidates, Rewritten, and
Skipped entries is stable and correct.
- Around line 158-207: The reference collection in structify.go is rescanning
p.TypesInfo.Uses once per file inside the package walk, which makes pass 1
O(files × uses) and can duplicate refInfo entries across package variants when
Tests: true. Refactor the logic around forEachPkg, refsByPos, and the per-file
loop so Uses is indexed by filename once per package, then dedupe references by
(filename, ident.Pos()) before appending or counting callers in
Target.NumCallers.
---
Nitpick comments:
In `@structify/analyzer.go`:
- Line 25: The analyzer’s default max-params threshold is duplicated, with
analyzerMaxParams in analyzer.go matching Config.maxParams() in structify.go.
Consolidate this into a single shared source of truth and have the analyzer use
that shared default so the diagnostic threshold and rewrite candidate threshold
stay aligned; update the analyzer logic and any related helpers in analyzer.go
to reference the shared config value instead of the local constant.
In `@structify/structify.go`:
- Line 158: The `walkErr` path in `structify` is dead code because `forEachPkg`
never assigns to the `walkErr` variable before it is checked later. Update the
indexing flow in `structify` and its `forEachPkg` callback so any
package-walk/indexing error is actually captured into `walkErr`, or remove the
unused variable and its later check if errors are already handled elsewhere.
Focus on the `structify` function and the `forEachPkg` closure to keep the error
handling consistent.
In `@structify/suppress.go`:
- Line 67: The trivial readFile wrapper is misplaced in the suppression parsing
area and should live with its actual consumer instead. Move readFile out of the
suppression-related file and place it alongside the planner code in structify.go
(or the closest consumer-owned location), then update any references so the
planner continues to call the same helper without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dab62232-1bfd-4ff8-85bc-b6f1e0b443b7
⛔ Files ignored due to path filters (2)
go.workis excluded by!**/*.workstructify/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
structify/analyzer.gostructify/cmd/structify/main.gostructify/go.modstructify/structify.gostructify/structify_test.gostructify/suppress.go
Instead of skipping a function whose ctx isn't the first parameter, reorder it: func F(vm, ctx, ...) becomes func F(ctx, arg FParams), and every call site moves its ctx argument text to the front. Safe because only non-ctx params are renamed to arg.Field — but a ctx ARGUMENT expression that references a parameter some rewrite renames cannot be moved textually, so that call site (and multiple-ctx signatures) still skip with a reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dern sorts - cmd/structify: validate and gofmt every rewritten file BEFORE writing any — rewrites are cross-file (signature + all callers), so a mid-loop failure must not strand a half-applied, non-compiling tree - Plan: index TypesInfo.Uses once per package instead of once per file (was O(files x uses)), and dedupe references by (func, ident) position so Tests:true package variants can't double-count NumCallers - sort.Slice -> slices.SortFunc/SortStableFunc with cmp comparators; candidate ordering now compares (file, line, col) numerically instead of lexicographic Position.String() Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
samePath treated 'p_test' (the external test package) as the defining package by trimming the _test suffix, emitting unqualified struct names where p.XxxParams is required. Only the in-package test variant shares PkgPath; compare exactly and let external test files hit the import scan. Found via ditto-backend's kgauth consent tests (package kgauth_test calling mgr.Issue). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
structify/structify_test.go (1)
525-560: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider also asserting
outeris rewritten to make the test's intent explicit.The test only checks that
inneris skipped; it doesn't assertouterends up inres.Rewritten. Per the planner logic (parameter-rename set is built from all candidates before individual gating), the skip check forinnerdoesn't strictly depend onouteractually succeeding — but adding an explicit assertion onouter's rewrite would better capture the scenario described in the test's comment (i.e., that the orphaning risk specifically arises fromouterbeing structified).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@structify/structify_test.go` around lines 525 - 560, The test for ctx-argument referencing rewritten params only verifies that inner is skipped, so the scenario described by the comment is not fully asserted. Update TestCtxArgReferencingRewrittenParamSkips to also check that outer appears in res.Rewritten, using the existing plan result and the outer/inner names to locate the assertions. Keep the current skip-reason check for inner, but add an explicit rewritten assertion for outer so the test clearly covers the orphaning case caused by outer’s parameter rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@structify/structify.go`:
- Around line 437-449: The ctx-hoisting logic in structify.go can change
evaluation order when a non-leading context.Context argument is moved ahead of
earlier arguments. Update the rewrite in structify/structify.go around the ctx
handling in the struct construction path so it either skips cases where
c.ctxIndex is not the first argument or emits a transformation that preserves
original argument evaluation order. Use the existing c.ctxIndex and
ref.call.Args logic to detect and avoid rewriting calls like f(a(), ctx(), b())
into a reordered form.
---
Nitpick comments:
In `@structify/structify_test.go`:
- Around line 525-560: The test for ctx-argument referencing rewritten params
only verifies that inner is skipped, so the scenario described by the comment is
not fully asserted. Update TestCtxArgReferencingRewrittenParamSkips to also
check that outer appears in res.Rewritten, using the existing plan result and
the outer/inner names to locate the assertions. Keep the current skip-reason
check for inner, but add an explicit rewritten assertion for outer so the test
clearly covers the orphaning case caused by outer’s parameter rename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2699988d-a129-405f-915e-c23d8a9be586
📒 Files selected for processing (3)
structify/cmd/structify/main.gostructify/structify.gostructify/structify_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- structify/cmd/structify/main.go
Hoisting a non-leading ctx moves its evaluation ahead of the arguments that preceded it, which is observable when either side has effects. Gate the hoist on it being unobservable: the ctx argument must be a bare identifier (effect- and panic-free) or context.Background()/TODO(), and every earlier argument must be free of calls (type conversions ok), channel receives, and function literals. Anything else skips with an evaluation-order reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New
structifymodule: finds functions with too many input parameters (default >4, matching the classic funcparamlint rule), generates aXxxParamsstruct from the parameter names, rewrites the signature and body, and — the important part — rewrites every caller across the loaded packages, including test files. A leadingcontext.Contextstays positional.Designed as the middle step of a codegen pipeline: codegen → structify → fieldalign (structs it generates get optimally packed by the next step).
Safety model
A signature change is only safe when every reference is visible and rewritable. Functions are skipped with a reason when they:
_) params:=(selector would be invalid there)//go:*///exportdirectives or a suppression commentCaller rewrites are zero-width insertions around existing argument expressions, so they compose with body rewrites happening inside those arguments — nested structified calls work in a single pass.
Testing
go build ./...clean, 28 conservative skips (18 interface methods)🤖 Generated with Claude Code
Summary by CodeRabbit
structifyCLI to report or automatically rewrite functions with more than the configured parameter limit into a single params-struct form, updating call sites accordingly._test.go, with deterministic edit application.