[WIP] ssa: python support kwargs

[visualfc]

pass last map[string]*py.Object to kwargs. #1236

python

def test_lib3(a, b, *args, c=10, **kwargs):

test lib

//go:linkname Test_lib py.test_lib
func Test_lib(x *py.Object, y *py.Object, __llgo_va_list ...interface{})

test main

mylib.Test_lib3(py.Long(1), py.Long(2), py.Str("helo world"), py.Long(100),
		py.Kwargs{"c": py.Long(102), "d": py.Str("hello world")}) 

demo

std.Print(py.Str("Hello world"), py.Kwargs{"flush": py.Long(1), "end": py.Str("$\n")})
# output: Hello world$

goplus/lib#20: add KwargsToDict

type Kwargs  map[string]*Object

func KwargsToDict(kw Kwargs) *Object {
	dict := NewDict()
	for k, v := range kw {
		dict.DictSetItem(FromGoString(k), v)
	}
	return dict
}

[niupilot]

Code Review Results

Todo List:

• Read and analyze ssa/package.go changes
• Read and analyze ssa/python.go changes
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate architecture and design decisions
• Provide comprehensive review feedback

Review Summary

This PR adds Python keyword arguments (kwargs) support to the SSA package. The implementation allows passing map[string]*py.Object as the final argument to Python functions with kwargs.

Issues Found

High Priority Issues

1. Missing Error Handling in Type Assertions ssa/python.go:377-384
The code performs type assertions and scope lookups without error handling:

• fn.Type().(*types.Signature) at line 91 could panic if the type is not a signature
• params.At(params.Len()-1).Name() at line 378 could panic if params is empty

2. Potential Memory Safety Issue ssa/python.go:380
The kwargs conversion call b.Call(b.Pkg.pyGoFunc("KwargsToDict"), args[len(args)-1]) assumes the external KwargsToDict function exists and handles the conversion safely, but there's no validation.

3. Magic String Constant ssa/python.go:378
The NameValist constant is used without being defined in this file, making the code dependent on external constants that may change.

Code Quality Issues

4. Inconsistent Commenting ssa/python.go:145-154
The tyCall() function was previously commented out but is now uncommented without explanation. The comment style is inconsistent with other functions.

5. Complex Conditional Logic ssa/python.go:377-384
The nested conditions in the kwargs detection logic are difficult to follow and could be refactored for clarity.

Architecture Assessment

Positive Aspects:

• Clean separation of concern with dedicated PyKwargs() type method
• Follows existing patterns for Python type handling
• Maintains backward compatibility

Areas for Improvement:

• The kwargs detection logic relies on parameter names and positions, which is fragile
• No validation of the kwargs map structure
• Heavy dependency on external KwargsToDict function

Recommendations

1. Add error handling for type assertions and bounds checking
2. Validate the external KwargsToDict function availability
3. Define or import the NameValist constant properly
4. Simplify the kwargs detection logic with helper functions
5. Add documentation explaining the kwargs conversion process

Overall, the feature implementation is on the right track but needs defensive programming practices and better error handling before merging.

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

[gemini-code-assist]

Summary of Changes

Hello @visualfc, I'm Gemini Code Assist¹! 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 Go-Python interoperability by enabling Go programs to call Python functions that accept keyword arguments (**kwargs). The changes involve updating the SSA representation to include a dedicated type for kwargs and modifying the function call mechanism to properly convert Go maps into Python dictionaries, ensuring seamless argument passing for named parameters.

Highlights

• Python Keyword Arguments (kwargs) Support: Introduced support for passing Python keyword arguments (kwargs) from Go to Python functions, allowing for more flexible function calls and improved interoperability.
• SSA Type Definition for Kwargs: Added a new pyKwargs type to the SSA program structure, representing map[string]*py.Object, to formally recognize and handle keyword arguments within the static single assignment form.
• Enhanced pyCall Builder: Modified the pyCall builder function to detect and correctly process kwargs. It now converts the Go map[string]*py.Object into a Python dictionary using a KwargsToDict helper before invoking PyObject_Call.

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

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

[niupilot]

Review: Python kwargs implementation needs defensive programming

Found several issues requiring attention:

• Missing bounds checking could cause runtime panics
• Unsafe type assertions need error handling
• Magic constants should be properly defined
• Complex conditional logic could be simplified for maintainability

The core kwargs functionality is well-designed, but needs defensive programming practices before merging.

[gemini-code-assist]

Code Review

This pull request introduces support for keyword arguments (kwargs) when calling Python functions from Go. The implementation correctly identifies when a map[string]*py.Object is passed as the last argument in a variadic function call, converts it to a Python dictionary using a helper function, and then uses PyObject_Call to perform the call with both positional and keyword arguments. The changes are logical and well-contained. I have a couple of suggestions to improve the maintainability of the new code.

[codecov]

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.04%. Comparing base (dba7bd4) to head (a2187c5).

Files with missing lines	Patch %	Lines
ssa/python.go	14.28%	11 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1321      +/-   ##
==========================================
- Coverage   90.18%   90.04%   -0.15%     
==========================================
  Files          43       43              
  Lines       12674    12696      +22     
==========================================
+ Hits        11430    11432       +2     
- Misses       1087     1106      +19     
- Partials      157      158       +1     

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

[niupilot]

Code Review Results

Todo List:

• Read and analyze ssa/package.go changes
• Read and analyze ssa/python.go changes
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate architecture and design decisions
• Provide comprehensive review feedback

Review Summary

This PR adds Python keyword arguments (kwargs) support to the SSA package. The implementation allows passing map[string]*py.Object as the final argument to Python functions with kwargs.

Issues Found

High Priority Issues

1. Missing Error Handling in Type Assertions ssa/python.go:87-92
The code performs type assertions without error handling:

• fn.Type().(*types.Signature) at line 91 could panic if the type is not a signature
• Need to add bounds checking for params.At(params.Len()-1) at line 378

2. Potential Memory Safety Issue ssa/python.go:380
The kwargs conversion call b.Call(b.Pkg.pyGoFunc("KwargsToDict"), args[len(args)-1]) assumes the external KwargsToDict function exists and handles the conversion safely, but there's no validation.

3. Missing Bounds Checking ssa/python.go:378
The code accesses params.At(params.Len()-1) without verifying params.Len() > 0, which could panic if params is empty.

Code Quality Issues

4. Inconsistent Commenting ssa/python.go:145-154
The tyCall() function was previously commented out but is now uncommented without explanation. The comment style is inconsistent with other functions.

5. Complex Conditional Logic ssa/python.go:377-384
The nested conditions in the kwargs detection logic are difficult to follow and could be refactored for clarity.

6. Magic String Dependency ssa/python.go:380
The code depends on the external function name "KwargsToDict" which creates a brittle coupling to the external library.

Architecture Assessment

Positive Aspects:

• Clean separation of concern with dedicated PyKwargs() type method
• Follows existing patterns for Python type handling
• Maintains backward compatibility
• Proper integration with existing variadic function detection

Areas for Improvement:

• The kwargs detection logic relies on parameter names and positions, which is fragile
• No validation of the kwargs map structure
• Heavy dependency on external KwargsToDict function availability

Recommendations

1. Add error handling for type assertions and bounds checking
2. Validate the external KwargsToDict function availability during initialization
3. Add bounds checking for parameter access to prevent panics
4. Simplify the kwargs detection logic with helper functions
5. Add documentation explaining the kwargs conversion process
6. Consider defensive programming with nil checks and validation

Specific Code Issues

The implementation is functionally correct but needs defensive programming practices. The kwargs detection logic at lines 377-384 should be refactored for better readability and safety.

Overall, the feature implementation is on the right track but needs defensive programming practices and better error handling before merging.

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

[niupilot]

Review Summary

This PR adds Python kwargs support to the SSA package. Found several issues requiring attention:

• Missing bounds checking in parameter access that could cause panics
• Unsafe type assertions without error handling
• Complex conditional logic that could be simplified for better readability

The implementation is functionally sound but needs defensive programming practices before merging.