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.

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

Default parameter value for catch binding #1483

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@chicoxyzzy
Copy link

chicoxyzzy commented Mar 9, 2018

Tests for tc39/ecma262#1126

@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Mar 9, 2018

well, this is just the tip for checking formal parameters. We strive to maintain at least most of the variants we can observe for the syntax. That means checking the FormalParameter will observe the binding patterns (destructuring, etc). We also need to check for edge cases as early errors, naming conflicts, verifying it's a single formalparameter (and not a list), etc.

Also, it would be great to avoid using eval whenever is possible.

We have some templates and cases in the src/ folder for destructuring that might be helpful for creating the new cases.

I'm interested to help with more information and support, just let me know.

@leobalter
Copy link
Member

leobalter left a comment

comment above

@chicoxyzzy

This comment has been minimized.

Copy link
Author

chicoxyzzy commented Mar 10, 2018

@leobalter thank you for your feedback! As far as I understand, test for most of destructuring patterns and other cases are already implemented here https://github.com/tc39/test262/tree/master/test/language/statements/try

As for avoiding eval, I'm not sure how to rewrite my code. For example,

try {
 throw undefined;
} catch (err = "foo") {
 assert.sameValue(err, 'foo');
}

will throw early syntax error on =. Should I use !assert.throws here?

Any help will be appreciated!

Also I noticed that assert.throws.early function has unused local variable ieval
https://github.com/tc39/test262/blob/master/harness/assert.js#L92

@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Mar 12, 2018

test for most of destructuring patterns and other cases are already implemented here https://github.com/tc39/test262/tree/master/test/language/statements/try

Yes, and some are generated. I'm experimenting here with a new template for catch initializers and I'll get you a feedback anytime soon.

(...) will throw early syntax error on =. Should I use !assert.throws here?

The early error is intentional, and a good sign. You should let this throw a Syntax Error while it's not implemented. It should only pass when it's not anymore. It will get green when the feature is available.

@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Mar 12, 2018

@chicoxyzzy I just reused some templates for destructuring cases of the catch initializer here: https://github.com/tc39/test262/compare/master...leobalter:catch-initializer?expand=1

There are also cases to be used from src/function-forms that would apply to the catch initializer, but I think some of them could need some clarification in the produced specs, some others are just useful for checking edge cases like:

try {
  throw undefined;
} catch (_ = (function() { throw new Test262Error(); }()) {}

or cases where falsy values won't trigger the initializer:

throw null;
throw NaN;
throw 0;
throw false;
throw "";
throw {};

also consider expanding the tests with throw void 0;

I would also check this is being set to a single FormalParameter so rest parameters and trailing commas are still not allowed and they should continue throwing a SyntaxError

@chicoxyzzy

This comment has been minimized.

Copy link
Author

chicoxyzzy commented Mar 12, 2018

Thank you for such detailed answer! I'll return to this PR in next few days.

@rwaldron

This comment has been minimized.

Copy link
Contributor

rwaldron commented May 22, 2018

This has Stage 4

Sorry about that, I had several issues/PRs opened and scrolled to their respective comment input forms and I posted the wrong update to this PR. My apologies for any confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.