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

Annex B.3.5 CatchParameter legacy remedy could be narrower #150

Closed
ajklein opened this Issue Oct 29, 2015 · 3 comments

Comments

Projects
None yet
4 participants
@ajklein
Contributor

ajklein commented Oct 29, 2015

The early errors for Try/Catch names includes the following passage:

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

However, this is modified in B.3.5 to allow code of the form:

try {
  ...
} catch (e) {
  var e = 2;
}

to run without complaint, presumably for legacy compat reasons. The spec text says:

"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."

But this means that the following is also allowed:

try {
  ...
} catch ({stack, code}) {
  var stack = 1;
  var code = 2;
}

What's the motivation for allowing the destructured form, given that there will be no legacy concerns in that case? One could imagine the spec could instead read:

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

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Oct 29, 2015

Member

Yes, this sounds like a good narrowing of the B.3.5 form of the rule.

Member

allenwb commented Oct 29, 2015

Yes, this sounds like a good narrowing of the B.3.5 form of the rule.

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Oct 30, 2015

Contributor

The current specification also allows this:

try {
  ...
} catch (e) {
  var [e] = ...
}

Should that form also be made invalid?

[...] unless BoundNames contains a single element and [...]

"unless CatchParameter is CatchParameter : BindingIdentifier and " to handle catch ({stack}) {...} correctly.

Contributor

anba commented Oct 30, 2015

The current specification also allows this:

try {
  ...
} catch (e) {
  var [e] = ...
}

Should that form also be made invalid?

[...] unless BoundNames contains a single element and [...]

"unless CatchParameter is CatchParameter : BindingIdentifier and " to handle catch ({stack}) {...} correctly.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Oct 30, 2015

Member

Yes, that final change should do it.

Member

michaelficarra commented Oct 30, 2015

Yes, that final change should do it.

kisg pushed a commit to paul99/v8mips that referenced this issue Nov 4, 2015

[es6] Implement destructuring binding in try/catch
The approach is to desugar

  try { ... }
  catch ({x, y}) { ... }

into

  try { ... }
  catch (.catch) {
    let x = .catch.x;
    let y = .catch.y;
    ...
  }

using the PatternRewriter's normal facilities. This has the side benefit
of throwing the appropriate variable conflict errors for declarations
made inside the catch block.

No change is made to non-destructured cases, which will hopefully save
us some work if tc39/ecma262#150 is adopted
in the spec.

There's one big problem with this patch, which is a lack of PreParser
support for the redeclaration errors. But it seems we're already lacking
good PreParser support for such errors, so I'm not sure that should
block this moving forward.

BUG=v8:811
LOG=y

Review URL: https://codereview.chromium.org/1417483014

Cr-Commit-Position: refs/heads/master@{#31797}

kisg pushed a commit to paul99/v8mips that referenced this issue Nov 4, 2015

Revert of [es6] Implement destructuring binding in try/catch (patchset
…v8#3 id:40001 of https://codereview.chromium.org/1417483014/ )

Reason for revert:
MSAN errors on arm64: http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20sim%20-%20MSAN/builds/5123/

Original issue's description:
> [es6] Implement destructuring binding in try/catch
>
> The approach is to desugar
>
>   try { ... }
>   catch ({x, y}) { ... }
>
> into
>
>   try { ... }
>   catch (.catch) {
>     let x = .catch.x;
>     let y = .catch.y;
>     ...
>   }
>
> using the PatternRewriter's normal facilities. This has the side benefit
> of throwing the appropriate variable conflict errors for declarations
> made inside the catch block.
>
> No change is made to non-destructured cases, which will hopefully save
> us some work if tc39/ecma262#150 is adopted
> in the spec.
>
> There's one big problem with this patch, which is a lack of PreParser
> support for the redeclaration errors. But it seems we're already lacking
> good PreParser support for such errors, so I'm not sure that should
> block this moving forward.
>
> BUG=v8:811
> LOG=y
>
> Committed: https://crrev.com/a316db995e6e4253664920652ed4e5a38b2caeba
> Cr-Commit-Position: refs/heads/master@{#31797}

TBR=rossberg@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:811

Review URL: https://codereview.chromium.org/1408063013

Cr-Commit-Position: refs/heads/master@{#31798}

@bterlson bterlson closed this in 02e46ab Nov 10, 2015

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