Skip to content
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

[spec] Normative: Add i64<->BigInt conversion in JS API #707

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

littledan
Copy link
Collaborator

@littledan littledan commented Feb 17, 2018

See the JS-BigInt-integration proposal here:

https://github.com/WebAssembly/JS-BigInt-integration

@jfbastien
Copy link
Member

This is a very significant change if applied towards the current working draft. Is your intent to keep this PR open until the public draft has shipped, and we can easily move non-editorial things into the spec?

@littledan
Copy link
Collaborator Author

@jfbastien I was imagining that this would stay as a PR for a while (at least until it meets the implementation requirements of WebAssembly proposals, for example). Is there something more that I should write up to frame this as a proper proposal?

I don't have a strong opinion about how this should relate to the W3C process, but for a comparison, ECMAScript continues development even during the several month IP opt out period, developing in the context of a working draft which follows the regular, official version cut. I don't see why this sort of model couldn't work for Wasm as well.

@jfbastien
Copy link
Member

We've usually followed this process:
https://github.com/WebAssembly/meetings/blob/master/process/phases.md
(with edits in a PR that @flagxor has to review still)

So you only go to the spec repo after having a fork in a separate repo.

@littledan
Copy link
Collaborator Author

OK, this has a fork with the new spec in it (https://github.com/littledan/spec/tree/bigint) and a tracking issue in the design repo (WebAssembly/design#1172). The commit message here describes at a high level what this proposal is doing.

Should I put this commit message into a README.md in that repository, or is it sufficient in this location at the top of the thread? Either way, is the next step is to propose this feature for some sort of stage advancement at a WebAssembly Community Group meeting?

@jfbastien
Copy link
Member

Tracking issues are the ones that I tag as such. The issue you link to is the "someone has an idea" step. Sorry, I know the process isn't great yet! Gotta improve what's written down.

Process would be that you put it on the agenda for the next CG meeting, and we'll poll to officially create a fork repo for it. Then we can transfer your repo to the organization, like TC39 does. Should pass easy, but I still want us to follow that process so we don't play favorites. It's also a really good way to keep the wider CG informed.

@littledan
Copy link
Collaborator Author

Oh, I see, the tracking issue and the fork have to be specifically approved. Makes sense. I didn't realize those words had special meanings here. I guess I'll put something on the CG agenda.

(In TC39, there are two processes for making changes: A formal staged proposal and a PR. We still don't play favorites, but sometimes smaller changes go through less paperwork.)

@xtuc
Copy link
Contributor

xtuc commented Feb 20, 2018

Thanks @littledan! Sorry for my ignorance, how can you see the rendered version of the document?

Copy link
Contributor

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

Does that means that a BigInt support is now required to be WebAssembly spec compliant?

Can we make it optional?

let module = new WebAssembly.Module(exportingModuleLongIdentityFn);
let instance = new WebAssembly.Instance(module);

let value = 2n ** 63n;
Copy link
Contributor

Choose a reason for hiding this comment

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

That assumes the host has the BigInt proposal support?

Copy link
Collaborator Author

@littledan littledan Feb 20, 2018

Choose a reason for hiding this comment

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

Yes, it does (this spec change requires the host to have BigInt support). When we split up this file, this test should probably be split off to let the others keep running on systems that don't support BigInt.

1. If |w| is of the form [=𝖿𝟨𝟦.𝖼𝗈𝗇𝗌𝗍=] |f64|, return [=the Number value=] for |f64|.
1. If |w| is of the form [=𝗂𝟨𝟦.𝖼𝗈𝗇𝗌𝗍=] |i64|,
1. Let |v| be [=signed_64=](|i64|).
1. Return a [=BigInt=] representing the mathematical value |v|.
Copy link
Contributor

@xtuc xtuc Feb 20, 2018

Choose a reason for hiding this comment

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

Does "mathematical value" have a special meaning here? I don't think we need to mention math, It's just a value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"Mathematical value" is a phrase used in the ECMAScript spec. It's discussed in this section but not really linkable. I'd be fine with deleting the word "mathematical" here if it's more confusing than helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, it's fine for me then. Let's maybe wait for another thought on this.

Copy link

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, but it should probably be "... of |v|". The link is https://tc39.es/ecma262/#mathematical-value, if you want to linkify it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that's right: ES explicitly says that in the phrase "the mathematical value of x", x is a Number (i.e., a IEEE double) - which it definitely can't be at this point, because then we've lost precision.

Note that the numbers coming from wasm are already treated as "mathematical values" below, as they're passed to "the Number value for x", where x is expected to be a MV. We might want to formalize this a little more, but I'm not quite sure how to best do that.

Unless you feel strongly, I'd like to defer the linkifying until after this is merged.

@rossberg
Copy link
Member

rossberg commented Feb 20, 2018

@xtuc, clearly we won't make this into a part of the Wasm standard before BigInt has become a part of the JS standard (it's getting there). At that point, though, there seems to be little point in making it optional.

@littledan
Copy link
Collaborator Author

@rossberg My hope is that this can be ready for implementations to ship at the same time as they are shipping BigInt.

I don't have a strong opinion about when it lands in the standard, but it really wouldn't be unusual at all for the Wasm standard to land while BigInt is at Stage 3 (given all the standards which make normative references to "Editor's Draft"s, this would be nothing--Stage 3 is something like "Candidate Recommendation").

@jfbastien
Copy link
Member

I don't have a strong opinion about when it lands in the standard, but it really wouldn't be unusual at all for the Wasm standard to land while BigInt is at Stage 3 (given all the standards which make normative references to "Editor's Draft"s, this would be nothing--Stage 3 is something like "Candidate Recommendation").

What do you mean by "wouldn't be unusual"? It would be helpful to have points of reference on how we approach this. I'd like to have it as a topic for an upcoming CG meeting.

@littledan
Copy link
Collaborator Author

@jfbastien The HTML spec already references BigInt, for example.

@xtuc
Copy link
Contributor

xtuc commented Dec 18, 2018

I think tthat this PR is obsolete, the specification and tests now lives under the proposal repo. Could we close this?

@xtuc
Copy link
Contributor

xtuc commented Mar 4, 2019

@littledan could we close this PR in favor of the proposal repo?

@littledan
Copy link
Collaborator Author

We'll have to reopen the PR if the proposal ever reaches Stage 5; what is the problem with leaving it open?

1. If [=Type=](|v|) is Number,
1. If |valtype| is [=i64=], throw a {{LinkError}} exception.
1. If [=Type=](|v|) is Number or BigInt,
1. If |valtype| is [=i64=] and [=Type=](|v|) is Number,
Copy link

Choose a reason for hiding this comment

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

This seem unnecessarily restrictive. Why not allow a Number to be passed here? The global gets allocated const regardless of the import's declared mutability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This has been discussed in WebAssembly/JS-BigInt-integration#12; allowing Numbers to be passed is likely to lead to rounding errors. We can revisit if it turns out there is an actual problem in practice.

1. If |w| is of the form [=𝖿𝟨𝟦.𝖼𝗈𝗇𝗌𝗍=] |f64|, return [=the Number value=] for |f64|.
1. If |w| is of the form [=𝗂𝟨𝟦.𝖼𝗈𝗇𝗌𝗍=] |i64|,
1. Let |v| be [=signed_64=](|i64|).
1. Return a [=BigInt=] representing the mathematical value |v|.
Copy link

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, but it should probably be "... of |v|". The link is https://tc39.es/ecma262/#mathematical-value, if you want to linkify it.

@Ms2ger Ms2ger requested a review from binji June 9, 2020 14:25
Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

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

The proposal is officially phase 5 now, so the PR can be merged: see https://lists.w3.org/Archives/Public/public-webassembly/2020Jun/0000.html

@littledan littledan merged commit 07a5590 into WebAssembly:master Jun 9, 2020
@Ms2ger Ms2ger deleted the bigint branch June 10, 2020 07:51
@ppKrauss
Copy link

ppKrauss commented Nov 1, 2020

Hi, this new API will offer some way to obtain the amount of bits of a BigInt?

Perhaps something as integerLogarithmBase2(BigInt)? (not a string conversion to count digits of the bits-string!).

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.

8 participants