Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions example/embed/di/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions example/embed/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions example/embedded-error/di/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions example/embedded-error/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions example/embedded/di/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions example/embedded/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions example/name-conflict/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions example/returns/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions example/simple/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions example/with-error/injector_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

98 changes: 94 additions & 4 deletions internal/plan/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ func Build(c ir.Container, idx *Index, opts Options) (Plan, []diag.Diag) {
}
}

renameOutputSteps(r.steps, outputs)

returnsErr := false
for _, s := range r.steps {
if s.Kind == StepKindProvider && s.Provider != nil && s.Provider.ReturnsError {
Expand Down Expand Up @@ -612,6 +614,10 @@ func deriveInputName(t types.Type) string {
if t == nil {
return "arg"
}
// Resolve Go 1.22+ type aliases (`type Client = valkey.Client`) so
// the alias's own name flows through instead of falling out to the
// "arg" sentinel.
t = types.Unalias(t)
if ptr, ok := t.(*types.Pointer); ok {
return deriveInputName(ptr.Elem())
}
Expand All @@ -623,6 +629,80 @@ func deriveInputName(t types.Type) string {
return "arg"
}

// renameOutputSteps renames non-input steps that produce a container
// output to lowerFirst(field name). This puts the field's own identifier
// at the call site (`tx := tx.New(...)` for a `Tx` field, suffixed if it
// would shadow an existing step name). Steps that are not bound to any
// output keep the type-derived name picked at resolution time.
//
// Renaming runs in two passes so that an output whose desired base name
// is currently held by another step that is also about to be renamed can
// claim the now-vacated name without unnecessarily suffixing. Without
// this, a hypothetical swap (step A holds "foo" and wants "db", step B
// holds "db" and wants "foo") would land on `foo2`/`db` instead of the
// clean `foo`/`db`.
func renameOutputSteps(steps []Step, outputs []Output) {
used := make(map[string]bool, len(steps))
for _, st := range steps {
used[st.VarName] = true
}

type pendingRename struct {
stepIdx int
base string
}
var renames []pendingRename
// A shared step (bound to more than one container field, e.g. when two
// fields request the same dependency) appears multiple times in
// outputs. Decide the rename for each step at the first valid output
// and skip any later occurrences so we don't queue the same step
// twice, which would leak the dropped candidate name into `used` and
// force unrelated steps onto a suffix.
decided := make(map[int]bool, len(outputs))
for _, o := range outputs {
if o.StepIndex < 0 || o.StepIndex >= len(steps) {
continue
}
if decided[o.StepIndex] {
continue
}
s := &steps[o.StepIndex]
if s.Kind == StepKindInput {
decided[o.StepIndex] = true
continue
}
base := lowerFirst(o.FieldName)
if base == "" {
continue
}
if base == s.VarName {
// Existing name already lines up with this field; no rename
// needed even if later outputs would have picked a different
// base.
decided[o.StepIndex] = true
continue
}
delete(used, s.VarName)
renames = append(renames, pendingRename{stepIdx: o.StepIndex, base: base})
decided[o.StepIndex] = true
}

for _, r := range renames {
pick := r.base
if used[pick] {
for i := 2; ; i++ {
try := fmt.Sprintf("%s%d", r.base, i)
if !used[try] {
pick = try
break
}
}
}
steps[r.stepIdx].VarName = pick
used[pick] = true
}
Comment on lines +650 to +703

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

When multiple container fields are bound to the same shared dependency (which resolves to a single step), outputs will contain multiple entries pointing to the same StepIndex.

In the current implementation, this causes the step to be added to renames multiple times with different base names (e.g., foo and bar). During the renaming loop, the step's VarName is overwritten multiple times (the last one wins), but all intermediate candidate names are marked as used in the used map.

This leaks unused names (like foo) into the used map, which can cause other unrelated steps that naturally want those names to be unnecessarily suffixed (e.g., forced to use foo2).

We can fix this by deduplicating the renames per StepIndex while preserving the deterministic order of renaming.

	renameOrder := make([]int, 0, len(outputs))
	renames := make(map[int]string)
	for _, o := range outputs {
		if o.StepIndex < 0 || o.StepIndex >= len(steps) {
			continue
		}
		s := &steps[o.StepIndex]
		if s.Kind == StepKindInput {
			continue
		}
		base := lowerFirst(o.FieldName)
		if base == "" || base == s.VarName {
			continue
		}
		if _, exists := renames[o.StepIndex]; !exists {
			delete(used, s.VarName)
			renames[o.StepIndex] = base
			renameOrder = append(renameOrder, o.StepIndex)
		}
	}

	for _, stepIdx := range renameOrder {
		base := renames[stepIdx]
		pick := base
		if used[pick] {
			for i := 2; ; i++ {
				try := fmt.Sprintf("%s%d", base, i)
				if !used[try] {
					pick = try
					break
				}
			}
		}
		steps[stepIdx].VarName = pick
		used[pick] = 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.

Applied in 87e5f01, with a small extension over the suggested form. Same core fix — only the first valid candidate for a given StepIndex is queued, so a step shared across two output fields no longer leaks its dropped candidate name into used.

The extra wrinkle: the gate also catches the "first base already matches the existing VarName" case, so a shared step whose current name aligns with the first field is left alone instead of being renamed to a different field's name on the second visit. Example:

type Container struct {
    DB     *DB `inject:""`   // step current "db" — already aligned
    Backup *DB `inject:""`   // would otherwise queue "backup" and rename
}

Without the gate, the variable ends up as backup, and the struct literal reads DB: backup, Backup: backup. With the gate, the variable stays db and aligns with at least the first field.

Two regression tests added: TestBuild_SharedStepDoesNotLeakCandidate (the leak you described — extra Twin *DB field, makes sure an unrelated Backup step still gets its natural name) and TestBuild_SharedStepPreservesMatchingFieldName (the matching-base case above).

}
Comment on lines +644 to +704

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

The current implementation of renameOutputSteps has an $O(M \times N)$ complexity (where $M$ is the number of outputs and $N$ is the number of steps) because it re-allocates and populates a used map of size $N$ in every iteration of the outer loop.

We can optimize this to $O(N)$ time complexity and a single map allocation by initializing the used map once at the beginning, and updating it dynamically as we rename steps.

func renameOutputSteps(steps []Step, outputs []Output) {
	used := make(map[string]bool, len(steps))
	for _, st := range steps {
		used[st.VarName] = true
	}

	for _, o := range outputs {
		if o.StepIndex < 0 || o.StepIndex >= len(steps) {
			continue
		}
		s := &steps[o.StepIndex]
		if s.Kind == StepKindInput {
			continue
		}
		base := lowerFirst(o.FieldName)
		if base == "" || base == s.VarName {
			continue
		}

		delete(used, s.VarName)

		pick := base
		if used[pick] {
			for i := 2; ; i++ {
				try := fmt.Sprintf("%s%d", base, i)
				if !used[try] {
					pick = try
					break
				}
			}
		}
		s.VarName = pick
		used[pick] = 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 call — applied in ea4ef55. The used map is built once over all steps, and each output rename now does delete(used, oldName) / used[newName] = true instead of rebuilding the whole set. Behavior is unchanged (no diff in any regenerated example) and the loop is now O(N + M) instead of O(M·N).

Comment on lines +644 to +704

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

In the current implementation of renameOutputSteps, the old name of a step is deleted from the used map and its new name is added during a single pass over the outputs. This can lead to unnecessary suffixing (e.g., db22 instead of db2) if a step's new name was previously occupied by another step that is also scheduled to be renamed later in the loop.

Using a two-pass approach resolves this: first, identify all steps that will be renamed and free up their old names from the used map; second, assign the new names in the original order of their last occurrence. This avoids naming collisions with names that are about to be vacated.

func renameOutputSteps(steps []Step, outputs []Output) {
	used := make(map[string]bool, len(steps))
	for _, st := range steps {
		used[st.VarName] = true
	}

	type renameInfo struct {
		base      string
		lastIndex int
	}
	renameInfos := make(map[int]renameInfo)
	for i, o := range outputs {
		if o.StepIndex < 0 || o.StepIndex >= len(steps) {
			continue
		}
		s := &steps[o.StepIndex]
		if s.Kind == StepKindInput {
			continue
		}
		base := lowerFirst(o.FieldName)
		if base == "" || base == s.VarName {
			continue
		}
		renameInfos[o.StepIndex] = renameInfo{
			base:      base,
			lastIndex: i,
		}
	}

	for stepIdx := range renameInfos {
		delete(used, steps[stepIdx].VarName)
	}

	for i, o := range outputs {
		info, ok := renameInfos[o.StepIndex]
		if !ok || info.lastIndex != i {
			continue
		}
		s := &steps[o.StepIndex]
		pick := info.base
		if used[pick] {
			for i := 2; ; i++ {
				try := fmt.Sprintf("%s%d", info.base, i)
				if !used[try] {
					pick = try
					break
				}
			}
		}
		s.VarName = pick
		used[pick] = 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.

Applied in 4d4e52a (slightly simpler shape than the suggestion — no lastIndex tracking, since each step appears in outputs at most once given how outputs is built):

var renames []pendingRename
for _, o := range outputs {
    // …skip irrelevant outputs…
    delete(used, s.VarName)
    renames = append(renames, pendingRename{stepIdx: o.StepIndex, base: base})
}
for _, r := range renames {
    pick := r.base
    if used[pick] { /* suffix loop */ }
    steps[r.stepIdx].VarName = pick
    used[pick] = true
}

Pass 1 vacates every old name first, pass 2 picks the new ones — so a swap (foo/dbdb/foo) lands cleanly instead of producing foo2. Covered by TestBuild_FieldNameSwapNoUnnecessarySuffix.


func varNameForEmbed(es embedSource, existing []Step) string {
// FieldName may be a dotted selector (e.g. "CommonInfra.DB") when the
// source comes from a promoted field; only the leaf segment is a valid
Expand Down Expand Up @@ -651,11 +731,21 @@ func varNameForEmbed(es embedSource, existing []Step) string {
}

func varNameForProvider(p *ir.Provider, existing []Step) string {
base := p.FuncName
if strings.HasPrefix(base, "New") && len(base) > 3 {
base = base[3:]
// Name the variable after what the call produces, not after the
// constructor function. `db.Open(...) *sql.DB` reads more naturally
// as `db := db.Open(...)` than `open := db.Open(...)`, and
// container-field-bound steps later get renamed once more to the
// destination field name.
base := deriveInputName(p.Result)

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

If a provider returns a type alias, deriveInputName will fail to recognize it as a named type (since it is represented as a *types.Alias in Go 1.22+), causing it to fall back to the function name. Resolving the alias using types.Unalias before deriving the name ensures consistent naming for type aliases.

Suggested change
base := deriveInputName(p.Result)
base := deriveInputName(types.Unalias(p.Result))

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.

Applied in 4d4e52a. Folded types.Unalias into deriveInputName itself so both call sites (buildInputs for arg-field naming and varNameForProvider for step naming) get the alias resolution for free:

func deriveInputName(t types.Type) string {
    if t == nil { return "arg" }
    t = types.Unalias(t)
    // …Pointer / Named handling as before…
}

Covered by TestBuild_TypeAliasResultDerivesAliasName, which sets up type Alias = Real; func New() Alias and verifies the intermediate variable lands on real rather than falling through to the function-name fallback.

if base == "arg" {
// Anonymous or unnamed result type — fall back to the function
// name for a less generic label than "arg".
base = p.FuncName
if strings.HasPrefix(base, "New") && len(base) > 3 {
base = base[3:]
}
base = lowerFirst(base)
}
base = lowerFirst(base)
if base == "" {
base = "v"
}
Expand Down
Loading
Loading