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

Normative: only coerce once in BigInt constructor #2812

Merged
merged 1 commit into from
Aug 4, 2022
Merged

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jul 6, 2022

(This was raised on the discourse.)

Consider

let first = true;
let v = {
  [Symbol.toPrimitive]() {
    console.log('called');
    if (first) {
      first = false;
      return 'string';
    } else {
      return 0n;
    }
  }
};
BigInt(v);

Per spec this should print called twice and complete normally - step 3 of the BigInt constructor will observe that the first call to ToPrimitive returns a value which is not a number and then step 4 will invoke ToBigInt on the original, pre-coercion value, which will invoke Symbol.toPrimitive again.

In V8, SpiderMonkey, JavaScriptCore, and XS, this code will in fact only print called once and then throw a TypeError because the initial 'string' can't be coerced to a BigInt - i.e., in step 4 they use the post-coercion value, not the pre-coercion value. That's the behavior proposed in this PR.

GraalJS and Engine262 implement the spec faithfully.

@bakkot bakkot added normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality labels Jul 6, 2022
@syg
Copy link
Contributor

syg commented Jul 6, 2022

haha oh no

@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Jul 6, 2022
@apaprocki
Copy link
Contributor

@bakkot So Date constructor 4c is basically the same issue?

let first = true;
let v = {
  [Symbol.toPrimitive]() {
    console.log('called');
    if (first) {
      first = false;
      return 0;
    } else {
      return 'string';
    }
  }
};
new Date(v);
[Log] called
< Wed Dec 31 1969 19:00:00 GMT-0500 (EST)

@bakkot
Copy link
Contributor Author

bakkot commented Jul 6, 2022

@apaprocki The Date constructor has a similar pattern but is currently specified correctly: it does Let v be ? ToPrimitive(value). [...] Let tv be ? ToNumber(v)., i.e., the second line uses the post-coercion value instead of the pre-coercion value.

(That second line does want a suppress-effects="user-code", though.)

@apaprocki
Copy link
Contributor

Ah, my brain read what you wrote (without seeing the 1-line diff) and thought this was to fix behavior to match the spec as opposed to the other way around. Thanks!

Copy link
Member

@littledan littledan left a comment

Choose a reason for hiding this comment

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

Wow thanks for the fix 😳

@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jul 19, 2022
@bakkot
Copy link
Contributor Author

bakkot commented Aug 2, 2022

Tests: tc39/test262#3631

@bakkot bakkot added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Aug 2, 2022
linusg pushed a commit to SerenityOS/serenity that referenced this pull request Aug 3, 2022
@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Aug 3, 2022
@ljharb ljharb merged commit f512200 into main Aug 4, 2022
@ljharb ljharb deleted the bigint-conversion branch August 4, 2022 18:52
DerekNonGeneric pushed a commit to DerekNonGeneric/tc39-ecma262 that referenced this pull request Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land. web reality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants