Skip to content

Use stack_effect_info for getting pop&push count#6913

Merged
youknowone merged 3 commits intoRustPython:mainfrom
ShaharNaveh:after-split-cleanup
Jan 31, 2026
Merged

Use stack_effect_info for getting pop&push count#6913
youknowone merged 3 commits intoRustPython:mainfrom
ShaharNaveh:after-split-cleanup

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Jan 31, 2026 •
edited by coderabbitai bot
Loading

Followup on #6893

Summary by CodeRabbit

  • Refactor
    • Streamlined internal code generation logic to improve maintainability and reduce code duplication. No changes to user-facing functionality or APIs.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 31, 2026 •
edited
Loading

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

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.

@youknowone youknowone enabled auto-merge (squash) January 31, 2026 08:58
@youknowone youknowone disabled auto-merge January 31, 2026 14:45
@youknowone youknowone merged commit c771427 into RustPython:main Jan 31, 2026
23 of 24 checks passed
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