feat: simplified enum type declaration - Phase 2#2779
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for compiling enum types (declared as type Name const (...)) in XGo, updating the preloading and loading mechanisms in cl/compile.go and cl/stmt.go, and adding corresponding test cases. The review feedback highlights two potential panic vulnerabilities: a nil pointer dereference when retrieving a type from the scope, and an out-of-bounds access when inferring the underlying type of an empty or malformed enum declaration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2779 +/- ##
==========================================
+ Coverage 94.11% 94.12% +0.01%
==========================================
Files 32 32
Lines 10147 10166 +19
==========================================
+ Hits 9550 9569 +19
Misses 427 427
Partials 170 170 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Summary
Good incremental step — the approach of threading an optional typ through loadConstSpecs/loadConsts and using a lazy getEnumTyp callback avoids duplicating const-loading logic, which is the right call. Two correctness issues need attention before merging.
Two compiler-panic risks in inferEnumUnderlyingType
enumType.Specs[0] and spec.Values[0] are both unguarded. An empty enum body (type Foo const ()) or a first member with no initializer (parse-error recovery can produce Values == nil) reaches these lines and panics the compiler instead of emitting a diagnostic. The fix is cheap guards:
func inferEnumUnderlyingType(ctx *blockCtx, enumType *ast.EnumType) types.Type {
if len(enumType.Specs) == 0 {
return types.Typ[types.Int] // fallback after parse error
}
spec := enumType.Specs[0].(*ast.ValueSpec)
if len(spec.Values) == 0 {
return types.Typ[types.Int]
}
compileExpr(ctx, 1, spec.Values[0])
return types.Default(ctx.cb.InternalStack().Pop().Type)
}Enum type silently overwritten after error in loadConsts
After ctx.handleErrorf(...) returns, execution falls through unconditionally to typ = toType(ctx, v.Type), overwriting the enum type with the user-supplied per-const type. The compiler then emits the constant with the wrong type despite having reported the error. Fix with else:
if v.Type != nil {
if typ != nil {
ctx.handleErrorf(v.Type.Pos(), v.Type.End(), "const type should be omitted for enum const")
} else {
typ = toType(ctx, v.Type)
}
}Scope().Lookup(name).Type() is nil-unsafe
Lookup returns nil when the name is absent (e.g. due to a prior error in doNewType). A nil .Type() call panics. Recommend a guard:
obj := ctx.pkg.Types.Scope().Lookup(name)
if obj == nil {
return nil
}
return obj.Type()Missing negative test coverage
The error diagnostic "const type should be omitted for enum const" and the empty-body case have no test fixtures. Given the fall-through bug in finding #2, at minimum one fixture exercising an enum const with an explicit type annotation is needed.
Minor: redundant expression compilation
inferEnumUnderlyingType compiles spec.Values[0] once to infer the type and then discards the result via Pop(). The same expression is compiled again when loadConsts processes the first member. For typical enum initializers this is negligible, but worth confirming that the double-compile of Values[0] does not produce duplicate recorder entries or diagnostics.
#2751