Use `stack_effect_info` for getting pop&push count by ShaharNaveh · Pull Request #6913 · RustPython/RustPython

youknowone

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)

crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snap is excluded by !**/*.snap

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

🔍 Trigger a full review

📝 Walkthrough

Walkthrough

The PR refactors stack effect computation in the codegen IR by replacing manual per-instruction decomposition with a unified stack_effect_info-based approach, reducing code complexity by 54 lines while maintaining existing behavior including underflow handling and LOAD_FAST optimization tracking.

Changes

Cohort / File(s)	Summary
Stack Effect Refactoring `crates/codegen/src/ir.rs`	Replaces extensive per-instruction pop/push mappings with unified `stack_effect_info`-derived pushes/pops computation. Removes large match statements for BinaryOp, LoadAttr, Call and other instructions. Updates variable naming from (pops, pushes) to (pushes, pops) while preserving downstream underflow handling and NOT_LOCAL-based LOAD_FAST optimization tracking.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Split stack_effect into pushed & popped #6893 — Directly corresponds to this PR's changes by introducing the split of stack_effect into pushed/popped that this refactoring relies on.
Remove SUBSCRIPT, JUMP_IF_{TRUE,FALSE}_OR_POP #6810 — Also modifies how instruction stack effects are represented and handled across the codebase.
LoadClosure as pseudo op #6789 — Modifies instruction handling in the same IR file with related stack-effect machinery changes.

Suggested reviewers

youknowone

Poem

🐰 Stack effects need not be verbose and grand,
Unite the wisdom, simplify the stand!
Fifty-four lines vanish, yet the logic stays,
A cleaner IR shines through the codegen maze! ✨

🚥 Pre-merge checks | ✅ 3

✅ 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 'Use `stack_effect_info` for getting pop&push count' clearly and specifically describes the main refactoring change: replacing manual stack effect decomposition with the `stack_effect_info`-based approach.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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