-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Upgrade typing to 3.13.5 #5850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Upgrade typing to 3.13.5 #5850
Conversation
WalkthroughThis update introduces docstring examples and the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonCode
participant RustCompiler
participant VM
User->>PythonCode: Define type alias with type parameters (PEP 695)
PythonCode->>RustCompiler: Compile type alias statement
RustCompiler->>RustCompiler: Push symbol table, compile type parameters, compile value, reorder stack, emit TypeAlias
RustCompiler->>VM: Bytecode with TypeAlias instruction
VM->>VM: Handle TypeAlias, accept None/tuple for type_params, create TypeAliasType
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vm/src/frame.rs (1)
1267-1285
: LGTM! Excellent CPython compatibility improvement.The implementation correctly handles both
None
and tuple values for type parameters, matching CPython's behavior for PEP 695 type aliasing. The logic is clear and well-commented.Minor suggestion: Consider updating the error message to be more descriptive:
- .map_err(|_| vm.new_type_error("Type params must be a tuple."))? + .map_err(|_| vm.new_type_error("Type params must be a tuple or None."))?vm/src/stdlib/typing.rs (1)
558-564
: Minor inefficiency & potential borrow bug inrepr_str
Calling
zelf.__name__()
goes through the Python attribute machinery and relies on auto-deref fromPy<…>
to&ParamSpec
.
Directly reading the stored field, as done forTypeVar
/TypeVarTuple
, avoids that overhead and side-steps any future borrow-checker surprises.- let name = zelf.__name__().str(vm)?; - Ok(format!("~{}", name)) + let name = zelf.name.str(vm)?; + Ok(format!("~{name}"))No change in behaviour; only leaner and more idiomatic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Lib/test/test_typing.py
(0 hunks)Lib/typing.py
(8 hunks)compiler/codegen/src/compile.rs
(1 hunks)vm/src/frame.rs
(1 hunks)vm/src/stdlib/typing.rs
(2 hunks)
💤 Files with no reviewable changes (1)
- Lib/test/test_typing.py
🧰 Additional context used
📓 Path-based instructions (1)
`Lib/**/*`: Files in the Lib/ directory (Python standard library copied from CPy...
Lib/**/*
: Files in the Lib/ directory (Python standard library copied from CPython) should be edited very conservatively; modifications should be minimal and only to work around RustPython limitations.
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
Lib/typing.py
🧬 Code Graph Analysis (1)
vm/src/stdlib/typing.rs (4)
vm/src/builtins/type.rs (4)
repr_str
(1301-1320)zelf
(1261-1261)zelf
(1263-1263)name
(430-435)vm/src/builtins/function.rs (2)
repr_str
(570-576)repr_str
(803-817)vm/src/builtins/genericalias.rs (1)
repr_str
(488-490)vm/src/builtins/descriptor.rs (2)
repr_str
(148-154)repr_str
(331-337)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (12)
vm/src/stdlib/typing.rs (1)
346-347
:ParamSpec
now advertisesRepresentable
– good additionExposing a custom
__repr__
onParamSpec
brings it in-line withTypeVar
/TypeVarTuple
, avoids falling back to the generic object repr, and unblocks doctest expectations.
Looks correct and self-contained.compiler/codegen/src/compile.rs (3)
1045-1062
: Well-implemented PEP 695 type parameter handling!The implementation correctly handles the complex compilation sequence for type aliases with type parameters:
- Symbol table management: Properly pushes and pops symbol table for type parameter scope
- Compilation order: Type parameters compiled first to make them available during value expression compilation
- Stack manipulation: Uses
Rotate2
to reorder stack from[type_params, value]
to[value, type_params]
as required byTypeAlias
instructionThe extensive comments clearly explain the rationale and stack states, making this complex compilation logic maintainable.
1063-1068
: Correct handling of type aliases without parameters.The else branch properly handles the simpler case where no type parameters are present:
- Compiles value expression first
- Pushes
None
(not empty tuple) to represent absence of type parameters, matching CPython behaviorThis ensures consistent stack layout for the
TypeAlias
instruction regardless of whether type parameters are present.
1070-1076
: Final stack preparation and instruction emission looks correct.The code correctly:
- Pushes the alias name as the final stack element
- Emits the
TypeAlias
instruction with the properly ordered stack:[value, type_params_or_none, name]
- Stores the resulting type alias using the name
This completes the PEP 695 type alias compilation implementation properly.
Lib/typing.py (8)
223-224
: LGTM! Good documentation enhancement.The ParamSpec examples in the docstring clearly demonstrate the intended behavior and align with the function's purpose.
268-269
: LGTM! Clear documentation examples.The examples effectively demonstrate how
_collect_type_parameters
works with ParamSpec and TypeVar combinations.
2097-2101
: LGTM! Proper protocol subclass checking restored.This change activates previously commented-out code that correctly raises TypeError when protocols with non-method members are used with
issubclass()
, providing a helpful error message with sorted member names.
534-544
: LGTM! Comprehensive documentation examples.The examples cover a wide range of type constructs and clearly demonstrate
get_origin
's behavior across different scenarios.
567-570
: LGTM! Clear examples for complex type argument extraction.The examples effectively demonstrate
get_args
's behavior with Union simplification, TypeVar substitution, and Callable argument extraction.
248-258
: LGTM! Practical TypedDict usage examples.The examples clearly demonstrate TypedDict class definition, type checking behavior, and runtime equality with regular dictionaries.
3715-3751
: Consider if this addition aligns with conservative editing guidelines.The
override
decorator implementation is well-written and follows established patterns (similar to thefinal
decorator), but this represents a significant new feature addition to the stdlib code. Given the coding guidelines stating that "modifications should be minimal and only to work around RustPython limitations," this change may be more substantial than intended for conservative editing.However, if this is part of coordinated PEP 698 support implementation, it appears correctly implemented with proper error handling and documentation.
Could you verify if adding the
override
decorator aligns with the project's conservative editing approach for stdlib files, or if this is part of a broader PEP 698 implementation effort?
777-785
: LGTM! Clear protocol members example.The example effectively demonstrates how
get_protocol_members
works with a Protocol class and shows the expected frozenset return value.
Summary by CodeRabbit
New Features
override
decorator for marking methods as overrides.ParamSpec
objects.Improvements
get_origin
,get_args
,TypedDict
, andget_protocol_members
.Bug Fixes