rm unboundType, unboundMapElemType; rm cb.NewAutoVar - #628
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the unboundType and unboundMapElemType types, along with the NewAutoVar function and all associated logic across the codebase. This cleanup simplifies type handling, particularly in ast.go, builtin.go, codebuild.go, and util_gengo.go, and updates the test suite to reflect these removals. I have no additional feedback to provide as there are no review comments.
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 #628 +/- ##
==========================================
+ Coverage 93.20% 93.58% +0.38%
==========================================
Files 29 29
Lines 7195 7113 -82
==========================================
- Hits 6706 6657 -49
+ Misses 415 388 -27
+ Partials 74 68 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Summary
This PR cleanly removes the unboundType/unboundMapElemType deferred-type-resolution mechanism and replaces NewAutoVar with explicit NewVar + string-based VarRef/VarVal. The structural deletions are thorough and consistent. Two correctness issues need attention before merge.
Confirmed issues:
-
zeroCompositeLittyp0out-parameter is silently dead — the caller (codebuild.go:770) readstyp0as theTypeof the returnedElement, but the new implementation never writes to*typ0. This is a latent correctness bug: the returned element's type may be stale/wrong for named/aliased types wheretyp0was supposed to be updated (previously theunboundTypebranch wrote*typ0 = t.tBound). Thetyp0parameter should either be removed from the signature (if genuinely unused in all paths) or the implementation should assign*typ0 = typbefore returning. -
Dropped test coverage for generic-function-result assignment (
typeparams_test.go) — the case exercisingfn1 = foo.Loader[*int, int]/fn1(nil, 1)is deleted rather than migrated. The scenario (generic instantiation result stored in a typed variable and then called) is distinct from the otherNewVarStart-based cases and should be re-expressed withNewVar(tySig, "fn1")+VarRef("fn1").
Minor / informational:
-
Redeclaration test removed without replacement (
error_msg_test.go) — thefoo redeclared in this blocksub-test inTestErrNewVaris dropped. Recommend re-adding an equivalent case usingNewVar/NewVarStartso theprevious declaration at ...error path stays covered. -
realTypecan be simplified (type_ext.go) — with onecaseremaining, a plainiftype-assertion is clearer than atype switch. -
VarRef/VarValuse position-insensitiveLookupParent— position istoken.NoPos, so shadowing semantics differ from a positioned lookup. Worth a one-line doc note.
| default: | ||
| ret.Type = toType(p, typ) | ||
| return &ast.CompositeLit{ | ||
| Type: toType(p, typ), |
There was a problem hiding this comment.
typ0 is passed by the caller to receive the resolved type (codebuild.go uses it as Element.Type), but this implementation never writes to *typ0. Remove the parameter if it is truly dead, or add *typ0 = typ before returning to preserve the caller's expectation.
| @@ -523,8 +522,6 @@ var MyInts = Int{1,2,3,4} | |||
| NewVarStart(tyInt, "n7").Val(fnAdd).Typ(tyString).Typ(tyInt).Index(2, 0).Val("hello").Val(1).Val(2).Val(3).SliceLit(tyIntSlice, 3).CallWith(2, 0, gogen.InstrFlagEllipsis).EndInit(1). | |||
There was a problem hiding this comment.
The fn1 case (generic instantiation result assigned to a typed variable and then called) was testing a distinct code-gen path. Please re-express it with NewVar(tySig, "fn1") + VarRef("fn1") rather than dropping the coverage entirely.
| @@ -321,14 +321,6 @@ func TestErrConst(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestErrNewVar(t *testing.T) { | |||
There was a problem hiding this comment.
The redeclaration sub-test (foo redeclared in this block ... previous declaration at ...) is removed with no equivalent replacement. Consider re-adding it using NewVar/NewVarStart so this error path and its position formatting stay tested.
No description provided.