Skip to content

docs: add pyclass design#21

Open
toaction wants to merge 3 commits into
goplus:mainfrom
toaction:docs/pyclass
Open

docs: add pyclass design#21
toaction wants to merge 3 commits into
goplus:mainfrom
toaction:docs/pyclass

Conversation

@toaction
Copy link
Copy Markdown
Contributor

@toaction toaction commented Sep 18, 2025

fix #14

llgo initial implementation: xgo-dev/llgo#1289

@niupilot
Copy link
Copy Markdown
Contributor

niupilot Bot commented Sep 18, 2025

Code Review Analysis

I've completed a comprehensive review of the pyclass design documentation. Here's my detailed feedback:

Review Summary

The pyclass design document provides a solid foundation for extending LLGo's Python integration to support classes and objects. However, there are several areas that need improvement regarding technical accuracy, completeness, and consistency.

Critical Issues Found

1. Technical Accuracy Problems

Inconsistent linkname directive usage (doc/pyclass-design.md:80, 88, 93)

  • The document mixes //go:linkname and //llgo:link directives inconsistently
  • Lines 80-81 use //go:linkname for constructor functions
  • Lines 88-89, 93-94 use //llgo:link for methods
  • This inconsistency could confuse implementers about which directive to use

Property linking mechanism unclear (doc/pyclass-design.md:105, 110)

  • Property getter/setter linking uses py.Animal.age.__get__ and py.Animal.age.__set__
  • This doesn't align with standard Python descriptor protocol
  • Python properties typically expose __get__, __set__, __delete__ on the property object itself, not as module-level symbols

2. Design Completeness Issues

Missing error handling strategy

  • No discussion of how Python exceptions should be handled in Go bindings
  • Critical for robust integration between Python and Go

Incomplete multiple inheritance solution

  • The document identifies memory layout problems with multiple inheritance but provides no concrete solution
  • This is a fundamental limitation that needs addressing

Missing discussion of method resolution order (MRO)

  • Python's MRO is crucial for multiple inheritance
  • No explanation of how this translates to Go struct embedding

3. Documentation Structure Issues

Inconsistent language usage

  • Document mixes English and Chinese in examples and comments
  • Should maintain consistent language throughout (preferably English for broader accessibility)

Missing concrete implementation examples

  • The document shows isolated code snippets but lacks complete, working examples
  • Would benefit from end-to-end examples showing the full workflow

4. Missing Architecture Considerations

No discussion of performance implications

  • Converting between Python objects and Go structs has performance costs
  • Should address memory management and garbage collection interactions

Missing type safety considerations

  • How are Python's dynamic types mapped to Go's static types?
  • No discussion of runtime type checking vs compile-time safety

Recommendations for Improvement

  1. Standardize directive usage - Choose either //go:linkname or //llgo:link and use consistently
  2. Clarify property mechanism - Provide detailed explanation of how Python descriptors map to Go methods
  3. Add error handling section - Document how Python exceptions are handled in Go bindings
  4. Expand multiple inheritance discussion - Either provide a solution or explicitly document the limitation
  5. Add complete examples - Include full working examples from Python class to Go usage
  6. Standardize language - Use English throughout for consistency
  7. Add performance considerations - Document memory and performance implications

Minor Issues

  • Missing table of contents for better navigation
  • Some code examples could be more realistic (e.g., the Animal/Dog hierarchy)
  • Cross-references to related LLGo documentation would be helpful

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

Copy link
Copy Markdown

@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 @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 introduces a foundational design document that details the approach for enabling LLGo to interact with Python classes and objects. It outlines a strategy for mapping Python class structures, methods, and properties to their Go equivalents, thereby expanding LLGo's integration capabilities within the Python ecosystem. The document also proactively identifies and discusses complex issues such as multiple inheritance and varied class initialization patterns, setting the stage for future implementation considerations.

Highlights

  • New Design Document: A new document, doc/pyclass-design.md, has been added to outline the design for integrating Python classes and objects with LLGo.
  • Problem Statement: The document addresses the current limitation of LLGo, which only supports calling Python functions and lacks support for handling Python classes and objects.
  • Proposed LLGo Bindings Design: The design proposes converting Python classes to Go structs, implementing single inheritance through embedding, and mapping Python methods (instance, class, static, special) and properties (get/set) to Go struct methods using llgo:link directives.
  • Identified Challenges: The document highlights two main challenges: handling Python's multiple inheritance due to Go's memory layout differences for embedded structs, and resolving ambiguities in class initialization methods (__init__ vs __new__) for both pure Python and C-implemented extension types.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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
Copy Markdown

@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 a design document for integrating Python class support into LLGo. The document is well-structured, covering the conversion of Python classes to Go structs, including methods and properties. It also correctly identifies key challenges like multiple inheritance and object initialization (__init__ vs. __new__). My review focuses on areas where the design could be more complete and specific. I've pointed out the need to clarify the handling of class/static methods and class attributes, which are currently not fully addressed. I've also suggested a more concrete approach for object initialization to make the design more robust.

Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Copy link
Copy Markdown
Contributor

@niupilot niupilot Bot left a comment

Choose a reason for hiding this comment

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

Found several technical accuracy and consistency issues that need attention:

Inconsistent linkname directive usage - Document mixes //go:linkname and //llgo:link directives
Missing error handling strategy - No discussion of Python exception handling in Go bindings
Incomplete multiple inheritance solution - Problem identified but no concrete solution provided
Language consistency - Mixed English/Chinese usage throughout document

Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
Comment thread doc/pyclass-design.md Outdated
@toaction
Copy link
Copy Markdown
Contributor Author

Design base on symbol link, details can see #34.

Comment thread doc/pyclass-design.md
Comment thread doc/pyclass-design.md Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.46%. Comparing base (f84dd79) to head (b08ea4d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #21   +/-   ##
=======================================
  Coverage   44.46%   44.46%           
=======================================
  Files           6        6           
  Lines         452      452           
=======================================
  Hits          201      201           
  Misses        232      232           
  Partials       19       19           

☔ 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.

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.

feat: support Python class

4 participants