fix: number.floor rounds toward -infinity (not toward zero)#5048
fix: number.floor rounds toward -infinity (not toward zero)#5048mvanhorn wants to merge 1 commit intoobjectionary:masterfrom
Conversation
The current floor uses as-i64.as-number which truncates toward zero. Mathematical floor rounds toward -infinity, which differs from truncation only for negative non-integers. Wrap the truncation in a correction step: if trunc is strictly greater than the original (only possible when the input is a negative non-integer), return trunc - 1; otherwise return trunc. Non-finite inputs (NaN, +infinity, -infinity) short-circuit via is-finite.not and are returned unchanged. Also updates two existing tests (tests-div-with-remainder and tests-floor-negative-fraction) that codified the incorrect toward-zero behavior, and adds regression tests for the small negative fraction (-0.1 -> -1) and negative-integer cases. is-finite is safe to reuse here because its definition at number.eo:23-28 does not recurse through floor (unlike is-integer, which is defined as is-finite and eq floor). Fixes objectionary#5043
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eo-runtime/src/main/eo/number.eo`:
- Around line 115-124: The floor method is incorrectly using bare `$` which
refers to the method object dataization and can create cycles; change those `$`
references inside the floor body to reference the enclosing number (use `^` or
bind the input to a name like `original` before transformations) so the
non-finite branch returns the original numeric value and the comparison
`trunc.gt` compares against that original number; update references in the floor
implementation (symbols: floor, trunc, is-finite, as-i64.as-number) and re-run
nan.floor, positive-infinity.floor and negative-infinity.floor tests on both JVM
and eo2js to confirm correct behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92c14285-06f5-440b-8a75-bfbe6aa48ae1
📒 Files selected for processing (1)
eo-runtime/src/main/eo/number.eo
| # For non-finite inputs (NaN, ±infinity) the input is returned unchanged. | ||
| [] > floor | ||
| as-i64.as-number > @ | ||
| if. > @ | ||
| is-finite.not | ||
| $ | ||
| if. | ||
| trunc.gt $ | ||
| trunc.minus 1 | ||
| trunc | ||
| as-i64.as-number > trunc |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Find any use of a bare `$` as a value inside method bodies in .eo files.
rg -nP --type-add 'eo:*.eo' --type=eo -C3 '^\s*\$\s*$|[^\w$]\$(\s|$)' eo-runtime/src/main/eo
# 2) Find uses of `^` (parent reference) to see the established idiom for
# "refer to the enclosing object from inside a method body".
rg -nP --type-add 'eo:*.eo' --type=eo -C3 '(^|[^\w])\^(\.|\s|$)' eo-runtime/src/main/eo | head -n 200
# 3) Surface the exact test objects that rely on floor for non-finite inputs,
# to confirm they actually invoke `.floor` (and are not shadowed by `.@`):
rg -nP --type-add 'eo:*.eo' --type=eo -C2 'floor' eo-runtime/src/main/eo/positive-infinity.eo eo-runtime/src/main/eo/negative-infinity.eoRepository: objectionary/eo
Length of output: 23296
Verify that $ refers to the enclosing number, not to the floor method itself.
In EO, ^ is the established pattern for referencing the enclosing parent object in method bodies. The implementation at lines 119 and 121 uses bare $, which is not found elsewhere in the codebase in this style.
Two locations are affected:
- Line 119 (
$as the non-finite branch result): if$dataizes throughfloor.@, this creates a cycle. - Line 121 (
trunc.gt $): comparingtruncagainst$dataizes the floor method rather than the input number.
Tests for negative-infinity.floor and positive-infinity.floor exist (negative-infinity.eo:502–503, positive-infinity.eo:515–516), but they must be confirmed to exercise the floor method correctly. If $ is incorrect, replace it with ^ (or an explicit binding like as-bytes.as-number > original). Verify that nan.floor, positive-infinity.floor, and negative-infinity.floor evaluate correctly on both JVM and eo2js runtimes before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eo-runtime/src/main/eo/number.eo` around lines 115 - 124, The floor method is
incorrectly using bare `$` which refers to the method object dataization and can
create cycles; change those `$` references inside the floor body to reference
the enclosing number (use `^` or bind the input to a name like `original` before
transformations) so the non-finite branch returns the original numeric value and
the comparison `trunc.gt` compares against that original number; update
references in the floor implementation (symbols: floor, trunc, is-finite,
as-i64.as-number) and re-run nan.floor, positive-infinity.floor and
negative-infinity.floor tests on both JVM and eo2js to confirm correct behavior.
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
1 participant
Summary
Changes
number.floorineo-runtime/src/main/eo/number.eofrom truncation-toward-zero to mathematical floor (round toward -∞), matching the existing doc comment ("less than or equal to it") and the behavior ofMath.floorin Java / Python / JS / Ruby.Fixes #5043.
Why this matters
The issue (syncended) traces it to the doc/implementation mismatch:
3.7.floor33-3.7.floor-4-3← wrong-0.1.floor-10← wrongfloor. 13.div -5-3-2← wrongPR #5042 (merged this morning) refactored the Java atom into an EO expression (
as-i64.as-number) but preserved the truncation-toward-zero behavior — that PR's description explicitly notes "See also: #5043 (separate behavioral fix forfloorrounding semantics)." This PR is that separate behavioral fix.Change
eo-runtime/src/main/eo/number.eofloor body from:to:
Non-finite inputs (NaN, +∞, -∞) short-circuit via
is-finite.notand are returned unchanged — this preserves the behavior the existingtests-positive-infinity-floor-is-equal-to-selfandtests-negative-infinity-floor-is-equal-to-selftests codify.is-nanis not a separate case becauseis-finitealready returns false for NaN, and the$-return branch returns NaN unchanged.For finite inputs:
truncis the integer part (toward zero).trunc > $is true only for negative non-integers (-3.7 → trunc=-3 > -3.7); in that one case subtract 1 to round toward -∞. For positive non-integers, zero, and integers,truncis already the correct floor.is-finiteis safe to reuse here because its definition atnumber.eo:23-28(is-nan.not and. not. or. eq positive-infinity eq negative-infinity) does not recurse through floor. The issue explicitly warns against reusingis-integerbecause that one is defined asis-finite and eq floorand would recurse.Tests
Two existing tests codified the buggy truncation-toward-zero behavior — both needed updating:
tests-div-with-remainder:floor(13 / -5)now expects-3(was-2)tests-floor-negative-fraction:-3.7.floornow expects-4(was-3); comment updated from "truncates toward zero" to "rounds toward -infinity"Two new regression tests added next to the existing floor tests:
tests-floor-small-negative-fraction:-0.1.floor = -1(guards the case where truncation would hit zero)tests-floor-of-negative-integer:-42.floor = -42(guards against over-eager subtraction in the integer case)Existing
tests-floor-positive-fraction(3.7.floor = 3) andtests-floor-of-integer(42.floor = 42) continue to pass unchanged.Testing
I was not able to complete a full local
mvn test -Dtest=EOnumberEOAtomTestrun in a reasonable window on my environment (~10+ min into the build without finishing). Trusting upstream CI to catch any EO syntax issues in the newif.nesting. Happy to iterate on the review.AI disclosure
This contribution was developed with AI assistance (Claude Code).
Summary by CodeRabbit
floorfunction to correctly handle edge cases and improve mathematical accuracy. Non-finite numbers are now returned unchanged, while negative numbers and fractions are properly rounded toward negative infinity, ensuring correct behavior across all input types.