export lazy pkgs#462
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors package registration from lazy to eager across numerous export files and updates the generation logic in pkg/gen.go. Feedback for pkg/gen.go highlights that the isLazyPkg function is fragile for nested paths and should be updated to check all parent prefixes. Additionally, the local variable lazyPkgs shadows a package-level map, and it is recommended to rename it along with cpkgs to more descriptive names like eagerPkgs to improve maintainability.
| func isLazyPkg(pkg string) bool { | ||
| if pkg == "unicode" { | ||
| return true | ||
| } | ||
| if _, ok := lazyPkgs[pkg]; ok { | ||
| return true | ||
| } | ||
| if pos := strings.Index(pkg, "/"); pos != -1 { | ||
| if _, ok := lazyPkgs[pkg[:pos]]; ok { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation of isLazyPkg only checks the first component of the package path (e.g., for a/b/c, it only checks a). This works for top-level packages but is fragile for nested entries in the lazyPkgs map like runtime/debug. A more robust approach is to check all parent prefixes in a loop. This ensures that subpackages of any lazy package are also treated as lazy, regardless of nesting depth.
References
- Prefer robust path handling logic over simple string prefix checks when dealing with hierarchical structures like Go package paths. (link)
- Ensure that logic for identifying package properties handles nested paths correctly to avoid inconsistent behavior between parent and child packages.
| var cpkgs []string | ||
| var opkgs []string | ||
| var lazyPkgs []string | ||
| for _, pkg := range npkgs { | ||
| if commonPkgs[pkg] { | ||
| cpkgs = append(cpkgs, pkg) | ||
| if isLazyPkg(pkg) { | ||
| lazyPkgs = append(lazyPkgs, pkg) | ||
| } else { | ||
| opkgs = append(opkgs, pkg) | ||
| cpkgs = append(cpkgs, pkg) | ||
| } | ||
| } | ||
| log.Println(cpkgs) | ||
| log.Println("lazy", lazyPkgs) | ||
|
|
||
| // common pkgs | ||
| // pkgs | ||
| cmd := exec.Command("qexp", "-outdir", ".", "-addtags", tags, "-filename", name) | ||
| cmd.Args = append(cmd.Args, cpkgs...) |
There was a problem hiding this comment.
The local variable lazyPkgs shadows the package-level map lazyPkgs. This makes the code harder to maintain and can lead to confusion. Additionally, renaming cpkgs to something more descriptive like eagerPkgs would improve clarity. Consider renaming these variables to avoid shadowing and improve readability.
References
- Use descriptive variable names that clearly indicate their purpose (e.g.,
eagerPkgsinstead ofcpkgs). (link) - Avoid shadowing package-level variables with local variables to prevent confusion and potential bugs.
| cmd = exec.Command("qexp", "-outdir", ".", "-addtags", tags, "-filename", name) | ||
| cmd.Args = append(cmd.Args, "-lazy") | ||
| cmd.Args = append(cmd.Args, opkgs...) | ||
| cmd.Args = append(cmd.Args, lazyPkgs...) |
There was a problem hiding this comment.
Update to use the renamed lazyPkgList variable to match the suggested changes in the previous hunk.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: "export lazy pkgs"
This PR inverts the lazy/eager registration strategy: previously a small commonPkgs allowlist was registered eagerly and everything else lazily; now a large lazyPkgs list is registered lazily and everything else eagerly. The intent is clear and the generated export files are consistent for go1.24–go1.26. A few issues are worth addressing.
Critical
- Variable shadowing (
pkg/gen.go):var lazyPkgs []stringinsidemain()silently shadows the package-levelvar lazyPkgs = map[string]bool{...}. The compiler does not warn. Renaming the local slice (e.g.lazyPkgList) would eliminate the ambiguity.
Important
unicodehandled outside its own map:"unicode"is commented out oflazyPkgsbut hardcoded inisLazyPkg()with no explanation. The likely reason is that keeping it in the map would cause the prefix-match path to also makeunicode/utf8andunicode/utf16lazy — but this intent is invisible to readers. Either restore the entry with a comment explaining whyutf8/utf16should remain eager, or document the special-case.- Inconsistent state in older go1.14–go1.23 export files:
isLazyPkgnow classifiesunicode,net/*,log,regexp, etc. as lazy, but only the three newest version files were regenerated. All go1.14–go1.23 export files for these packages still useRegisterPackage(eager). If intentional, a note would help clarify.
Minor
cpkgsis now a misleading name: it previously stood for "common packages". It now holds the non-lazy (eager) packages.eagerPkgswould pair cleanly withlazyPkgs.- Deprecated
io/ioutil.WriteFile: the generator usesioutil.WriteFile;os.WriteFilehas been preferred since Go 1.16. - Empty PR description: given the scope (inverted semantics + hundreds of regenerated files), a brief motivation note would help future
git logreaders.
| npkgs := normalPkgs(pkgs) | ||
| var cpkgs []string | ||
| var opkgs []string | ||
| var lazyPkgs []string |
There was a problem hiding this comment.
The local variable lazyPkgs []string shadows the package-level var lazyPkgs = map[string]bool{...} declared above. Go permits this but it is a readability trap — a reader scanning main() who sees lazyPkgs being appended and passed to qexp -lazy cannot tell at a glance that there is a separate map with the same name. Consider renaming this slice to lazyPkgList (or selectedLazyPkgs) to make the distinction explicit.
| "os/exec": true, | ||
| "os/signal": true, | ||
| "os/user": true, | ||
| //"unicode": true, |
There was a problem hiding this comment.
"unicode" is commented out here but then hardcoded as a special case in isLazyPkg() below. Without a comment explaining why, this looks like an oversight. The likely reason is that placing "unicode" in the map would cause the prefix-match path (lines 86–90) to also classify unicode/utf8 and unicode/utf16 as lazy — which is apparently not desired. Consider adding:
// "unicode" is handled as a special case in isLazyPkg.
// Placing it here would cause the prefix-match path to also
// classify unicode/utf8 and unicode/utf16 as lazy.
// "unicode": true,| } | ||
|
|
||
| func isLazyPkg(pkg string) bool { | ||
| if pkg == "unicode" { |
There was a problem hiding this comment.
This hardcoded special-case for unicode (bypassing the map) deserves a short comment explaining why it is not simply an entry in lazyPkgs. Without it, the asymmetry between unicode and all other lazy packages looks like an unintentional inconsistency.
| log.Println(pkgs) | ||
|
|
||
| npkgs := normalPkgs(pkgs) | ||
| var cpkgs []string |
There was a problem hiding this comment.
cpkgs previously stood for "common packages". After renaming commonPkgs → lazyPkgs, this variable now holds the eager (non-lazy) packages, making the c prefix opaque. Renaming to eagerPkgs would be self-documenting and pair cleanly with lazyPkgs / lazyPkgList.
1 participant
No description provided.