Add proposal-promise-any #6641

Kingwl · 2021-03-16T08:07:08Z

Closes #6301
FIxes #6299

lib/Runtime/Library/JavascriptError.cpp

rhuanjl · 2021-03-16T21:25:47Z

Thanks for carrying on with this.

FYI on test fails:

2 of them are for the bytecode regen, did you run tools/regenByteCode.py with flags? For a regen before checking in the code it needs to be run without flags.
The windows/MSVC ones are to do with tests in tests/DebuggerCommon that check what properties various objects have - the baseline files need AggregateError adding in a stack of places - slightly tedious, can generate the files by running each failing test with ch or can manually update them.

Kingwl · 2021-03-17T10:42:53Z

did you run tools/regenByteCode.py with flags

Yep. It will fail without flags. And I haven't find out why.

can generate the files by running each failing test with ch or can manually update them.

Okay, Thanks.

There's already existed test262. Should we add more test about that?

rhuanjl · 2021-03-17T11:24:43Z

did you run tools/regenByteCode.py with flags

Yep. It will fail without flags. And I haven't find out why.

If you're on windows it needs to be run in a developer command prompt as it calls MsBuild; on macOS/Linux I can't think of it reason for it to fail.

When run fully it generates 4 files - covering each combination of: x64 or x86 with Jit OR no Jit. Your PR currently has updates for x64 Jit and x86 jit but doesn't have the versions for noJit.

can generate the files by running each failing test with ch or can manually update them.

Okay, Thanks.

There's already existed test262. Should we add more test about that?

I was commenting on the existing test cases that are failing (because they're checking properties of the GlobalObject and not expecting to find AggregateError) we'll need new test cases as well BUT I was wrong - much easier fix don't need to update all the tests - just add AggregateError to this list to take it out of these tests:

ChakraCore/bin/ch/DbgController.js

Lines 63 to 128 in 11d9375

    
           [ 
        
               "#__proto__", 
        
               "globalThis", 
        
               "Array", 
        
               "ArrayBuffer", 
        
               "Atomics", 
        
               "Boolean", 
        
               "chWriteTraceEvent", 
        
               "CollectGarbage", 
        
               "console", 
        
               "DataView", 
        
               "Date", 
        
               "decodeURI", 
        
               "decodeURIComponent", 
        
               "encodeURI", 
        
               "encodeURIComponent", 
        
               "Error", 
        
               "escape", 
        
               "eval", 
        
               "EvalError", 
        
               "Float32Array", 
        
               "Float64Array", 
        
               "Function", 
        
               "Infinity", 
        
               "Int16Array", 
        
               "Int32Array", 
        
               "Int8Array", 
        
               "Intl", 
        
               "isFinite", 
        
               "isNaN", 
        
               "JSON", 
        
               "Map", 
        
               "Math", 
        
               "NaN", 
        
               "Number", 
        
               "Object", 
        
               "parseFloat", 
        
               "parseInt", 
        
               "print", 
        
               "Promise", 
        
               "Proxy", 
        
               "RangeError", 
        
               "read", 
        
               "readbuffer", 
        
               "readline", 
        
               "ReferenceError", 
        
               "Reflect", 
        
               "RegExp", 
        
               "Set", 
        
               "SharedArrayBuffer", 
        
               "String", 
        
               "Symbol", 
        
               "SyntaxError", 
        
               "TypeError", 
        
               "Uint16Array", 
        
               "Uint32Array", 
        
               "Uint8Array", 
        
               "Uint8ClampedArray", 
        
               "undefined", 
        
               "unescape", 
        
               "URIError", 
        
               "WeakMap", 
        
               "WeakSet", 
        
               "WebAssembly", 
        
               "WScript", 
        
           ].forEach(function (name) {

Kingwl · 2021-03-17T12:41:00Z

Yeah. I know about that. I mean: I saw we have 262test. And should I add some case even they has overlapping with test262.
Thanks!

rhuanjl · 2021-03-17T13:19:18Z

Yeah. I know about that. I mean: I saw we have 262test. And should I add some case even they has overlapping with test262.
Thanks!

We do need specific tests; test262 is not run on check in - the test262 folder just tests part of the harness.

Kingwl · 2021-03-17T14:21:15Z

Okay. I think I'd better copy some tests from test262.

Kingwl · 2021-03-23T08:26:54Z

Sorry I'm not sure what TTD is. And what should I do for TTD. Would you mind give some suggestion? thanks!

rhuanjl · 2021-03-23T08:32:39Z

TTD = "Time Travel Debug" it is support for running javascript backwards for debugging purposes - it's a unique feature of Chakracore - sadly I'm not sure if anyone uses it at the moment BUT I'd like to keep it working if we can.

TTD Snap methods save the current JS state out to a file, Inflate methods take information from the file to recreate a specific JS state. So you'll probably need Snap/Inflate support for AggregateError. Perhaps look at how Snap/Inflate work for something containing a list of Promises.

Kingwl · 2021-03-23T08:38:10Z

Seems the spec has some changes between this and the previous PR.

According the latest spec. We needn't extends JavascriptError anymore. I'll update these soon.

rhuanjl · 2021-03-23T08:47:52Z

Seems the spec has some changes between this and the previous PR.

According the latest spec. We needn't extends JavascriptError anymore. I'll update these soon.

That sounds strange to me, I thought one of the key purposes of AggregateError (instead of an Array of Errors) was so it would test true as instanceOf Error is that no longer the case?

Kingwl · 2021-03-23T08:51:00Z

Nope. That's not related about runtime behavior. It's talk about the implementations in this PR -> class JavascriptAggregateError : public JavascriptError

rhuanjl · 2021-03-23T08:54:20Z

Nope. That's not related about runtime behavior. It's talk about the implementation of this PR -> class JavascriptAggregateError : public JavascriptError

If there's no detectable runtime difference the implementation doesn't matter - spec details about implementation are only authoritative if runtime can detect them.

That said - I'd need to check the details but I imagine having it internally extend JavascriptError makes sense for a lot of reasons - is there a disadvantage you're aware of? Or an observable instance of incorrect runtime behaviour?

Kingwl · 2021-03-23T08:57:39Z

One reason is that all the previous Errors directly reference to JavascriptError. And the property errors is just a property (In my memory, there's a internal slots and a getter { to create a array from the internal slots } before).
As the result. We just use the JavascriptError and set the property.

Kingwl · 2021-03-23T09:26:04Z

BTW: Should this behind a flag?

MadProbe · 2021-03-23T10:43:35Z

This proposal is stage 4, no need in a flag.

rhuanjl · 2021-03-23T17:28:03Z

This proposal is stage 4, no need in a flag.

Sometimes it's good to have a flag even for a stage 4 proposal/feature - if it introduces a bug or a strange issue can be helpful for debugging to turn it off OR if the bug is too serious to be able to disable the feature and a ship a version without it - hopefully unnecessary but useful to have there with any particularly invasive features.

pleath · 2021-03-23T17:42:45Z

+1. I'd suggest keeping any non-trivial feature behind a flag, at least until it's been successfully released. Even then, there can be surprising compat impact out in the world, and it's nice to be able to flip the switch to off while you investigate.

Kingwl · 2021-03-24T07:08:48Z

I think this one is basically done. But there's something I'm not sure:

In ES^PromiseAsync.js. Seems the test done before everything tests finished.
I have try to copy all tests of promise.any from test262. but there's tooooo many cases...

rhuanjl

I've not finished - but I've raised several points that need addressing above - found some spec deviations and a few segfaults/hard crashes as well as some less important style points.

lib/Runtime/Library/JavascriptPromise.cpp

lib/Runtime/Library/JavascriptError.cpp

lib/Runtime/Library/JavascriptLibrary.cpp

lib/Runtime/Library/JavascriptError.cpp

test/Error/error_cause_with_aggregate_error.js

test/es6/ES6PromiseAsync.baseline

test/Error/aggregate_error.js

Kingwl · 2021-04-01T10:10:15Z

PTAL. Major issues has been resolved (I think).

lib/Runtime/Library/JavascriptPromise.cpp

rhuanjl · 2021-04-02T17:07:29Z

I'm sorry I merged another PR with a bytecode regen - you will need to fix the merge conflict and then re-run the bytecode regen script.

Kingwl · 2021-04-14T09:41:36Z

PTAL.

rhuanjl · 2021-04-15T08:09:51Z

Thanks for the further updates, I'll aim to pull this down and re-review tomorrow, sorry for the delay - not a small diff.

rhuanjl

Mostly looking good now - one serious bug which needs fixing + a test adding.

And it would be good if the AggregateError tests could be a bit nicer (similar format and clarity etc. to the other test files)

lib/Runtime/Library/JavascriptPromise.cpp

test/Error/aggregate_error.js

rhuanjl · 2021-04-16T21:32:42Z

test/Error/aggregate_error.js

+            var case9 = {
+                [Symbol.iterator]() {
+                    return {
+                        next() {
+                            return undefined;
+                        }
+                    }
+                }
+            };
+            assert.throws(() => {
+                var obj = new AggregateError(case9);
+            }, TypeError);


The next few tests appear to be about iterators that don't provide objects -would be good to specify that.

Sorry I'm not sure what‘s the point.

Basically that I'd like it to be clearer when re-reading the test file what it's doing/what these cases are for.

It appears from reading them that they're testing behaviour when an iterator doesn't provide an object - but there's no comment description saying that that is what they're doing/why they're here.

Thanks for the response. I have tried to add some comments above the case. Does they seems enough?

test/Error/aggregate_error.js

test/es6/ES6Promise.js

test/Error/error_cause_with_aggregate_error.js

Kingwl · 2021-04-20T09:50:17Z

Thanks for the review. I would like to take a look in few days.

…nto proposal-promise-any

rhuanjl · 2021-04-21T14:00:58Z

Thanks for all your work on this @Kingwl

Kingwl · 2021-04-21T15:06:40Z

Thanks for your review!

Add AggregateError Ctor and prototype

acfa7d3

Kingwl changed the title ~~Add AggregateError Ctor and prototype~~ Add proposal-promise-any Mar 16, 2021

Add throwAggregateErrror function

8e1e8aa

rhuanjl reviewed Mar 16, 2021

View reviewed changes

lib/Runtime/Library/JavascriptError.cpp Show resolved Hide resolved

Kingwl added 5 commits March 23, 2021 13:31

Add promise.any

fa2e93f

Merge branch 'master' into proposal-promise-any

1b2010f

Add aggregate into ignore

6887c8b

Fix bytecode

0a7fa0c

fix copyright

c6129ad

Kingwl added 3 commits March 23, 2021 17:12

Simplify code

2a6fedc

Fix unicode characters

da57f7d

Fix unicode characters

87d36ee

Add tests for aggregate error

233093c

Kingwl marked this pull request as ready for review March 24, 2021 07:06

rhuanjl self-assigned this Mar 24, 2021

rhuanjl reviewed Mar 25, 2021

View reviewed changes

Fix some CR issues.

a5058d7

Kingwl commented Mar 25, 2021

View reviewed changes

test/es6/ES6PromiseAsync.baseline Outdated Show resolved Hide resolved

rhuanjl reviewed Mar 25, 2021

View reviewed changes

Kingwl added 5 commits April 1, 2021 17:27

Fix some CR issues

0d097a7

Add some case

dfeb078

Fix cr issues

f3aee6a

Update copyright

2225ff9

Merge branch 'master' into proposal-promise-any

3b5d57a

rhuanjl reviewed Apr 2, 2021

View reviewed changes

lib/Runtime/Library/JavascriptPromise.cpp Show resolved Hide resolved

Kingwl added 3 commits April 7, 2021 16:25

Merge branch 'master' into proposal-promise-any

6902d29

Fix incorrect call arguments

caddaf3

Merge branch 'master' into proposal-promise-any

a8528b7

Merge branch 'master' into proposal-promise-any

c9825c6

rhuanjl reviewed Apr 16, 2021

View reviewed changes

Kingwl added 3 commits April 21, 2021 18:06

Add more tests and fix

8d5e6bd

Merge branch 'proposal-promise-any' of github.com:Kingwl/ChakraCore i…

7fa26cd

…nto proposal-promise-any

Merge branch 'master' into proposal-promise-any

9fcf2c0

rhuanjl merged commit 8220c85 into chakra-core:master Apr 21, 2021

Kingwl deleted the proposal-promise-any branch April 22, 2021 02:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proposal-promise-any #6641

Add proposal-promise-any #6641

Kingwl commented Mar 16, 2021

rhuanjl commented Mar 16, 2021

Kingwl commented Mar 17, 2021

rhuanjl commented Mar 17, 2021

Kingwl commented Mar 17, 2021

rhuanjl commented Mar 17, 2021

Kingwl commented Mar 17, 2021

Kingwl commented Mar 23, 2021 •

edited

Loading

rhuanjl commented Mar 23, 2021

Kingwl commented Mar 23, 2021 •

edited

Loading

rhuanjl commented Mar 23, 2021 •

edited

Loading

Kingwl commented Mar 23, 2021 •

edited

Loading

rhuanjl commented Mar 23, 2021

Kingwl commented Mar 23, 2021 •

edited

Loading

Kingwl commented Mar 23, 2021

MadProbe commented Mar 23, 2021

rhuanjl commented Mar 23, 2021

pleath commented Mar 23, 2021

Kingwl commented Mar 24, 2021

rhuanjl left a comment

Kingwl commented Apr 1, 2021 •

edited

Loading

rhuanjl commented Apr 2, 2021

Kingwl commented Apr 14, 2021

rhuanjl commented Apr 15, 2021

rhuanjl left a comment

rhuanjl Apr 16, 2021

Kingwl Apr 21, 2021

rhuanjl Apr 21, 2021

Kingwl Apr 21, 2021

Kingwl commented Apr 20, 2021

rhuanjl commented Apr 21, 2021

Kingwl commented Apr 21, 2021

Add proposal-promise-any #6641

Add proposal-promise-any #6641

Conversation

Kingwl commented Mar 16, 2021

rhuanjl commented Mar 16, 2021

Kingwl commented Mar 17, 2021

rhuanjl commented Mar 17, 2021

Kingwl commented Mar 17, 2021

rhuanjl commented Mar 17, 2021

Kingwl commented Mar 17, 2021

Kingwl commented Mar 23, 2021 • edited Loading

rhuanjl commented Mar 23, 2021

Kingwl commented Mar 23, 2021 • edited Loading

rhuanjl commented Mar 23, 2021 • edited Loading

Kingwl commented Mar 23, 2021 • edited Loading

rhuanjl commented Mar 23, 2021

Kingwl commented Mar 23, 2021 • edited Loading

Kingwl commented Mar 23, 2021

MadProbe commented Mar 23, 2021

rhuanjl commented Mar 23, 2021

pleath commented Mar 23, 2021

Kingwl commented Mar 24, 2021

rhuanjl left a comment

Choose a reason for hiding this comment

Kingwl commented Apr 1, 2021 • edited Loading

rhuanjl commented Apr 2, 2021

Kingwl commented Apr 14, 2021

rhuanjl commented Apr 15, 2021

rhuanjl left a comment

Choose a reason for hiding this comment

rhuanjl Apr 16, 2021

Choose a reason for hiding this comment

Kingwl Apr 21, 2021

Choose a reason for hiding this comment

rhuanjl Apr 21, 2021

Choose a reason for hiding this comment

Kingwl Apr 21, 2021

Choose a reason for hiding this comment

Kingwl commented Apr 20, 2021

rhuanjl commented Apr 21, 2021

Kingwl commented Apr 21, 2021

Kingwl commented Mar 23, 2021 •

edited

Loading

Kingwl commented Mar 23, 2021 •

edited

Loading

rhuanjl commented Mar 23, 2021 •

edited

Loading

Kingwl commented Mar 23, 2021 •

edited

Loading

Kingwl commented Mar 23, 2021 •

edited

Loading

Kingwl commented Apr 1, 2021 •

edited

Loading