Bytecode parity - for-loop cleanup, return duplication, late jump threading fix - #7580
Conversation
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefines codegen and IR control flow, updates COMPARE_OP encoding/formatting, adjusts VM frame/coroutine lifecycle and traceback handling, adds AST deepcopy, centralizes thread stack-size logic and threading tests, and updates disassembly to emit raw instruction streams with cache-awareness. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
@youknowone time=2026-04-10T11:11:07.604Z level=INFO msg="reviewdog: failed to post a review comment: POST https://api.github.com/repos/RustPython/RustPython/pulls/7580/reviews: 403 Resource not accessible by integration []" reviewdog: This GitHub Token doesn't have write permission of Review API [1], so reviewdog will report results via logging command [2] and create annotations similar to github-pr-check reporter as a fallback. [1]: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target [2]: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions reviewdog: found at least one issue with severity greater than or equal to the given level: error This has something to do with the repository settings, can you please take a look? then it will be able to make committable comments with auto-fixes. It works on forks, I guess there's some default setting that was changed |
Sorry, something went wrong.
|
posting requires write permission. But the permission is read-only |
Sorry, something went wrong.
| emit!(self, Instruction::PopTop); | ||
| } else { | ||
| emit!(self, Instruction::PopIter); | ||
| emit!(self, Instruction::PopTop); |
There was a problem hiding this comment.
if both node of branch PopTop, why not out of branch?
Sorry, something went wrong.
| const TIMEOUT_MAX: f64 = (TIMEOUT_MAX_IN_MICROSECONDS / 1_000_000) as f64; | ||
| const TIMEOUT_MAX_SECONDS: f64 = (TIMEOUT_MAX_IN_MICROSECONDS / 1_000_000) as f64; | ||
|
|
||
| #[pyattr(name = "TIMEOUT_MAX")] |
There was a problem hiding this comment.
what's the point of this change? why not const ?
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/vm/src/coroutine.rs (1)76-88: ⚠️ Potential issue | 🔴 Critical
Guard cleanup from preflight already executing errors before clearing frame state
maybe_close() currently treats all Err(_) as terminal. When run_with_context() fails its preflight check (already executing), finalize_send_result() still calls maybe_close(), and Line 87 now clears locals/stack while the frame may still be running. That violates clear_locals_and_stack()’s safety assumption (“frame is not executing”) and can corrupt execution state.
Proposed fix🤖 Prompt for AI Agentsfn maybe_close(&self, res: &PyResult<ExecutionResult>) { + // `run_with_context` can return preflight errors (e.g. "already executing") + // without entering/resuming the frame. Do not close/clear in that case. + if self.running.load() { + return; + } match res { Ok(ExecutionResult::Return(_)) | Err(_) => { self.closed.store(true); // Frame is no longer suspended; allow frame.clear() to succeed. self.frame.owner.store( FrameOwner::FrameObject as i8, core::sync::atomic::Ordering::Release, ); // Completed generators/coroutines should not keep their locals // alive while the wrapper object itself remains referenced. self.frame.clear_locals_and_stack(); } Ok(ExecutionResult::Yield(_)) => {} } }Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/coroutine.rs` around lines 76 - 88, maybe_close() currently treats any Err(_) as terminal and clears the frame even for preflight "already executing" failures; update maybe_close(&self, res: &PyResult<ExecutionResult>) to detect the specific preflight/already-executing error returned by run_with_context()/finalize_send_result() and skip the close/clear sequence for that case. Concretely, pattern-match Err(e) and if e corresponds to the already-executing/preflight error (use the same error type/variant or message check used by run_with_context), simply return without calling self.closed.store(...), self.frame.owner.store(...), or self.frame.clear_locals_and_stack(); only perform the closing/clearing for true terminal results (Ok(ExecutionResult::Return(_)) or other non-preflight Err cases).
Verify each finding against the current code and only fix it if needed. Inline comments: In `@crates/codegen/src/ir.rs`: - Around line 3676-3678: Do not count old_next as a predecessor because the cloned block is jump-only; remove (or disable) the increment of predecessors[old_next.idx()] (the BlockIdx::NULL check may remain) so propagate_line_numbers won't treat old_next as having an extra predecessor; update the logic around old_next in the same block that manipulates predecessors to avoid adding a fall-through edge for jump-only cloned blocks referenced by old_next. --- Outside diff comments: In `@crates/vm/src/coroutine.rs`: - Around line 76-88: maybe_close() currently treats any Err(_) as terminal and clears the frame even for preflight "already executing" failures; update maybe_close(&self, res: &PyResult<ExecutionResult>) to detect the specific preflight/already-executing error returned by run_with_context()/finalize_send_result() and skip the close/clear sequence for that case. Concretely, pattern-match Err(e) and if e corresponds to the already-executing/preflight error (use the same error type/variant or message check used by run_with_context), simply return without calling self.closed.store(...), self.frame.owner.store(...), or self.frame.clear_locals_and_stack(); only perform the closing/clearing for true terminal results (Ok(ExecutionResult::Return(_)) or other non-preflight Err cases).
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 493c9d45-8b4b-48a5-bbcb-4b87d10ecb4c
📥 CommitsReviewing files that changed from the base of the PR and between 9cf7bcd and a082b4c.
⛔ Files ignored due to path filters (4)- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snap is excluded by !**/*.snap
- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snap is excluded by !**/*.snap
- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snap is excluded by !**/*.snap
- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snap is excluded by !**/*.snap
- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
- crates/compiler-core/src/bytecode/instruction.rs
- crates/compiler-core/src/bytecode/oparg.rs
- crates/vm/src/coroutine.rs
- crates/vm/src/frame.rs
- crates/vm/src/stdlib/_thread.rs
- scripts/dis_dump.py
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: (module 'dis test_yield_from' not found) Legend:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/vm/src/stdlib/sys/monitoring.rs (1)394-403: ⚠️ Potential issue | 🟡 Minor
Add PopTop to the excluded opcodes list in the monitoring instrumentation pass, or verify synthetic cleanup instructions are marked NO_LOCATION.
Lines 400, 465, and 487 exclude PopIter from INSTRUMENTED_LINE tracking, but this PR replaces loop break/return cleanup from PopIter to PopTop (lines 1520, 9933). The synthetic PopTop instructions are emitted without set_no_location() calls (unlike other synthetic instructions in exception handling), which means they inherit the break/return statement's location and will be included in INSTRUMENTED_LINE events. Either add PopTop to the exclusion patterns at these three locations, or confirm and document that all synthetic cleanup PopTop emissions are guaranteed to be marked as NO_LOCATION in the linetable.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/sys/monitoring.rs` around lines 394 - 403, The monitoring instrumentation currently excludes PopIter from INSTRUMENTED_LINE tracking but not PopTop, which causes synthetic PopTop cleanup to be instrumented; update the monitoring instrumentation pass to also exclude Instruction::PopTop wherever Instruction::PopIter is excluded (the three exclusion match sites in monitoring.rs) so PopTop is omitted from INSTRUMENTED_LINE events, or alternatively ensure every synthetic emission of PopTop calls set_no_location() at the emitter site so it carries NO_LOCATION; reference Instruction::PopTop, Instruction::PopIter, INSTRUMENTED_LINE, and set_no_location() when making the change.
crates/codegen/src/ir.rs (1)🤖 Prompt for all review comments with AI agents3767-3769: ⚠️ Potential issue | 🟡 Minor
Don’t count old_next as a predecessor here.
The cloned block is jump-only, so it never falls through to old_next. Incrementing this count makes later line-number propagation treat old_next as multi-predecessor and can suppress location propagation unnecessarily.
Suggested fix🤖 Prompt for AI Agentspredecessors[target.idx()] -= 1; predecessors.push(1); - if old_next != BlockIdx::NULL { - predecessors[old_next.idx()] += 1; - }Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 3767 - 3769, The code increments predecessors[old_next.idx()] for old_next when old_next != BlockIdx::NULL, but since the cloned block is jump-only and never falls through to old_next this incorrectly marks old_next as having an extra predecessor; remove the increment of predecessors for old_next in this branch (i.e., do not modify the predecessors array for old_next in the cloned-block path) so that later line-number propagation treats old_next correctly as a single-predecessor block; locate the logic around old_next, predecessors, and BlockIdx::NULL in the function handling the cloned block and delete or skip the predecessors[old_next.idx()] += 1 operation.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@crates/codegen/src/ir.rs`: - Around line 3323-3325: The helper block_is_exceptional currently only checks Block.except_handler and Block.preserve_lasti and thus misses cleanup-only unwind blocks (e.g., POP_EXCEPT ... RERAISE), allowing exception-cleanup opcodes to be moved into normal flow; update block_is_exceptional to also consider is_exception_cleanup_block() (or inline that predicate) so it returns true for cleanup-only unwind blocks, and/or replace calls to block_is_exceptional at the callers inline_small_or_no_lineno_blocks and the two conditional reorder passes with a combined predicate that ORs is_exception_cleanup_block, ensuring those passes treat cleanup-only unwind blocks as exceptional. In `@crates/vm/src/coroutine.rs`: - Around line 160-170: The running.load() precheck currently races with frame mutation because locals_to_fast() is called before acquiring the exclusive running guard; move the frame mutation inside the critical section by calling locals_to_fast() only after the guard is acquired (i.e. inside the closure passed to run_with_context or after compare_exchange succeeds), so the frame state is touched while running is true. Update both the coroutine.resume path (where locals_to_fast() is called before run_with_context/compare_exchange) and the send() path to perform locals_to_fast() after entering the run_with_context closure (or immediately after successfully setting running) and keep finalize_send_result/result handling unchanged. Ensure you reference the running flag access, locals_to_fast(), run_with_context(), resume(), and send() when applying the change. --- Outside diff comments: In `@crates/vm/src/stdlib/sys/monitoring.rs`: - Around line 394-403: The monitoring instrumentation currently excludes PopIter from INSTRUMENTED_LINE tracking but not PopTop, which causes synthetic PopTop cleanup to be instrumented; update the monitoring instrumentation pass to also exclude Instruction::PopTop wherever Instruction::PopIter is excluded (the three exclusion match sites in monitoring.rs) so PopTop is omitted from INSTRUMENTED_LINE events, or alternatively ensure every synthetic emission of PopTop calls set_no_location() at the emitter site so it carries NO_LOCATION; reference Instruction::PopTop, Instruction::PopIter, INSTRUMENTED_LINE, and set_no_location() when making the change. --- Duplicate comments: In `@crates/codegen/src/ir.rs`: - Around line 3767-3769: The code increments predecessors[old_next.idx()] for old_next when old_next != BlockIdx::NULL, but since the cloned block is jump-only and never falls through to old_next this incorrectly marks old_next as having an extra predecessor; remove the increment of predecessors for old_next in this branch (i.e., do not modify the predecessors array for old_next in the cloned-block path) so that later line-number propagation treats old_next correctly as a single-predecessor block; locate the logic around old_next, predecessors, and BlockIdx::NULL in the function handling the cloned block and delete or skip the predecessors[old_next.idx()] += 1 operation.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e402ee32-9ee5-4d0e-97b3-8782fee21d7a
📥 CommitsReviewing files that changed from the base of the PR and between a082b4c and 41e6ffe.
⛔ Files ignored due to path filters (6)- Lib/test/test_dis.py is excluded by !Lib/**
- Lib/test/test_yield_from.py is excluded by !Lib/**
- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snap is excluded by !**/*.snap
- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snap is excluded by !**/*.snap
- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snap is excluded by !**/*.snap
- crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snap is excluded by !**/*.snap
- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
- crates/compiler-core/src/bytecode/instruction.rs
- crates/compiler-core/src/bytecode/oparg.rs
- crates/vm/src/coroutine.rs
- crates/vm/src/frame.rs
- crates/vm/src/stdlib/_thread.rs
- crates/vm/src/stdlib/sys/monitoring.rs
- scripts/dis_dump.py
- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)crates/codegen/src/ir.rs (2)🤖 Prompt for all review comments with AI agents3324-3326: ⚠️ Potential issue | 🟠 Major
Treat cleanup-only unwind blocks as exceptional.
block_is_exceptional() still returns false for POP_EXCEPT ... RERAISE cleanup blocks, so inline_small_or_no_lineno_blocks() and the conditional reorder passes can still move exception-cleanup bytecode into normal flow.
Suggested fix🤖 Prompt for AI Agentsfn block_is_exceptional(block: &Block) -> bool { - block.except_handler || block.preserve_lasti + block.except_handler || block.preserve_lasti || is_exception_cleanup_block(block) }Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 3324 - 3326, block_is_exceptional currently only checks Block.except_handler and Block.preserve_lasti, so cleanup-only unwind blocks (POP_EXCEPT ... RERAISE) are not treated as exceptional; update fn block_is_exceptional(block: &Block) -> bool to also return true when the block represents a cleanup unwind (e.g. check a field like block.cleanup_unwind or block.is_cleanup_unwind), ensuring such blocks are considered exceptional by inline_small_or_no_lineno_blocks() and the conditional reorder passes.
3776-3778: ⚠️ Potential issue | 🟡 Minor
Don’t count old_next as a predecessor here.
This cloned block is jump-only, so it never falls through to old_next. Incrementing that count can make propagate_line_numbers() treat old_next as multi-predecessor and skip location propagation unnecessarily.
Suggested fix🤖 Prompt for AI Agentspredecessors[target.idx()] -= 1; predecessors.push(1); - if old_next != BlockIdx::NULL { - predecessors[old_next.idx()] += 1; - }Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 3776 - 3778, The code increments predecessors[old_next.idx()] for cloned jump-only blocks, incorrectly marking old_next as having an extra predecessor; remove the increment (the if block that checks old_next != BlockIdx::NULL and does predecessors[old_next.idx()] += 1) so cloned jump-only blocks do not contribute a predecessor count, which prevents propagate_line_numbers() from misclassifying old_next as multi-predecessor.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@extra_tests/snippets/builtin_exec_thread_import.py`: - Line 9: The exec() call is intentionally used and triggers S102; add an explicit per-line ruff/flake8-bandit waiver by appending a noqa suppression to the exec invocation (e.g., change the exec(code, scope, scope) line to include "# noqa: S102") so the linter accepts this intentional use of exec() in builtin_exec_thread_import.py. --- Duplicate comments: In `@crates/codegen/src/ir.rs`: - Around line 3324-3326: block_is_exceptional currently only checks Block.except_handler and Block.preserve_lasti, so cleanup-only unwind blocks (POP_EXCEPT ... RERAISE) are not treated as exceptional; update fn block_is_exceptional(block: &Block) -> bool to also return true when the block represents a cleanup unwind (e.g. check a field like block.cleanup_unwind or block.is_cleanup_unwind), ensuring such blocks are considered exceptional by inline_small_or_no_lineno_blocks() and the conditional reorder passes. - Around line 3776-3778: The code increments predecessors[old_next.idx()] for cloned jump-only blocks, incorrectly marking old_next as having an extra predecessor; remove the increment (the if block that checks old_next != BlockIdx::NULL and does predecessors[old_next.idx()] += 1) so cloned jump-only blocks do not contribute a predecessor count, which prevents propagate_line_numbers() from misclassifying old_next as multi-predecessor.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: eb8da607-9dc3-40e1-ad80-14b6bdb57b40
📥 CommitsReviewing files that changed from the base of the PR and between 41e6ffe and 28b324d.
📒 Files selected for processing (10)- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
- crates/vm/src/coroutine.rs
- crates/vm/src/stdlib/_ast/python.rs
- crates/vm/src/stdlib/_thread.rs
- crates/vm/src/stdlib/os.rs
- crates/vm/src/stdlib/sys/monitoring.rs
- crates/vm/src/vm/mod.rs
- extra_tests/snippets/builtin_exec_thread_import.py
- extra_tests/snippets/thread_import_stack.py
- crates/codegen/src/compile.rs
- crates/vm/src/stdlib/sys/monitoring.rs
- crates/vm/src/stdlib/_thread.rs
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)extra_tests/snippets/stdlib_threading.py (1)🤖 Prompt for all review comments with AI agents23-23: ⚠️ Potential issue | 🟠 Major
Suppress intentional exec() to pass Ruff S102.
Line 23 intentionally uses exec, but it currently trips linting. Add an explicit inline waiver so CI does not fail.
Proposed fix- exec(code, scope, scope) + exec(code, scope, scope) # noqa: S102 - intentional threaded exec regression testAs per coding guidelines: **/*.py: "Use ruff for linting Python code".
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@extra_tests/snippets/stdlib_threading.py` at line 23, The exec call using variables code and scope intentionally triggers S102; suppress it by adding an inline Ruff waiver on the same line — change the exec(code, scope, scope) invocation to include the noqa comment for S102 (e.g., append " # noqa: S102") so the intentional exec is allowed by CI while keeping behavior unchanged.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@crates/vm/src/frame.rs`: - Around line 1525-1534: The code currently skips creating a traceback entry when frame.code.locations.get(idx) returns None; change this to fall back to a previous mapped location or the function's first line instead of dropping the frame. In the block that builds the traceback (where you call frame.code.locations.get(idx), PyTraceback::new and exception.set_traceback_typed), if get(idx) is None attempt frame.code.locations.get(idx.saturating_sub(1)) and if that is also None use a fallback like frame.code.first_line (or the first location's line), then construct PyTraceback::new with that line and set_traceback_typed as before. Apply the same fallback logic to the other analogous block (the one around the second occurrence you noted) so lasti/f_lineno restoration still runs and the frame always gets a traceback entry. --- Duplicate comments: In `@extra_tests/snippets/stdlib_threading.py`: - Line 23: The exec call using variables code and scope intentionally triggers S102; suppress it by adding an inline Ruff waiver on the same line — change the exec(code, scope, scope) invocation to include the noqa comment for S102 (e.g., append " # noqa: S102") so the intentional exec is allowed by CI while keeping behavior unchanged.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 20afd674-e56e-42f3-aeb6-293e58ef6a31
📥 CommitsReviewing files that changed from the base of the PR and between 28b324d and 9f76666.
📒 Files selected for processing (3)- crates/vm/src/frame.rs
- crates/vm/src/stdlib/_thread.rs
- extra_tests/snippets/stdlib_threading.py
Sorry, something went wrong.
| if let Some((loc, _end_loc)) = frame.code.locations.get(idx) { | ||
| let next = exception.__traceback__(); | ||
| let new_traceback = PyTraceback::new( | ||
| next, | ||
| frame.object.to_owned(), | ||
| idx as u32 * 2, | ||
| loc.line, | ||
| ); | ||
| exception.set_traceback_typed(Some(new_traceback.into_ref(&vm.ctx))); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Don't drop the current frame when locations.get(idx) misses.
Avoiding the panic is good, but if this lookup returns None for the first raise in this frame, the exception now escapes without a traceback entry for the current frame at all. That also prevents the lasti restore at Line 1702 from running, so f_lineno can stay stale after cleanup. Please fall back to the previous mapped line / first line rather than skipping PyTraceback::new.
Also applies to: 1587-1603
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 1525 - 1534, The code currently skips creating a traceback entry when frame.code.locations.get(idx) returns None; change this to fall back to a previous mapped location or the function's first line instead of dropping the frame. In the block that builds the traceback (where you call frame.code.locations.get(idx), PyTraceback::new and exception.set_traceback_typed), if get(idx) is None attempt frame.code.locations.get(idx.saturating_sub(1)) and if that is also None use a fallback like frame.code.first_line (or the first location's line), then construct PyTraceback::new with that line and set_traceback_typed as before. Apply the same fallback logic to the other analogous block (the one around the second occurrence you noted) so lasti/f_lineno restoration still runs and the frame always gets a traceback entry.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)extra_tests/snippets/stdlib_threading.py (1)🤖 Prompt for all review comments with AI agents13-13: Use bounded join() to keep snippet tests fail-fast.
Lines 13 and 28 use unbounded joins. If an import/threading deadlock regresses, this can stall the snippets suite until global CI timeout.
Proposed fix🤖 Prompt for AI Agents- thread.join() + thread.join(timeout=5) + assert not thread.is_alive(), "thread did not finish in time"Verify each finding against the current code and only fix it if needed. In `@extra_tests/snippets/stdlib_threading.py` at line 13, The snippet uses unbounded thread.join() calls (thread.join()) which can hang CI; replace both occurrences with a bounded join like thread.join(timeout=5) and immediately check thread.is_alive(), raising or failing the test if it remains alive (e.g., if thread.is_alive(): raise RuntimeError("thread did not terminate within timeout")). Update both calls of thread.join() in stdlib_threading.py accordingly.
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extra_tests/snippets/stdlib_threading.py`:
- Around line 7-15: The worker thread can raise during import/exec and
Thread.join() won't surface that exception, causing a misleading KeyError on the
assertion; modify the worker (function named worker used when creating
threading.Thread) to catch any Exception and store it into the shared outcome
dict (e.g., outcome["exc"] = sys.exc_info() or outcome["exc"] =
traceback.format_exc()) alongside outcome["name"], then after thread.join() in
the main flow check outcome for an exception and re-raise or fail with the
stored exception details before performing the assert on outcome["name"]; apply
the same pattern to the other similar block that uses threading.Thread/join
covering lines 21-29.
---
Nitpick comments:
In `@extra_tests/snippets/stdlib_threading.py`:
- Line 13: The snippet uses unbounded thread.join() calls (thread.join()) which
can hang CI; replace both occurrences with a bounded join like
thread.join(timeout=5) and immediately check thread.is_alive(), raising or
failing the test if it remains alive (e.g., if thread.is_alive(): raise
RuntimeError("thread did not terminate within timeout")). Update both calls of
thread.join() in stdlib_threading.py accordingly.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 417e6fe1-ae21-4620-87c5-231da02f6fa7
📥 CommitsReviewing files that changed from the base of the PR and between 9f76666 and c56f9fc.
📒 Files selected for processing (3)- crates/codegen/src/ir.rs
- crates/vm/src/stdlib/_thread.rs
- extra_tests/snippets/stdlib_threading.py
- crates/codegen/src/ir.rs
- crates/vm/src/stdlib/_thread.rs
Sorry, something went wrong.
…eading fix - Use POP_TOP instead of POP_ITER for for-loop break/return cleanup - Expand duplicate_end_returns to clone final return for jump predecessors - Restrict late jump threading pass to unconditional jumps only - Skip exception blocks in inline/reorder passes - Simplify threaded_jump_instr NoInterrupt handling
…eading fix (#7580) - Use POP_TOP instead of POP_ITER for for-loop break/return cleanup - Expand duplicate_end_returns to clone final return for jump predecessors - Restrict late jump threading pass to unconditional jumps only - Skip exception blocks in inline/reorder passes - Simplify threaded_jump_instr NoInterrupt handling Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests