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 refactors the internal 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
|
![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 refactors ClassDefs to support embedded fields. The Fields and Tags slices have been removed from the ClassDefs struct and replaced with an addFld callback function, which decouples field management from ClassDefs. This is a good design improvement. The NewAndInit method is updated to handle embedded fields, and new tests have been added to cover this functionality. My main feedback is to add a check in NewAndInit to enforce the documented contract that fn and typ cannot both be nil.
| func (p *ClassDefs) NewAndInit(fn F, tag string, pos token.Pos, typ types.Type, names ...string) { | ||
| // initializes them by `fn` (can be nil). `typ` and `fn` can not be both nil. | ||
| func (p *ClassDefs) NewAndInit(fn F, pos token.Pos, typ types.Type, names ...string) { | ||
| pkg := p.pkg |
There was a problem hiding this comment.
The function comment on line 596 states that typ and fn cannot both be nil. However, the function body does not enforce this, which could lead to addFld being called with a nil type, potentially causing issues later. It's good practice to enforce this contract at the beginning of the function.
if fn == nil && typ == nil {
panic("gogen.NewAndInit: fn and typ cannot both be nil")
}
pkg := p.pkg
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #563 +/- ##
==========================================
- Coverage 96.03% 96.03% -0.01%
==========================================
Files 24 24
Lines 6717 6727 +10
==========================================
+ Hits 6451 6460 +9
- Misses 198 199 +1
Partials 68 68 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| return t.Obj().Name() | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Missing nil validation and interface type handling
The embedName function should validate the input and handle interface types explicitly:
- Nil check: If
typis nil, this will panic on type assertion - Interface types: Named interfaces are handled by
*types.Named, but unnamed interface types (e.g.,interface{ Read() }) will silently return"", which could cause issues - Nil Obj() check:
t.Obj()could potentially return nil for Named/Alias types
Recommendation:
func embedName(typ types.Type) string {
if typ == nil {
return ""
}
if t, ok := typ.(*types.Pointer); ok {
typ = t.Elem()
}
switch typ := typ.(type) {
case *types.Basic:
return typ.Name()
case *types.Named:
if obj := typ.Obj(); obj != nil {
return obj.Name()
}
case *typesalias.Alias:
if obj := typ.Obj(); obj != nil {
return obj.Name()
}
}
// Return empty or panic for unsupported types
return ""
}
type_var_and_const.go
Outdated
| // NewAndInit creates variables with specified `typ` (can be nil) and `names`, and | ||
| // initializes them by `fn` (can be nil). | ||
| func (p *ClassDefs) NewAndInit(fn F, tag string, pos token.Pos, typ types.Type, names ...string) { | ||
| // initializes them by `fn` (can be nil). `typ` and `fn` can not be both nil. |
There was a problem hiding this comment.
Unclear documentation for parameter constraints
The current documentation is ambiguous. Based on the implementation:
- When
fn == nil:typmust NOT be nil - When
fn != nil:typCAN be nil (inferred from initialization)
Recommendation:
// NewAndInit creates variables with specified `typ` (can be nil) and `names`, and
// initializes them by `fn` (can be nil). When `fn` is nil (no initialization),
// `typ` must not be nil. When names is empty, creates an embedded field.Also consider enforcing this programmatically:
if typ == nil && fn == nil {
panic("NewAndInit: typ and fn cannot both be nil")
}
| p.Fields = append(p.Fields, fld) | ||
| p.Tags = append(p.Tags, tag) | ||
| if embed { | ||
| p.addFld(0, embedName(typ), typ, true) |
There was a problem hiding this comment.
Potential nil function pointer dereference
The code calls p.addFld() without validating that p.addFld is non-nil. If ClassDefsStart is called with addFld = nil, this will cause a panic.
Recommendation: Add validation in ClassDefsStart:
func (p *Package) ClassDefsStart(
recv *types.Var,
addFld func(idx int, name string, typ types.Type, embed bool),
) *ClassDefs {
if addFld == nil {
panic("ClassDefsStart: addFld callback must not be nil")
}
// ...
}
| scope := cb.current.scope | ||
| recvName := ident(recv.Name()) | ||
| if embed { | ||
| names = []string{embedName(typ)} |
There was a problem hiding this comment.
Validate embedName result
When embedName(typ) returns an empty string (for unsupported types), creating a field with an empty name could violate Go's struct field naming requirements.
Recommendation:
if embed {
name := embedName(typ)
if name == "" {
panic("invalid embedded type: cannot determine field name")
}
names = []string{name}
}
| ) *ClassDefs { | ||
| if debugInstr { | ||
| log.Println("ClassDefsStart") | ||
| } |
There was a problem hiding this comment.
Missing parameter documentation
The addFld callback parameter needs documentation explaining its purpose and parameters:
Recommendation:
// ClassDefsStart starts a classfile fields declaration block.
// The addFld callback is invoked for each field added via NewAndInit:
// - idx: field index within the current NewAndInit call
// - name: field name (or type name for embedded fields)
// - typ: field type
// - embed: true if this is an embedded field
// recv is the receiver variable for the generated XGo_Init method.
// Should call (*ClassDefs).End() to end it.
Code Review SummaryThis PR successfully adds embedded field support to Key strengths:
Main concerns to address:
All specific issues have been commented inline. Overall, this is a well-executed refactoring that needs minor hardening around edge cases. |
1 participant
No description provided.