LibJS: Improve correctness of rounding and bitwise operations

Patch from Anonymous

9 files changed, 106 insertions, 19 deletions

diff --git a/Userland/Libraries/LibJS/Runtime/MathObject.cpp b/Userland/Libraries/LibJS/Runtime/MathObject.cpp
index bdd3ec6e17..579a40636c 100644
--- a/Userland/Libraries/LibJS/Runtime/MathObject.cpp
+++ b/Userland/Libraries/LibJS/Runtime/MathObject.cpp
@@ -154,7 +154,16 @@ JS_DEFINE_NATIVE_FUNCTION(MathObject::round)
         return {};
     if (number.is_nan())
         return js_nan();
-    return Value(::round(number.as_double()));
+    double intpart = 0;
+    double frac = modf(number.as_double(), &intpart);
+    if (intpart >= 0) {
+        if (frac >= 0.5)
+            intpart += 1.0;
+    } else {
+        if (frac < -0.5)
+            intpart -= 1.0;
+    }
+    return Value(intpart);
 }
 
 JS_DEFINE_NATIVE_FUNCTION(MathObject::max)
diff --git a/Userland/Libraries/LibJS/Runtime/Value.cpp b/Userland/Libraries/LibJS/Runtime/Value.cpp
index adc2411889..deccdf7c99 100644
--- a/Userland/Libraries/LibJS/Runtime/Value.cpp
+++ b/Userland/Libraries/LibJS/Runtime/Value.cpp
@@ -465,6 +465,7 @@ BigInt* Value::to_bigint(GlobalObject& global_object) const
     }
 }
 
+// FIXME: These two conversions are wrong for JS, and seem likely to be footguns
 i32 Value::as_i32() const
 {
     return static_cast<i32>(as_double());
@@ -495,9 +496,17 @@ i32 Value::to_i32(GlobalObject& global_object) const
     auto number = to_number(global_object);
     if (global_object.vm().exception())
         return INVALID;
-    if (number.is_nan() || number.is_infinity())
+    double value = number.as_double();
+    if (!isfinite(value) || value == 0)
         return 0;
-    return number.as_i32();
+    auto abs = fabs(value);
+    auto int_val = floor(abs);
+    if (signbit(value))
+        int_val = -int_val;
+    auto int32bit = fmod(int_val, 4294967296.0);
+    if (int32bit >= 2147483648.0)
+        int32bit -= 4294967296.0;
+    return static_cast<i32>(int32bit);
 }
 
 u32 Value::to_u32(GlobalObject& global_object) const
@@ -506,11 +515,14 @@ u32 Value::to_u32(GlobalObject& global_object) const
     auto number = to_number(global_object);
     if (global_object.vm().exception())
         return INVALID;
-    if (number.is_nan() || number.is_infinity())
+    double value = number.as_double();
+    if (!isfinite(value) || value == 0)
         return 0;
-    if (number.as_double() <= 0)
-        return 0;
-    return number.as_u32();
+    auto int_val = floor(fabs(value));
+    if (signbit(value))
+        int_val = -int_val;
+    auto int32bit = fmod(int_val, NumericLimits<u32>::max() + 1.0);
+    return static_cast<u32>(int32bit);
 }
 
 size_t Value::to_length(GlobalObject& global_object) const
@@ -613,7 +625,7 @@ Value bitwise_and(GlobalObject& global_object, Value lhs, Value rhs)
     if (both_number(lhs_numeric, rhs_numeric)) {
         if (!lhs_numeric.is_finite_number() || !rhs_numeric.is_finite_number())
             return Value(0);
-        return Value((i32)lhs_numeric.as_double() & (i32)rhs_numeric.as_double());
+        return Value(lhs_numeric.to_i32(global_object.global_object()) & rhs_numeric.to_i32(global_object.global_object()));
     }
     if (both_bigint(lhs_numeric, rhs_numeric))
         return js_bigint(global_object.heap(), lhs_numeric.as_bigint().big_integer().bitwise_and(rhs_numeric.as_bigint().big_integer()));
@@ -636,7 +648,7 @@ Value bitwise_or(GlobalObject& global_object, Value lhs, Value rhs)
             return rhs_numeric;
         if (!rhs_numeric.is_finite_number())
             return lhs_numeric;
-        return Value((i32)lhs_numeric.as_double() | (i32)rhs_numeric.as_double());
+        return Value(lhs_numeric.to_i32(global_object.global_object()) | rhs_numeric.to_i32(global_object.global_object()));
     }
     if (both_bigint(lhs_numeric, rhs_numeric))
         return js_bigint(global_object.heap(), lhs_numeric.as_bigint().big_integer().bitwise_or(rhs_numeric.as_bigint().big_integer()));
@@ -659,7 +671,7 @@ Value bitwise_xor(GlobalObject& global_object, Value lhs, Value rhs)
             return rhs_numeric;
         if (!rhs_numeric.is_finite_number())
             return lhs_numeric;
-        return Value((i32)lhs_numeric.as_double() ^ (i32)rhs_numeric.as_double());
+        return Value(lhs_numeric.to_i32(global_object.global_object()) ^ rhs_numeric.to_i32(global_object.global_object()));
     }
     if (both_bigint(lhs_numeric, rhs_numeric))
         return js_bigint(global_object.heap(), lhs_numeric.as_bigint().big_integer().bitwise_xor(rhs_numeric.as_bigint().big_integer()));
@@ -704,6 +716,8 @@ Value unary_minus(GlobalObject& global_object, Value lhs)
 
 Value left_shift(GlobalObject& global_object, Value lhs, Value rhs)
 {
+    // 6.1.6.1.9 Number::leftShift
+    // https://tc39.es/ecma262/#sec-numeric-types-number-leftShift
     auto lhs_numeric = lhs.to_numeric(global_object.global_object());
     if (global_object.vm().exception())
         return {};
@@ -715,7 +729,10 @@ Value left_shift(GlobalObject& global_object, Value lhs, Value rhs)
             return Value(0);
         if (!rhs_numeric.is_finite_number())
             return lhs_numeric;
-        return Value((i32)lhs_numeric.as_double() << (i32)rhs_numeric.as_double());
+        // Ok, so this performs toNumber() again but that "can't" throw
+        auto lhs_i32 = lhs_numeric.to_i32(global_object.global_object());
+        auto rhs_u32 = rhs_numeric.to_u32(global_object.global_object());
+        return Value(lhs_i32 << rhs_u32);
     }
     if (both_bigint(lhs_numeric, rhs_numeric))
         TODO();
@@ -725,6 +742,8 @@ Value left_shift(GlobalObject& global_object, Value lhs, Value rhs)
 
 Value right_shift(GlobalObject& global_object, Value lhs, Value rhs)
 {
+    // 6.1.6.1.11 Number::signedRightShift
+    // https://tc39.es/ecma262/#sec-numeric-types-number-signedRightShift
     auto lhs_numeric = lhs.to_numeric(global_object.global_object());
     if (global_object.vm().exception())
         return {};
@@ -736,7 +755,10 @@ Value right_shift(GlobalObject& global_object, Value lhs, Value rhs)
             return Value(0);
         if (!rhs_numeric.is_finite_number())
             return lhs_numeric;
-        return Value((i32)lhs_numeric.as_double() >> (i32)rhs_numeric.as_double());
+        // Ok, so this performs toNumber() again but that "can't" throw
+        auto lhs_i32 = lhs_numeric.to_i32(global_object.global_object());
+        auto rhs_u32 = rhs_numeric.to_u32(global_object.global_object());
+        return Value(lhs_i32 >> rhs_u32);
     }
     if (both_bigint(lhs_numeric, rhs_numeric))
         TODO();
@@ -746,6 +768,8 @@ Value right_shift(GlobalObject& global_object, Value lhs, Value rhs)
 
 Value unsigned_right_shift(GlobalObject& global_object, Value lhs, Value rhs)
 {
+    // 6.1.6.1.11 Number::unsignedRightShift
+    // https://tc39.es/ecma262/#sec-numeric-types-number-unsignedRightShift
     auto lhs_numeric = lhs.to_numeric(global_object.global_object());
     if (global_object.vm().exception())
         return {};
@@ -757,7 +781,10 @@ Value unsigned_right_shift(GlobalObject& global_object, Value lhs, Value rhs)
             return Value(0);
         if (!rhs_numeric.is_finite_number())
             return lhs_numeric;
-        return Value((unsigned)lhs_numeric.as_double() >> (i32)rhs_numeric.as_double());
+        // Ok, so this performs toNumber() again but that "can't" throw
+        auto lhs_u32 = lhs_numeric.to_u32(global_object.global_object());
+        auto rhs_u32 = rhs_numeric.to_u32(global_object.global_object()) % 32;
+        return Value(lhs_u32 >> rhs_u32);
     }
     global_object.vm().throw_exception<TypeError>(global_object.global_object(), ErrorType::BigIntBadOperator, "unsigned right-shift");
     return {};
@@ -1085,9 +1112,9 @@ bool abstract_eq(GlobalObject& global_object, Value lhs, Value rhs)
         if ((lhs.is_number() && !lhs.is_integer()) || (rhs.is_number() && !rhs.is_integer()))
             return false;
         if (lhs.is_number())
-            return Crypto::SignedBigInteger { lhs.as_i32() } == rhs.as_bigint().big_integer();
+            return Crypto::SignedBigInteger { lhs.to_i32(global_object.global_object()) } == rhs.as_bigint().big_integer();
         else
-            return Crypto::SignedBigInteger { rhs.as_i32() } == lhs.as_bigint().big_integer();
+            return Crypto::SignedBigInteger { rhs.to_i32(global_object.global_object()) } == lhs.as_bigint().big_integer();
     }
 
     return false;
@@ -1194,12 +1221,12 @@ TriState abstract_relation(GlobalObject& global_object, bool left_first, Value l
     bool x_lower_than_y;
     if (x_numeric.is_number()) {
         x_lower_than_y = x_numeric.is_integer()
-            ? Crypto::SignedBigInteger { x_numeric.as_i32() } < y_numeric.as_bigint().big_integer()
-            : (Crypto::SignedBigInteger { x_numeric.as_i32() } < y_numeric.as_bigint().big_integer() || Crypto::SignedBigInteger { x_numeric.as_i32() + 1 } < y_numeric.as_bigint().big_integer());
+            ? Crypto::SignedBigInteger { x_numeric.to_i32(global_object.global_object()) } < y_numeric.as_bigint().big_integer()
+            : (Crypto::SignedBigInteger { x_numeric.to_i32(global_object.global_object()) } < y_numeric.as_bigint().big_integer() || Crypto::SignedBigInteger { x_numeric.to_i32(global_object.global_object()) + 1 } < y_numeric.as_bigint().big_integer());
     } else {
         x_lower_than_y = y_numeric.is_integer()
-            ? x_numeric.as_bigint().big_integer() < Crypto::SignedBigInteger { y_numeric.as_i32() }
-            : (x_numeric.as_bigint().big_integer() < Crypto::SignedBigInteger { y_numeric.as_i32() } || x_numeric.as_bigint().big_integer() < Crypto::SignedBigInteger { y_numeric.as_i32() + 1 });
+            ? x_numeric.as_bigint().big_integer() < Crypto::SignedBigInteger { y_numeric.to_i32(global_object.global_object()) }
+            : (x_numeric.as_bigint().big_integer() < Crypto::SignedBigInteger { y_numeric.to_i32(global_object.global_object()) } || x_numeric.as_bigint().big_integer() < Crypto::SignedBigInteger { y_numeric.to_i32(global_object.global_object()) + 1 });
     }
     if (x_lower_than_y)
         return TriState::True;
diff --git a/Userland/Libraries/LibJS/Tests/builtins/Math/rounding-modes.js b/Userland/Libraries/LibJS/Tests/builtins/Math/rounding-modes.js
new file mode 100644
index 0000000000..b46251151b
--- /dev/null
+++ b/Userland/Libraries/LibJS/Tests/builtins/Math/rounding-modes.js
@@ -0,0 +1,28 @@
+test("basic rounding", () => {
+    expect(Math.round(1.25)).toBe(1);
+    expect(Math.round(-1.25)).toBe(-1);
+    expect(Math.round(1.5)).toBe(2);
+    expect(Math.round(-1.5)).toBe(-1);
+    expect(Math.round(1.75)).toBe(2);
+    expect(Math.round(-1.75)).toBe(-2);
+    expect(Math.round(1)).toBe(1);
+    expect(Math.round(-1)).toBe(-1);
+    expect(Math.round(4294967296.5)).toBe(4294967297);
+    expect(Math.round(-4294967296.5)).toBe(-4294967296);
+    expect(Math.round(4294967297)).toBe(4294967297);
+    expect(Math.round(-4294967297)).toBe(-4294967297);
+});
+test("basic floor", () => {
+    expect(Math.floor(1.25)).toBe(1);
+    expect(Math.floor(-1.25)).toBe(-2);
+    expect(Math.floor(1.5)).toBe(1);
+    expect(Math.floor(-1.5)).toBe(-2);
+    expect(Math.floor(1.75)).toBe(1);
+    expect(Math.floor(-1.75)).toBe(-2);
+    expect(Math.floor(1)).toBe(1);
+    expect(Math.floor(-1)).toBe(-1);
+    expect(Math.floor(4294967296.5)).toBe(4294967296);
+    expect(Math.floor(-4294967296.5)).toBe(-4294967297);
+    expect(Math.floor(4294967297)).toBe(4294967297);
+    expect(Math.floor(-4294967297)).toBe(-4294967297);
+});
diff --git a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-and.js b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-and.js
index 5eb4f19cc3..a60a69e7fc 100644
--- a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-and.js
+++ b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-and.js
@@ -40,6 +40,9 @@ test("basic numeric and", () => {
     expect(5 & 3).toBe(1);
     expect(5 & 4).toBe(4);
     expect(5 & 5).toBe(5);
+
+    expect(0xffffffff & 0).toBe(0);
+    expect(0xffffffff & 0xffffffff).toBe(-1);
 });
 
 test("and with non-numeric values", () => {
diff --git a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-left-shift.js b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-left-shift.js
index d0c9d8d5f9..07c5c9028e 100644
--- a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-left-shift.js
+++ b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-left-shift.js
@@ -40,6 +40,11 @@ test("basic numeric shifting", () => {
     expect(5 << 3).toBe(40);
     expect(5 << 4).toBe(80);
     expect(5 << 5).toBe(160);
+
+    expect(0xffffffff << 0).toBe(-1);
+    expect(0xffffffff << 16).toBe(-65536);
+    expect(0xffff0000 << 16).toBe(0);
+    expect(0xffff0000 << 15).toBe(-2147483648);
 });
 
 test("shifting with non-numeric values", () => {
diff --git a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-or.js b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-or.js
index 485d1226d2..28ef3d12a7 100644
--- a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-or.js
+++ b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-or.js
@@ -40,6 +40,10 @@ test("basic numeric or", () => {
     expect(5 | 3).toBe(7);
     expect(5 | 4).toBe(5);
     expect(5 | 5).toBe(5);
+
+    expect(0xffffffff | 0).toBe(-1);
+    expect(0xffffffff | 0xffffffff).toBe(-1);
+    expect(2147483648 | 0).toBe(-2147483648);
 });
 
 test("or with non-numeric values", () => {
diff --git a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-right-shift.js b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-right-shift.js
index f071c5da92..efdc4b79f1 100644
--- a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-right-shift.js
+++ b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-right-shift.js
@@ -26,6 +26,10 @@ test("basic numeric shifting", () => {
     expect(42 >> 3).toBe(5);
     expect(42 >> 4).toBe(2);
     expect(42 >> 5).toBe(1);
+
+    expect(0xffffffff >> 0).toBe(-1);
+    expect(0xffffffff >> 16).toBe(-1);
+    expect((0xf0000000 * 2) >> 16).toBe(-8192);
 });
 
 test("numeric shifting with negative lhs values", () => {
diff --git a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-unsigned-right-shift.js b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-unsigned-right-shift.js
index 4a950e18ef..62283631b3 100644
--- a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-unsigned-right-shift.js
+++ b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-unsigned-right-shift.js
@@ -26,6 +26,10 @@ test("basic numeric shifting", () => {
     expect(42 >>> 3).toBe(5);
     expect(42 >>> 4).toBe(2);
     expect(42 >>> 5).toBe(1);
+
+    expect(0xffffffff >>> 0).toBe(4294967295);
+    expect(0xffffffff >>> 16).toBe(65535);
+    expect((0xf0000000 * 2) >>> 16).toBe(57344);
 });
 
 test("numeric shifting with negative lhs values", () => {
diff --git a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-xor.js b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-xor.js
index 356ad01ca4..452b27088f 100644
--- a/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-xor.js
+++ b/Userland/Libraries/LibJS/Tests/operators/binary-bitwise-xor.js
@@ -40,6 +40,9 @@ test("basic numeric xor", () => {
     expect(5 ^ 3).toBe(6);
     expect(5 ^ 4).toBe(1);
     expect(5 ^ 5).toBe(0);
+
+    expect(0xffffffff ^ 0).toBe(-1);
+    expect(0xffffffff ^ 0xffffffff).toBe(0);
 });
 
 test("xor with non-numeric values", () => {