Implement round-half-even float.fromhex in common float_ops by youknowone · Pull Request #8234 · RustPython/RustPython

youknowone · 2026-07-07T15:31:36Z

Summary

float.fromhex delegated to literal::float::from_hex, which is backed by hexf_parse. That reader is meant for exact literals: it rejects any inexact hexadecimal input and cannot tell overflow apart from a parse error, so float.fromhex could not round and never raised OverflowError. test_from_hex was consequently marked @unittest.expectedFailure.

This adds a dedicated hex-float reader in common::float_ops that reproduces the coefficient/exponent parsing, round-half-even rounding, and overflow detection that float.fromhex requires, and routes the classmethod through it.

Changes

common::float_ops::from_hex(&str) -> Result<f64, HexFloatError>: round-half-even over the full exponent range (subnormals down to 0x1p-1074), the inf/nan spellings, surrounding ASCII whitespace, a saturating exponent parse, and Invalid/TooLong/Overflow error kinds.
float.fromhex now maps Overflow → OverflowError and Invalid/TooLong → ValueError, with the messages Python uses.
Removed @unittest.expectedFailure from test_from_hex.
Added unit tests: exact bit patterns (including round-half-even ties), inf/nan sign bits, whitespace, and the error kinds.

Verification

from_hex output was compared bit-for-bit against CPython's float.fromhex over 125,000 inputs (curated + fuzzed, including near-halfway rounding and the DBL_MAX boundary): 0 mismatches.
cargo test -p rustpython-common and python -m test test_float pass, with test_from_hex now green.

— commented by Claude

Summary by CodeRabbit

New Features
- float.fromhex() now supports a broader set of Python-style hexadecimal floating-point inputs, including inf, nan, signs, whitespace, and full exponent parsing.
- Parsing now returns more specific errors for invalid, overly long, and overflow cases.
Bug Fixes
- Improved numeric accuracy and rounding for hexadecimal float conversion.
- Better handling of edge cases such as trailing whitespace and special floating-point values.

Add float_ops::from_hex returning Result<f64, HexFloatError>, which parses the coefficient and exponent, rounds round-half-even over the full exponent range, and detects overflow. Route float.fromhex through it: raise OverflowError for values too large to represent and ValueError for invalid input. Remove the expectedFailure on test_from_hex. Assisted-by: Claude

coderabbitai · 2026-07-07T15:32:34Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 75b911e0-c6f3-42e5-856c-e9b7e6e42922

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

Use the checkbox below for a quick retry:

🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a new from_hex implementation in crates/common/src/float_ops.rs implementing Python's float.fromhex grammar with a public HexFloatError enum, along with parsing helpers and tests. Updates PyFloat.fromhex to use this parser with specific exception mapping.

Changes

Hex Float Parsing

Layer / File(s)	Summary
HexFloatError type and parsing helpers `crates/common/src/float_ops.rs`	Adds public `HexFloatError` enum (`Invalid`, `TooLong`, `Overflow`) and internal helpers/constants for whitespace detection, hex digit decoding, case-insensitive `inf`/`nan` parsing, and correctly-rounded `ldexp` scaling.
Main from_hex parser and finalization `crates/common/src/float_ops.rs`	Implements `from_hex` covering leading whitespace, sign, `inf`/`nan` fast paths, `0x` prefix, coefficient/fraction hex digits, mandatory `p`/`P` exponent parsing, overflow/underflow detection, and half-even rounding; adds `finish_hex` for trailing-whitespace validation and sign application.
Unit tests for from_hex `crates/common/src/float_ops.rs`	Adds `from_hex_tests` module validating exact IEEE-754 bit results (including ties), `inf`/`nan` payloads, whitespace handling, and error mapping.
PyFloat.fromhex integration `crates/vm/src/builtins/float.rs`	Switches `PyFloat.fromhex` to `float_ops::from_hex` with `HexFloatError`-based mapping to `overflow_error`/`value_error`, removing the prior `.trim()` call.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Python Caller
  participant PyFloat as PyFloat.fromhex
  participant FloatOps as float_ops::from_hex
  participant Finish as finish_hex

  Caller->>PyFloat: fromhex(string)
  PyFloat->>FloatOps: from_hex(string.as_str())
  FloatOps->>FloatOps: parse whitespace, sign, inf/nan, 0x prefix, digits, exponent
  FloatOps->>Finish: finish_hex(remaining, sign, value)
  Finish-->>FloatOps: Ok(f64) or HexFloatError
  FloatOps-->>PyFloat: Result<f64, HexFloatError>
  alt Overflow
    PyFloat-->>Caller: raise OverflowError
  else TooLong / Invalid
    PyFloat-->>Caller: raise ValueError
  else Ok
    PyFloat-->>Caller: return float
  end

Related Issues: None referenced in the provided summary.

Related PRs: None referenced in the provided summary.

Suggested labels: rust, float, parsing

Suggested reviewers: None specified.

🐰 A hex string hops through digits and dots,
Past "inf" and "nan" and exponent thoughts,
Rounded just right, half-even and true,
Then errors get sorted—Overflow, Value too.
A tidy little parser, born anew!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main change: a new round-half-even hex float parser in common float_ops for float.fromhex parity.

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

coderabbitai

🧹 Nitpick comments (1)

crates/common/src/float_ops.rs (1)
503-515: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use is_ascii_digit() here

Clippy will flag this manual digit test; hex_byte_at(bytes, idx).is_ascii_digit() is clearer and removes the duplicate lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/common/src/float_ops.rs` around lines 503 - 515, The hex float
exponent parsing in float_ops::hex_byte_at/exp handling uses a manual byte range
check that Clippy flags and repeats the same lookup. Update the digit validation
in this block to use the byte’s is_ascii_digit() method instead, and keep the
surrounding exp_start/index advancement logic unchanged so the parsing behavior
stays the same.
Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/common/src/float_ops.rs`:
- Around line 503-515: The hex float exponent parsing in
float_ops::hex_byte_at/exp handling uses a manual byte range check that Clippy
flags and repeats the same lookup. Update the digit validation in this block to
use the byte’s is_ascii_digit() method instead, and keep the surrounding
exp_start/index advancement logic unchanged so the parsing behavior stays the
same.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: c4465655-ccab-4da5-bcd7-babda2b29650

📥 Commits

Reviewing files that changed from the base of the PR and between 3f33073 and 9aba933.

⛔ Files ignored due to path filters (1)

Lib/test/test_float.py is excluded by !Lib/**

📒 Files selected for processing (2)

crates/common/src/float_ops.rs
crates/vm/src/builtins/float.rs

github-actions · 2026-07-07T15:38:21Z

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] test: cpython/Lib/test/test_float.py (TODO: 3)
[x] test: cpython/Lib/test/test_strtod.py (TODO: 2)

dependencies:

dependent tests: (no tests depend on float)

Legend:

[+] path exists in CPython
[x] up-to-date, [ ] outdated

ShaharNaveh · 2026-07-07T15:34:26Z

+        b'0'..=b'9' => (c - b'0') as i32,
+        b'a'..=b'f' => (c - b'a') as i32 + 10,
+        b'A'..=b'F' => (c - b'A') as i32 + 10,
+        _ => -1,


Is there a reason for not returning an Option/Result?

ShaharNaveh · 2026-07-07T15:35:07Z

+}
+
+#[inline]
+fn is_py_space(c: u8) -> bool {


Suggested change

fn is_py_space(c: u8) -> bool {

const fn is_py_space(c: u8) -> bool {

I thought we already had a function for detecting a py_space

ShaharNaveh · 2026-07-07T15:35:44Z

+/// Read byte at `i`, returning a NUL terminator past the end so that
+/// digit/sign/space scans stop at the string boundary.
+#[inline]
+fn hex_byte_at(bytes: &[u8], i: usize) -> u8 {


Same thing, should it return an Option instead?

ShaharNaveh · 2026-07-07T15:36:01Z

+    }
+}
+
+/// `t` must be an ASCII-lowercase literal. Returns true iff every byte of `t`


Suggested change

/// `t` must be an ASCII-lowercase literal. Returns true iff every byte of `t`

/// `t` must be an ASCII-lowercase literal. Returns true if every byte of `t`

ShaharNaveh · 2026-07-07T15:36:51Z

+
+/// Returns `(retval, endptr)`. When the text at `p` is not inf/nan, `retval` is
+/// `-1.0` and `endptr == p`.
+fn parse_inf_or_nan(bytes: &[u8], p: usize) -> (f64, usize) {


same question

ShaharNaveh · 2026-07-07T15:37:20Z

+
+/// Correctly-rounded scalbn. Every call site scales an already-representable
+/// value, so the result is exact.
+fn ldexp(x: f64, mut n: i32) -> f64 {


can this be const?

ShaharNaveh · 2026-07-07T15:41:26Z

+    }
+
+    // [p <exponent>]
+    let exp = if hex_byte_at(bytes, idx) == b'p' || hex_byte_at(bytes, idx) == b'P' {


Suggested change

let exp = if hex_byte_at(bytes, idx) == b'p' || hex_byte_at(bytes, idx) == b'P' {

let exp = if matches!(hex_byte_at(bytes, idx), b'p' | b'P') {

ShaharNaveh · 2026-07-07T15:41:58Z

+    let exp = if hex_byte_at(bytes, idx) == b'p' || hex_byte_at(bytes, idx) == b'P' {
+        idx += 1;
+        let exp_start = idx;
+        if hex_byte_at(bytes, idx) == b'-' || hex_byte_at(bytes, idx) == b'+' {


same like above

ShaharNaveh · 2026-07-07T15:42:16Z

+        if hex_byte_at(bytes, idx) == b'-' || hex_byte_at(bytes, idx) == b'+' {
+            idx += 1;
+        }
+        if !(b'0' <= hex_byte_at(bytes, idx) && hex_byte_at(bytes, idx) <= b'9') {


same like above

ShaharNaveh · 2026-07-07T15:42:24Z

+            return Err(HexFloatError::Invalid);
+        }
+        idx += 1;
+        while b'0' <= hex_byte_at(bytes, idx) && hex_byte_at(bytes, idx) <= b'9' {


same like above

…prefix Assisted-by: Claude

youknowone · 2026-07-07T16:12:25Z

Thanks for the review — addressed in 51b3c1a:

byte_at (was hex_byte_at) now returns Option<u8> (bytes.get(i).copied()); call sites use == Some(..) / matches!(.., Some(b'p' | b'P')), which also folds in the matches! suggestions.
hex_from_char now returns Option<u8> and is const fn; the scans go through a small hex_digit_at peek-and-decode helper.
parse_inf_or_nan now returns Option<(f64, usize)> (no more -1.0 sentinel).
whitespace helper — you're right that it already existed: the copy is is_py_ascii_whitespace in sre_engine. I hoisted a shared pub const fn is_py_ascii_whitespace into common::str and use it here. I deliberately left sre_engine's private copy in place: sre_engine is a standalone low-level crate (it only depends on wtf8), and making it depend on common would pull the whole runtime (malachite-bigint, icu, …) into a published regex crate. Happy to have sre_engine delegate to the shared one instead if you'd prefer that tradeoff.
ldexp is now const fn; the case_insensitive_match doc wording is fixed.
Dropped the redundant test_ prefix on the unit tests (this was the clippy::redundant_test_prefix Lint failure).

— commented by Claude

Assisted-by: Claude

… sre_engine Assisted-by: Claude

youknowone · 2026-07-07T17:10:54Z

Update on the whitespace helper (after some more thought on where it belongs): instead of common::str, the shared is_py_ascii_whitespace now lives in rustpython-wtf8 and is used by both common::float_ops and sre_engine. wtf8 is the string crate that both already depend on, so this adds zero new dependency edges, and sre_engine's previous private copy is removed in favor of it. (rustpython-literal was considered but reverted, since it would have added a new dependency to sre_engine.) See bd187bf91.

For the record, on two related questions that came up: the hex-float parser can't be borrowed from ruff (Python source has no hex-float literals, so ruff's number lexer only handles hex/oct/bin integers + decimal floats — the only fromhex in ruff is vendored typeshed stubs), and ldexp isn't reused from pymath because pymath only exposes ldexp_bigint(f64, BigInt), which would force a BigInt allocation per call for what is an i32 exponent here — hence the small self-contained scalbn.

— commented by Claude

Remove the private copy of is_py_ascii_whitespace in bytes_inner.rs and import the shared rustpython_common::wtf8::is_py_ascii_whitespace. Assisted-by: Claude

coderabbitai Bot reviewed Jul 7, 2026

View reviewed changes

ShaharNaveh requested changes Jul 7, 2026

View reviewed changes

youknowone marked this pull request as draft July 7, 2026 15:55

Address review: Option-returning byte helpers, const fns, drop test_ …

51b3c1a

…prefix Assisted-by: Claude

youknowone added 2 commits July 8, 2026 01:19

Whitelist fdigits and inity in cspell

e9bc34f

Assisted-by: Claude

Home is_py_ascii_whitespace in rustpython-wtf8; share with common and…

bd187bf

… sre_engine Assisted-by: Claude

Use shared wtf8 is_py_ascii_whitespace in bytes_inner

b5b66b6

Remove the private copy of is_py_ascii_whitespace in bytes_inner.rs and import the shared rustpython_common::wtf8::is_py_ascii_whitespace. Assisted-by: Claude

	fn is_py_space(c: u8) -> bool {
	const fn is_py_space(c: u8) -> bool {

	/// `t` must be an ASCII-lowercase literal. Returns true iff every byte of `t`
	/// `t` must be an ASCII-lowercase literal. Returns true if every byte of `t`

	let exp = if hex_byte_at(bytes, idx) == b'p' \|\| hex_byte_at(bytes, idx) == b'P' {
	let exp = if matches!(hex_byte_at(bytes, idx), b'p' \| b'P') {

Uh oh!

Conversation

youknowone commented Jul 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Verification

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Review skipped

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 7, 2026

📦 Library Dependencies

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Jul 7, 2026

Uh oh!

youknowone commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

youknowone commented Jul 7, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 7, 2026 •

edited

Loading