BE-525: HashQL: Remove implicit type-widening from Integer operations, and make comparison size aware#8693
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
PR SummaryMedium Risk Overview Updates Reviewed by Cursor Bugbot for commit 1d64eff. Bugbot is set up for automated code reviews on this repo. Configure here. |
🤖 Augment PR SummarySummary: This PR fixes HashQL MIR interpreter integer semantics by removing implicit type-widening on overflow and making comparisons size-aware. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## bm/be-515-hashql-remove-offset-0-materialization-fence-from-generated #8693 +/- ##
==========================================================================================================
+ Coverage 53.92% 66.24% +12.32%
==========================================================================================================
Files 778 934 +156
Lines 57232 84770 +27538
Branches 3839 4465 +626
==========================================================================================================
+ Hits 30863 56160 +25297
- Misses 26041 28062 +2021
- Partials 328 548 +220
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 07de63c. Configure here.
07de63c to
b87ca3d
Compare
a0db929 to
58bba8c
Compare
58bba8c to
2a89970
Compare
b87ca3d to
1d64eff
Compare


🌟 What is the purpose of this PR?
Integer arithmetic in the MIR interpreter previously silently promoted overflowing
i128operations to floating-point results via aNumericenum. This was semantically incorrect: the language specifies arbitrary-precision integers, and silently widening tof64loses precision and masks errors. This PR replaces that silent promotion with explicit overflow detection, surfacing a user-facingIntegerOverflowruntime error when addition, subtraction, or negation exceeds the 128-bit range supported by the interpreter.As a side effect, the
Inttype's equality, ordering, and hashing semantics are corrected so that booleans (1-bit integers) are treated as a distinct type from integers (128-bit), rather than comparing equal to numeric integers with the same value. This restores type-level correctness forBoolvsIntcomparisons throughout the value layer.🔍 What does this change?
RuntimeError::IntegerOverflowvariant with anoperationfield, and a correspondinginteger_overflowdiagnostic emitted under theRuntimeLimitcategory with a note that the interpreter supports up to 128-bit integers.Neg,Add<Int>, andSub<Int>operator impls onInt(which returnedNumericand silently promoted tof64on overflow) withchecked_neg,checked_add, andchecked_submethods that returnOption<Int>.Numericenum entirely, along with theFrom<Numeric> for Valueconversion and all associated forwarding impls.RuntimeError::IntegerOverflowonNone.Int'sPartialEq,Ord, andHashimplementations to include thesizefield, so thatInt::from(true)(1-bit) andInt::from(1_i32)(128-bit) are no longer considered equal.PartialEq<Int> for Num,PartialOrd<Int> for Num, and their reverses to returnfalse/Nonewhen theIntis a boolean, since booleans are not a subtype ofNumber.Value::Ordto guard the cross-typeInteger/Numberordering arms with!this.is_bool()/!other.is_bool().InstSimplifyVisitor::eval_un_opto returnOption<Int>and skip constant folding for negation ofi128::MIN.inst_simplifytest confirming thatneg(i128::MIN)is not folded.int.rsandmod.rsto reflect the corrected bool/int distinction.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
🛡 What tests cover this?
integer_add_overflow,integer_sub_overflow,integer_neg_overflow— each confirms aRuntimeLimitdiagnostic is produced.inst_simplifytest:const_fold_neg_overflow— confirms that negation ofi128::MINis left unfolded.IntandValuecovering corrected equality, ordering, hashing, and cross-typeNum/Intcomparisons.❓ How to test this?
cargo test -p hashql-mirand confirm all tests pass.const_fold_neg_overflow.snapmatches the expected unfold output.RuntimeLimitand a message referencing the 128-bit limit.