Fix type_params lifetime in symboltable #5844
Conversation
WalkthroughThis update removes the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant TypingProtocolTests
TestRunner->>TypingProtocolTests: Run protocol/generic protocol tests
TypingProtocolTests->>TypingProtocolTests: Perform runtime checks, subclass checks, instantiation checks
TypingProtocolTests-->>TestRunner: Report test results
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vm/src/protocol/buffer.rs (1)
384-385: Clarify TODO wording👍 Good catch on fixing the typo.
You may also want to add a brief note explaining that “column-major” here is equivalent to NumPy / Fortran (‘F’) order to avoid ambiguity for future contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Lib/test/test_typing.py(6 hunks)compiler/codegen/src/compile.rs(2 hunks)vm/src/builtins/memory.rs(1 hunks)vm/src/protocol/buffer.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`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/test/test_typing.py
`Lib/test/**/*`: Tests in Lib/test often use markers such as '# TODO: RUSTPYTHON...
Lib/test/**/*: Tests in Lib/test often use markers such as '# TODO: RUSTPYTHON', 'unittest.skip("TODO: RustPython ")', or 'unittest.expectedFailure' with a '# TODO: RUSTPYTHON ' comment when modifications are made.
NEVER comment out or delete any test code lines except for removing '@unittest.expectedFailure' decorators and upper TODO comments.
NEVER modify test assertions, test logic, or test data in Lib/test files.
The only acceptable modifications to test files are: (1) removing '@unittest.expectedFailure' decorators and the upper TODO comments when tests actually pass, (2) adding '@unittest.expectedFailure' decorators when tests cannot be fixed.
When a test cannot pass due to missing language features, keep it as expectedFailure and document the reason.
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
Lib/test/test_typing.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When a test fails due to unsupported Python syntax or features, keep the test as '@unittest.expectedFailure' and document the reason, rather than modifying the test logic or data.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Minimize modifications to CPython standard library files in the Lib/ directory; bug fixes should be made through Rust code modifications whenever possible.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Follow Rust best practices for error handling and memory management in all RustPython Rust code.
Lib/test/test_typing.py (1)
undefined
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When a test fails due to unsupported Python syntax or features, keep the test as '@unittest.expectedFailure' and document the reason, rather than modifying the test logic or data.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
🔇 Additional comments (8)
compiler/codegen/src/compile.rs (2)
233-233: LGTM! Excellent debugging improvement.Adding
#[track_caller]to theunwrap_internalfunction will provide much better error location tracking when this function panics, showing the caller's location rather than the location inside this function. This significantly improves debugging experience for internal compiler errors.
1727-1730: Good fix! Correct symbol table lifetime management.Moving the type parameters symbol table pop to after
compile_normal_callensures the symbol table remains available throughout the entire class compilation process, including during the class constructor call compilation. This fixes a potential issue where type parameters might not be accessible when needed during class construction.The previous timing could have caused type parameter resolution issues during the
__build_class__call.Lib/test/test_typing.py (6)
2928-2930: LGTM - Protocol code correctly uncommented.The runtime checkable generic protocol definition is syntactically correct and follows proper PEP 695 generic syntax.
2990-3000: LGTM - Protocol inheritance test code correctly uncommented.The uncommented code properly tests that protocols cannot inherit from nominal classes, with appropriate TypeError expectations.
3010-3012: Verify consistency between expectedFailure decorator and uncommented code.Similar to the previous case, there's an
@unittest.expectedFailuredecorator on line 3010, but protocol code is being uncommented. Ensure the decorator is also removed if the test now passes.
3044-3047: LGTM - Protocol init preservation test code correctly uncommented.The uncommented protocol code correctly defines a protocol with an
__init__method to test that it doesn't get overridden.
3472-3476: Verify consistency between expectedFailure decorator and uncommented code.Again, there's an
@unittest.expectedFailuredecorator on line 3472, but protocol code is being uncommented. This pattern suggests the decorators should also be removed if these tests now pass.
2943-2947: Verify consistency between expectedFailure decorator and uncommented code.The code shows an
@unittest.expectedFailuredecorator on line 2943, but protocol code is being uncommented below. If this test now passes (as suggested by the AI summary), the decorator should also be removed.#!/bin/bash # Check if expectedFailure decorators are being removed in this PR # Look for any remaining TODO: RUSTPYTHON comments with expectedFailure decorators rg -A 2 -B 2 "@unittest\.expectedFailure" Lib/test/test_typing.py
Summary by CodeRabbit
Tests
Style
Chores