Skip to content

fix(emit): rename local vars and params that collide with import aliases#6

Merged
mickamy merged 2 commits into
mainfrom
fix/var-name-collision
Jun 11, 2026
Merged

fix(emit): rename local vars and params that collide with import aliases#6
mickamy merged 2 commits into
mainfrom
fix/var-name-collision

Conversation

@mickamy

@mickamy mickamy commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Summary

The generated code could pick a local variable or parameter name that shadows an imported package alias. When a downstream line then used the shadowed package — e.g. db.Open(...) after db, err := config.NewDB() — the call resolved against the local value instead of the package, producing a broken constructor that failed to compile.

This change adds a small renaming pass in the emitter. After imports are collected, every input and step name is checked against the set of names already taken by import aliases (and a small set of always-reserved identifiers like err); collisions are suffixed with a numeric counter (dbdb2).

Trigger

The atode codebase hit this with the following shape:

type Infra struct {
    _   config.KV
    DB  *sql.DB       `inject:"with=db.Open"`
    Tx  tx.Transactor `inject:""`
    KVS kv.Client     `inject:"with=kv.New"`
}

The generated constructor imported both db (for db.Open) and kv (for kv.New) and then declared local variables also called db and kv, which immediately collided. A context.Context arg parameter similarly shadowed the context import.

Drive-by

example/returns/injector_gen.go regenerates to greeter2 := greeter.NewGreeter(). The previous output (greeter := greeter.NewGreeter()) compiled by accident — Go evaluates the RHS of := before binding the LHS — but any later reference to the package would have broken. The renamed variable is the same generation now applied consistently.

Test plan

  • internal/emit: new TestEmit_VarNameDoesNotShadowImportedPackage (uses Imports.New(reserved...) to inject collision candidates and asserts the rendered body does not assign to those names)
  • make lint reports 0 issues
  • make test-e2e regenerates every example and passes go vet + go build

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a mechanism to prevent generated variable and parameter names from colliding with imported package aliases. It adds an assignNames function to resolve name collisions by appending a counter suffix and updates the code generation logic to use these collision-free identifiers. The feedback suggests optimizing this process by directly accessing the unexported im.used map within the same package, which would eliminate the need for the new Reserved() method and avoid unnecessary slice allocations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread internal/emit/emit.go
Comment on lines +144 to +148
func assignNames(im *Imports, p plan.Plan) names {
taken := map[string]bool{}
for _, a := range im.Reserved() {
taken[a] = true
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since assignNames and Imports are in the same package, we can directly access im.used instead of calling im.Reserved(). This avoids allocating an intermediate slice and doing a double iteration, improving both performance and memory usage.

Suggested change
func assignNames(im *Imports, p plan.Plan) names {
taken := map[string]bool{}
for _, a := range im.Reserved() {
taken[a] = true
}
func assignNames(im *Imports, p plan.Plan) names {
taken := make(map[string]bool, len(im.used)+3)
for a := range im.used {
taken[a] = true
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — applied in 8ec97c8. assignNames now reads im.used directly and Reserved() is removed entirely (it had no other callers). Saves one allocation per assignNames call and keeps the API surface narrower.

Comment thread internal/emit/imports.go Outdated
Comment on lines +84 to +95
// Reserved returns every identifier the import tracker is currently
// holding off-limits: alias names assigned to imported packages plus any
// pre-claimed reserved names supplied to New. Callers that need to pick a
// non-colliding identifier for a local variable or parameter should treat
// this set as taken.
func (im *Imports) Reserved() []string {
out := make([]string, 0, len(im.used))
for name := range im.used {
out = append(out, name)
}
return out
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since assignNames is in the same emit package, it can directly access the unexported im.used map. This makes the Reserved() method obsolete. We can remove Reserved() entirely to simplify the API and avoid unnecessary slice allocations.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 8ec97c8Reserved() removed entirely from imports.go since assignNames is its only caller and now reads im.used directly.

@mickamy mickamy merged commit ba86489 into main Jun 11, 2026
5 checks passed
@mickamy mickamy deleted the fix/var-name-collision branch June 11, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant