Conversation
Summary of ChangesHello @xushiwei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
==========================================
+ Coverage 96.04% 96.07% +0.02%
==========================================
Files 24 25 +1
Lines 6727 6778 +51
==========================================
+ Hits 6461 6512 +51
Misses 198 198
Partials 68 68 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a NewTuple function to create tuple-like struct types. It also adds a "virtual fields" mechanism to allow accessing the tuple elements by their original names, not just by their ordinal _i names. The implementation is clean and the accompanying test case verifies the functionality correctly. I've made a few suggestions in type_tuple.go to improve code clarity and extensibility, mainly around using an interface for virtual fields and cleaning up some commented-out code.
| /* | ||
| // vFields defines the interface for virtual fields of a struct type. | ||
| type vFields interface { // virtual fields | ||
| FindField(cb *CodeBuilder, t *types.Struct, name string, arg *Element, src ast.Node) bool | ||
| FieldRef(cb *CodeBuilder, t *types.Struct, name string, x ast.Expr, src ast.Node) bool | ||
| } | ||
| */ | ||
|
|
||
| type vFields = *tupleFields |
There was a problem hiding this comment.
The vFields interface is commented out and a type alias is used instead. This limits the extensibility of the virtual fields mechanism. It's recommended to use the interface to allow for different kinds of virtual fields in the future. This would make the design more robust and open for extension.
// vFields defines the interface for virtual fields of a struct type.
type vFields interface { // virtual fields
FindField(cb *CodeBuilder, t *types.Struct, name string, arg *Element, src ast.Node) bool
FieldRef(cb *CodeBuilder, t *types.Struct, name string, x ast.Expr, src ast.Node) bool
}| /* | ||
| func (p *Package) vFields(t *types.Struct) (vft vFields, ok bool) { | ||
| vft, ok = p.cb.vfts[t] | ||
| return | ||
| } | ||
| */ |
| fields []*types.Var | ||
| } | ||
|
|
||
| func (p *tupleFields) FindField(cb *CodeBuilder, t *types.Struct, name string, arg *Element, src ast.Node) bool { |
| return false | ||
| } | ||
|
|
||
| func (p *tupleFields) FieldRef(cb *CodeBuilder, t *types.Struct, name string, x ast.Expr, src ast.Node) bool { |
| } | ||
| } | ||
| if p.refVField(o, name, x, src) { | ||
| return true |
There was a problem hiding this comment.
Virtual field placement issue: Virtual fields are checked after direct fields but before embedded fields. This breaks Go's field resolution semantics where embedded fields are promoted to the same level as direct fields.
If a tuple with virtual field "x" embeds a struct that also has field "x", the virtual field will incorrectly shadow the embedded field.
Recommendation: Move this virtual field check to after embedded field recursion (after the loop at lines 1587-1599) to match Go's semantics.
| for i, fld := range p.fields { | ||
| if fld.Name() == name { | ||
| cb.stk.Ret(1, &Element{ | ||
| Val: selector(arg, tupleFieldName(i)), |
There was a problem hiding this comment.
Missing nil safety check: The selector() function will panic if arg is nil. Consider adding a nil check:
if arg == nil {
return false
}This defensive check would prevent potential nil pointer dereferences.
| return false | ||
| } | ||
|
|
||
| // NewTuple creates a tuple type with the given fields. |
There was a problem hiding this comment.
Documentation clarity: The Godoc should clarify that with withName=true, fields can be accessed by BOTH their original names (e.g., "x", "y") AND ordinal names ("_0", "_1"). The current comment doesn't make this dual-access capability clear.
Suggested addition:
// NewTuple creates a tuple type with the given fields.
// The fields are always accessible by ordinal names: _0, _1, ...
// If withName is true, the fields are ALSO accessible by their original names
// through a virtual fields mechanism.| // virtual fields mechanism. | ||
| func (p *Package) NewTuple(withName bool, fields ...*types.Var) *types.Struct { | ||
| ordinals := make([]*types.Var, len(fields)) | ||
| for i, fld := range fields { |
There was a problem hiding this comment.
Input validation: Consider adding validation for nil fields to make debugging easier:
for i, fld := range fields {
if fld == nil {
panic("NewTuple: nil field at index " + strconv.Itoa(i))
}
// ...
}Also consider documenting the behavior when multiple fields have the same name (first match wins).
Code Review SummaryThis PR implements a clean virtual fields mechanism for tuple types. The implementation is well-structured with minimal invasiveness (only 3 integration points in codebuild.go). However, there's one important issue with field resolution order that should be addressed. Key concerns:
Positive aspects: Clean separation of concerns, proper type checking, minimal memory overhead, good recorder integration. |
No description provided.