-
Notifications
You must be signed in to change notification settings - Fork 480
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
BigInt arithmetic type errors #1282
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-addition-operator-plus-runtime-semantics-evaluation | ||
description: BigInt addition type errors | ||
info: > | ||
The Addition Operator ( + ) | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lprim be ? ToPrimitive(lval). | ||
6. Let rprim be ? ToPrimitive(rval). | ||
... | ||
8. Let lnum be ? ToNumeric(lprim). | ||
9. Let rnum be ? ToNumeric(rprim). | ||
10. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntAdditionOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 + value2); | ||
assert.throws(error, () => value2 + value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-binary-bitwise-operators-runtime-semantics-evaluation | ||
description: BigInt bitwise and type errors | ||
info: > | ||
Binary Bitwise Operators | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 & value2); | ||
assert.throws(error, () => value2 & value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-binary-bitwise-operators-runtime-semantics-evaluation | ||
description: BigInt bitwise or type errors | ||
info: > | ||
Binary Bitwise Operators | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 | value2); | ||
assert.throws(error, () => value2 | value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-binary-bitwise-operators-runtime-semantics-evaluation | ||
description: BigInt bitwise xor type errors | ||
info: > | ||
Binary Bitwise Operators | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 ^ value2); | ||
assert.throws(error, () => value2 ^ value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-multiplicative-operators-runtime-semantics-evaluation | ||
description: BigInt division type errors | ||
info: > | ||
Multiplicative Operators | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(1n, function (value2) { | ||
assert.throws(error, () => value1 / value2); | ||
assert.throws(error, () => value2 / value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-exp-operator-runtime-semantics-evaluation | ||
description: BigInt exponentiation type errors | ||
info: > | ||
Exponentiation Operator | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let base be ? ToNumeric(leftValue). | ||
6. Let exponent be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(1n, function (value2) { | ||
assert.throws(error, () => value1 ** value2); | ||
assert.throws(error, () => value2 ** value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-left-shift-operator-runtime-semantics-evaluation | ||
description: BigInt left shift type errors | ||
info: > | ||
The Left Shift Operator ( << ) | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 << value2); | ||
assert.throws(error, () => value2 << value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-multiplicative-operators-runtime-semantics-evaluation | ||
description: BigInt remainder type errors | ||
info: > | ||
Multiplicative Operators | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(1n, function (value2) { | ||
assert.throws(error, () => value1 % value2); | ||
assert.throws(error, () => value2 % value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-multiplicative-operators-runtime-semantics-evaluation | ||
description: BigInt multiplication type errors | ||
info: > | ||
Multiplicative Operators | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 * value2); | ||
assert.throws(error, () => value2 * value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-signed-right-shift-operator-runtime-semantics-evaluation | ||
description: BigInt right shift type errors | ||
info: > | ||
The Signed Right Shift Operator ( >> ) | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 >> value2); | ||
assert.throws(error, () => value2 >> value1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-subtraction-operator-minus-runtime-semantics-evaluation | ||
description: BigInt subtraction type errors | ||
info: > | ||
The Subtraction Operator ( - ) | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(lval). | ||
6. Let rnum be ? ToNumeric(rval). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 - value2); | ||
assert.throws(error, () => value2 - value1); | ||
}); | ||
}); |
27 changes: 27 additions & 0 deletions
27
test/language/expressions/unsigned-right-shift/bigint-err.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright (C) 2017 Igalia, S.L. All rights reserved. | ||
// This code is governed by the BSD license found in the LICENSE file. | ||
|
||
/*--- | ||
esid: sec-unsigned-right-shift-operator-runtime-semantics-evaluation | ||
description: BigInt unsigned right shift type errors | ||
info: > | ||
The Unsigned Right Shift Operator ( >>> ) | ||
|
||
Runtime Semantics: Evaluation | ||
|
||
... | ||
5. Let lnum be ? ToNumeric(leftValue). | ||
6. Let rnum be ? ToNumeric(rightValue). | ||
7. If Type(lnum) does not equal Type(rnum), throw a TypeError | ||
exception. | ||
... | ||
includes: [typeCoercion.js] | ||
features: [BigInt, Symbol, Symbol.toPrimitive, arrow-function] | ||
---*/ | ||
|
||
testNotCoercibleToBigIntOperand(function (error, value1) { | ||
testCoercibleToBigIntOperand(0n, function (value2) { | ||
assert.throws(error, () => value1 >>> value2); | ||
assert.throws(error, () => value2 >>> value1); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have a sample of what calls we should expect here in this current case? It's being hard for me to traverse this code through all the abstraction.
a list just saying the expected values of value1 and value2 would be really helpful to tell if I am not wrong traversing this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I ran these locally, I encountered a confusing failure:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition in this patch seems pretty analogous to lots of other things in typeCoercion.js, like testNotCoercibleToBigInt. Are those too confusing too? The nested calls are sort of like nested for loops, just done as calls so that we're sure to have the assert in this file.
I'm not sure how you ran the test locally, but when I ran this locally with V8, I saw it succeed. I'd guess that such an error could come from an implementation which doesn't do ToPrimitive properly, e.g., throwing on + between a BigInt and some kinds of objects that should be convertable to BigInt, rather than doing the conversion. An issue is that an unadorned MyError is thrown from a few different paths, so it's hard to figure out which one is failing.
Coercion is especially tricky with BigInt arithmetic operators, especially addition, so I'm glad this test gets at different cases in a comprehensive way. However, I'm not sure if we get much abstraction value by factoring out testNotCoercibleToBigIntAdditionOperand ; I imagine that's only useful in this particular test.
Another strategy, if the abstraction is too confusing, would be to generate many tests. I believe @thejoshwolfe has done this for certain other tests. I don't have a strong opinion one way or the other on which is better. I think if we allow some things in typeCoercion.js, we should allow nested calls like this for things which take multiple arguments; otherwise, where is the line exactly? Maybe there are disadvantages to typeCoercion.js overall in terms of debugging, though. I'm not really sure, since I haven't had the experience of working through a failure yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My d8 is built from 2d60b72890
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I'm just one patch ahead of you and it seems like that exact bug was just fixed: https://chromium-review.googlesource.com/c/v8/v8/+/716863 , where in particular objects which had to get coerced to BigInts weren't being coerced properly for certain operators, and a valueOf method that returned a BigInt would lead to a TypeError. Good thing this test exists!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did a manual check for the assertions, registering each assertion call. The list is still incomplete, but enough to observe what is being tested.
here it goes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, reading from this list, I have to ask what is the purpose of having too many assertions. The target being tested are just the same. The cases where you coerce a valid operand should be fine if everything else you're testing is just checking for the error caused by the other operand.
This is just like:
and then you expand every possibility of valid coercions for the operand that is not poisoned. They are redundant and unhelpful if any assertion fails. Stressing the poisoned value should be the goal here, right?
There are even cases where both operands are poisoned and you can't even observe what part is failing:
Leads you to assert
poisoned1 + poisoned2
will cause a TypeError, from one operand or another. So the assertion might even produce a false positive if only one of them coerces to an invalid value type.Being able to read and understand just the tests for this single file to finally write this here took me a very long time. I wonder if it's really worth making readability obscure using a weak abstraction. If this test ever needs maintenance, it will be really hard to change anything, specially not being able to tell if the abstractions won't affect anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for clarification, I've been working on this for a long time and after some editions I removed something relevant to explain my output blob:
the simple
{}
is a short for the valid object being coerced. so when you read:{@@toPrimitive: 1} + {}
it's a short and vague notation for:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to statically generate the code for these tests, to make it clearer what assertions are actually being made?
I don't really understand why the "poisoned+non_poisoned" tests are problematic (since they're not incorrect or completely equivalent), but it's easy enough to simplify them. I'll remove the "poisoned+poisoned" assertions; there are already better tests for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets my +1