prefer nb_bool slot in try_to_bool instead of bool #6328

YashSuthar983 · 2025-12-04T19:54:43Z

Fixed: #6113

Summary by CodeRabbit

Chores
- Improved boolean evaluation performance by adding a faster internal path for boolean checks.
Bug Fixes
- Preserved and unified error behavior for invalid boolean/length results so type and value errors remain consistent.

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

coderabbitai · 2025-12-04T19:54:48Z

Caution

Review failed

The pull request is closed.

Walkthrough

Prioritizes a precomputed numeric boolean slot (as_number.boolean) in try_to_bool, falling back to __bool__ descriptor and then to __len__ if needed. Preserves existing error propagation and type checks for non-bool or negative __len__ results.

Changes

Cohort / File(s)	Summary
Boolean slot optimization `crates/vm/src/builtins/bool.rs`	Added a fast-path that uses the `as_number.boolean` slot when present to obtain truthiness directly; retained fallback to `__bool__` descriptor (with error propagation and type validation) and then to `__len__` (requiring non-negative integer).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pay attention to:
- crates/vm/src/builtins/bool.rs control-flow around slot vs descriptor resolution.
- Preservation of exact error messages for non-bool __bool__ returns and negative __len__.
- Correct extraction of raw boolean value via get_value and interactions with AsNumber layout.

Possibly related PRs

Fix PyNumber::boolean #6111: Changes to numeric nb_bool semantics related to the numeric boolean slot handling.

Poem

🐇
I nibble slots where booleans hide,
A fast-path hop, no methods tried.
If descriptors wake, I'll step aside,
Else count the length and decide—
Hooray, truth found with bunny pride! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main optimization: preferring the nb_bool slot over bool in try_to_bool, which aligns with the changeset's primary objective.
Linked Issues check	✅ Passed	The PR implements the required objective from #6113: updating try_to_bool to prefer nb_bool (number slot boolean) over calling bool for better performance.
Out of Scope Changes check	✅ Passed	All changes are focused on the try_to_bool optimization within bool.rs and directly address the linked issue requirement; no out-of-scope changes detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 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 628f59e and 9e6591e.

📒 Files selected for processing (1)

crates/vm/src/builtins/bool.rs (1 hunks)

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

Fixed: RustPython#6113 Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

62-62: Consider renaming bool_obj for clarity.

This variable holds the result of calling __len__, which is an integer length, not a boolean. A name like len_obj or len_result would be more semantically accurate.

-                        let bool_obj = method.call((), vm)?;
-                        let int_obj = bool_obj.downcast_ref::<PyInt>().ok_or_else(|| {
+                        let len_obj = method.call((), vm)?;
+                        let int_obj = len_obj.downcast_ref::<PyInt>().ok_or_else(|| {
                             vm.new_type_error(format!(
                                 "'{}' object cannot be interpreted as an integer",
-                                bool_obj.class().name()
+                                len_obj.class().name()
                             ))
                         })?;

📜 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 2b90e82 and 628f59e.

📒 Files selected for processing (1)

crates/vm/src/builtins/bool.rs (1 hunks)

🧰 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/vm/src/builtins/bool.rs

🧬 Code graph analysis (1)

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

crates/vm/src/builtins/range.rs (2)

__bool__ (273-275)

PyInt (389-390)

⏰ 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 tests under miri
GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Check Rust code with clippy
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Ensure compilation on various targets
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)

🔇 Additional comments (3)

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

42-43: LGTM! Correct implementation of the nb_bool optimization path.

This correctly prioritizes the precomputed boolean numeric slot over dynamic method resolution, aligning with the PR objective.

45-58: LGTM! Correct __bool__ fallback with proper type validation.

The error propagation, type checking, and value extraction are all handled correctly.

42-81: Well-structured implementation that correctly prioritizes nb_bool.

The control flow properly implements the expected precedence (nb_bool → __bool__ → __len__ → default true), with appropriate error handling at each stage. This aligns with Python's truth value testing protocol and satisfies the PR objective.

youknowone

Thank you!
In principle, if the Python method __bool__ is defined, there must also be an nb_bool, so it seems there’s still some incorrect implementation left somewhere.

crates/vm/src/builtins/bool.rs

prefer nb_bool slot in try_to_bool instead of __bool__

628f59e

Fixed: RustPython#6113 Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>

YashSuthar983 force-pushed the nb_bool branch from 50cff20 to 628f59e Compare December 4, 2025 20:07

YashSuthar983 marked this pull request as ready for review December 4, 2025 20:07

coderabbitai bot reviewed Dec 4, 2025

View reviewed changes

youknowone approved these changes Dec 4, 2025

View reviewed changes

crates/vm/src/builtins/bool.rs Show resolved Hide resolved

Update crates/vm/src/builtins/bool.rs

9e6591e

youknowone merged commit c2ca9a7 into RustPython:main Dec 4, 2025
11 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

prefer nb_bool slot in try_to_bool instead of bool #6328

prefer nb_bool slot in try_to_bool instead of bool #6328

Uh oh!

YashSuthar983 commented Dec 4, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 4, 2025 •

edited

Loading

Review failed

Uh oh!

coderabbitai bot left a comment

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

prefer nb_bool slot in try_to_bool instead of __bool__ #6328

prefer nb_bool slot in try_to_bool instead of __bool__ #6328

Uh oh!

Conversation

YashSuthar983 commented Dec 4, 2025 • 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 Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

prefer nb_bool slot in try_to_bool instead of bool #6328

prefer nb_bool slot in try_to_bool instead of bool #6328

YashSuthar983 commented Dec 4, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 4, 2025 •

edited

Loading