Skip to content

Conversation

@osa1
Copy link
Member

@osa1 osa1 commented Sep 5, 2025

This PR adds another Int64 implementation that is just a wrapper around int, to be used in targets where int is 64 bits.

Because dart doc generates using the native class, all documentation comments are moved to the native class.

The conditional import is implemented based on availability of the library dart:html, which is only available on dart2js.

Improves #172. It could be further improved by having Int64 as an extension class when targeting native and Wasm, but that would be a breaking change, and we would have to remove the IntX superclass as extensions cannot extend or implement other classes.

This PR adds another `Int64` implementation that is just a wrapper
around `int`, to be used in targets where `int` is 64 bits.

Becuase `dart doc` generates using the native class, all documentation
comments are moved to the native class.

The conditional import is implemented based on availability of the
library `dart:html`, which is only available on dart2js.

Improves dart-lang#172. It could be further improved by having `Int64` as an
extension class when targeting native and Wasm, but that would be a
breaking change.
@github-actions
Copy link

github-actions bot commented Sep 5, 2025

PR Health

Breaking changes ⚠️
Package Change Current Version New Version Needed Version Looking good?
fixnum Breaking 1.1.1 1.2.0-wip 2.0.0
Got "1.2.0-wip" expected >= "2.0.0" (breaking changes)
⚠️

This check can be disabled by tagging the PR with skip-breaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

Coverage ⚠️
File Coverage
pkgs/fixnum/lib/fixnum.dart 💔 Not covered
pkgs/fixnum/lib/src/int32.dart 💚 94 %
pkgs/fixnum/lib/src/int64.dart 💔 0 % ⬇️ 100 %
pkgs/fixnum/lib/src/int64_emulated.dart 💔 Not covered
pkgs/fixnum/lib/src/int64_native.dart 💚 96 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

This check can be disabled by tagging the PR with skip-license-check.

@osa1 osa1 marked this pull request as ready for review September 5, 2025 11:26
@osa1 osa1 requested a review from a team as a code owner September 5, 2025 11:26
Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume most code that doesn't do arithmetics is just copied, so LGTM.

(Any comments are nice-to-have, but not something introduced by this CL.)

factory Int64.fromInts(int high, int low) =>
Int64((high << 32) | (low & 0xFFFFFFFF));

factory Int64.fromBytes(List<int> bytes) => Int64(((bytes[7] & 0xFF) << 56) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If the bytes is a Uint8List, could possibly use an internal getterto read the bytes. I don't know if doing the type check costs more than just reading eight bytes.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't know. This is existing code, we can benchmark and optimize it later if needed. For now doing the same as before won't regress anything so it's safe.

// `dart:html` is only available on dart2js (dart2wasm won't support it), so we
// can check availability of it to test whether we're compiling to JS. Other
// targets (AOT, JIT, Wasm) support 64-bit `int`s.
export 'int64_native.dart' if (dart.library.html) 'int64_emulated.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dart:html is deprecated. When it goes away, which we should assume it will, this version of the package will then break. (If the library goes away in Dart 4.0, the SDK versioning will probably handle it, but it could be sooner.)

We could add some kind of ensurance against running int64_native.dart with JS number semantics.

Maybe add something like this to the native library:

class _Assert {
  const _Assert(bool b) : assert(b);
}
const _notWebNumbers = _Assert(0x10000000000000 != 0x10000000000000 + 1);

That should run that assert at compile-time.

We could also assert that you do have web numbers in the int64_emulated.dart file, just to catch the detection going wrong in the other direction for some reason.

(We should just define some compilation environment keys that allows asking the question directly, like if (dart.runtime.js_numbers) ....)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dart:html is deprecated. When it goes away, which we should assume it will, this version of the package will then break.

Good point. I'm not sure how else to import different things in dart2js and dart2wasm. The only libraries we can use right now are the deprecated ones as dart2wasm won't support those.

(We should just define some compilation environment keys that allows asking the question directly, like if (dart.runtime.js_numbers) ...

That's dart-lang/sdk#53811, which was opened in 2023.

It's probably not going to happen, we should find another way for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to use dart.library.html for now, because (1) AFAIK there's no other way (other than the deprecated interop libs, dart:html or others) to import different things in dart2wasm and dart2js (2) we already have a few other uses (e.g. in protobuf) (3) we need to release another way to distinguish dart2wasm from dart2js before removing dart:html because we can't just start to use the same code with dart2wasm and dart2js, that will cause performance regression and other breakage (due to number semantics differences, and maybe other differences too).

When we release this other way to distinguish dart2wasm from dart2js, we will have to revisit all these use sites and update them, but that's just life.

@lrhn do you see any other way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do wasm complies support dart:js_interop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this with the dart2js team and it looks like we have some work to do before we can consider removing dart:html. Merging...

} else if (value is int) {
return Int64(value);
} else if (value is Int32) {
return value.toInt64() as Int64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This as Int64 looks unnecessary.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because analyzer cannot properly analyze this package with conditional imports, it thinks I have two Int64s:

  error - lib/src/int64_emulated.dart:172:14 - A value of type 'Int64 (where Int64 is defined in
          /usr/local/google/home/omersa/dart/core/pkgs/fixnum/lib/src/int64_native.dart)' can't be returned from the method '_promote' because it has a return
          type of 'Int64 (where Int64 is defined in /usr/local/google/home/omersa/dart/core/pkgs/fixnum/lib/src/int64_emulated.dart)'. - return_of_invalid_type
           - Int64 is defined in /usr/local/google/home/omersa/dart/core/pkgs/fixnum/lib/src/int64_native.dart at lib/src/int64_emulated.dart:15:28.
           - Int64 is defined in /usr/local/google/home/omersa/dart/core/pkgs/fixnum/lib/src/int64_emulated.dart at lib/src/int64_emulated.dart:14:7.

@eernstg
Copy link
Member

eernstg commented Sep 5, 2025

Just a drive-by comment: I guess Int64 couldn't be an extension type on otherwise suitable platforms, because it must be a subtype of IntX?

@osa1
Copy link
Member Author

osa1 commented Sep 5, 2025

Just a drive-by comment: I guess Int64 couldn't be an extension type on otherwise suitable platforms, because it must be a subtype of IntX?

Good point. I don't know why Int64 and Int32 need a common superclass. Does anyone know?

@eernstg
Copy link
Member

eernstg commented Sep 5, 2025

Perhaps because IntX <: Comparable<IntX>, which allows various sorting related APIs to be used. So it may not be important that Int32 and Int64 have a shared supertype, but people may want to sort lists of either.

@osa1
Copy link
Member Author

osa1 commented Sep 5, 2025

Couldn't we just implement Comparable directly in Int64 and Int32?

@eernstg
Copy link
Member

eernstg commented Sep 5, 2025

I don't know if this will suffice:

extension type Int64(int value) implements num {
  // ...
}

int foo<X extends Comparable<X>>(X x1, X x2) => x1.compareTo(x2); // OK.

void main() => print(foo(Int64(42), Int64(24)));

It makes APIs callable even though they require X extends Comparable<X> for a type parameter X, so it might be possible to use an extension type like this in places where the code has previously used class Int64 extends IntX {...}. Feels like something that would be nice to see before believing it, though. ;-D

One difficulty would be that a return type could be IntX using the old approach (because IntX <: Comparable<IntX>, but it is not true that Int64 <: Comparable<Int64>, so the inferred type argument to foo would be IntX), and this will now have type num (which is inferred because num <: Comparable<num>).

However, I suspect that IntX is hardly ever used as the type of anything, and all those sort related APIs might very well return void.

@lrhn
Copy link
Member

lrhn commented Sep 7, 2025

I don't think Int32 is actually needed for anything.

The original design apparently tried to make number-equivalent behavior for the two classes, including being comparable to each other and generally being compatible with int.

That's not what users needed. The vast majority of uses are just for ID numbers, and would work without any operations other than parse, toString and hashCode/==.

@osa1
Copy link
Member Author

osa1 commented Sep 8, 2025

I don't think Int32 is actually needed for anything.

FWIW we have internal users of this type and even IntX.

@osa1 osa1 merged commit a4dc873 into dart-lang:main Sep 9, 2025
15 checks passed
@osa1 osa1 deleted the optimize_int64 branch September 9, 2025 08:45
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Sep 9, 2025
Revisions updated by `dart tools/rev_sdk_deps.dart`.

ai (https://github.com/dart-lang/ai/compare/1547a83..bc90433):
  bc90433  2025-09-04  Greg Spencer  Add pub add format guidance (dart-lang/ai#278)
  845c126  2025-09-04  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/ai#277)
  5568958  2025-09-04  Jacob MacDonald  add a GEMINI.md file (dart-lang/ai#279)

core (https://github.com/dart-lang/core/compare/5c3e2c3..a4dc873):
  a4dc8738  2025-09-09  Ömer Sinan Ağacan  Implement `Int64` as a wrapper for `int` when targeting native and Wasm (dart-lang/core#905)
  1aa58ef5  2025-09-08  Devon Carew  [fixnum] update the min. required dart sdk (dart-lang/core#907)
  60f2b5d3  2025-09-08  Ömer Sinan Ağacan  Run fixnum tests with dart2wasm (dart-lang/core#906)

dartdoc (https://github.com/dart-lang/dartdoc/compare/53222e2..efff7c1):
  efff7c1f  2025-09-08  Sarah Zakarias  Remove  runtime_renderers.dart (dart-lang/dartdoc#4101)
  4e2daf57  2025-09-05  Sarah Zakarias  Rename LanguageFeature and remove FeatureSet mixin (dart-lang/dartdoc#4099)
  ecc48d00  2025-09-02  dependabot[bot]  Bump the github-actions group across 1 directory with 3 updates (dart-lang/dartdoc#4098)

protobuf (https://github.com/dart-lang/protobuf/compare/0a13935..971bcae):
  971bcae  2025-09-08  Ömer Sinan Ağacan  Use a map to cache the `valueOf` functions for enums (google/protobuf.dart#1047)
  8750ed7  2025-09-05  Ömer Sinan Ağacan  Sync internal Kythe support improvements (google/protobuf.dart#1048)

web (https://github.com/dart-lang/web/compare/a152054..e2daa3a):
  e2daa3a  2025-09-04  Nikechukwu  [interop] Add support for Intersection types (dart-lang/web#451)

webdev (https://github.com/dart-lang/webdev/compare/52ad019..23aefeb):
  23aefebe  2025-09-05  jensjoha  Fix CI flake in dwds/test/hot_restart_breakpoints_test.dart (dart-lang/webdev#2685)

Change-Id: I36cfbcdd884e8e0e38e24a9c7f3bcfcc15fdd9ff
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/449000
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
Auto-Submit: Devon Carew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants