FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

General code nits by ShaharNaveh · Pull Request #8135 · RustPython/RustPython · GitHub

  1. RustPython
  2. RustPython

Repository navigation

General code nits - #8135

Merged
youknowone merged 5 commits into
RustPython:mainfrom
ShaharNaveh:nits-8
Jun 22, 2026
ConversationCommits (5)ChecksFiles changed
Merged

General code nits#8135
youknowone merged 5 commits into
RustPython:mainfrom
ShaharNaveh:nits-8

Conversation

ShaharNaveh commented Jun 20, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

Summary by CodeRabbit

  • Refactor

    • Refactored code generation and bytecode handling to use modernized internal APIs.
    • Improved truthiness evaluation for constant values.
    • Updated method signatures for improved type safety and clarity.
    • Reformatted generic bounds and import statements for consistency.

  • Chores

    • Added dependency to support numeric operations.
    • Added compiler linting attributes to improve code quality checks.

coderabbitai Bot commented Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 9062cd97-e49b-43ee-9b3a-15a82f45c661

📥 Commits

Reviewing files that changed from the base of the PR and between fe2a7db and eef5950.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • crates/codegen/src/compile.rs
  • crates/codegen/src/ir.rs
  • crates/codegen/src/string_parser.rs
  • crates/compiler-core/Cargo.toml
  • crates/compiler-core/src/bytecode.rs
  • crates/vm/src/function/buffer.rs
  • crates/vm/src/function/builtin.rs
  • crates/vm/src/function/either.rs
  • crates/vm/src/function/getset.rs
  • crates/vm/src/function/method.rs
  • crates/vm/src/py_io.rs
  • crates/vm/src/py_serde.rs
  • crates/vm/src/readline.rs
📝 Walkthrough

Wait, I have some ranges referenced in the layer that are not in my all_range_ids list (range_5282, range_9274, range_1331, range_12402). Let me re-check — these don't appear in the all_range_ids list. Let me re-read the compile.rs ranges more carefully and reassign properly.

Walkthrough

Migrates compile.rs and ir.rs from bytecode::-qualified Instruction::* variant construction and CodeFlags/MakeFunctionFlags references to Opcode/PseudoOpcode-centric APIs. Adds ConstantData::truthiness() to compiler-core backed by num-traits. Applies where-clause reformatting, #[must_use]/const fn annotations, and import consolidation across VM utility files.

Changes

Compiler/codegen opcode API migration

Layer / File(s) Summary
ConstantData::truthiness() and num-traits dep
crates/compiler-core/Cargo.toml, crates/compiler-core/src/bytecode.rs		 Adds num-traits dependency, imports Zero, and introduces ConstantData::truthiness() -> bool with per-variant Python-style truth semantics, replacing the local constant_truthiness helpers in compile.rs and ir.rs.
compile.rs: import rewiring and CodeFlags/MakeFunctionFlags migration
crates/codegen/src/compile.rs		 Updates imports to non-qualified bytecode types and migrates all CodeFlags/MakeFunctionFlags/MakeFunctionFlag construction sites across module init, function flags, async/generator transitions, coroutine setup, docstring handling, type-params scopes, and comprehension scopes.
compile.rs: symbol table lookup and free-variable filtering
crates/codegen/src/compile.rs		 Refactors symbol table entry lookup to let-else form, restructures free_names filtering for FREE_CLASS, rewrites get_ref_type using current_symbol_table() and matches!, and refactors class-scope special-case symbol predicate.
compile.rs: Opcode/PseudoOpcode emission and assertion sites
crates/codegen/src/compile.rs		 Switches instruction construction from Instruction::* to Opcode::*/PseudoOpcode::* at Resume, BuildList/Set/Map, jump, break/continue, scope-entry/exit assertion, and function-attribute wiring sites; replaces Self::constant_truthiness with value.truthiness().
compile.rs: test assertions updated to CodeFlags
crates/codegen/src/compile.rs		 Updates all compile.rs unit tests from bytecode::CodeFlags-qualified assertions to direct CodeFlags identifiers for METHOD, NESTED, FUTURE_ANNOTATIONS, GENERATOR, ASYNC_GENERATOR, and COROUTINE flags.
ir.rs: CFG optimizer migration to Opcode/PseudoOpcode APIs
crates/codegen/src/ir.rs		 Migrates all CFG optimization passes (jump lowering, const folding, list/set/tuple folding, swap threading, static swaps, load-const optimization, NOP removal, superinstruction insertion, pseudo-op conversion) from Instruction::* construction and real() matching to Opcode::*/PseudoOpcode::* assignment and real_opcode()/pseudo_opcode() predicates; removes local constant_truthiness; makes Block::is_empty/is_pseudo_target const fn; adds derives to LoadFastInstrFlag and StackEffects.
ir.rs: test updates to opcode-based construction and assertions
crates/codegen/src/ir.rs		 Updates ir.rs unit tests to construct instructions via Opcode::*/PseudoOpcode::*.into() encoding and assert via real_opcode()-based matches! predicates.
string_parser.rs: EscapedChar derive additions
crates/codegen/src/string_parser.rs		 Adds Clone, Copy, Debug, Eq, PartialEq derives to EscapedChar.

VM utility cleanups

Layer / File(s) Summary
VM function/ module: where-clause, #[must_use], and doc cleanups
crates/vm/src/function/buffer.rs, crates/vm/src/function/builtin.rs, crates/vm/src/function/either.rs, crates/vm/src/function/getset.rs, crates/vm/src/function/method.rs		 Moves inline generic bounds to where-clauses in buffer.rs, builtin.rs, and either.rs; adds #[must_use] to PyMethodDef::const_copy; reformats getset.rs module doc.
VM py_io, py_serde, readline: import and annotation cleanups
crates/vm/src/py_io.rs, crates/vm/src/py_serde.rs, crates/vm/src/readline.rs		 Consolidates fmt/ops imports from core in py_io.rs, removes a .to_owned() allocation; marks PyObjectDeserializer::new as #[must_use] const fn; shortens ReadlineResult::Io error type path.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

  • [RFC] Split oparg type from the opcode enum #6746 — The PR directly implements the opcode/API separation described in the RFC, migrating all Instruction::* variant construction sites in codegen and the CFG optimizer to Opcode/PseudoOpcode-centric forms.

Possibly related PRs

  • RustPython/RustPython#7757: Both PRs switch instruction recognition/rewriting in ir.rs from Instruction variant matching to Opcode/PseudoOpcode-based APIs via real_opcode()/pseudo_opcode(), directly overlapping in the optimizer layer.
  • RustPython/RustPython#7942: Both PRs modify constant truthiness handling in codegen — this PR removes the local constant_truthiness helper and adds ConstantData::truthiness(), while that PR changed constant-folding behavior for ConstantData::Tuple.
  • RustPython/RustPython#7926: Both PRs touch compile.rs symbol/scope handling and class-scope special-name logic, making the changes directly overlapping at the symbol-table lookup and dunder-name classification level.

Suggested reviewers

  • fanninpm
  • youknowone

Poem

🐇 Hop, hop, away from bytecode:: haze,
Opcode::Resume now leads the maze!
ConstantData knows when truth is zero,
MakeFunctionFlags — a refactor hero.
The rabbit tidied every where-clause too,
Less noise, more signal — the bytecode flew! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'General code nits' is vague and generic, using non-descriptive language that does not convey meaningful information about the actual changes in the PR. Consider a more specific title that highlights the main changes, such as 'Refactor bytecode APIs to use Opcode/PseudoOpcode types' or 'Convert generic trait bounds to where-clause syntax'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 81.32% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches 🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 merged commit 9bb6ba8 into RustPython:main Jun 22, 2026
26 checks passed
coderabbitai Bot mentioned this pull request Jun 22, 2026
Merged
2 tasks
coderabbitai Bot mentioned this pull request Jul 10, 2026
Merged
youknowone pushed a commit that referenced this pull request Sep 16, 2026
General code nits (#8135)
c6e254f 
* General nitpicks

* Move Compiler::constant_truthiness to a ConstantData method

* More code cleanup

* bytecode::CodeFlags -> CodeFlags

* More code clenaups
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL