more typing #5840
more typing #5840
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces stricter documentation for test code modifications, refactors internal module naming and usage in the typing implementation, and enhances several typing-related classes with new magic methods and comparison logic. Test suite decorators and comments indicating expected failures are removed, and a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonCode
participant RustPythonTyping as Typing Module (decl)
participant VM
User->>PythonCode: Define function with @typing.override
PythonCode->>RustPythonTyping: Call override(func)
RustPythonTyping->>PythonCode: Set func.__override__ = True
User->>PythonCode: Subclass ParamSpec/TypeVarTuple/ParamSpecArgs/ParamSpecKwargs
PythonCode->>RustPythonTyping: Call mro_entries()
RustPythonTyping->>PythonCode: Raise TypeError (subclassing not allowed)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vm/src/stdlib/typing.rs (2)
347-351: Minor inconsistency in error message formatting.While the
mro_entriesimplementations correctly prevent subclassing, there's an inconsistency in error message construction:
- ParamSpec uses
.to_owned()on a string literal (line 349)- Others use
.to_owned()on formatted stringsConsider removing the unnecessary
.to_owned()from the string literal:- Err(vm.new_type_error("Cannot subclass an instance of ParamSpec".to_owned())) + Err(vm.new_type_error("Cannot subclass an instance of ParamSpec"))Also applies to: 654-658, 756-760, 835-839
788-825: Consider extracting common comparison logic to reduce duplication.Both
ParamSpecArgsandParamSpecKwargshave identical comparison implementations. This violates the DRY principle.Extract the common logic into a helper function:
fn compare_by_origin<T>( origin: &PyObjectRef, other: &PyObject, op: PyComparisonOp, vm: &VirtualMachine, ) -> PyResult<PyComparisonValue> { let eq = |other: PyObjectRef| -> PyResult<bool> { if let Ok(other_origin) = other.get_attr("__origin__", vm) { return Ok(origin.is(&other_origin)); } Ok(false) }; match op { PyComparisonOp::Eq => eq(other.to_owned()) .map(Into::into) .unwrap_or(PyComparisonValue::NotImplemented), PyComparisonOp::Ne => eq(other.to_owned()) .map(|result| (!result).into()) .unwrap_or(PyComparisonValue::NotImplemented), _ => Ok(PyComparisonValue::NotImplemented), } }Then use it in both implementations:
impl Comparable for ParamSpecArgs { fn cmp( zelf: &crate::Py<Self>, other: &PyObject, op: PyComparisonOp, vm: &VirtualMachine, ) -> PyResult<PyComparisonValue> { compare_by_origin(&zelf.__origin__, other, op, vm) } }Also applies to: 867-904
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/copilot-instructions.md(1 hunks)Lib/test/test_typing.py(0 hunks)vm/src/frame.rs(6 hunks)vm/src/stdlib/typing.rs(8 hunks)
💤 Files with no reviewable changes (1)
- Lib/test/test_typing.py
🧰 Additional context used
🧠 Learnings (3)
.github/copilot-instructions.md (8)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: For documentation in RustPython, generate it with 'cargo doc --no-deps --all' and consult the architecture and development guide documents for further information.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: For Rust code in the RustPython project, always run 'cargo clippy' to lint code before completing tasks, and fix any warnings or lints introduced by your changes.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: In most cases, Python code in RustPython should not be edited; bug fixes should be made through Rust code modifications.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: Follow Rust best practices for error handling and memory management when contributing to RustPython.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: For Rust code in the RustPython project, always use the default rustfmt code style by running 'cargo fmt' to format code.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: To determine what to implement next in RustPython, run './whats_left.py' to get a list of unimplemented methods.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: Minimize modifications to CPython standard library files in the Lib/ directory of RustPython.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: For custom Python code in RustPython, follow PEP 8 style and use ruff for linting.
vm/src/frame.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: When implementing Python functionality in Rust for RustPython, use the macro system such as 'pyclass', 'pymodule', and 'pyfunction'.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: Follow Rust best practices for error handling and memory management when contributing to RustPython.
vm/src/stdlib/typing.rs (3)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: When adding a Python module implemented in Rust to the RustPython interpreter, use 'vm.add_native_module' with the module name and the module's make_module function.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: When implementing Python functionality in Rust for RustPython, use the macro system such as 'pyclass', 'pymodule', and 'pyfunction'.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: For documentation in RustPython, generate it with 'cargo doc --no-deps --all' and consult the architecture and development guide documents for further information.
🧬 Code Graph Analysis (1)
vm/src/frame.rs (1)
vm/src/stdlib/typing.rs (9)
make_typevar(311-329)bound(98-110)bound(376-381)new(918-928)name(78-80)name(353-355)name(619-621)make_paramspec(563-573)make_typevartuple(739-745)
🪛 markdownlint-cli2 (0.17.2)
.github/copilot-instructions.md
188-188: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (6)
.github/copilot-instructions.md (1)
186-206: LGTM! Clear documentation of test modification rules.The new section provides explicit guidelines for handling test code modifications, which aligns well with the changes made to
test_typing.pywhere@unittest.expectedFailuredecorators were removed.vm/src/stdlib/typing.rs (4)
3-14: Module renaming is well-executed.The internal module rename from
_typingtodeclimproves code clarity while maintaining the external API through the#[pymodule(name = "_typing")]attribute.
47-53: Correct implementation of theoverridedecorator.The function properly implements PEP 698's
@overridedecorator by setting the__override__attribute, which matches CPython's behavior.
659-674: LGTM! Proper handling of TypeVarTuple substitution.The implementation correctly prevents bare TypeVarTuple substitution and delegates complex preparation logic to the typing module.
983-996: Proper handling of exec contexts in module attribution.The updated logic correctly handles cases where no module name is available (e.g., in exec contexts) by explicitly setting
__module__to None, which aligns with CPython's behavior.vm/src/frame.rs (1)
20-20: Import paths correctly updated to match typing module refactoring.All references have been consistently updated from
typing::_typing::totyping::following the module restructuring.Also applies to: 1237-1290
Summary by CodeRabbit
New Features
overridedecorator for marking functions as overrides.ParamSpec,TypeVarTuple,ParamSpecArgs,ParamSpecKwargs) with new behaviors to prevent subclassing and unsupported operations.ParamSpecArgsandParamSpecKwargs.Documentation
Chores
@unittest.expectedFailuredecorators and related TODO comments from typing-related tests.