feat: pygen support class #46
Conversation
modify mappingproxy to dict method
Summary of ChangesHello @toaction, 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 enhances the Highlights
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 adds support for generating Go bindings for Python classes. The changes include parsing class structures in pydump and generating corresponding Go code in pygen.
The implementation is comprehensive, covering methods, properties, and inheritance. However, I've found a few critical issues related to potential panics from nil pointer dereferences, which should be addressed. I've also identified a bug in how property link names are generated. Additionally, there are some opportunities for code improvement regarding consistency and avoiding side effects.
Overall, this is a great feature addition. Please see my detailed comments for suggestions.
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| base := &symbol.Base{ | ||
| Name: c.GoString(baseObj.GetAttrString(c.Str("__name__")).CStr()), | ||
| Module: c.GoString(baseObj.GetAttrString(c.Str("__module__")).CStr()), |
There was a problem hiding this comment.
Critical: Nil pointer dereference risk
GetAttrString() can return nil, which would cause a panic when calling .CStr(). This needs defensive error handling:
| base := &symbol.Base{ | |
| Name: c.GoString(baseObj.GetAttrString(c.Str("__name__")).CStr()), | |
| Module: c.GoString(baseObj.GetAttrString(c.Str("__module__")).CStr()), | |
| nameObj := baseObj.GetAttrString(c.Str("__name__")) | |
| if nameObj == nil { | |
| return nil, fmt.Errorf("base class of %s missing __name__ attribute", name) | |
| } | |
| defer nameObj.DecRef() | |
| moduleObj := baseObj.GetAttrString(c.Str("__module__")) | |
| if moduleObj == nil { | |
| return nil, fmt.Errorf("base class of %s missing __module__ attribute", name) | |
| } | |
| defer moduleObj.DecRef() | |
| base := &symbol.Base{ | |
| Name: c.GoString(nameObj.CStr()), | |
| Module: c.GoString(moduleObj.CStr()), |
Additionally: Missing DecRef() calls cause memory leaks. Python C API objects need explicit reference counting management.
|
|
||
| // get class parents | ||
| func parseBases(class *py.Object, name string) ([]*symbol.Base, error) { | ||
| basesObj := class.GetAttrString(c.Str("__bases__")) // tuple |
There was a problem hiding this comment.
Critical: Memory leak - missing DecRef()
All Python objects returned by GetAttrString() need their reference counts decremented:
| basesObj := class.GetAttrString(c.Str("__bases__")) // tuple | |
| basesObj := class.GetAttrString(c.Str("__bases__")) // tuple | |
| if basesObj == nil { | |
| return nil, fmt.Errorf("can't get __bases__ from %s", name) | |
| } | |
| defer basesObj.DecRef() |
| func (ctx *context) genProperties(pkg *gogen.Package, cls *symbol.Class) { | ||
| for _, property := range cls.Properties { | ||
| name := cls.Name + property.Name | ||
| if property.Getter != "" { |
There was a problem hiding this comment.
Critical bug: Missing dot separator in property name
This generates incorrect linkname directives like py.Dogage.__get__ instead of py.Dog.age.__get__. Missing dot causes runtime linking failures.
| if property.Getter != "" { | |
| name := cls.Name + "." + property.Name |
| for _, cls := range classes { | ||
| if len(cls.Bases) == 0 { | ||
| log.Fatalf("Class %s has no base", cls.Name) |
There was a problem hiding this comment.
Code quality: Use error return instead of log.Fatal
log.Fatalf terminates the entire program. In library code, return an error to let the caller decide how to handle it:
| for _, cls := range classes { | |
| if len(cls.Bases) == 0 { | |
| log.Fatalf("Class %s has no base", cls.Name) | |
| for _, cls := range classes { | |
| if len(cls.Bases) == 0 { | |
| return fmt.Errorf("class %s has no base", cls.Name) | |
| } |
This requires changing genClasses signature to return error.
| switch typeName { | ||
| case "classmethod": | ||
| val = val.GetAttrString(c.Str("__func__")) | ||
| if val == nil { | ||
| return nil, fmt.Errorf("can't get __func__ of %s", name) | ||
| } | ||
| sym, err := parseMethod(val, name, typeName, true) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cls.ClassMethods = append(cls.ClassMethods, sym) | ||
| case "staticmethod": | ||
| val = val.GetAttrString(c.Str("__func__")) | ||
| if val == nil { |
There was a problem hiding this comment.
Code quality: Code duplication for classmethod/staticmethod
Both cases duplicate the __func__ retrieval pattern. Extract to a helper:
func getFunc(obj *py.Object, name string) (*py.Object, error) {
funcObj := obj.GetAttrString(c.Str("__func__"))
if funcObj == nil {
return nil, fmt.Errorf("can't get __func__ of %s", name)
}
return funcObj, nil
}Then use: val, err := getFunc(val, name)
| getter := val.GetAttrString(c.Str("fget")) | ||
| if getter != nil { | ||
| // (self) -> value | ||
| property.Getter = "()" | ||
| } |
There was a problem hiding this comment.
Code quality: Property getter signature hardcoded
The getter signature is hardcoded to "()" without parsing the actual function signature, losing type information:
| getter := val.GetAttrString(c.Str("fget")) | |
| if getter != nil { | |
| // (self) -> value | |
| property.Getter = "()" | |
| } | |
| getter := val.GetAttrString(c.Str("fget")) | |
| if getter != nil { | |
| defer getter.DecRef() | |
| sym := &symbol.Symbol{ | |
| Name: name, | |
| Type: "property", | |
| } | |
| sig, err := getSignature(getter, sym, true) | |
| if err != nil { | |
| return nil, err | |
| } | |
| property.Getter = sig |
| objPtr *types.Pointer | ||
| ret *types.Tuple | ||
| py gogen.PkgRef | ||
| structs map[string]*types.Named |
There was a problem hiding this comment.
Code style: Inconsistent spacing
Use spaces consistently (tabs mixed with spaces):
| structs map[string]*types.Named | |
| structs map[string]*types.Named |
Code Review SummaryGood work adding Python class support! The implementation is well-structured with clear separation between parsing and generation. However, critical issues must be fixed before merge: 🔴 Must Fix
🟡 Should Fix
✅ Strengths
|
1 participant
fix #47
[require]feat: pydump support Python defined class: #44