Implementation of Error cause proposal #6621
Conversation
|
@rhuanjl can you review this PR? |
test/Error/error_cause.js
Outdated
| { | ||
| name: "Error().cause === undefined", | ||
| body: function () { | ||
| assert.isFalse("cause" in SyntaxError(), "Error().cause should not be defined if specified by cause property of options parameter in *Error constructor"); |
There was a problem hiding this comment.
I don't understand the error message here - do you mean "Error().cause should not be defined if specified by cause property of options parameter in *Error constructor"
test/Error/error_cause.js
Outdated
| } | ||
| }, | ||
| { | ||
| name: "Error().cause === undefined", |
There was a problem hiding this comment.
Should also have a case where we call the constructor with 1 parameter and check it has a message but not a cause.
test/Error/error_cause.js
Outdated
| assert.areEqual(hasCounter, 1, `hasCounter should be 1`); | ||
| assert.areEqual(getCounter, 1, `getCounter should be 1`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should add a cause with an options parameter BUT without a cause property - confirm that cause doesn't get created.
| { | ||
| ScriptContext* scriptContext = function->GetScriptContext(); | ||
|
|
||
| bool isCtorSuperCall = (callInfo.Flags & CallFlags_New) && newTarget != nullptr && !JavascriptOperators::IsUndefined(newTarget); | ||
| JavascriptString* messageString = nullptr; | ||
| Var cause = nullptr; |
There was a problem hiding this comment.
Could this be defined inside the if statement where it's used? it doesn't look like it's used outside of it.
| @@ -65,6 +66,14 @@ namespace Js | |||
| pError->SetNotEnumerable(PropertyIds::message); | |||
| } | |||
|
|
|||
| if (JavascriptOperators::IsObject(options) && JavascriptOperators::HasProperty(VarTo<RecyclableObject>(options), PropertyIds::cause)) | |||
There was a problem hiding this comment.
Can use UnsafeVarTo<RecyclableObject> here as the IsObject has checked it.
(VarTo = AssertOrFailFast if this conversion is likely to be invalid; UnsafeVarTo = just do the conversion)
test/Error/error_cause.js
Outdated
| { | ||
| name: "Cause property is not added to error if options parameter doesn't have the cause property", | ||
| body: function () { | ||
| assert.isFalse('cause' in ErrorConstructor(message, { }), `Cause property must not be added to error if options parameter doesn't have the cause property`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we also have a case where the options parameter isn't an object - maybe do one with a string and one with a number.
Other than that, looks good to merge.
| <test> | ||
| <default> | ||
| <files>error_cause.js</files> | ||
| <compile-flags>-args summary -endargs</compile-flags> |
There was a problem hiding this comment.
You may have guessed but just an FYI this gives -args summary -endargs as extra command line arguments for the test.
Which in turn makes WScript.Arguments[0] = "summary" - so that testRunner.runTests gets called with verbose: false
The reason for doing this to set the "verbose" option of a test to false rather than having it hard coded in the test file is so when you run the test yourself the default will be verbose = true(to print out all the test cases within the file) vs when it's run here through the test runner it prints only failures + the word "pass" or "fail" at the end.
Error cause proposal is stage 3.
There are no 262 tests to check against yet.