Update trivial inference for Python 3.14 #37248
Conversation
Summary of ChangesHello @jrmccluskey, 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 focuses on updating the type inference mechanism to be fully compatible with Python 3.14. It incorporates changes to CPython opcodes and their behaviors, along with refinements to native type compatibility, to ensure accurate and robust type inference in the latest Python environment. 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
|
|
Assigning reviewers: R: @claudevdm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
/gemini review |
![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 updates the trivial type inference logic to support changes in Python 3.14, particularly new and modified opcodes. The changes look mostly correct and well-commented.
I have a few points of feedback:
- There's a critical logic inversion in how keyword arguments are detected for
CALL_FUNCTION_EXon Python 3.14. - A block of tests related to slicing has been commented out, which suggests a potential regression or incomplete implementation that should be addressed.
- A magic number is used for an opcode value, which could be replaced with a programmatically determined constant for better maintainability.
Details are in the specific comments.
| # CALL_FUNCTION_EX does not take an arg in 3.14, instead the | ||
| # signaling for kwargs is done via a PUSH_NULL instruction | ||
| # right before CALL_FUNCTION_EX. | ||
| arg = ~last_op_push_null & 1 |
There was a problem hiding this comment.
The logic to determine has_kwargs appears to be inverted from what the comment describes. The comment states that PUSH_NULL signals the presence of keyword arguments, which means if last_op_push_null is 1, has_kwargs should be 1. However, the expression ~last_op_push_null & 1 evaluates to 0 when last_op_push_null is 1, and 1 when it's 0.
If the comment is correct, the logic should be arg = last_op_push_null.
It's also worth noting that the Python 3.14 release notes state that PUSH_NULL is used to signal keyword arguments for the CALL instruction, not CALL_FUNCTION_EX. This might also need to be reviewed.
| arg = ~last_op_push_null & 1 | |
| arg = last_op_push_null |
| # self.assertReturnType( | ||
| # typehints.List[int], lambda v: v[::-1], [typehints.List[int]]) | ||
| # self.assertReturnType( | ||
| # typehints.Tuple[int], lambda v: v[::-1], [typehints.Tuple[int]]) | ||
| # self.assertReturnType(str, lambda v: v[::-1], [str]) | ||
| # self.assertReturnType(typehints.Any, lambda v: v[::-1], [typehints.Any]) | ||
| # self.assertReturnType(typehints.Any, lambda v: v[::-1], [object]) |
There was a problem hiding this comment.
These tests for slicing have been commented out. This is not a good practice as it may hide a regression or an incomplete feature. The tests should be updated to work with the new Python 3.14 opcode behavior for slicing. If this is intended to be addressed in a follow-up, please add a TODO with a link to a tracking issue.
| if arg == 26: | ||
| return binary_subscr(state, arg) |
There was a problem hiding this comment.
Using the magic number 26 here is not ideal for maintainability. It would be better to define a constant by introspecting opcode._nb_ops to find the index of 'NB_SUBSCR', similar to how _div_binop_args is handled. This would make the code more robust against future changes in Python's opcodes.
For example, you could define a constant at the top of the file and use it here:
# At top of file
if sys.version_info >= (3, 11):
# ...
try:
_NB_SUBSCR_OP = [op[0] for op in opcode._nb_ops].index('NB_SUBSCR')
except (AttributeError, ValueError):
_NB_SUBSCR_OP = -1
else:
# ...
_NB_SUBSCR_OP = -1
# In symmetric_binary_op
if arg == _NB_SUBSCR_OP:
return binary_subscr(state, arg)
| self.assertReturnType(str, lambda v: v[::-1], [str]) | ||
| self.assertReturnType(typehints.Any, lambda v: v[::-1], [typehints.Any]) | ||
| self.assertReturnType(typehints.Any, lambda v: v[::-1], [object]) | ||
| # self.assertReturnType( |
There was a problem hiding this comment.
Are these intentionally commented out?
2 participants
Updates the trivial inference state machine with new 3.14 CPython opcodes + updates behavior of existing opcodes. Also updates some native type compatibility code to catch new behaviors for accessing args in bare types.
Note: The typehint test suites pass in a local environment running 3.14, but this PR does not enable Python 3.14 support in the repository.
Part of #37247
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.