Skip to content

Let const in catchclause#2235

Merged
mhegazy merged 4 commits into
microsoft:masterfrom
DickvdBrink:letConstInCatchclause
Mar 6, 2015
Merged

Let const in catchclause#2235
mhegazy merged 4 commits into
microsoft:masterfrom
DickvdBrink:letConstInCatchclause

Conversation

@DickvdBrink
Copy link
Copy Markdown
Contributor

This is my attempt at solving #2186.
If this isn't the right way of solving this, feel free to close this PR.

I didn't implement this part:

It is a Syntax Error if any element of the BoundNames of CatchParameter also occurs in the VarDeclaredNames of Block

Because it felt like a breaking change (but that might not be a bad thing in ES6 mode though)
but I couldn't find a browser which actually did this, tried IE 11, Firefox Nightly and Chrome (Stable).
And the spec also mentions (B.3.5):

The Block of a Catch clause may contain var declarations that bind a name that is also bound by the CatchParameter

So I'm not sure if a var should error (or am I totally not understanding the spec here?)

Tests are coming :)

@JsonFreeman
Copy link
Copy Markdown
Contributor

The spec is so indecisive on the point about the var in the catch block. They even suggest two options for how to treat it. The first one is the one you mentioned in section 13.14.1. The second one is from B.3.5:

It is a Syntax Error if any element of the BoundNames of CatchParameter also occurs in the
VarDeclaredNames of Block, unless that element is only bound by a VariableStatement or the
VariableDeclarationList of a for statement, or the ForBinding of a for-in statement.

The relaxation of the
normal static semantic rule does not apply to names only bound by for-of statements.

The spec is trying to disallow it for all new constructs, but not for old constructs. In my opinion, we should only error for the var case if the var is initialized, because then it will overwrite the catch parameter.

Comment thread src/compiler/checker.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be a binding pattern?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't see the previous condition that checks it's an identifier

@JsonFreeman
Copy link
Copy Markdown
Contributor

👍

mhegazy added a commit that referenced this pull request Mar 6, 2015
@mhegazy mhegazy merged commit 59c3a7d into microsoft:master Mar 6, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants