GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Remove `_dis` module by ShaharNaveh · Pull Request #6690 · RustPython/RustPython

Conversation

ShaharNaveh commented Jan 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

After #6638 Lib/_opcode_metadata.py is generated for our bytecode, so it might be possible to use dis.py and not patch it at all

Summary by CodeRabbit

Refactor
- Standardized internal code flag naming conventions across compiler components and the runtime
Chores
- Removed the Python code disassembly (dis) module from the standard library

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot commented Jan 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

📝 Walkthrough

Walkthrough

This PR systematically renames CodeFlags enum variants to standardize naming conventions, removing prefixes like IS_, HAS_, and NEW_ across the codebase. The NAME_MAPPING constant is removed from CodeFlags, which necessitates removal of the entire dis module that depended on it.

Changes

Cohort / File(s)	Summary
CodeFlags Definition `crates/compiler-core/src/bytecode.rs`	Renamed public bitflag constants: `NEW_LOCALS`→`NEWLOCALS`, `IS_GENERATOR`→`GENERATOR`, `IS_COROUTINE`→`COROUTINE`, `HAS_VARARGS`→`VARARGS`, `HAS_VARKEYWORDS`→`VARKEYWORDS`, `IS_OPTIMIZED`→`OPTIMIZED`. Removed public `NAME_MAPPING` constant.
Code Generation `crates/codegen/src/compile.rs`, `crates/codegen/src/ir.rs`	Updated all CodeFlags variant references to use renamed flags in compilation logic, flag-setting, and boundary checks for variable argument handling.
VM Runtime `crates/vm/src/builtins/function.rs`, `crates/vm/src/builtins/function/jit.rs`, `crates/vm/src/frame.rs`	Updated CodeFlags variant checks in function execution, argument packing, varargs/kwargs handling, generator/coroutine detection, and yield operations to use new flag names. Added explicit positional-argument validation in function.rs when `VARARGS` flag is absent.
Module Removal `crates/stdlib/src/dis.rs`, `crates/stdlib/src/lib.rs`	Removed entire `dis` module implementation that depended on the now-removed `CodeFlags::NAME_MAPPING` constant. Deleted module registration from init list.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

youknowone

Poem

🐰 A hoppy refactor hops along the way,
Flags rename themselves to brighter day,
OLD_NAMES → NEWNAMES bright and clear,
Where dis departs, the codebase shines here! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title only describes removing the _dis module but the changeset also includes substantial CodeFlags renaming across multiple files, which is the larger systemic change.	Consider a more comprehensive title like 'Remove _dis module and standardize CodeFlags naming' or 'Remove _dis module and align CodeFlags with generated opcode metadata' to reflect the scope of changes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.78% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Jan 10, 2026

Copy link

Copy Markdown

Contributor

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin try-update-dis

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh added 2 commits

January 10, 2026 12:34


          Remove _dis module

376d5ba


          Adjust CodeFlag

ec25472

ShaharNaveh force-pushed the try-update-dis branch from 4ceacf1 to ec25472 Compare

January 10, 2026 10:34

ShaharNaveh added 4 commits

January 10, 2026 12:37


          Fix jit

24444cb


          Merge remote-tracking branch 'upstream/main' into try-update-dis

958792f


          Unmark passing test

0c735b0


          Unmark more tests

022dbc5

ShaharNaveh marked this pull request as ready for review

January 10, 2026 11:38

coderabbitai Bot reviewed

Jan 10, 2026

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

crates/vm/src/builtins/function.rs (3)

127-155: LGTM! Proper validation for excess positional arguments.

The rename from HAS_VARARGS to VARARGS aligns with standard Python CodeFlags naming conventions. The new else branch (lines 131-155) correctly enforces Python semantics by rejecting excess positional arguments when the function doesn't accept *args. The error message properly indicates the valid range of positional arguments and handles grammatical number agreement correctly.

158-164: LGTM! Flag rename aligns with standard conventions.

The rename from HAS_VARKEYWORDS to VARKEYWORDS is consistent with the broader refactoring to align with standard Python CodeFlags naming.

417-423: LGTM! Flag rename aligns with standard conventions.

The rename from NEW_LOCALS to NEWLOCALS is consistent with the standard Python CodeFlags naming, removing the underscore prefix.

🤖 Fix all issues with AI agents

In @crates/vm/src/builtins/function.rs:
- Around line 439-440: No code changes required; the rename from
IS_GENERATOR/IS_COROUTINE to bytecode::CodeFlags::GENERATOR and ::COROUTINE and
the match logic that constructs generator/coroutine/async generator objects are
correct as-is, so leave the uses of
code.flags.contains(bytecode::CodeFlags::GENERATOR) and
code.flags.contains(bytecode::CodeFlags::COROUTINE) and the surrounding
match/creation logic unchanged.

🧹 Nitpick comments (1)

crates/codegen/src/compile.rs (1)

6369-6383: Minor: avoid shadowing flags when adding COROUTINE in comprehensions.

This is correct as-is, but the double let flags = ...; let flags = if ... is a small readability hit.

Proposed refactor

-        let flags = bytecode::CodeFlags::NEWLOCALS | bytecode::CodeFlags::OPTIMIZED;
-        let flags = if is_async {
-            flags | bytecode::CodeFlags::COROUTINE
-        } else {
-            flags
-        };
+        let mut flags = bytecode::CodeFlags::NEWLOCALS | bytecode::CodeFlags::OPTIMIZED;
+        if is_async {
+            flags |= bytecode::CodeFlags::COROUTINE;
+        }

📜 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 9e225f5 and 022dbc5.

⛔ Files ignored due to path filters (4)

Lib/dis.py is excluded by !Lib/**
Lib/test/test_dis.py is excluded by !Lib/**
Lib/test/test_inspect/test_inspect.py is excluded by !Lib/**
Lib/test/test_unittest/testmock/testasync.py is excluded by !Lib/**

📒 Files selected for processing (8)

crates/codegen/src/compile.rs
crates/codegen/src/ir.rs
crates/compiler-core/src/bytecode.rs
crates/stdlib/src/dis.rs
crates/stdlib/src/lib.rs
crates/vm/src/builtins/function.rs
crates/vm/src/builtins/function/jit.rs
crates/vm/src/frame.rs

💤 Files with no reviewable changes (2)

crates/stdlib/src/lib.rs
crates/stdlib/src/dis.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 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/codegen/src/ir.rs
crates/vm/src/builtins/function/jit.rs
crates/vm/src/builtins/function.rs
crates/vm/src/frame.rs
crates/codegen/src/compile.rs
crates/compiler-core/src/bytecode.rs

🧬 Code graph analysis (1)

crates/vm/src/builtins/function.rs (1)

crates/vm/src/frame.rs (2)

locals (158-194)

locals (2402-2406)

⏰ 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). (2)

GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (windows-2025)

🔇 Additional comments (16)

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

374-379: LGTM! Clean flag naming standardization.

The renamed constants remove unnecessary prefixes, improving readability. Bit positions remain unchanged, ensuring binary compatibility.

1554-1561: Flag usage correctly updated.

The method properly uses the renamed VARARGS and VARKEYWORDS flags to determine argument structure.

crates/vm/src/builtins/function/jit.rs (1)

73-81: Flag check correctly updated for JIT argument validation.

The updated flag names maintain the same logic: preventing JIT compilation of functions with variable argument counts.

crates/codegen/src/ir.rs (1)

335-336: Correct flag usage in cell2arg calculation.

The total argument count properly includes variadic arguments using the renamed flags.

crates/vm/src/frame.rs (3)

189-191: Flag usage correctly updated.

The OPTIMIZED flag check properly controls freevar mapping in the locals collection logic.

1003-1009: Coroutine context check correctly updated.

The COROUTINE flag properly enforces that coroutines can only be yielded from within coroutine contexts.

1674-1678: Async generator wrapping logic correctly updated.

The COROUTINE flag determines whether to wrap yielded values for async generators. The logic is correct: wrap only when arg == 0 (direct yield) and the COROUTINE flag is set.

crates/codegen/src/compile.rs (9)

377-404: Verify module CodeFlags initialization matches intended co_flags (NEWLOCALS vs empty).

Compiler::new initializes the module ir::CodeInfo.flags as CodeFlags::NEWLOCALS (Line 380), while enter_scope uses CodeFlags::empty() for CompilerScope::Module (Line 749). If standard dis.py compatibility depends on exact co_flags, please confirm the module-level choice is intentional (and consistent with how CodeObject.co_flags is computed).

747-769: Scope default flags update looks consistent with the renamed CodeFlags.

Function-like scopes default to NEWLOCALS | OPTIMIZED, while module/class remain empty(). This aligns with using COROUTINE/GENERATOR as additional markers elsewhere.

1298-1323: make_qualname: using OPTIMIZED for “function parent” detection is reasonable—ensure no non-function scopes set it.

is_function_parent now keys off parent.flags.contains(CodeFlags::OPTIMIZED) (Line 1301) plus name guards. Please double-check there aren’t any non-function scopes that set OPTIMIZED while having a non-"<...>" name, or they’ll get .<locals>. qualnames unexpectedly.

1911-1919: Async return check updated correctly to CodeFlags::GENERATOR.

2064-2090: Function entry flags + varargs/kwargs flags updated correctly.

push_output(NEWLOCALS | OPTIMIZED, ...) plus VARARGS / VARKEYWORDS mutations look consistent with the rename and prior behavior.

3103-3120: Coroutine flag set via flags.set(COROUTINE, is_async) is clean and clear.

3233-3241: Type-params scope flags updated consistently.

Also applies to: 3664-3672

7042-7044: Generator marking updated correctly to CodeFlags::GENERATOR.

1-20: Please run formatting/lints/tests for the flag rename sweep.

Per repo guidelines for **/*.rs, make sure cargo fmt and cargo clippy are clean, and that dis-/opcode-metadata-related tests/snapshots still match expectations after the CodeFlags rename and _dis removal.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/builtins/function.rs

Comment on lines +439 to +440

		let is_gen = code.flags.contains(bytecode::CodeFlags::GENERATOR);
		let is_coro = code.flags.contains(bytecode::CodeFlags::COROUTINE);

coderabbitai Bot Jan 10, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

LGTM! Flag renames align with standard conventions.

The renames from IS_GENERATOR to GENERATOR and IS_COROUTINE to COROUTINE are consistent with standard Python CodeFlags naming, removing the IS_ prefix. The match logic correctly handles all combinations for creating the appropriate generator/coroutine/async generator objects.

🤖 Prompt for AI Agents

In @crates/vm/src/builtins/function.rs around lines 439 - 440, No code changes
required; the rename from IS_GENERATOR/IS_COROUTINE to
bytecode::CodeFlags::GENERATOR and ::COROUTINE and the match logic that
constructs generator/coroutine/async generator objects are correct as-is, so
leave the uses of code.flags.contains(bytecode::CodeFlags::GENERATOR) and
code.flags.contains(bytecode::CodeFlags::COROUTINE) and the surrounding
match/creation logic unchanged.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Trigger CI

e7f3b08

ShaharNaveh requested a review from youknowone

January 10, 2026 12:57

youknowone approved these changes

Jan 10, 2026

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! This moment came sooner than I expected!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh reacted with hooray emoji

All reactions

🎉 1 reaction

youknowone merged commit a400386 into RustPython:main

Jan 10, 2026

13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request

Jan 15, 2026


          Remove _dis module (RustPython#6690)

e034ebe

* Remove `_dis` module

* Adjust CodeFlag

coderabbitai Bot mentioned this pull request

Jan 22, 2026

Update types from v3.14.2 #6833

Merged

ShaharNaveh mentioned this pull request

Apr 23, 2026

Remove unused rust impl for formatting dis output #7660

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

ShaharNaveh commented Jan 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

github-actions Bot commented Jan 10, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShaharNaveh commented Jan 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Jan 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.