Skip to content

Conversation

visualfc
Copy link
Member

@visualfc visualfc commented Sep 10, 2025

type Animal struct {
    py.Object
}

//go:linkname NewAnimal py.Animal
func NewAnimal(name *Animal) *Animal

//llgo:link (*Animal).Speak py.Animal.speak
func (p *Animal) Speak() {}

//llgo:link (*Animal).Age py.Animal.age.__get__
func (p *Animal) Age() *py.Object { return nil}

//llgo:link (*Animal).SetAge py.Animal.age.__set__
func (p *Animal) SetAge(age *py.Object)

//go:linkname DogName py.Dog.DOGNAME
var DogName *py.Object

//go:linkname DogClass py.Dog
var DogClass *py.Object

Copy link

niupilot bot commented Sep 10, 2025

Code Review for PR #1289

Todo List:

  • Analyze context and gather information about changed files
  • Read and understand cl/instr.go changes
  • Read and understand ssa/python.go changes
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate architecture and design decisions
  • Verify documentation consistency
  • Provide comprehensive review feedback

Summary

This PR makes two related changes to Python function symbol handling:

  1. Symbol separator change: Changes Python function symbol naming from dot-separated (.) to dollar-separated ($) format
  2. Method signature handling: Adds receiver context handling for Python functions with receivers

Detailed Review

Positive Aspects

Consistent Symbol Format: The changes ensure consistent use of $ as the separator for Python symbols throughout the codebase. This aligns with the existing pysymPrefix pattern and makes symbol parsing more uniform.

Proper Method Handling: Adding receiver context to method signatures in PyNewFunc is architecturally sound and ensures method signatures are properly handled.


🔍 Code Quality & Best Practices

cl/instr.go:439

// BEFORE
fnName := pysymPrefix + mod + "." + name

// AFTER  
fnName := pysymPrefix + mod + "$" + name

Good: This change maintains consistency with the modOf function which already expects $ separator.

ssa/python.go:597

// NEW CODE
if recv := sig.Recv(); recv != nil {
    sig = FuncAddCtx(recv, sig)
}

Good: Proper nil check before calling FuncAddCtx. This follows the existing pattern used elsewhere in the codebase (see ssa/type_cvt.go:64,191).

ssa/python.go:643

// BEFORE
if pos := strings.LastIndexByte(name, '.'); pos > 0 {

// AFTER
if pos := strings.LastIndexByte(name, '$'); pos > 0 {

Good: Critical fix that makes modOf function work correctly with the new symbol format.


🚨 Potential Issues

1. Breaking Change Risk
This appears to be a breaking change for any existing Python modules that depend on the dot-separated symbol format. Consider:

  • Are there existing Python modules that might break?
  • Is migration handling needed for backward compatibility?

2. Symbol Collision Risk
Using $ as a separator could potentially create conflicts if module names or function names contain $ characters. The code should validate that module/function names don't contain the separator character.

3. Missing Edge Case Handling
The modOf function doesn't handle cases where the name doesn't contain a $ character properly - it will panic with "unreachable". Consider adding validation or better error handling.


💡 Suggestions for Improvement

1. Add Input Validation

// In modOf function, consider adding validation:
func modOf(name string) string {
    if pos := strings.LastIndexByte(name, '$'); pos > 0 {
        return name[:pos]
    }
    // Better error message with the actual name
    panic(fmt.Sprintf("invalid python symbol name format: %q, expected format: 'module$function'", name))
}

2. Consider Symbol Escaping
If module or function names might contain $, consider implementing an escaping mechanism or using a different separator that's less likely to appear in identifiers.


📚 Documentation Consistency

No documentation updates needed: This appears to be an internal symbol format change that doesn't affect the public API. The change is consistent with existing internal patterns.


🏗️ Architecture & Design

Sound architectural decision: The changes improve consistency in symbol handling and ensure proper method signature processing. The modifications align well with the existing LLGO Python integration design.

The receiver context addition ensures that:

  • Method signatures retain receiver information
  • Proper context is maintained for Python method calls
  • Consistency with other parts of the codebase that handle receivers

🧪 Testing Recommendations

Consider adding tests for:

  1. Python function symbol generation with the new $ format
  2. Method receiver handling in Python functions
  3. Edge cases in modOf function parsing
  4. Backward compatibility verification (if needed)

Overall Assessment

✅ This is a well-implemented change that improves symbol consistency and adds necessary method receiver handling. The code quality is good and follows existing patterns in the codebase.

Key strengths:

  • Consistent symbol formatting
  • Proper receiver handling for methods
  • Follows established coding patterns
  • No breaking changes to public APIs

Areas for consideration:

  • Potential breaking changes for existing Python modules
  • Input validation for symbol names
  • Better error handling in edge cases

The changes appear to be part of ongoing improvements to LLGO's Python integration and are architecturally sound.


👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @visualfc, 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 introduces initial support for Python function receivers and refactors the internal symbol naming convention for Python functions. These changes aim to enhance the interoperability between Go and Python by allowing more complex Python constructs to be represented and processed correctly.

Highlights

  • Python Function Naming: Modified the internal naming convention for Python functions by replacing the "." separator with "$" in generated symbols, improving symbol management.
  • Receiver Support for Python Functions: Introduced logic to correctly handle function signatures that include a receiver within PyNewFunc, enabling proper integration of receiver-bound Python methods.
  • Module Name Parsing Update: Updated the modOf utility function to align with the new "$" symbol separator for parsing module names, ensuring consistency across the codebase.
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for handling the recv operation in the Python backend of the compiler. It modifies the instr.go file to change the function name prefix and updates python.go to handle the recv signature and module symbol loading. The changes aim to improve the compiler's ability to interact with Python code, particularly in scenarios involving asynchronous communication.

Copy link

codecov bot commented Sep 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (1ee4da9) to head (e27e090).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1289      +/-   ##
==========================================
- Coverage   90.18%   90.13%   -0.06%     
==========================================
  Files          43       43              
  Lines       12674    12679       +5     
==========================================
- Hits        11430    11428       -2     
- Misses       1087     1094       +7     
  Partials      157      157              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@visualfc visualfc changed the title [WIP] python: support recv python: support recv method Sep 10, 2025
@visualfc visualfc force-pushed the python_method branch 4 times, most recently from 1a053a0 to ae87ced Compare September 11, 2025 01:16
@toaction
Copy link

无法获取类属性

类声明:

class Dog(Animal):

    DOGNAME = "Dog"

    def __init__(self, name, age):
        self._name = name
        self._age = age

    @property
    def age(self):
        return self._age

LLGo Bindings:

type Dog struct {
	py.Object
}

//go:linkname NewDog py.Dog
func NewDog(name *py.Object, age *py.Object) *Dog


//llgo:link (*Dog).Age py.Dog.age.__get__
func (p *Dog) Age() *py.Object { return nil}


//llgo:link (*Dog).DogName py.Dog.DOGNAME.__get__
func (p *Dog) DogName() *py.Object { return nil}

调用

d := dog1.NewDog(py.Str("Dog"), py.Long(3))
fmt.Println(c.GoString(d.Age().Str().CStr()))
fmt.Println(c.GoString(d.DogName().Str().CStr()))

结果

3
signal: segmentation fault

@visualfc
Copy link
Member Author

visualfc commented Sep 26, 2025

无法获取类属性

类声明:

class Dog(Animal):

    DOGNAME = "Dog"

    def __init__(self, name, age):
        self._name = name
        self._age = age

    @property
    def age(self):
        return self._age

LLGo Bindings:

type Dog struct {
	py.Object
}

//go:linkname NewDog py.Dog
func NewDog(name *py.Object, age *py.Object) *Dog


//llgo:link (*Dog).Age py.Dog.age.__get__
func (p *Dog) Age() *py.Object { return nil}


//llgo:link (*Dog).DogName py.Dog.DOGNAME.__get__
func (p *Dog) DogName() *py.Object { return nil}

调用

d := dog1.NewDog(py.Str("Dog"), py.Long(3))
fmt.Println(c.GoString(d.Age().Str().CStr()))
fmt.Println(c.GoString(d.DogName().Str().CStr()))

结果

3
signal: segmentation fault

//go:linkname DogName py.Dog.DOGNAME
var DogName *py.Object
( DogName is readonly )

//go:linkname DogClass py.Dog
var DogClass *py.Object
( DogClass.GetAttr(py.Str("DOGNAME")) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

py.Object size for Multiple inheritance
2 participants