Skip to content

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNaveh ShaharNaveh commented Dec 2, 2025

Implements python/cpython#100771 (partially)

Summary by CodeRabbit

  • Refactor
    • Simplified how expression printing is executed: printing is now routed through a unified intrinsic call path with explicit result cleanup. Behaviour and output remain unchanged for users, while the internal execution flow and stack handling have been streamlined.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

Walkthrough

Replaces the specialized PrintExpr instruction with an intrinsic-based flow: the compiler emits CallIntrinsic1(IntrinsicFunction1::Print) followed by Pop, the bytecode removes PrintExpr and adds IntrinsicFunction1::Print, and the VM dispatches printing via the intrinsic invoking sys.displayhook.

Changes

Cohort / File(s) Summary
Code Generation
crates/codegen/src/compile.rs
Replaced direct PrintExpr emission with CallIntrinsic1(IntrinsicFunction1::Print) then Pop in both top-level expression-statement paths.
Bytecode Definitions
crates/compiler-core/src/bytecode.rs
Added IntrinsicFunction1::Print. Removed Instruction::PrintExpr, its stack-effect handling, and disassembly formatting path.
VM Execution
crates/vm/src/frame.rs
Removed PrintExpr handler and print_expr() method. Added intrinsic dispatcher case for IntrinsicFunction1::Print that calls sys.displayhook with the argument.

Sequence Diagram(s)

sequenceDiagram
  participant Codegen
  participant Bytecode
  participant VM
  participant Sys as sys.displayhook

  Note over Codegen,Bytecode: Compilation
  Codegen->>Bytecode: emit CallIntrinsic1(Print)
  Codegen->>Bytecode: emit Pop

  Note over Bytecode,VM: Execution
  VM->>VM: execute CallIntrinsic1(Print)
  VM->>Sys: invoke displayhook(arg)
  Sys-->>VM: return
  VM->>VM: execute Pop (cleanup)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Attention areas:
    • Both codegen locations emit the exact intrinsic + pop sequence.
    • VM intrinsic dispatcher correctly mirrors previous PrintExpr semantics (argument handling, exceptions).
    • Bytecode enum and stack-effect updates are consistent across consumers (disassembler, tests).

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 I nibble bytes and hop between lanes,
I swapped the old print for intrinsic chains.
CallIntrinsic hums, then Pop wipes the track,
sys.displayhook answers — no rabbit needs slack! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: moving PrintExpr from a dedicated bytecode instruction to an IntrinsicFunction1 variant, which aligns with all modifications across compile.rs, bytecode.rs, and frame.rs.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70c3bf4 and fcf6dec.

📒 Files selected for processing (3)
  • crates/codegen/src/compile.rs (2 hunks)
  • crates/compiler-core/src/bytecode.rs (1 hunks)
  • crates/vm/src/frame.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/compiler-core/src/bytecode.rs
  • crates/codegen/src/compile.rs
  • crates/vm/src/frame.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run tests under miri
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone merged commit f49c185 into RustPython:main Dec 3, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants