qexp: export types.Alias to vars#448
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 introduces a mechanism to manage and export type aliases by refactoring alias-related logic into a new Alias struct and generating corresponding initialization code. The exportPkg function was updated to support an $INIT placeholder in templates for alias registration. Review feedback suggests updating the IsEmpty method to include the Alias field to ensure packages containing only aliases are correctly processed, and correcting a misleading comment in the Alias struct definition.
| Source string | ||
| usedPkg bool | ||
| TypesData []byte | ||
| AliasInit string |
There was a problem hiding this comment.
The IsEmpty method (defined at line 195) should be updated to include a check for the Alias field. Currently, a package that only contains aliases will be incorrectly identified as empty, which causes it to use the template_empty_pkg template that lacks the necessary alias registration logic.
|
|
||
| type Alias struct { | ||
| alias map[string]*aliasVar // alias string => var | ||
| used map[string]bool // used alias name => count |
There was a problem hiding this comment.
The comment for the used field is misleading. It states => count, but the map value type is bool. It should be updated to accurately describe the field's purpose as a set of used names.
| used map[string]bool // used alias name => count | |
| used map[string]bool // used alias name |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: qexp: export types.Alias to vars
This PR refactors the alias* free functions into an Alias struct with state, enabling alias variables to be collected and emitted as package-level var declarations via a new $INIT template placeholder. The approach is sound, but there are a few issues worth addressing.
|
|
||
| type Alias struct { | ||
| alias map[string]*aliasVar // alias string => var | ||
| used map[string]bool // used alias name => count |
There was a problem hiding this comment.
The used field comment says "used alias name => count" but the type is map[string]bool, not a count map. This appears to be a copy-paste from count. Also, the count field has no comment at all.
Suggested fix:
used map[string]bool // set of generated variable names already in use
count map[string]int // collision counter per base name| func aliasNamed(t *types.Named, pkg *types.Package) (string, bool) { | ||
| type aliasVar struct { | ||
| name string // alias var name | ||
| info string // &alias.Alias{} / &alias.Builtin{} |
There was a problem hiding this comment.
The exported Alias type and NewAlias constructor are inconsistent with the rest of the file — all other helpers (aliasVar, aliasNamed, aliasType, etc.) are unexported. Alias is only instantiated from within this package, so exporting it creates an unintentional API surface without any documentation or test coverage. Consider lowercasing to alias / newAlias.
| count map[string]int | ||
| } | ||
|
|
||
| func NewAlias() *Alias { |
There was a problem hiding this comment.
The redundant used + count dual-map design can be simplified. Since p.count[base] > 0 already implies the name has been seen, used is fully redundant and the deduplication loop can use a single map:
base := info.name
for i := p.count[base]; p.count[info.name] > 0 || (i > 0 && info.name == base); {
...
}Alternatively, just drop used and replace every p.used[x] check with p.count[x] > 0 (after initialising newly-seen names to 1).
| named = t.Obj().Name() | ||
| info := &aliasVar{} | ||
| if opkg == nil { | ||
| info.name = "alias_" + t.Obj().Name() |
There was a problem hiding this comment.
Security / code injection: info.name is assembled from raw type-object and package names with no identifier validity check:
info.name = "alias_" + t.Obj().Name() // line 654
info.name = "alias_" + named // line 660
info.name = "alias_" + opkg.Name() + "_" + t.Obj().Name() // line 663This string is later emitted unquoted as a Go identifier in the generated var (...) block. If t.Obj().Name() or opkg.Name() contains characters that are invalid in a Go identifier (e.g. -, /, spaces), format.Source will reject the output with an opaque error. A go/token.IsIdentifier guard (or equivalent sanitization) on each component before assembly would make this robust and produce a clear error message rather than a format.Source failure.
| } | ||
| if flagExportAlias && len(ma.alias) > 0 { | ||
| var inits []string | ||
| for _, info := range ma.alias { |
There was a problem hiding this comment.
The inits slice is not preallocated. Since len(ma.alias) is already known, prefer:
inits := make([]string, 0, len(ma.alias))This avoids repeated growth copies.
| ) | ||
|
|
||
| func init() { | ||
| func init() {$INIT |
There was a problem hiding this comment.
The $INIT placeholder sits immediately after the opening brace of func init(). When AliasInit is non-empty it expands to a var (...) block, making the var declarations function-local — they are not package-level variables and cannot be referenced outside the function.
The $INIT expansion (the var block) should be placed before the func init() declaration, not inside it, if the intent is to create package-level variables accessible to the rest of the package or by external consumers.
| ) | ||
|
|
||
| func init() { | ||
| func init() {$INIT |
There was a problem hiding this comment.
The empty-package templates (e.g. template_empty_pkg, template_emtpy_link_pkg) do not include the $INIT placeholder. The strings.NewReplacer unconditionally maps "$INIT" → pkg.AliasInit, so the replacement is silently a no-op for those templates. If a package that is considered empty could still have alias types, the AliasInit content would be silently dropped. If this is intentional (empty packages can never have aliases), a comment clarifying the invariant would help.
1 participant
No description provided.