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

Allow set default value to CatchParameter #1121

Open
chicoxyzzy opened this Issue Feb 26, 2018 · 24 comments

Comments

Projects
None yet
6 participants
@chicoxyzzy
Contributor

chicoxyzzy commented Feb 26, 2018

There is some inconsistency between CatchParameter and BindingElement (i.e. function parameters).

Example from babel/babel#7434:

try {
  throw undefined;
} catch ({foo = 'bar'} = {}) {
  console.log(foo);
}

This is Syntax Error according to current spec grammar, but are there any reasons why assigning default value is not allowed here while destructuring (and default parameters inside of it) work fine?

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Feb 26, 2018

Contributor

My proposal is to change

CatchParameter[Yield, Await]:
  BindingIdentifier[?Yield, ?Await]
  BindingPattern[?Yield, ?Await]

to

CatchParameter[Yield, Await]:
  SingleNameBinding[?Yield, ?Await]
  BindingPattern[?Yield, ?Await]Initializer[+In, ?Yield, ?Await]opt

or

CatchParameter[Yield, Await]:
  BindingIdentifier[?Yield, ?Await]Initializer[+In, ?Yield, ?Await]opt
  BindingPattern[?Yield, ?Await]Initializer[+In, ?Yield, ?Await]opt

I'll be happy to file PR here and in test262 repo if this proposal sounds ok

Contributor

chicoxyzzy commented Feb 26, 2018

My proposal is to change

CatchParameter[Yield, Await]:
  BindingIdentifier[?Yield, ?Await]
  BindingPattern[?Yield, ?Await]

to

CatchParameter[Yield, Await]:
  SingleNameBinding[?Yield, ?Await]
  BindingPattern[?Yield, ?Await]Initializer[+In, ?Yield, ?Await]opt

or

CatchParameter[Yield, Await]:
  BindingIdentifier[?Yield, ?Await]Initializer[+In, ?Yield, ?Await]opt
  BindingPattern[?Yield, ?Await]Initializer[+In, ?Yield, ?Await]opt

I'll be happy to file PR here and in test262 repo if this proposal sounds ok

@chicoxyzzy chicoxyzzy changed the title from Allow set default parameter to CatchParameter to Allow set default value to CatchParameter Feb 26, 2018

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Feb 26, 2018

Contributor

To be clear, this problem was reported in Babel and it was caused by 3rd party library (regenerator in this case). Since it already possible to catch any object or primitive value, it looks weird that it's possible to use destructuring and default parameters inside it, but impossible to set default value for CatchParameter.

Contributor

chicoxyzzy commented Feb 26, 2018

To be clear, this problem was reported in Babel and it was caused by 3rd party library (regenerator in this case). Since it already possible to catch any object or primitive value, it looks weird that it's possible to use destructuring and default parameters inside it, but impossible to set default value for CatchParameter.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Feb 28, 2018

Member

I can see the consistency argument for this change, but I'm wondering--when would you actually want to throw undefined?

Member

littledan commented Feb 28, 2018

I can see the consistency argument for this change, but I'm wondering--when would you actually want to throw undefined?

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Feb 28, 2018

Contributor

@littledan personally I don't want to throw undefined or any other primitive or object except real Error :) This may happen in 3rd party libraries.

Contributor

chicoxyzzy commented Feb 28, 2018

@littledan personally I don't want to throw undefined or any other primitive or object except real Error :) This may happen in 3rd party libraries.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Feb 28, 2018

Member

This was an intentional design decision. Because the expectation is there should always be some object coercible value (ie, not undefined or null) that was thrown.

If you want to explore changing something, how about exploring to refusing to throw undefined.

WRT changing CatchParameter, I don't think we should do it. Is somebody really wants to restructure a catch parameter and are concerned about null or undefined they can move the destructuring into the body of the catch clause.

Member

allenwb commented Feb 28, 2018

This was an intentional design decision. Because the expectation is there should always be some object coercible value (ie, not undefined or null) that was thrown.

If you want to explore changing something, how about exploring to refusing to throw undefined.

WRT changing CatchParameter, I don't think we should do it. Is somebody really wants to restructure a catch parameter and are concerned about null or undefined they can move the destructuring into the body of the catch clause.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 28, 2018

Member

can someone clarify: is it “a default value in the catch binding” that’s currently disallowed, or only in the presence of destructuring, or is it destructuring in the catch binding that’s currently disallowed?

Member

ljharb commented Feb 28, 2018

can someone clarify: is it “a default value in the catch binding” that’s currently disallowed, or only in the presence of destructuring, or is it destructuring in the catch binding that’s currently disallowed?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 28, 2018

Member

(Separately, even if you can’t throw undefined, you’ll always be able to await Promise.reject() which causes an undefined to be thrown; preventing throwing undefined i suspect is a nonstarter)

Member

ljharb commented Feb 28, 2018

(Separately, even if you can’t throw undefined, you’ll always be able to await Promise.reject() which causes an undefined to be thrown; preventing throwing undefined i suspect is a nonstarter)

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Feb 28, 2018

Member

It's a BindingIdentifier or a BindingPattern, neither of which may have a initializer.

Why in the world does await Promise.reject() throw undefined. It sounds like we've just made catching undefined a normal expectation and that seems like a significant change to how exceptions had previously been used in the ES specification.

Member

allenwb commented Feb 28, 2018

It's a BindingIdentifier or a BindingPattern, neither of which may have a initializer.

Why in the world does await Promise.reject() throw undefined. It sounds like we've just made catching undefined a normal expectation and that seems like a significant change to how exceptions had previously been used in the ES specification.

@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Feb 28, 2018

Contributor

A couple use cases for throwing undefined:

  1. For awhile in the pre/early days of ES6, some transpilers used a try..catch hack for transpiling let block-scoped declarations. For example, my let-er project:

    {
        let x;
        console.log(x);
        x = 1;
        console.log(x);
    }

    became:

    try { throw undefined; } catch (x) {
       console.log(x);
       x = 1;
       console.log(x);
    }
  2. If you have multiple paths that can throw exceptions, and some of those paths are only to "escape" (e.g., break out of) a try block:

    try {
       var x = foo();
       var y = bar(x);
       if (y < 10) throw undefined;   // kinda like break; or return;
       var z = baz(y);
    }
    catch (err) {
       if (err != undefined) console.log(err);
    }

Both of these are uses that I've actually done in real production code. May not be "ideal" or "recommended", but I'm quite certain that throw undefined is not unprecedented, and can't just be dismissed.

Contributor

getify commented Feb 28, 2018

A couple use cases for throwing undefined:

  1. For awhile in the pre/early days of ES6, some transpilers used a try..catch hack for transpiling let block-scoped declarations. For example, my let-er project:

    {
        let x;
        console.log(x);
        x = 1;
        console.log(x);
    }

    became:

    try { throw undefined; } catch (x) {
       console.log(x);
       x = 1;
       console.log(x);
    }
  2. If you have multiple paths that can throw exceptions, and some of those paths are only to "escape" (e.g., break out of) a try block:

    try {
       var x = foo();
       var y = bar(x);
       if (y < 10) throw undefined;   // kinda like break; or return;
       var z = baz(y);
    }
    catch (err) {
       if (err != undefined) console.log(err);
    }

Both of these are uses that I've actually done in real production code. May not be "ideal" or "recommended", but I'm quite certain that throw undefined is not unprecedented, and can't just be dismissed.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 28, 2018

Member

Catching undefined has always been possible; and I’d say with the introduction of Promises that can reject with a nullish reason, that was permanently cemented in ES2015 (due to the intended parallels between rejection and throwing)

Member

ljharb commented Feb 28, 2018

Catching undefined has always been possible; and I’d say with the introduction of Promises that can reject with a nullish reason, that was permanently cemented in ES2015 (due to the intended parallels between rejection and throwing)

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Feb 28, 2018

Contributor

While it's certainly possible, I'm not sure it's a case which merits having explicit grammar support to handle.

Contributor

bakkot commented Feb 28, 2018

While it's certainly possible, I'm not sure it's a case which merits having explicit grammar support to handle.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Feb 28, 2018

Member

From the spec side it’s explicit support, sure, but from the user side it looks like an explicit and surprising restriction imo.

Member

ljharb commented Feb 28, 2018

From the spec side it’s explicit support, sure, but from the user side it looks like an explicit and surprising restriction imo.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Feb 28, 2018

Member

@getify for 2., would labeled blocks work? See the MDN article for an example.

Member

littledan commented Feb 28, 2018

@getify for 2., would labeled blocks work? See the MDN article for an example.

@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Feb 28, 2018

Contributor

@littledan not in this particular case because what you want is the code in the catch to have a chance to run.

Contributor

getify commented Feb 28, 2018

@littledan not in this particular case because what you want is the code in the catch to have a chance to run.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Feb 28, 2018

Member

I mean, you could decouple the breaking-out and the catch block as follows, eliminating the need for the conditional and use of undefined as a sentinel; might be more straightforward:

try {
   label: {
      var x = foo();
      var y = bar(x);
      if (y < 10) break label;
      var z = baz(y);
   }
}
catch (err) {
   console.log(err);
}
Member

littledan commented Feb 28, 2018

I mean, you could decouple the breaking-out and the catch block as follows, eliminating the need for the conditional and use of undefined as a sentinel; might be more straightforward:

try {
   label: {
      var x = foo();
      var y = bar(x);
      if (y < 10) break label;
      var z = baz(y);
   }
}
catch (err) {
   console.log(err);
}
@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Feb 28, 2018

Contributor

@littledan yes that could work in certain circumstances. but any code after the labeled block (aka any sentinel conditional code) should only run in the break case and not in the normal flow case, so that has to be accounted for in some way. also, adding extra blocks (esp if there's more than one) has the potential extra complication of juggling block-scoped declarations.

Contributor

getify commented Feb 28, 2018

@littledan yes that could work in certain circumstances. but any code after the labeled block (aka any sentinel conditional code) should only run in the break case and not in the normal flow case, so that has to be accounted for in some way. also, adding extra blocks (esp if there's more than one) has the potential extra complication of juggling block-scoped declarations.

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Mar 2, 2018

Contributor

undefined couldn't be avoided for some 3rd party code and it is possible to throw undefined since ES3, also I don't see any strong arguments against of Initialiser in CatchParameter. Sure, one can use check inside of body of catch clause but there is no single reason for such restriction.

Since this code is valid

try {
  // ...
} catch ({foo = 'bar'}) {
  // do something with foo
}

I don't see any reasons why one should write

try {
  // ...
} catch (e) {
  const foo = typeof e === 'undefined' || typeof e.foo === 'undefined'
    ? 'bar'
    : e.foo;
  // do something with foo
}

instead of just

try {
  // ...
} catch ({foo = 'bar'} = {}) {
  // do something with foo
}
Contributor

chicoxyzzy commented Mar 2, 2018

undefined couldn't be avoided for some 3rd party code and it is possible to throw undefined since ES3, also I don't see any strong arguments against of Initialiser in CatchParameter. Sure, one can use check inside of body of catch clause but there is no single reason for such restriction.

Since this code is valid

try {
  // ...
} catch ({foo = 'bar'}) {
  // do something with foo
}

I don't see any reasons why one should write

try {
  // ...
} catch (e) {
  const foo = typeof e === 'undefined' || typeof e.foo === 'undefined'
    ? 'bar'
    : e.foo;
  // do something with foo
}

instead of just

try {
  // ...
} catch ({foo = 'bar'} = {}) {
  // do something with foo
}
@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 2, 2018

Member

Honestly it's hard for me to understand when it would be useful to use destructuring in a catch binding, so I'm having trouble making a mental model to weigh @chicoxyzzy 's vs @allenwb 's logic. Could you give an example of a case you'd want to use destructuring with an object literal in a catch binding?

Member

littledan commented Mar 2, 2018

Honestly it's hard for me to understand when it would be useful to use destructuring in a catch binding, so I'm having trouble making a mental model to weigh @chicoxyzzy 's vs @allenwb 's logic. Could you give an example of a case you'd want to use destructuring with an object literal in a catch binding?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Mar 2, 2018

Member

Destructuring works with any object including an error instance; I’ve used it for destructuring out a message, stack trace, and error code.

Member

ljharb commented Mar 2, 2018

Destructuring works with any object including an error instance; I’ve used it for destructuring out a message, stack trace, and error code.

@getify

This comment has been minimized.

Show comment
Hide comment
@getify

getify Mar 2, 2018

Contributor

@littledan @ljharb I have too... especially useful to apply defaults to properties of an Error object that may not be present in all engines, like stack, etc.

Contributor

getify commented Mar 2, 2018

@littledan @ljharb I have too... especially useful to apply defaults to properties of an Error object that may not be present in all engines, like stack, etc.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 2, 2018

Member

OK, this makes perfect sense--you want to read these properties of the object through destructuring, but default to an empty object if a library you're calling out to throws null or undefined. Did you consider that case when deciding against supporting it, @allenwb ?

If we want to do this spec change, I wonder if we should just get rid of the CatchParameter production and use BindingElement instead, directly in the grammar of Catch. Would that do something different from your second proposal, @chicoxyzzy ? Did you have a particular reason why the first alternative might be preferred?

Member

littledan commented Mar 2, 2018

OK, this makes perfect sense--you want to read these properties of the object through destructuring, but default to an empty object if a library you're calling out to throws null or undefined. Did you consider that case when deciding against supporting it, @allenwb ?

If we want to do this spec change, I wonder if we should just get rid of the CatchParameter production and use BindingElement instead, directly in the grammar of Catch. Would that do something different from your second proposal, @chicoxyzzy ? Did you have a particular reason why the first alternative might be preferred?

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Mar 2, 2018

Member

When this was discussed, I think everybody assume that the exception parameter would always be object coercible. Destructuring of exception parameter is definitely useful and I'm sure that most people aren't going to add a ={} just in case. But I don't think it would hurt anything to allow it. The people who don't expect it will still get an error.

I believe all you need to do is redefine CatchParameter as:

CatchParameter : FormalParameter

My recollection is that the only reason it wasn't defined that way in ES2015 was to excluded the default value case.

Member

allenwb commented Mar 2, 2018

When this was discussed, I think everybody assume that the exception parameter would always be object coercible. Destructuring of exception parameter is definitely useful and I'm sure that most people aren't going to add a ={} just in case. But I don't think it would hurt anything to allow it. The people who don't expect it will still get an error.

I believe all you need to do is redefine CatchParameter as:

CatchParameter : FormalParameter

My recollection is that the only reason it wasn't defined that way in ES2015 was to excluded the default value case.

@chicoxyzzy

This comment has been minimized.

Show comment
Hide comment
@chicoxyzzy

chicoxyzzy Mar 3, 2018

Contributor

I think CatchParameter : FormalParameter change should solve everything 👍
Should I create new proposal repo and find champion for it or just create PRs to ecma262 and test262 repos for review and further discussions?

Maybe it's a good topic to discuss on March meeting?

Contributor

chicoxyzzy commented Mar 3, 2018

I think CatchParameter : FormalParameter change should solve everything 👍
Should I create new proposal repo and find champion for it or just create PRs to ecma262 and test262 repos for review and further discussions?

Maybe it's a good topic to discuss on March meeting?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Mar 3, 2018

Member

Maybe we should start with a PR, and we can see if it gains consensus in March?

Member

ljharb commented Mar 3, 2018

Maybe we should start with a PR, and we can see if it gains consensus in March?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment