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
62 changes: 56 additions & 6 deletions internal/plan/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,14 @@ func Build(c ir.Container, idx *Index, opts Options) (Plan, []diag.Diag) {
diags = append(diags, emDiags...)

r := &resolver{
idx: idx,
inputs: inputs,
overrides: overrides,
embeds: embeds,
stepByKey: map[string]int{},
active: map[string]bool{},
idx: idx,
inputs: inputs,
overrides: overrides,
embeds: embeds,
stepByKey: map[string]int{},
active: map[string]bool{},
selfPkgPath: c.PkgPath,
selfFuncName: constructorName,
}

var outputs []Output
Expand Down Expand Up @@ -175,6 +177,15 @@ type resolver struct {
steps []Step
stepByKey map[string]int
active map[string]bool

// selfPkgPath and selfFuncName identify this container's own
// generated constructor. They are used to filter the by-type lookup
// so a container whose `inject:"returns"` declares an interface that
// matches an unrelated provider does not see its own previously
// emitted constructor as a candidate (a self-loop that would also
// produce a spurious "multiple providers" error).
selfPkgPath string
selfFuncName string
}

// embedSource describes one exported field of an inject:"embed" input that
Expand All @@ -192,6 +203,44 @@ func (r *resolver) resolveField(f ir.Field) (int, []diag.Diag) {
return r.resolveByType(f.Type, f.Pos, "field "+f.Name)
}

// excludeSelfProvider drops the container's own previously generated
// constructor from the candidate list so a `inject:"returns"` field does
// not pick itself up via type lookup. Callers that name a provider
// explicitly via inject:"with=..." are not affected.
//
// The common case is that the self-provider is not in the candidate
// list at all (most fields request types unrelated to the container's
// own return type), so the function returns the original slice without
// allocating when there is nothing to exclude.
func (r *resolver) excludeSelfProvider(candidates []*ir.Provider) []*ir.Provider {
if r.selfFuncName == "" {
return candidates
}
selfIdx := -1
for i, c := range candidates {
if c.PkgPath == r.selfPkgPath && c.FuncName == r.selfFuncName {
selfIdx = i
break
}
}
if selfIdx < 0 {
return candidates
}
// Boundary cases — the self-provider sits at one end of the slice,
// so a sub-slice is enough and no allocation is needed. This covers
// the very common case of a single matching candidate.
if selfIdx == 0 {
return candidates[1:]
}
if selfIdx == len(candidates)-1 {
return candidates[:selfIdx]
}
kept := make([]*ir.Provider, 0, len(candidates)-1)
kept = append(kept, candidates[:selfIdx]...)
kept = append(kept, candidates[selfIdx+1:]...)
return kept
Comment on lines +226 to +241

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

We can optimize excludeSelfProvider to avoid allocations when the self-provider is at the very beginning or the very end of the candidates list (including the common case where it is the only candidate). By returning a sub-slice of candidates directly, we avoid allocating a new slice.

	if selfIdx < 0 {
		return candidates
	}
	if selfIdx == 0 {
		return candidates[1:]
	}
	if selfIdx == len(candidates)-1 {
		return candidates[:selfIdx]
	}
	kept := make([]*ir.Provider, 0, len(candidates)-1)
	kept = append(kept, candidates[:selfIdx]...)
	kept = append(kept, candidates[selfIdx+1:]...)
	return kept

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 0f1f216. Boundary checks added before the middle-of-slice case:

if selfIdx == 0 {
    return candidates[1:]
}
if selfIdx == len(candidates)-1 {
    return candidates[:selfIdx]
}

This catches the most common shape — a single matching candidate that is itself the self-provider — without any allocation, plus the head/tail cases. The interior case still allocates len-1 exactly.

}
Comment on lines +215 to +242

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 common case, the container's own constructor is not present in the candidate list. The current implementation always allocates a new slice (via candidates[:0:0] and append) even when no candidates are excluded.

We can optimize this by first checking if the self-provider is present in the candidates. If it is not, we can return the original candidates slice directly, avoiding any allocation. If it is present, we can pre-allocate the new slice with the exact required capacity (len(candidates) - 1) to avoid multiple slice growth allocations.

func (r *resolver) excludeSelfProvider(candidates []*ir.Provider) []*ir.Provider {
	if r.selfFuncName == "" {
		return candidates
	}
	found := false
	for _, c := range candidates {
		if c.PkgPath == r.selfPkgPath && c.FuncName == r.selfFuncName {
			found = true
			break
		}
	}
	if !found {
		return candidates
	}
	kept := make([]*ir.Provider, 0, len(candidates)-1)
	for _, c := range candidates {
		if c.PkgPath == r.selfPkgPath && c.FuncName == r.selfFuncName {
			continue
		}
		kept = append(kept, c)
	}
	return kept
}

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 0bdec5e, with a small tweak over the suggestion: rather than running two passes through candidates when the self-provider is present, the first pass records its index and the second pass uses append(..., candidates[:idx]...) + append(..., candidates[idx+1:]...). Same fast-path behavior (no allocation when self is absent, exact-capacity allocation otherwise), one pass through the slice when self is found.

selfIdx := -1
for i, c := range candidates {
    if c.PkgPath == r.selfPkgPath && c.FuncName == r.selfFuncName {
        selfIdx = i
        break
    }
}
if selfIdx < 0 {
    return candidates
}
kept := make([]*ir.Provider, 0, len(candidates)-1)
kept = append(kept, candidates[:selfIdx]...)
kept = append(kept, candidates[selfIdx+1:]...)
return kept


func (r *resolver) resolveByType(want types.Type, pos token.Position, parent string) (int, []diag.Diag) {
tk := TypeKey(want)

Expand All @@ -210,6 +259,7 @@ func (r *resolver) resolveByType(want types.Type, pos token.Position, parent str
}

candidates := r.idx.LookupByType(want)
candidates = r.excludeSelfProvider(candidates)
if len(candidates) == 0 {
return -1, []diag.Diag{
diag.Errorf(pos, "no provider for %s (required by %s)", TypeString(want), parent),
Expand Down
29 changes: 29 additions & 0 deletions internal/plan/plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,35 @@ type Container struct {
}
}

func TestBuild_SelfGeneratedProviderIgnored(t *testing.T) {
t.Parallel()

// A `inject:"returns"` container whose previously generated
// constructor is now visible to the provider scan must not pick that
// constructor up as a candidate when resolving its own field, or it
// would loop forever. The unrelated, non-self provider remains usable.
src := `package test
type Greeter interface{ Greet() string }
type greeterImpl struct{}
func (greeterImpl) Greet() string { return "" }
func NewGreeterImpl() Greeter { return greeterImpl{} }

// Pretend this came from a previous generation of NewWrapper.
func NewWrapper() Greeter { return nil }

type wrapper struct {
service Greeter ` + "`inject:\"returns\"`" + `
}
`
p, ds := build(t, src, "wrapper", plan.Options{})
if diag.HasErrors(ds) {
t.Fatalf("expected the self provider NewWrapper to be filtered out, got %v", ds)
}
if len(p.Steps) != 1 || p.Steps[0].Provider == nil || p.Steps[0].Provider.FuncName != "NewGreeterImpl" {
t.Errorf("expected NewGreeterImpl to be the only provider step; got %+v", p.Steps)
}
}

func TestBuild_FieldNameTakenForcesSuffix(t *testing.T) {
t.Parallel()

Expand Down
9 changes: 8 additions & 1 deletion internal/scan/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,15 @@ func providersInPackage(pkg *packages.Package) ([]ir.Provider, []diag.Diag) {
providers []ir.Provider
diags []diag.Diag
)
// Generated files are intentionally NOT skipped here. Containers in
// generated files would re-trigger generation (and are filtered by
// containersInPackage), but provider functions emitted by an earlier
// generation are ordinary top-level Go functions whose results other
// packages may legitimately consume via type lookup. Skipping them
// would hide the constructors injector itself produced from downstream
// containers.
for _, file := range pkg.Syntax {
if file == nil || isGeneratedFile(file) {
if file == nil {
continue
}
for _, decl := range file.Decls {
Expand Down
10 changes: 7 additions & 3 deletions internal/scan/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,13 @@ func (d *DB) NewSelf() *DB { return d }
}
}

func TestProviders_GeneratedFileIgnored(t *testing.T) {
func TestProviders_GeneratedFileScanned(t *testing.T) {
t.Parallel()

// Provider functions emitted in a previous generation must remain
// discoverable so downstream containers in other packages can resolve
// against the injector-generated constructors. Containers in the same
// file are still filtered out by containersInPackage.
src := `// Code generated by injector. DO NOT EDIT.

package test
Expand All @@ -78,8 +82,8 @@ func NewDB() *DB { return nil }
if diag.HasErrors(ds) {
t.Fatalf("unexpected error diags: %v", ds)
}
if len(ps) != 0 {
t.Errorf("expected generated file to be ignored, got %d providers", len(ps))
if len(ps) != 1 || ps[0].FuncName != "NewDB" {
t.Errorf("expected NewDB to be picked up from a generated file, got %+v", ps)
}
}

Expand Down
Loading