Merge pull request #6 from mickamy/fix/var-name-collision

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

Author: mickamy

example/returns/injector_gen.go

@@ -100,7 +100,8 @@ func collectImports(im *Imports, p plan.Plan) {
 func writeContainer(w io.Writer, im *Imports, p plan.Plan) error {
 	name := p.ConstructorName
 	structName := p.Container.StructName
-	paramSig := formatParams(im, p.Inputs)
+	ns := assignNames(im, p)
+	paramSig := formatParams(im, p.Inputs, ns)
 	retSig := im.QualifyType(p.ReturnType)
 
 	fmt.Fprintf(w, "// %s initializes dependencies and constructs %s.\n", name, structName)
@@ -110,29 +111,86 @@ func writeContainer(w io.Writer, im *Imports, p plan.Plan) error {
 		fmt.Fprintf(w, "func %s(%s) %s {\n", name, paramSig, retSig)
 	}
 
-	wrote, err := writeSteps(w, im, p, retSig)
+	wrote, err := writeSteps(w, im, p, ns, retSig)
 	if err != nil {
 		return err
 	}
 
-	writeStructLiteral(w, p, wrote)
+	writeStructLiteral(w, p, ns, wrote)
 	if p.ReturnsError {
 		fmt.Fprint(w, ", nil")
 	}
 	fmt.Fprint(w, "\n}\n\n")
 
 	if p.EmitMust {
-		writeMustVariant(w, im, p)
+		writeMustVariant(w, im, p, ns)
 	}
 	return nil
 }
 
+// names holds the final, collision-free identifiers used when rendering one
+// plan. Indices line up with plan.Inputs / plan.Steps.
+type names struct {
+	inputs []string
+	steps  []string
+}
+
+// assignNames picks variable and parameter names for the plan that do not
+// collide with imported package aliases. Each name is derived from the
+// plan-level name (Input.Name / Step.VarName) and suffixed with a counter
+// when a collision is detected. Steps that reference an input share that
+// input's renamed identifier so the function signature and the body stay
+// in sync.
+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
+	}
+	// Reserve identifiers the generated body always uses.
+	taken["err"] = true
+	taken["v"] = true
+	taken["_"] = true
+
+	pick := func(base string) string {
+		if base == "" || base == "_" {
+			base = "v"
+		}
+		if !taken[base] {
+			taken[base] = true
+			return base
+		}
+		for i := 2; ; i++ {
+			try := fmt.Sprintf("%s%d", base, i)
+			if !taken[try] {
+				taken[try] = true
+				return try
+			}
+		}
+	}
+
+	ns := names{
+		inputs: make([]string, len(p.Inputs)),
+		steps:  make([]string, len(p.Steps)),
+	}
+	for i, in := range p.Inputs {
+		ns.inputs[i] = pick(in.Name)
+	}
+	for i, s := range p.Steps {
+		if s.Kind == plan.StepKindInput {
+			ns.steps[i] = ns.inputs[s.InputIndex]
+			continue
+		}
+		ns.steps[i] = pick(s.VarName)
+	}
+	return ns
+}
+
 // writeSteps emits step lines and reports whether any visible code was
 // written. Pure-input plans (e.g. a container whose only inputs are
 // non-blank inject:"arg" fields) produce no step lines, and the caller
 // uses the bool to decide whether to leave a blank line before the
 // struct literal.
-func writeSteps(w io.Writer, im *Imports, p plan.Plan, retSig string) (bool, error) {
+func writeSteps(w io.Writer, im *Imports, p plan.Plan, ns names, retSig string) (bool, error) {
 	zeroExpr := "nil"
 	if !isNilable(p.ReturnType) {
 		// For non-nilable return types we need an explicit zero value;
@@ -141,30 +199,29 @@ func writeSteps(w io.Writer, im *Imports, p plan.Plan, retSig string) (bool, err
 	}
 
 	wrote := false
-	for _, s := range p.Steps {
+	for i, s := range p.Steps {
 		switch s.Kind {
 		case plan.StepKindInput:
 			// The input is already a function parameter — nothing to emit.
 		case plan.StepKindEmbedField:
-			in := p.Inputs[s.InputIndex]
-			fmt.Fprintf(w, "\t%s := %s.%s\n", s.VarName, in.Name, s.EmbedFieldName)
+			fmt.Fprintf(w, "\t%s := %s.%s\n", ns.steps[i], ns.inputs[s.InputIndex], s.EmbedFieldName)
 			wrote = true
 		case plan.StepKindProvider:
 			if s.Provider == nil {
 				return wrote, fmt.Errorf("provider step %q has nil Provider", s.VarName)
 			}
 			args := make([]string, 0, len(s.ArgSteps))
 			for _, idx := range s.ArgSteps {
-				args = append(args, p.Steps[idx].VarName)
+				args = append(args, ns.steps[idx])
 			}
 			call := im.QualifyProvider(s.Provider)
 			if s.Provider.ReturnsError {
-				fmt.Fprintf(w, "\t%s, err := %s(%s)\n", s.VarName, call, strings.Join(args, ", "))
+				fmt.Fprintf(w, "\t%s, err := %s(%s)\n", ns.steps[i], call, strings.Join(args, ", "))
 				fmt.Fprint(w, "\tif err != nil {\n")
 				fmt.Fprintf(w, "\t\treturn %s, err\n", zeroExpr)
 				fmt.Fprint(w, "\t}\n")
 			} else {
-				fmt.Fprintf(w, "\t%s := %s(%s)\n", s.VarName, call, strings.Join(args, ", "))
+				fmt.Fprintf(w, "\t%s := %s(%s)\n", ns.steps[i], call, strings.Join(args, ", "))
 			}
 			wrote = true
 		}
@@ -189,56 +246,51 @@ func isNilable(t types.Type) bool {
 	return false
 }
 
-func writeStructLiteral(w io.Writer, p plan.Plan, leadingBlank bool) {
+func writeStructLiteral(w io.Writer, p plan.Plan, ns names, leadingBlank bool) {
 	if leadingBlank {
 		fmt.Fprint(w, "\n")
 	}
 	fmt.Fprintf(w, "\treturn &%s{\n", p.Container.StructName)
 	for _, o := range p.Outputs {
-		v := p.Steps[o.StepIndex].VarName
-		fmt.Fprintf(w, "\t\t%s: %s,\n", o.FieldName, v)
+		fmt.Fprintf(w, "\t\t%s: %s,\n", o.FieldName, ns.steps[o.StepIndex])
 	}
 	fmt.Fprint(w, "\t}")
 }
 
 // writeMustVariant emits MustNewX, which delegates to NewX and panics on
 // error.
-func writeMustVariant(w io.Writer, im *Imports, p plan.Plan) {
+func writeMustVariant(w io.Writer, im *Imports, p plan.Plan, ns names) {
 	name := "Must" + p.ConstructorName
-	paramSig := formatParams(im, p.Inputs)
+	paramSig := formatParams(im, p.Inputs, ns)
 	retSig := im.QualifyType(p.ReturnType)
 
 	fmt.Fprintf(w,
 		"// %s initializes dependencies and constructs %s or panics on failure.\n",
 		name, p.Container.StructName)
 	fmt.Fprintf(w, "func %s(%s) %s {\n", name, paramSig, retSig)
 	if p.ReturnsError {
-		fmt.Fprintf(w, "\tv, err := %s(%s)\n", p.ConstructorName, formatArgs(p.Inputs))
+		fmt.Fprintf(w, "\tv, err := %s(%s)\n", p.ConstructorName, formatArgs(ns))
 		fmt.Fprint(w, "\tif err != nil {\n")
 		fmt.Fprint(w, "\t\tpanic(err)\n")
 		fmt.Fprint(w, "\t}\n")
 		fmt.Fprint(w, "\treturn v\n")
 	} else {
-		fmt.Fprintf(w, "\treturn %s(%s)\n", p.ConstructorName, formatArgs(p.Inputs))
+		fmt.Fprintf(w, "\treturn %s(%s)\n", p.ConstructorName, formatArgs(ns))
 	}
 	fmt.Fprint(w, "}\n\n")
 }
 
-func formatParams(im *Imports, inputs []plan.Input) string {
+func formatParams(im *Imports, inputs []plan.Input, ns names) string {
 	if len(inputs) == 0 {
 		return ""
 	}
 	parts := make([]string, 0, len(inputs))
-	for _, in := range inputs {
-		parts = append(parts, in.Name+" "+im.QualifyType(in.Type))
+	for i, in := range inputs {
+		parts = append(parts, ns.inputs[i]+" "+im.QualifyType(in.Type))
 	}
 	return strings.Join(parts, ", ")
 }
 
-func formatArgs(inputs []plan.Input) string {
-	parts := make([]string, 0, len(inputs))
-	for _, in := range inputs {
-		parts = append(parts, in.Name)
-	}
-	return strings.Join(parts, ", ")
+func formatArgs(ns names) string {
+	return strings.Join(ns.inputs, ", ")
 }

internal/emit/emit.go

@@ -377,6 +377,38 @@ type Container struct {
 	}
 }
 
+func TestEmit_VarNameDoesNotShadowImportedPackage(t *testing.T) {
+	t.Parallel()
+
+	// Single-file fixture where the result type's lowercased name ("db",
+	// "context") collides with a top-level identifier that mimics a
+	// package import: declaring a local `db` would break any later
+	// db.Open(...) call. Inject the reserved aliases into the Imports
+	// tracker by hand to mirror the multi-file scenario.
+	src := `package myapp
+type DB struct{}
+type Context struct{}
+func NewDB() *DB { return nil }
+func NewContext() Context { return Context{} }
+type Container struct {
+	DB  *DB     ` + "`inject:\"with=NewDB\"`" + `
+	Ctx Context ` + "`inject:\"\"`" + `
+}
+`
+	p := buildPlan(t, src)
+
+	// Reserve "db" and "context" as if those were import aliases visible
+	// to the generated file. The assignNames pass must avoid them.
+	im := emit.New("myapp", "db", "context")
+	got := emit.RenderForTest(t, im, p)
+
+	for _, bad := range []string{"\tdb :=", "\tcontext :="} {
+		if strings.Contains(got, bad) {
+			t.Errorf("generated body shadows reserved alias (matched %q):\n%s", bad, got)
+		}
+	}
+}
+
 func TestEmit_NonBlankArgStoresInField(t *testing.T) {
 	t.Parallel()
 

internal/emit/emit_test.go

@@ -0,0 +1,22 @@
+package emit
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/mickamy/injector/internal/plan"
+)
+
+// RenderForTest collects imports for p into the supplied tracker (whose
+// reserved aliases the caller has already set up) and renders just the
+// container constructor body. It is meant for tests that need to inject
+// reserved import names without round-tripping through Emit.
+func RenderForTest(t *testing.T, im *Imports, p plan.Plan) string {
+	t.Helper()
+	collectImports(im, p)
+	var buf bytes.Buffer
+	if err := writeContainer(&buf, im, p); err != nil {
+		t.Fatalf("writeContainer: %v", err)
+	}
+	return buf.String()
+}