Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new import-merging approach can still produce incorrect type resolution when import aliases differ between files, risking invalid generated output in real-world packages.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes internal/gen’s handling of anonymous embedded structs whose declarations live in a different file within the same Go package, ensuring their fields are included in generated helpers and that any needed imports from the declaring file are available during type resolution.
Changes:
- Extend embedded-struct handling to load and process same-package declarations when
go/parsercan’t resolveIdent.Obj. - Update
loadNamedStructTypeto also return the declaring file’s imports and reuse a sharednewImporthelper. - Add a focused regression test (plus a small test harness) covering cross-file value and pointer embeddings.
File summaries
| File | Description |
|---|---|
| internal/gen/utils.go | loadNamedStructType now returns declaring-file imports; adds newImport/fileImports helpers. |
| internal/gen/generator.go | Uses newImport; loads embedded struct declarations from same package when Obj is missing and merges declaring-file imports. |
| internal/gen/generator_support_test.go | Adds helper to generate from ad-hoc sources and assert generated struct fields. |
| internal/gen/embedded_cross_file_test.go | Regression test for cross-file embedded structs (value and pointer embeddings). |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The embedded struct's field types use the aliases of the file declaring it, which may | ||
| // import packages this file does not; merge them so those types still resolve. | ||
| mergeImports(&p.Imports, imports) | ||
| return addEmbeddedFields(st, typeName, pkgName) |
| for name, content := range files { | ||
| if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { | ||
| t.Fatalf("write %s: %v", name, err) | ||
| } | ||
| } |
1d37b4f to
ab742cf
Compare
|
Addressed the review. The shared test helper now creates parent directories before writing, so a nested key like The alias finding was right, and it was reachable in both directions: merging into Rather than merge, the embedded struct's fields now resolve against a copy of the file whose
|
There was a problem hiding this comment.
🟡 Changes recommended
There are review comments that should be addressed, including misplaced doc comments and a potentially expensive repeated package-load path introduced for common embeddings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| // Helper function to load and process a struct type declared in another file or package | ||
| loadAndProcessExternalStruct := func(pkgName, typeName string) bool { | ||
| st, err := loadNamedStructType(p.goModDir, p.getFullImportPath(pkgName), typeName) | ||
| st, imports, err := loadNamedStructType(p.goModDir, p.getFullImportPath(pkgName), typeName) | ||
| if err != nil || st == nil { | ||
| return false | ||
| } | ||
| return addEmbeddedFields(st, typeName, pkgName) | ||
| // The embedded struct's field types are written in the aliases of the file declaring it, | ||
| // which this file need not share and may bind to another path. They resolve against that | ||
| // file's imports, on a copy, so neither file's aliases reach the other. | ||
| declaring := *p | ||
| declaring.Imports = append(append([]Import{}, imports...), p.Imports...) | ||
| return addEmbeddedFields(&declaring, st, typeName, pkgName) | ||
| } |
| // mergeImports appends imports from src into dst if not already present (by Path) | ||
|
|
||
| // shouldSkipFile checks if a file contains the generated code header and should be skipped | ||
|
|
||
| func mergeImports(dst *[]Import, src []Import) { |
ab742cf to
0038d12
Compare
|
Addressed the second round. Both findings were right, and the first was mine. The doc comments for On the repeated loads: The shared test helper also reads the whole output tree rather than its top level, which matters here: this pull request's new test is the one that writes sources under subdirectories. |
There was a problem hiding this comment.
🟡 Changes recommended
The new memoization holds a global mutex across packages.Load, which can unnecessarily serialize and stall generation under concurrent use.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| func loadNamedStructType(modRoot, pkgPath, name string) (*ast.StructType, []Import, error) { | ||
| key := modRoot + "\x00" + pkgPath + "\x00" + name | ||
|
|
||
| loadedStructsMu.Lock() | ||
| defer loadedStructsMu.Unlock() | ||
|
|
||
| if cached, ok := loadedStructs[key]; ok { | ||
| return cached.st, cached.imports, cached.err | ||
| } | ||
| st, imports, err := loadNamedStructTypeUncached(modRoot, pkgPath, name) | ||
| loadedStructs[key] = loadedStruct{st: st, imports: imports, err: err} | ||
|
|
||
| return st, imports, err | ||
| } |
0038d12 to
3e691bc
Compare
|
Fair point, and fixed. The lock now guards the map only: it is taken to read, released across the load, and taken again to store. Two callers racing on one key both load, which costs a redundant load and no correctness, where holding it across |
There was a problem hiding this comment.
🟢 Approval recommended
The changes address the described resolution gap with a targeted implementation and add focused regression tests covering both cross-file embedding and import-alias conflicts.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Problem
An anonymous embedded struct declared in another file of the same package loses all of its fields in the generated helper.
go/parserresolves identifiers within one file, so the embedded identifier has noObjwhen its declaration lives elsewhere, and the generator silently skipped it. The common case is a sharedCommonFieldsstruct (ID, CreatedAt, UpdatedAt, DeletedAt) incommon.go, embedded by every model: none of the models' helpers had those four fields.Fix
When an embedded identifier has no
Objand the package path is known, the generator loads the package and processes the declaration the way it already does for a struct from another package.loadNamedStructTypenow also returns the imports of the file declaring the struct, which are merged into the current file's imports, so the embedded fields' types resolve through the aliases their own file uses.Test
TestEmbeddedStructDeclaredInAnotherFilegenerates over two files of one package:Basein one, a model embeddingBaseby value and another embedding*Basein the other. It assertsBasegets its own helper and both embeddings carry its fields.The first commit adds a small test helper (
generateFromSources,containsField) that writes ad hoc sources into a throwaway module and runs the generator over them. The other pull requests from this fork share that commit; git merges the identical addition without conflict, so they can land in any order.🤖 Generated with Claude Code