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

[WIP] Default parameter value for catch binding #2162

Closed
wants to merge 6 commits into from

Conversation

chicoxyzzy
Copy link
Member

Tests for tc39/ecma262#1126

Based on @leobalter's commits from #1483 (comment)

leobalter
leobalter previously approved these changes Jul 10, 2019
Copy link
Member

@leobalter leobalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a note: I'm glad we didn't have tests to check that catch (e = {}) was wrong before. This is where I appreciate we avoid checking for non existing syntax, or we would eventually need to search for needles in a haystack for random cases.

Just in case, I did some searches mostly based on catch\s?\(.*=.*\)\s?\{ and checked all the current existing cases.

@leobalter
Copy link
Member

the current set is fine, this is still missing cases with a SingleNameBinding and a present Initializer. (like I mentioned before, try{ throw undefined; } catch(e = 'oi!') { console.log(e == 'oi!'); }

@leobalter
Copy link
Member

leobalter commented Jul 10, 2019

The files in src/function-forms/*.case might give direction for the missing cases here, but they are not directly applied.

The major difference there is that it aims for FormalParameters and not FormalParameter, so you can't rely on lists.

This way could just skip the test generation and write static test files.

Some of the cases interesting for coverage:

  • Abrupt completion returned by evaluation of initializer:
try {
 throw {}; // won't trigger the error
} catch (_ = (function() { throw new Test262Error(); }())) {
 // assert this block will be evaluated
}

// vs

try {
 throw undefined; // will trigger the error
} catch (_ = (function() { throw new Test262Error(); }())) {
 // assert this block will not be evaluated
} // also, split this test for cases with and without a finally block and verify its execution.
  • Assert initializer won't be triggered if binding value is falsy but not undefined. (false, '', 0, null, etc):
try {
  throw undefined;
} catch (e = 42) {
  // assert e === 42
}

// and 

try {
  throw null; // and other cases where the value is 0, false, '', etc.
} catch (e = 42) {
  // assert the value is the thrown value, and not 42.
}
  • The initializer is the binding name:
var e = 42;

try {
} catch(e = e) { // no error, initializer is not used
  // assert this block will not be called
}

try {
  throw 'foo';
} catch(e = e) { // no error, initializer is not used
  // assert e === 'foo'
}

try {
  throw undefined;
} catch(e = e) {} // ReferenceError
// from specs, all the way to: Return ? base.GetBindingValue(GetReferencedName(V), IsStrictReference(V))

// split the case with and without a finally block

@chicoxyzzy
Copy link
Member Author

I think that I should use feature field somehow, but I don't understand how exactly. Should I just use catch-initializer in both templates and cases?

@chicoxyzzy
Copy link
Member Author

Or should I create a separate directory for my test templates and cases?

@chicoxyzzy
Copy link
Member Author

@leobalter I need your help :) Should I create new directory with my new templates and cases? Or should I rewrite my templates somehow?

@leobalter
Copy link
Member

I need your help :) Should I create new directory with my new templates and cases? Or should I rewrite my templates somehow?

yes, please. If it ends up just like another folder, you can reuse it then. A new folder you give you some freedom to author tests and you can just copy other existing templates.

@leobalter
Copy link
Member

In a second thought, let me finish looking at the current tests.

@leobalter
Copy link
Member

Closing this PR as the ECMA-262 change has found no consensus at the TC39 meeting in 2019-10-01.

Thanks for the hard work, @chicoxyzzy!

@leobalter leobalter closed this Oct 1, 2019
@chicoxyzzy chicoxyzzy deleted the catch-initializer branch October 1, 2019 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants