New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The BigInt class does not have a const constructor #33893

Open
jacob314 opened this Issue Jul 17, 2018 · 10 comments

Comments

Projects
None yet
6 participants
@jacob314
Copy link
Member

jacob314 commented Jul 17, 2018

The BigInt class lacks a const constructor. This causes problems porting some Dart 1 code that used > 64 bit ints to Dart 2 as const values have to stop being const.

@jcollins-g has more details on the challenges this causes making the rpc library work with Dart 2.

@jcollins-g

This comment has been minimized.

Copy link
Contributor

jcollins-g commented Jul 17, 2018

The extremely short version is that the rpc package makes use of an annotation, ApiProperty, to describe interfaces:

https://github.com/dart-lang/rpc/blob/6b276372a3118305a2778c675c63f1afe0d8ca60/lib/src/annotations.dart#L71

This annotation class has properties that refer to the minimum and maximum value of integers as well as the kind of integer (including uint64). For example:

https://github.com/dart-lang/rpc/blob/0cdaa74b59e1982f07e53b7dfbea3d11e7fb4b2c/test/src/parser/api_property_int_test.dart#L54

Without a way to refer to constant BigInts (and preferably, BigInt literals), the ported API is going to look pretty ugly inside and out -- possibly requiring the use of string representations of integers and even more runtime type checking than these classes already perform. Several other workarounds have proven impractical or even more ugly so far, but I'm open to suggestions.

@matanlurey

This comment has been minimized.

Copy link
Contributor

matanlurey commented Jul 17, 2018

Not that this is a great idea, but it is an idea, AngularDart has recommended static methods, i.e.:

BigInt getMinValue() => ...

@ApiProperty(minValue: getMinValue)
...

... for this reason (with other, non-const classes).

@jcollins-g

This comment has been minimized.

Copy link
Contributor

jcollins-g commented Jul 17, 2018

@matanlurey Hmm. That seems like it'd be more helpful if we had BigInt non-const literals(??) but no const constructor. Otherwise it seems like I'm just exporting the ugliness of string parsing to my clients. But maybe there are use cases where this would save us from always having to parse strings.

@matanlurey

This comment has been minimized.

Copy link
Contributor

matanlurey commented Jul 17, 2018

Yeah. It's sad, but it's an option :-/

@jacob314

This comment has been minimized.

Copy link
Member

jacob314 commented Jul 17, 2018

Const constructors for BigInt should be trivial in DDC and soon in Dart2JS as in JavaScript running on Chrome
BigInt(10003) === BigInt(10003)

@lrhn

This comment has been minimized.

Copy link
Member

lrhn commented Jul 18, 2018

BigInt cannot have a const constructor without cheating.
The internal structure includes a Uint32List which also does not have a const constructor. In any case, a const constructor cannot create instances of anything that depends on its arguments. It wouldn't really be a const constructor, but just a compiler hack.

Literals would be tricky since, as stated above, the values aren't really constant. All other number literals are constant values.

Integrating BigInt into the language, with constant values and literals, is an option. We should probably prohibit people from extending or implementing BigInt if we want that.

One option is to add a BigInt.fromInt constant constructor and have a separate implementation class for that case, one which stores the value as a normal int and only creates the Uint32List if necessary. That would work, at the cost of making the code polymoprhic.

@jcollins-g

This comment has been minimized.

Copy link
Contributor

jcollins-g commented Jul 18, 2018

I think integrating BigInt into the language is the only way to go long term if we want to solve this -- if prohibiting extending/implementing BigInt makes that more possible I'd be in favor.

In the meantime, my workaround (BigInt literals as strings where we need them const, and runtime type checking) seems to function without invasive client code changes. Despite it looking a little ridiculous and being slow. :-)

@jacob314

This comment has been minimized.

Copy link
Member

jacob314 commented Jul 18, 2018

+100 on integrating into the language. On the web site, a const compatible version of BigInt is being integrated into JS so this is something that could be efficiently done on both the VM and Dart2Js.

@sigmundch

This comment has been minimized.

Copy link
Member

sigmundch commented Jul 18, 2018

Note that dart2js still has to support old browsers like ie11, so even though we'd take advantage of Bigints in JS engines once available, we wont be able to do it always.

I guess that if we hide the entire big-int implementation in our patch files, we could present it as a const type at that point, regardless of how it's implemented

@jcollins-g

This comment has been minimized.

Copy link
Contributor

jcollins-g commented Jul 30, 2018

@lhrn I would also like the specification to be a little bit more specific about what you can always guarantee about Dart integers. If >=53 bit precision must always available, that should be explicitly stated somewhere in the language specification. The Appendix entry strongly implies that, but the specification seems to reference the implementation for precision. If I were implementing Dart on a Commodore 64, limiting precision to 8 bits would still arguably be in spec as that would fall into the category of other considerations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment