Int64.== shouldn't throw an exception

[iinozemtsev]

It tries to _promote the value and fails, when an object has an unrelated type:

Invalid argument (other): not an int, Int32 or Int64: Instance of 'NullValue'
package:fixnum/src/int64_native.dart 185:5                                               Int64._promote
package:fixnum/src/int64_native.dart 273:43                                              Int64.==

[osa1]

Good catch, I'll fix it.

I noticed that we're unintentionally handling the exceptions when testing operator ==. Here's a repro:

import 'package:test/test.dart';

class C {
  bool operator ==(Object? other) {
    if (other is! C) {
      print("Throwing in operator ==");
      throw 'wat';
    }
    return identical(this, other);
  }
}

void main() {
  test('test', () {
    expect(123, isNot(equals(C())));
  });
}

This test passes even though it throws an exception in operator ==. So throwing an exception in operator == is considered as "not equal" when using isNot and equals to test it.

Not sure if this is intentional in package:test.. In any case, it looks like we need to test this more directly with expect(true, a == b).

Edit: this is being fixed in dart-lang/test#2543.

[jakemac53]

This does look intentional... no idea why though. The git history seems a bit borked but its over 7 years old.

[osa1]

Actually the operator == was always broken, it wasn't symmetric before:

core/pkgs/fixnum/lib/src/int64.dart

Lines 521 to 541 in 1aa58ef

	/// Returns [:true:] if this [Int64] has the same numeric value as the
	/// given object. The argument may be an [int] or an [IntX].
	@override
	bool operator ==(Object other) {
	Int64? o;
	if (other is Int64) {
	o = other;
	} else if (other is int) {
	if (_h == 0 && _m == 0) return _l == other;
	// Since we know one of [_h] or [_m] is non-zero, if [other] fits in the
	// low word then it can't be numerically equal.
	if ((_MASK & other) == other) return false;
	o = Int64(other);
	} else if (other is Int32) {
	o = other.toInt64();
	}
	if (o != null) {
	return _l == o._l && _m == o._m && _h == o._h;
	}
	return false;
	}

Note that it handles int so Int64(1) == 1 will be true but 1 == Int64(1) won't.

The new code has the same issue.

We can handle Int32 and Int64 (types in the package) but not types in outside of the package as that breaks symmetry.

Fixing this will inevitably break code, but only break code that's already broken (because they will only break if they realy on the broken operator ==).

Any thoughts on this? cc @lrhn

[lrhn]

I never really liked the Int64 class. It feels like the class is trying to do everything, and doesn't do any of it efficiently.
It feels like it wasn't designed with clear use-cases in mind, but instead tried to cover everything the author could think of, which means trying to serve different goals at the same time.
If we wanted to change it, we should do that with some clear use-cases in mind.

The class tries too hard to be like a num class, and almost nobody needs that. They just need a 64-bit ID number with no computation.

For those which do want computations, Int64 also tries to accept int as arguments, which is what causes == to be asymmetric. That makes it "convenient" if you're doing computations, fx myInt64++ works, but I'm not sure doing large 64-bit computations is something that needs to be convenient. If large computations happen, they should also be efficient, and Int64 isn't that efficient on web.

If we want to preserve the existing functionality, then there isn't anything we can do about ==. Someone might be doing if (myInt64 == 0) somewhere.

Having a == which is not symmetric is ... dangerous, but not a disaster. If it's documented that it's only for "direct use", like if (myInt64 == 0), and that you should not mix Int64 and int in collections, then it's mostly a curiosity and convenience feature. And probably not worth changing at this point.

TL;DR: I wouldn't try to change anything without actually having a clear goal. Small changes to functionality just risk breaking existing code, without actually getting to a better place.

[iinozemtsev]

Redesigned Int64 2.0 would be nice, but maybe we can just remove throwing for now to unblock core rolls into sdk?