Skip to content

Remove ImportNameless bytecode #6325

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.

Sign up for GitHub

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

Jump to bottom
Merged
youknowone merged 1 commit into from
Dec 2, 2025
Merged

Remove ImportNameless bytecode #6325

youknowone merged 1 commit into from
Dec 2, 2025

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNaveh ShaharNaveh commented Dec 2, 2025 •
edited by coderabbitai bot
Loading

This is CPython does it:
https://github.com/python/cpython/blob/8183fa5e3f78ca6ab862de7fb8b14f3d929421e0/Python/compile.c#L3960-L3961

Summary by CodeRabbit

Release Notes

  • Refactor
    • Simplified import handling by consolidating the import instruction implementation. Import operations now use a unified approach across all scenarios, reducing code complexity and maintenance overhead.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025 •
edited
Loading

Walkthrough

The changes remove the ImportNameless instruction variant from the import handling pipeline. The codegen module now unconditionally emits ImportName with an empty string for the module name when no module is present, eliminating the previous conditional branching between ImportName and ImportNameless.

Changes

Cohort / File(s) Summary
Import Instruction Removal
crates/compiler-core/src/bytecode.rs		 Removed ImportNameless enum variant from Instruction and deleted related branches in stack_effect and fmt_dis functions.
Codegen Compilation Update
crates/codegen/src/compile.rs		 Replaced conditional emission of ImportName or ImportNameless with uniform ImportName emission; uses empty string as module name when module is absent.
VM Execution Path Removal
crates/vm/src/frame.rs		 Removed ImportNameless instruction branch from ExecutingFrame::execute_instruction.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify that empty string module names in the codegen change are correctly handled throughout the runtime execution
  • Confirm all references to ImportNameless have been removed across the codebase
  • Validate that the uniform ImportName approach with empty module strings doesn't introduce edge case regressions

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 A nameless import hops away,
One path remains, no more the fray,
Empty strings now do the trick,
Clean and simple, very slick!
The code flows smooth, no branches tangled,
Import logic, nicely wrangled. ✨

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 title 'Remove ImportNameless bytecode' directly and specifically summarizes the main change: removing the ImportNameless bytecode variant and its associated handling across three files.
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 d287d1e and a8a489e.

📒 Files selected for processing (3)
  • crates/codegen/src/compile.rs (1 hunks)
  • crates/compiler-core/src/bytecode.rs (1 hunks)
  • crates/vm/src/frame.rs (0 hunks)
💤 Files with no reviewable changes (1)
  • crates/vm/src/frame.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

  • crates/compiler-core/src/bytecode.rs
  • crates/codegen/src/compile.rs
🧬 Code graph analysis (1)
crates/codegen/src/compile.rs (3)
crates/derive-impl/src/pymodule.rs (1)
  • module_name (437-439)
crates/derive-impl/src/util.rs (1)
  • module (428-457)
crates/codegen/src/ir.rs (1)
  • idx (35-37)
⏰ 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: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run tests under miri
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (2)
crates/codegen/src/compile.rs (1)

1354-1365: Unified ImportName handling for nameless from imports looks correct

Using module.as_ref().map_or("", |s| s.as_str()) and always emitting ImportName keeps the stack contract identical to CPython: level and from_list are consumed and the module object is pushed, with nameless from-imports represented by an empty-string entry in co_names. This removes the need for a separate ImportNameless opcode while preserving observable behavior, assuming the VM’s ImportName now treats "" the same way the old nameless variant did.

Please double‑check that the updated ImportName execution path in crates/vm/src/frame.rs explicitly handles the empty‑string name in the same way the removed ImportNameless variant did, and that there are tests (or disassembly snapshots) covering from . import x / from ... import x cases.

crates/compiler-core/src/bytecode.rs (1)

1604-1611: ImportName stack effect -1 matches its two-arg import usage

Defining ImportName { .. } => -1 is consistent with the compiler pushing level and from_list before the opcode and expecting a single module object afterward (net 2 pops, 1 push). Combined with ImportFrom { .. } => 1, this keeps the import stack discipline coherent after removing ImportNameless.

If you have a stack‑effect test or disassembly snapshot suite for import opcodes, it’s worth re‑running/expanding it to confirm the computed effects for import x and from ... import y still match CPython’s behavior.

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 305fb48 into RustPython:main Dec 2, 2025
13 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Dec 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ShaharNaveh @youknowone