Fix invalid casts in is/2 #2777

adri326 · 2025-01-17T19:26:17Z

This fixes #2772 and a few of other issues I found:

rnd_i would first convert big floats into an i64, before converting them into an IBig
round/1 would add 0.5 to big integers, losing precision beyond ±2 ^ 53
gcd/2 would attempt to cast its second argument to an isize before computing the gcd using IBig::gcd, triggering a panic!()
min/2 and max/2 would return the float version of their arguments if their types didn't match

This PR is accompanied by a test suite (that @UWN might be interested in) for the behavior of is/2, both after being compiled and being metacalled. Implementation-specific behavior (like bitshifting negative numbers) is excluded from this test suite.

I ran the integration tests with cargo-tarpaulin to get a report and help me find uncovered edge cases. I chose not to test special cases like pow(integer, integer), since it's unclear to me whether or not small Number::Integer should be returned from computations (like X is 2 ^ 64 - 2 ^ 64).

Each of these changes (including the addition of the new test suite) were placed in their own commits.

adri326 · 2025-01-17T22:35:46Z

(might as well fix round/1 while I'm at it)

src/machine/arithmetic_ops.rs

triska · 2025-01-18T06:58:34Z

Thank you a lot, this is ideal: With this I can address #2771 entirely in Prolog, based on solid underlying functionality in the core!

src/arithmetic.rs

Skgland · 2025-01-20T18:09:42Z

src/machine/arithmetic_ops.rs

-            Ok(Number::Float(cmp::max(OrderedFloat(f1), OrderedFloat(f2))))
+            match OrderedFloat(f1).cmp(&OrderedFloat(f2)) {
+                cmp::Ordering::Less => Ok(n2),
+                cmp::Ordering::Equal => Ok(Number::Float(OrderedFloat(f1))),


Is it intentional/relevant that the returned value in the equal case has changed?
I think this would only be relevant for an edge case ¹ of 0.0 and -0.0 which OrderedFloat compares as equal if I am not mistaken.
cmp::max specifies that in the case of equality the second argument is returned, now the first argument is returned.

Footnotes

NaN values should have already been eliminated by result_f ↩

This is replicating the behavior of SWI-Prolog, where I return a floating-point value if both terms are equal. The idea (as far as I understand) is to make max(1, 1.0) return the same value as max(1.0, 1).

That still leaves max(0.0, -0.0) and max(-0.0, 0.0), SWI-Prolog treats 0.0 and 0 to be larger than -0.0, as defined by your linked documentation.
But the implementation here returns the first argument converted to float i.e. -0.0 for max(-0.0, 0).

Though I am not sure if one is currently able to produce -0.0 in scryer-prolog.

Though I am not sure if one is currently able to produce -0.0 in scryer-prolog.

In ISO 0.0 and -0.0 are the same.

?- 0.0 == -0.0. true.

In ISO 0.0 and -0.0 are the same.

While they compare as numerically equal, the SWI-Prolog documentation of max contains the following text

However, the special float -0.0 is smaller than 0.0 as well as the integer 0

Which sounds to me like max should treat 0.0 and 0 as being larger than -0.0,
though I am not sure if this is just an implementation detail or ISO required.

For FFI it would be good to to properly support unusual/invalid float values like ±infinity, ±nan and -0.0 similar to SWI-Prolog https://www.swi-prolog.org/pldoc/man?section=ieee-float which allows setting flags to switch from ISO behavior to IEEE float behavior.
(I am not trying to say that this should be implemented as part of this PR)

SWI is not ISO in many areas like this one.

tests/scryer/cli/src_tests/arithmetic.stdin

adri326 · 2025-01-26T21:44:32Z

Somehow my tests did not cover these cases, but upon refactoring the implementation of >>/2 and <</2, I noticed that the following cases were handled improperly:

?- X is 1 >> 64.
    X = 1, unexpected.
    X = 0. % expected, but not found

?- X is 1 << 64.
    X = 1, unexpected.
    X = 18446744073709551616. % expected, but not found

?- X is 1 << 63.
    X = -9223372036854775808, unexpected.
    X = 9223372036854775808. % expected, but not found

I shan't exclude the fix from this PR :)

Fixes mthom#2772. The current implementation of `rnd_i` incorrectly casts `f` (an `f64`) into an `i64`, before casting it into an `Integer`. This fixes that issue by using `Integer::try_from(f)` instead, and failing if `f` is infinite or NaN. A fixme is left for a future PR to properly handle the resulting errors in floor/1 and friends (right now they can only be triggered through FFI).

The original issue can be reproduced with `X is round(2 ^ 54 + 1) - 2 ^ 54, X = 1.`

The implementation for gcd/2 would cast the second argument to an isize.

It now behaves the same way as SWI-Prolog.

adri326 · 2025-01-26T23:08:09Z

(squashed the earlier fixups)

(the push below fixes a typo)

This extensively tests the behavior of is/2, both when compiled and in metacalls.

adri326 commented Jan 17, 2025

View reviewed changes

src/machine/arithmetic_ops.rs Outdated Show resolved Hide resolved

adri326 commented Jan 17, 2025

View reviewed changes

src/machine/arithmetic_ops.rs Show resolved Hide resolved

adri326 force-pushed the fix-2772-rnd_i-clipping branch from 508e901 to a739390 Compare January 17, 2025 22:44

adri326 changed the title ~~Fix rnd_i clipping floats that don't fit in Fixnum~~ Fix invalid casts in is/2 Jan 20, 2025

adri326 force-pushed the fix-2772-rnd_i-clipping branch from 1afa051 to 3b6c209 Compare January 20, 2025 16:09

Skgland reviewed Jan 20, 2025

View reviewed changes

Skgland mentioned this pull request Jan 20, 2025

add ffi tests & fix ffi #2786

Draft

adri326 added 4 commits January 27, 2025 00:02

Fix round() losing precision on bigints

d3361c1

The original issue can be reproduced with `X is round(2 ^ 54 + 1) - 2 ^ 54, X = 1.`

Fix X is gcd(1, 2 ^ 64) triggering a panic

e2d1a2b

The implementation for gcd/2 would cast the second argument to an isize.

Fix min/2 and max/2 returning the cast version of its arguments

f3a92c6

It now behaves the same way as SWI-Prolog.

adri326 force-pushed the fix-2772-rnd_i-clipping branch from ed71062 to f6abafe Compare January 26, 2025 23:07

adri326 added 2 commits January 27, 2025 00:10

Fix integer overflow in >>/2 and <</2

20735ea

Add integration tests for arithmetic operators

0cde8f9

This extensively tests the behavior of is/2, both when compiled and in metacalls.

adri326 force-pushed the fix-2772-rnd_i-clipping branch from f6abafe to 0cde8f9 Compare January 26, 2025 23:11

mthom merged commit 5a869e8 into mthom:master Jan 27, 2025
13 checks passed

adri326 mentioned this pull request Jan 30, 2025

Donating a library for reasoning about CBOR #2800

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid casts in is/2 #2777

Fix invalid casts in is/2 #2777

adri326 commented Jan 17, 2025 •

edited

Loading

adri326 commented Jan 17, 2025

triska commented Jan 18, 2025

Skgland Jan 20, 2025

adri326 Jan 20, 2025

Skgland Jan 20, 2025

Skgland Jan 20, 2025

UWN Jan 22, 2025

Skgland Jan 22, 2025 •

edited

Loading

UWN Jan 22, 2025

adri326 commented Jan 26, 2025

adri326 commented Jan 26, 2025 •

edited

Loading

Fix invalid casts in is/2 #2777

Fix invalid casts in is/2 #2777

Conversation

adri326 commented Jan 17, 2025 • edited Loading

adri326 commented Jan 17, 2025

triska commented Jan 18, 2025

Skgland Jan 20, 2025

Choose a reason for hiding this comment

Footnotes

adri326 Jan 20, 2025

Choose a reason for hiding this comment

Skgland Jan 20, 2025

Choose a reason for hiding this comment

Skgland Jan 20, 2025

Choose a reason for hiding this comment

UWN Jan 22, 2025

Choose a reason for hiding this comment

Skgland Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

UWN Jan 22, 2025

Choose a reason for hiding this comment

adri326 commented Jan 26, 2025

adri326 commented Jan 26, 2025 • edited Loading

adri326 commented Jan 17, 2025 •

edited

Loading

Skgland Jan 22, 2025 •

edited

Loading

adri326 commented Jan 26, 2025 •

edited

Loading