[WIP] Add promise.any #6301

Kingwl · 2019-10-08T10:46:51Z

WIP, not finally patch

AggregateError
Promise.any
tests

rhuanjl · 2019-10-08T16:17:20Z

One note to help you - the test failures are because the self-hosted Javascript for JsBuiltins and INTL needs to be regenerated (a side effect of having added the ENTRY for any to JnDirectFields.h).

To fix this you'll need to run the script called RegenAllByteCode.cmd it will create new bytecode headers you'll need to commit.

Kingwl · 2019-10-08T16:33:14Z

@rhuanjl Thanks, I realized that.

rhuanjl · 2019-10-08T17:25:25Z

The script should have given you 8 files - it appears you only checked in 4 of them.

Kingwl · 2019-10-13T16:46:28Z

Hi, I think i need some help...

I have several questions:

What is the 'internal slot'? IMO, Could I think of it as member of some c++ object in runtime?
If 1 is current, Could I simply add a member into JavascriptError ? or extend that one?
What is The List type in ChakraCore, Could I think of it as JavascriptArray?

Thanks

rhuanjl · 2019-10-13T21:12:12Z

an internal slot from the JS spec = a property that can't be accessed from javascript (but can be accessed within runtime methods when needed) - normally done in CC by adding a property/member to the relevant C++ class
not sure what you're asking in 2 - depends what internal slot you need to add
If the spec asks for a list as an internal slot I'd reccomend using something based on ChakraCore's SList type - look at how JavascriptPromiseReactionList is done (search for that in JavascriptPromise.cpp and JavascriptPromise.h as a model to base off of)

Kingwl · 2019-10-14T03:03:45Z

@rhuanjl Thank you for your help!

zenparsing · 2019-10-14T03:40:47Z

@Kingwl In addition to what @rhuanjl says, I think it would be better if we could subclass JavascriptError instead of adding the "errors" list directly to JavascriptError. I'm not sure how difficult that will be, though, given that we aren't currently subclassing JavascriptError (and we are currently using a lot of macros for error types, which may or may not work for a subclass).

Also, thanks for working on this one!

Kingwl · 2019-10-14T05:19:33Z

@zenparsing Thanks for the response
BTW have we got some existed way to regen bytecode that not on windows?

rhuanjl · 2019-10-14T10:03:49Z

The scripts that regen bytecode only work on windows BUT you could fairly easily hack it to work on macOS or Linux for testing BUT not for submitting because you need to submit updated bytecode for x64 and x86 - generating the x86 bytecode requires an x86 build of CC and currently on macOS and Linux you can only do x64 builds.

Kingwl · 2019-10-27T12:22:09Z

It's too hard to (seems) make that work...
I'll do some refect and add more test case soon...

rhuanjl

Thank you for what you've done here, this is an amazing effort. I hope you'd like to continue to work on it? I'm sorry for the big delay from October - the future ownership of ChakraCore is transitioning, see #6384 for more details.

I've done an initial review - and left some comments, additionally:

some tests for AggregateError when used on its own are needed
I've not been through the time travel debug code yet - we'll also need TTD behaviour tests, though could potentially submit this with TTD as a pending/todo part.

rhuanjl · 2020-04-11T18:07:54Z

lib/Runtime/Language/JavascriptOperators.cpp

@@ -2139,6 +2139,19 @@ using namespace Js;
        return TRUE;
    }

+    JavascriptArray* JavascriptOperators::IterableToList(RecyclableObject* items, ScriptContext* scriptContext) {


As far as I can see this is only used once, could you just do the steps inline without making a new method for it?

rhuanjl · 2020-04-11T18:10:39Z

lib/Runtime/Library/JavascriptError.cpp

+        return pError;
+    }
+
+    void __declspec(noreturn) JavascriptAggregateError::ThrowAggregateError(ScriptContext* scriptContext, RecyclableObject* errors, int32 hCode, EXCEPINFO* pei)


Do we need all these variants for ThrowAggregateError? Where are they used?

rhuanjl · 2020-04-11T18:13:45Z

lib/Runtime/Library/JavascriptError.cpp

@@ -484,6 +582,12 @@ namespace Js
        pError->m_errorType = errorType;
    }

+    void JavascriptAggregateError::SetErrorsProperties(JavascriptAggregateError* pError, RecyclableObject* errors)


Could these steps be inlined when used rather than being a new method, considering how short it is?

rhuanjl · 2020-04-11T18:17:45Z

lib/Runtime/Library/JavascriptError.cpp

+        JavascriptLibrary* library = scriptContext->GetLibrary();
+        JavascriptAggregateError* thisError = VarTo<JavascriptAggregateError>(args[0]);
+
+        RecyclableObject* iterator = JavascriptOperators::GetIterator(thisError->GetErrors(), scriptContext);


Should do a nullptr check on the errors property - and return a 0 length array if errors is nullptr.

rhuanjl · 2020-04-11T18:19:25Z

lib/Runtime/Library/JavascriptError.cpp

+        Var newTarget = args.GetNewTarget();
+        RecyclableObject* errors = args.Info.Count > 1 ? JavascriptOperators::IterableToList(VarTo<RecyclableObject>(args[1]), scriptContext) : library->CreateArray(0);
+        Var message = args.Info.Count > 2 ? args[2] : library->GetUndefined();
+        pError->SetErrors(errors);


Should make the errors a nullptr if length would be 0 and skip allocating the array - though will need to add a nullptr check when using it (though that should be added for safety anyway).

lib/Runtime/Library/JavascriptLibrary.cpp

rhuanjl · 2020-04-11T18:45:06Z

lib/Runtime/Library/JavascriptPromise.cpp

+
+        AUTO_TAG_NATIVE_LIBRARY_ENTRY(function, callInfo, _u("Promise.any"));
+
+        // 1. Let C be the this value.


Should either run the Spec comments all the way through or emit them entirely. I favour having them all the way through if you can.

rhuanjl · 2020-04-11T18:47:42Z

lib/Runtime/Library/JavascriptPromise.cpp

+        // 3. Let promiseCapability be NewPromiseCapability(C).
+        JavascriptPromiseCapability* promiseCapability = NewPromiseCapability(constructor, scriptContext);
+
+        RecyclableObject* constructorObject = VarTo<RecyclableObject>(constructor);


Could use UnsafeVarTo (excludes a type check) as you've implicitly checked that this conversion will work with the IsObject check above - though perhaps move this up to be straight under the if to make that obvious.

rhuanjl · 2020-04-11T18:50:06Z

lib/Runtime/Library/JavascriptPromise.cpp

+    }
+
+    void JavascriptPromiseAnyRejectElementFunction::SetAlreadyCalled(const bool is)
+    {


nit: if using a setter for this no need for a parameter as you'll only ever call it to set to true

rhuanjl · 2020-04-11T18:53:29Z

test/es6/ES6PromiseAsync.js

+			function FakePromise(fn) {
+			  function resolve() { echo(`Test #${index} - resolve called`); throw new Error('oops'); }
+			  function reject(e) { echo(`Test #${index} - reject called: ${e.message}`) }
+			  fn(resolve, reject);
+			  this.then = function(onResolve, onReject) {};
+			}
+
+			FakePromise.resolve = function() {};
+			Promise.any.call(FakePromise, []);


This test case appears to be broken, or at least does not produce the result in the baseline.

Kingwl · 2020-04-21T07:23:58Z

Sorry for the delay too.
I'll take a look soon. Thanks for the review.

rhuanjl · 2020-04-21T08:04:16Z

Sorry for the delay too.
I'll take a look soon. Thanks for the review.

Great thanks.

Also, FYI, when you need to regenerate the bytecode headers again - I've recently enabled regenerating the bytecode on macOS or linux if you rebase on the latest master you'll find a script that can do this: https://github.com/microsoft/ChakraCore/blob/master/tools/xplatRegenByteCode.py

rhuanjl · 2021-03-14T18:29:45Z

@Kingwl are you likely to pick this up again? Would be good to get it finished

Kingwl · 2021-03-15T02:54:36Z

Sorry again. I almost(well, exactly) forgot about this issue.
I'd like to try to take this down.

BTW: I have join Microsoft. :XD

rhuanjl · 2021-03-15T08:27:21Z

It would be great to get this in if we can, though I note further to the points above:

to solve the bytecode file conflicts you'll need to drop the changes you made to them then run the script tools/regenByteCode.py
hopefully the JavascriptError.cpp and JavascriptPromise.cpp conflicts aren't too bad
this is no longer maintained by microsoft so you'll need to sign our new contribution agreement (add signing it as another commit: https://github.com/chakra-core/ChakraCore/blob/master/ContributionAgreement.md)

MadProbe · 2021-03-15T19:01:06Z

@Kingwl, when you will work on this PR, please update AggregateError to complete implementation of Error cause proposal and resolve a TODO in error_cause.js test (Edit: Note that you must write separate tests for AggregateError because of errors parameter)

Kingwl changed the title ~~[WIP] Add promise.any~~ [WIP][NOT READY] Add promise.any Oct 8, 2019

Kingwl force-pushed the promise-prototype-any branch from 29f2370 to e87d1ee Compare October 8, 2019 10:59

Kingwl force-pushed the promise-prototype-any branch from e87d1ee to a99e4b0 Compare October 8, 2019 16:31

Add promise.any

4294792

Kingwl force-pushed the promise-prototype-any branch from a99e4b0 to 4294792 Compare October 9, 2019 06:40

add AggregateError

c6985de

Kingwl force-pushed the promise-prototype-any branch from 6e09f83 to c6985de Compare October 9, 2019 17:00

Merge branch 'master' into promise-prototype-any

044d5fa

Kingwl force-pushed the promise-prototype-any branch from cbd9bbf to 044d5fa Compare October 10, 2019 17:25

Merge branch 'master' into promise-prototype-any

17c9c2c

Kingwl added 5 commits October 26, 2019 23:59

update aggr errors

c63fb50

add test case

d17657f

update bytecode

26b387c

Merge branch 'master' into promise-prototype-any

2bb0513

fix incorrect error

59b7bd1

Kingwl marked this pull request as ready for review October 27, 2019 12:25

add some test case

940ab4b

Kingwl changed the title ~~[WIP][NOT READY] Add promise.any~~ [WIP] Add promise.any Oct 27, 2019

fix ineteral slots

f1cefdf

mathiasbynens mentioned this pull request Mar 30, 2020

Advance to stage 4 tc39/proposal-promise-any#4

Closed

13 tasks

rhuanjl reviewed Apr 11, 2020

View reviewed changes

MadProbe mentioned this pull request Mar 15, 2021

Path to stage 4! tc39/proposal-error-cause#2

Closed

28 tasks

Kingwl mentioned this pull request Mar 16, 2021

Add proposal-promise-any #6641

Merged

Kingwl closed this Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add promise.any #6301

[WIP] Add promise.any #6301

Kingwl commented Oct 8, 2019 •

edited

Loading

rhuanjl commented Oct 8, 2019 •

edited

Loading

Kingwl commented Oct 8, 2019

rhuanjl commented Oct 8, 2019

Kingwl commented Oct 13, 2019

rhuanjl commented Oct 13, 2019

Kingwl commented Oct 14, 2019

zenparsing commented Oct 14, 2019

Kingwl commented Oct 14, 2019 •

edited

Loading

rhuanjl commented Oct 14, 2019

Kingwl commented Oct 27, 2019 •

edited

Loading

rhuanjl left a comment •

edited

Loading

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

rhuanjl Apr 11, 2020

Kingwl commented Apr 21, 2020

rhuanjl commented Apr 21, 2020

rhuanjl commented Mar 14, 2021

Kingwl commented Mar 15, 2021

rhuanjl commented Mar 15, 2021

MadProbe commented Mar 15, 2021 •

edited

Loading


		AUTO_TAG_NATIVE_LIBRARY_ENTRY(function, callInfo, _u("Promise.any"));

		// 1. Let C be the this value.

[WIP] Add promise.any #6301

[WIP] Add promise.any #6301

Conversation

Kingwl commented Oct 8, 2019 • edited Loading

rhuanjl commented Oct 8, 2019 • edited Loading

Kingwl commented Oct 8, 2019

rhuanjl commented Oct 8, 2019

Kingwl commented Oct 13, 2019

rhuanjl commented Oct 13, 2019

Kingwl commented Oct 14, 2019

zenparsing commented Oct 14, 2019

Kingwl commented Oct 14, 2019 • edited Loading

rhuanjl commented Oct 14, 2019

Kingwl commented Oct 27, 2019 • edited Loading

rhuanjl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kingwl commented Apr 21, 2020

rhuanjl commented Apr 21, 2020

rhuanjl commented Mar 14, 2021

Kingwl commented Mar 15, 2021

rhuanjl commented Mar 15, 2021

MadProbe commented Mar 15, 2021 • edited Loading

Kingwl commented Oct 8, 2019 •

edited

Loading

rhuanjl commented Oct 8, 2019 •

edited

Loading

Kingwl commented Oct 14, 2019 •

edited

Loading

Kingwl commented Oct 27, 2019 •

edited

Loading

rhuanjl left a comment •

edited

Loading

MadProbe commented Mar 15, 2021 •

edited

Loading