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.

Sign up for GitHub

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

Jump to bottom

BigInt arithmetic type errors #1282

Closed
wants to merge 2 commits into from
Closed

BigInt arithmetic type errors #1282

wants to merge 2 commits into from

Conversation

cxielarko
Copy link
Contributor

Arithmetic expressions with one BigInt operand and one non-BigInt operand should always throw an exception. (The new typeCoercion.js functions are for testing the ToNumeric operation.)

leobalter
leobalter reviewed Oct 16, 2017
View reviewed changes
test/language/expressions/addition/bigint-err.js

testNotCoercibleToBigIntAdditionOperand(function (error, value1) {
testCoercibleToBigIntOperand(0n, function (value2) {
assert.throws(error, () => value1 + value2);
Copy link
Member

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.

Copy link
Contributor

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:

FAIL test/language/expressions/addition/bigint-err.js
  Expected a MyError but got a TypeError

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

rwaldron in ~/clonez/test262 on cxielarko-bigint-expr-err*
$ test262-harness --hostType d8 --hostPath `which d8` --hostArgs="--harmony_bigint" test/language/expressions/addition/bigint-err.js
FAIL test/language/expressions/addition/bigint-err.js
  Expected a MyError but got a TypeError

FAIL test/language/expressions/addition/bigint-err.js
  Expected a MyError but got a TypeError

Ran 2 tests
0 passed
2 failed

My d8 is built from 2d60b72890

Copy link
Member

@littledan littledan Oct 16, 2017 •
edited
Loading

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!

Copy link
Member

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:


testNotCoercibleToBigIntAdditionOperand(test)
  testNotCoercibleToPrimitive(hint: "number", test)
    test(error: TypeError, value1: {[@@toPrimitive]: 1});
      testCoercibleToBigIntOperand(value: 0n, test)
        test(value2: 0n);
          assert.throws(error: TypeError, ()  => {[@@toPrimitive]: 1} + 0n) ************ assertion **********
          assert.throws(error: TypeError, ()  => 0n + {[@@toPrimitive]: 1}) ************ assertion **********
        testPrimitiveWrappers(primitiveValue: 0n , hint: "number", test)
          if (primitiveValue != null) {
            test(value2: Object(primitiveValue: 0n))
              assert.throws(error: TypeError, ()  => {[@@toPrimitive]: 1} + Object(0n)) ************ assertion **********
              assert.throws(error: TypeError, ()  => Object(0n) + {[@@toPrimitive]: 1}) ************ assertion **********
            testCoercibleToPrimitiveWithMethod(hint: "number", method: () => primitiveValue: 0n, test: test);
              if (hint === "number") {
                methodNames = ["valueOf", "toString"];
              }
              
              test(value2: {
                [Symbol.toPrimitive]: (method: () => primitiveValue: 0n),
                [methodNames[0]]: function() { throw new Test262Error(); },
                [methodNames[1]]: function() { throw new Test262Error(); },
              });
                assert.throws(TypeError, ()  => {@@toPrimitive: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {@@toPrimitive: 1}) ************ assertion **********
              test(value2: {
                [methodNames[0]]: method,
                [methodNames[1]]: function() { throw new Test262Error(); },
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {} ) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********
              
            
              if (hint === "number") {
                test({
                  [methodNames[1]]: method,
                });
                  assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {} ) ************ assertion **********
                  assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********
              }
              
              test({
                [Symbol.toPrimitive]: undefined,
                [methodNames[0]]: method,
                [methodNames[1]]: method,
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********
              test({
                [Symbol.toPrimitive]: null,
                [methodNames[0]]: method,
                [methodNames[1]]: method,
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********

              test({
                [methodNames[0]]: null,
                [methodNames[1]]: method,
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********
              
              test({
                [methodNames[0]]: 1,
                [methodNames[1]]: method,
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********
              test({
                [methodNames[0]]: {},
                [methodNames[1]]: method,
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********

              test({
                [methodNames[0]]: function() { return {}; },
                [methodNames[1]]: method,
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********
              test({
                [methodNames[0]]: function() { return Object(1); },
                [methodNames[1]]: method,
              });
                assert.throws(TypeError, ()  => {[@@toPrimitive]: 1} + {}) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {[@@toPrimitive]: 1} ) ************ assertion **********
  
    test(error: TypeError, value1: {[@@toPrimitive]: {}});
      ...
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + 0n) ************ assertion **********
      assert.throws(TypeError, ()  => 0n + {@@toPrimitive: {}}) ************ assertion **********
      
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + Object(0n)) ************ assertion **********
      assert.throws(TypeError, ()  => Object(0n) + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: {}}) ************ assertion **********
      

    test(TypeError, {[Symbol.toPrimitive]: function() { return Object(1); }});
      ...
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + 0n) ************ assertion **********
      assert.throws(TypeError, ()  => 0n + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + Object(0n)) ************ assertion **********
      assert.throws(TypeError, ()  => Object(0n) + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => Object(1)} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => Object(1)}) ************ assertion **********
      

    test(TypeError, {[Symbol.toPrimitive]: function() { return {}; }});
      ...
      assert.throws(TypeError, () => {@@toPrimitive: () => {}} + 0n) ************ assertion **********
      assert.throws(TypeError, ()  => 0n + {@@toPrimitive: () => {}}) ************ assertion **********
      
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + Object(0n)) ************ assertion **********
      assert.throws(TypeError, ()  => Object(0n) + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      assert.throws(TypeError, ()  => {@@toPrimitive: () => {}} + {}) ************ assertion **********
      assert.throws(TypeError, ()  => {} + {@@toPrimitive: () => {}}) ************ assertion **********
      
    test(MyError, {[Symbol.toPrimitive]: function() { throw new MyError(); }});
      ...
      obj = {@@toPrimitive: () => { throw new MyError() }}
      assert.throws(MyError, () => obj + 0n) ************ assertion **********
      assert.throws(MyError, () => 0n + obj) ************ assertion **********
      assert.throws(MyError, () => obj + Object(0n)) ************ assertion **********
      assert.throws(MyError, () => Object(0n) + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
      assert.throws(MyError, ()  => obj + {}) ************ assertion **********
      assert.throws(MyError, ()  => {} + obj) ************ assertion **********
     
    testCoercibleToPrimitiveWithMethod
      ...
      ???
    
    testUnsuitableMethod
      test(TypeError, {valueOf:null, toString:null});
    testUnsuitableMethod
      test(TypeError, {valueOf:1, toString:1});
    testUnsuitableMethod
      test(TypeError, {valueOf:{}, toString:{}});
    testUnsuitableMethod
      test(TypeError, {valueOf:function() { return Object(1); }, toString:function() { return Object(1); }});
    testUnsuitableMethod
      test(TypeError, {valueOf:function() { return {}; }, toString:function() { return {}; }});


   ....

Copy link
Member

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:

assert.throws(Error, function() { poisoned + okcoercion });
assert.throws(Error, function() { okcoercion + poisoned });

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:

testUnsuitableMethod
      test(TypeError, {valueOf:1, toString:1});

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.

Copy link
Member

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:

              if (hint === "number") {
                methodNames = ["valueOf", "toString"];
              }
              
              test(value2: {
                [Symbol.toPrimitive]: (method: () => primitiveValue: 0n),
                [methodNames[0]]: function() { throw new Test262Error(); },
                [methodNames[1]]: function() { throw new Test262Error(); },
              });
                assert.throws(TypeError, ()  => {@@toPrimitive: 1} + {}`) ************ assertion **********
                assert.throws(TypeError, ()  => {} + {@@toPrimitive: 1}) ************ assertion **********

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:

{@@toPrimitive: 1} + {@@toPrimitive() { return primitiveValue }, valueOf: function() { throw new Test262Error(); }, function() { throw new Test262Error(); }}

Copy link
Contributor Author

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.

Copy link
Contributor

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?

This gets my +1

@thejoshwolfe
Copy link
Contributor

Would it be too much trouble to also throw in operators: &, ^, |, <<, >>, >>>? Looks like whatever solution ends up working for -, *, /, %, ** will work for all these operators, because the spec semantics are identical before calling the actual T::method(lnum, rnum).

Specifically, these operators all have this spec text:

  5. Let lnum be ? ToNumeric(lValue).
  6. Let rnum be ? ToNumeric(rValue).
  7. If Type(lnum) does not equal Type(rnum), throw a TypeError exception.

(The extra TypeError for >>> is past this part of the spec.)

(The semantics for + have special cases for String values, so it doesn't fit quite as cleanly in with the rest of the operators.)

@thejoshwolfe thejoshwolfe mentioned this pull request Oct 20, 2017
Open
Robin Templeton added 2 commits October 24, 2017 11:48
[BigInt] arithmetic type errors
b087c0a
PR updates
1e35c65 
 * fix features typo
 * remove poisoned-value testing
 * add tests for additional operators
@cxielarko
Copy link
Contributor Author

I updated the tests to exclude "poisoned" values, and also added tests for bitwise operations and shifts.

@littledan
Copy link
Member

I believe @thejoshwolfe is working on a version of these tests which is based on test generation. So it should be OK wait to land this until we can look at those tests instead.

@leobalter
Copy link
Member

Some of these should be covered by the tests for type coercion tests (found on the git log) and we are planning to avoid this current strategy from this PR. I'm happy to support working on the current strategy we already have in the project.

@leobalter leobalter closed this Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwaldron rwaldron rwaldron left review comments

@littledan littledan littledan left review comments

@leobalter leobalter leobalter left review comments

Assignees

@littledan littledan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cxielarko @thejoshwolfe @littledan @leobalter @rwaldron