Promise.withResolvers() returned object had resolve and reject functions swapped

[tomatosalat0]

When using Promise.withResolvers(), the returned object had resolve and reject swapped. Meaning that

const p = Promise.withResolvers();
p.resolve();
return p.promise;

did return a rejected promise.

and

const p = Promise.withResolvers();
p.reject();
return p.promise;

dir return a resolved promise.

---

The fix: The record PromiseCapability had the following property order

internal sealed record PromiseCapability(
    JsValue PromiseInstance,
    ICallable Resolve,   // <-- Resolve first, Reject last
    ICallable Reject,
    JsValue RejectObj,   // <-- Reject first, Resolve first
    JsValue ResolveObj
);

The method PromiseConstructor.NewPromiseCapability() however flipped the two last properties when calling the PromiseCapability constructor.

---

Refactoring: In addition to fix the parameter order in PromiseConstructor.NewPromiseCapability(), I had to change two methods, which already relied on the flipped assignment: "Promise.race" and "Promise.all". While deconstruction can be nice, I think that the deconstruction usage of PromiseCapability might be a reason that this flip was not caught earlier. So:

• Instead of deconstructing into many unused _ variables, I directly use PromiseInstance.[Property], which makes it easier to spot mistakes
• I changed the order of the last two properties of PromiseCapability to keep the order consistent.

[sebastienros]

So much joy seeing quality PRs popping up!

[tomatosalat0]

Is the PR Check / windows still a bit flaky or did I actually break something? 😆

[sebastienros]

Looks like a failed test:

2024-10-23T21:42:51.4017623Z [xUnit.net 00:00:10.37]     Jint.Tests.Runtime.ExecutionConstraintTests.ShouldThrowExecutionCanceled [FAIL]
2024-10-23T21:42:51.4018827Z 
2024-10-23T21:42:51.6631090Z ##[error]Assert.Throws() Failure: No exception was thrown
Expected: typeof(Jint.Runtime.ExecutionCanceledException)
2024-10-23T21:42:51.8682570Z 
2024-10-23T21:42:51.8687422Z   Failed Jint.Tests.Runtime.ExecutionConstraintTests.ShouldThrowExecutionCanceled [5 s]
2024-10-23T21:42:51.8690084Z   Error Message:
2024-10-23T21:42:51.8692809Z    Assert.Throws() Failure: No exception was thrown
2024-10-23T21:42:51.8723118Z Expected: typeof(Jint.Runtime.ExecutionCanceledException)
2024-10-23T21:42:51.8726916Z   Stack Trace:
2024-10-23T21:42:52.0308317Z      at Jint.Tests.Runtime.ExecutionConstraintTests.ShouldThrowExecutionCanceled() in D:\a\jint\jint\Jint.Tests\Runtime\ExecutionConstraintTests.cs:line 35
2024-10-23T21:42:52.8069584Z    at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
2024-10-23T21:42:52.8226232Z    at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[tomatosalat0]

Hm, interesting. I am currently continuously keep that test running locally (windows machine) and it rejects to fail - everything stays green 🤔. Can you just re-run the PR check again to see if it actually fails again?

[tomatosalat0]

After trying to understand the test, I would say: that's a flaky one and it's failure is not a result of the changes above.

Code coverage for that test shows that it doesn't touch any promise related code. In addition, the test uses ThreadPool.QueueUserWorkItem to queue a method and "hopes" that that method gets invoked before the Engine test code completes. This can work in 99% of the time, but I guess I just had bad luck here.

Change this failing test is not that difficult, but I would rather do that in a separate PR because it is quite unrelated.

[lahma]

Great work, thank you! Weird that test262 suite didn't cover this..