cl: allow same names across different enum types - #2790

Code Review

This pull request introduces support for handling shared enum constants (such as None across multiple enum types) by refactoring symbol loading to use ast.Node instead of token.Pos and implementing automatic renaming of conflicting constants. Feedback on these changes highlights several critical improvements: ensuring shared constants are always loaded even if unreferenced, adding defensive checks in constNameLoader.load() to prevent compiler panics on lookup failures or unsupported types, avoiding potential name collisions in getTempConstName by verifying name uniqueness, and using ctx.handleErrorf instead of panicking on type redeclarations to preserve multi-error reporting.

This PR introduces a clean mechanism for allowing the same constant name across different enum types. The refactoring (replacing positional token.Pos pairs with ast.Node, inlining setNamesLoader, and updating getRecvTypeName to return *ast.Ident) is a straightforward API improvement. Below are the findings worth addressing.

constNameLoader.load() — unchecked type assertion can produce an opaque panic

cl/compile.go, constNameLoader.load() line ~349:

val := p.scope.Lookup(cnp.newName.Name).(*types.Const).Val()

p.scope.Lookup can return nil if the renamed symbol was never registered (e.g., a prior error in cnp.fn() was recovered and execution continued). A nil return causes a nil-pointer runtime.Error which is recovered by the outer defer/recover in NewPackage, but surfaces as a non-positioned error with no source location rather than a clean diagnostic. A guard is needed:

obj := p.scope.Lookup(cnp.newName.Name)
if obj == nil {
    return // earlier error already reported
}
val := obj.(*types.Const).Val()

compileConstVal — raw panic on Float/Complex/Bool produces a non-positioned error

cl/compile.go, compileConstVal() default: branch (~line 330):

panic("unsupported const kind (only string and int) across different enum")

XGo enum constants can be float or bool (e.g., None = 1.5). If two enums share such a name, this bare panic(string) is recovered by the compiler's defer/recover net but surfaced with no source position and an internal-sounding message, unlike the well-positioned handleErrorf message emitted for value mismatches. compileConstVal should receive a source position (or the blockCtx) and call ctx.handleErrorf instead of panicking.

getTypeLoader — panic instead of handleErrorf suppresses subsequent errors

cl/compile.go, getTypeLoader() (~line 408):

panic(ctx.newCodeErrorf(
    at.Pos(), at.End(), "%s redeclared in this block
	previous declaration at %v",
    name, ctx.Position(t.node().Pos())))

The previous code called handleErrorf (which accumulates errors and continues). The panic escapes the local function and is caught by the nearest defer/recover boundary (loadSymbol or the outer NewPackage recover), which aborts processing of that symbol. This means subsequent symbols in the same file are not processed, resulting in fewer error messages per compilation run than before. The test in compile_spx_test.go already demonstrates the consequence: the expected error output was trimmed from three messages to one. This is a UX regression for multi-error scenarios and should be noted; if intentional, a comment explaining why panic is preferred here would help.

getTempConstName — generated names can collide with user identifiers

cl/compile.go, getTempConstName() (~line 368):

return stringutil.Concat("_", name, "_", strconv.Itoa(p.idxConstName))

Names like _None_1 are valid XGo identifiers. If a user declares var _None_1 = ... in the same package, the rename will produce a redeclaration error with a confusing message. Consider a prefix that cannot appear in user source, e.g., a double underscore or a Unicode sentinel.

Minor: addConstNamePos parameter named new shadows the builtin

cl/compile.go, addConstNamePos(name string, old, new *constNamePos): using new as a parameter name shadows Go's built-in new. This is legal but generally avoided in Go style. Consider cur or added.

This is a second review pass after the initial findings were posted. The core PR logic is sound and well-tested for the string/int cases. The new items below are not covered by the existing open threads.

compileConstVal — negative integer constant produces a malformed BasicLit

v.ExactString() on a constant.Int whose value is negative returns e.g. "-1". A Go *ast.BasicLit with Kind: INT and Value: "-1" is not a valid integer literal (a BasicLit must be unsigned; negation requires a *ast.UnaryExpr). This means enum types like None = -1 colliding across two enum types would produce malformed AST that can fail downstream constant evaluation or printing. Consider constructing the literal via constant.Val.String() with an explicit sign check, or use a *goast.UnaryExpr wrapping goast.BasicLit for negative values.

Related: v.ExactString() for constant.String returns a Go-quoted string (e.g. ""weekend day""), which coincidentally is valid Go syntax, but this relies on an undocumented coincidence between go/constant internal representation and Go syntax.

constNameLoader.load() — only the first entry ever emits the shared constant; loop continues past handleErrorf

When constant.Compare(val, NEQ, oldVal) is true (entries > 0), handleErrorf is called but continue is not. The loop presses on, which is harmless today (no later mutation), but if the structure of load() grows, callers should know that an error on entry i does not short-circuit the remaining iterations. More importantly: only entry 0 emits the shared const None = ...; entries 1..N just validate. If for any reason i==0 is also the mismatching entry (impossible today since the first entry is always the anchor), the shared const would never be emitted. This is a latent correctness assumption worth a comment.

getTypeLoader called with nil at at line 894 loses position info and can hit nil dereference

getTypeLoader(ctx, ctx.syms, nil, trecv.Name).load()  // cl/compile.go:894

The purpose of changing getRecvTypeName to return *ast.Ident (instead of string) was to preserve position info through the loader. However, this call at line 894 still passes nil for at, discarding the available trecv ident. More critically: if a later code path triggers the redeclaration panic inside getTypeLoader with at == nil (line 408: at.Pos() would nil-panic before the newCodeErrorf message can be formatted). Pass trecv here to be consistent with the refactoring intent and avoid a nil dereference.

addConstNamePos lazy-initializes ctx.consts inconsistently with the rest of pkgCtx

All other maps on pkgCtx (syms, classes, overpos, lbinames, etc.) are initialized once in the pkgCtx constructor. ctx.consts is nil-checked and allocated inside addConstNamePos on first use. While safe, this is inconsistent with the struct's initialization pattern and is a latent footgun if a second writer is added without the nil guard. Initialize consts centrally alongside the other fields.

In-place AST mutation of ValueSpec.Names is fragile across tooling passes

rename() (line 309) writes directly into spec.Names[p.idx], permanently rewriting the parsed AST node. This is safe within a single-shot compilation, but any tooling that caches or reuses a parsed *ast.File across calls to NewPackage (e.g., language servers, incremental compilers, test harnesses that re-parse the same AST) will see the mangled identifier. Consider threading the renamed identifier through the loader state without mutating the source AST — for instance, storing renamedName string on constNamePos and using it as the effective name only within load() without touching spec.Names.

View job run