1175174 - Don't allow variable redeclaration in for-of statement

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case 1:
---
try {} catch (e) { for (var e of []) {} }
---

Expected: Throws early SyntaxError
Actual: No error


Test case 2:
---
try { throw null; } catch (e) { eval("for (var e of []) {}") }
---

Expected: Throws runtime SyntaxError
Actual: No error


ES2015, rev38:
13.15.1 Static Semantics: Early Errors
18.2.1.2 Runtime Semantics: EvalDeclarationInstantiation( body, varEnv, lexEnv, strict) - step 6.d
B.3.5 VariableStatements in Catch blocks
https://bugs.ecmascript.org/show_bug.cgi?id=4339

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

I Tested this but found the case 1. now would throw

SyntaxError: redeclaration of identifier 'e'

But the case 2. still works. I would take a look.

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

I've found that in the `Parser::bindVar`, if the case is with eval statement, it will go to the early returning line of

    if (defs.empty())
        return pc->define(parser->tokenStream, name, pn, Definition::VAR);

And the following check conditions include the one successfully prevents the first case (without the eval) would not be triggered. I'm not sure if the condition is what it should reach (while right now it doesn't), but for the first case it would eventually fall to the line reports JSMSG_REDECLARED_VAR error.

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

I suspect that the case passes the test incorrectly, because the map of the environment while evaling is empty, which means it have nothing from the previous catch block.

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

One thing I noticed is the case failed correctly would throw:

    "SyntaxError: redeclaration of identifier 'e' in catch:"

The message looks more general than in |for..of|. Despite that, if I change the test code to:

    try { throw null; } catch (e) { var e = 3 }

It will perform without any error. However, this looks conflict with the error message, which ban all existed identifiers in catch to be redeclared.

Comment 5

[Greg Weng [:snowmantw][:gweng][:λ]]

This case

    try { throw null; } catch (e) { eval("var e = true;for (var e of []) {}") }

Will also trigger the |defs.empty()| as true. So I think the issue is |var e of []| doesn't check the environment well. Although the first case throws in a so far mysteriously "correct" way.

Comment 6

[Greg Weng [:snowmantw][:gweng][:λ]]

I think it's because when eval, the |bindVar| of |for...of| will check if it's in catch as in the non-eval case, but since it's in another environment, so the check and the non-existed check of redeclaration, which this bug would addresses, won't stop it to redeclare the variable. Compare to that, in the first case, although the check in |for...of| is still non-existed, but the "in catch" check could work coincidentally, so it will throw.

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

The check in bindVar:

    scopeStmt->type == StmtType::CATCH

Won't work for the second case, because the |scopeStmt| is BLOCK, not CATCH.

Comment 8

[Greg Weng [:snowmantw][:gweng][:λ]]

So if this is the place to fix, maybe there should be something could hint the check now it's in a CATCH block outside the eval. I now have no idea on how to solve it, but a naive solution is to add or to look up an existed flag about the eval.

Comment 9

[Greg Weng [:snowmantw][:gweng][:λ]]

Well, maybe first the eval should bring flag as "eval in catch", and then to the statements inside the eval. However, I feel this way pollutes too much. The second choice is to "check through" to the outer scope until it catches the "eval". Although I don't know if this works.

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

Since I don't actually know who I should ask, I checked the code and found the author of latest change. Shuyu, could you give me some advice or forward NI to whom may know this issue well?

Comment 11

[Jeff Walden [:Waldo]]

This is probably on me to take, as part of bug 449811 inanities.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

So from reading the patch in Bug 449811 Comment 23, I guess it relates to the new |emitForInOrOfDeclaration|? Also, if it's not time to solve this bug yet, I may cancel the assignation first, and set the dependency to Bug 449811?

Comment 13

[Jeff Walden [:Waldo]]

Technically, comment 0's testcase 1, I believe, shouldn't be a syntax error.  It would just happen that every for-loop iteration would assign to the catch-variable |e|, not to the |var| declared by the for-loop.

As the dependency notes, correct behavior here should mostly fall out of the frontend rewrite.  And if it doesn't quite, it should be done after that lands, and no sooner.

Comment 14

[Shu-yu Guo [:shu]]

Fixed by bug 1263355.

Comment 15

[André Bargull [:anba]]

The second test case is still reproducible for me (at rev a551f534773c).

Comment 16

[André Bargull [:anba]]

And if I'm reading B.3.5 correctly, this case should also throw a SyntaxError:
---
try { throw null; } catch (e) { eval("function* e(){}") }
---

Comment 17

[André Bargull [:anba]]

And related: There seems to be a spec bug in B.3.5. https://github.com/tc39/ecma262/issues/150 made `try {} catch ({e}) {var e}` a syntax error, but `try {} catch ({e}) {eval("var e")}` is still allowed per the current spec. (I think it's a spec bug, because the change was motived by the V8 team and V8 throws a syntax error for `try {} catch ({e}) {eval("var e")}`.)

Comment 18

[André Bargull [:anba]]

(In reply to André Bargull from comment #17)
> ...

`try {} catch ({e}) {eval("var e")}` -> `try {throw {}} catch ({e}) {eval("var e")}`, to ensure the eval call is actually evaluated.

Comment 19

[Shu-yu Guo [:shu]]

(In reply to André Bargull from comment #16)
> And if I'm reading B.3.5 correctly, this case should also throw a
> SyntaxError:
> ---
> try { throw null; } catch (e) { eval("function* e(){}") }
> ---

Really? What's the rule that causes this?

Comment 20

[André Bargull [:anba]]

18.2.1.3 Runtime Semantics: EvalDeclarationInstantiation
Step 1: Let varNames be the VarDeclaredNames of body.
  -> 15.1.5 Static Semantics: VarDeclaredNames
    -> 13.2.9 Static Semantics: TopLevelVarDeclaredNames
      -> 3rd production (StatementListItem : Declaration) applies here
=> varNames = « "g" »

18.2.1.3 Runtime Semantics: EvalDeclarationInstantiation
Step 5.d.ii.2: Loops over varNames
 -> Step 5.d.ii.2.a.i is replaced by B.3.5 VariableStatements in Catch Blocks

  1. If thisEnvRec is not the Environment Record for a Catch clause, throw a SyntaxError exception.
  2. If name is bound by any syntactic form other than a FunctionDeclaration, a VariableStatement, the VariableDeclarationList of a for statement, or the ForBinding of a for-in statement, throw a SyntaxError exception. 

When |thisEnvRec| is the environment record of the Catch clause, step 2 from B.3.5 applies. The |name| "g" is bound by a GeneratorDeclaration, so none of the exclusions apply which means step 2 throws a SyntaxError. (It's quite possible I missed something here, because it's been a while since I last looked up this part of the spec.)

Comment 21

[Shu-yu Guo [:shu]]

(In reply to André Bargull from comment #20)
> 18.2.1.3 Runtime Semantics: EvalDeclarationInstantiation
> Step 1: Let varNames be the VarDeclaredNames of body.
>   -> 15.1.5 Static Semantics: VarDeclaredNames
>     -> 13.2.9 Static Semantics: TopLevelVarDeclaredNames
>       -> 3rd production (StatementListItem : Declaration) applies here
> => varNames = « "g" »
> 
> 18.2.1.3 Runtime Semantics: EvalDeclarationInstantiation
> Step 5.d.ii.2: Loops over varNames
>  -> Step 5.d.ii.2.a.i is replaced by B.3.5 VariableStatements in Catch Blocks
> 
>   1. If thisEnvRec is not the Environment Record for a Catch clause, throw a
> SyntaxError exception.
>   2. If name is bound by any syntactic form other than a
> FunctionDeclaration, a VariableStatement, the VariableDeclarationList of a
> for statement, or the ForBinding of a for-in statement, throw a SyntaxError
> exception. 
> 
> When |thisEnvRec| is the environment record of the Catch clause, step 2 from
> B.3.5 applies. The |name| "g" is bound by a GeneratorDeclaration, so none of
> the exclusions apply which means step 2 throws a SyntaxError. (It's quite
> possible I missed something here, because it's been a while since I last
> looked up this part of the spec.)

That... looks right? Unending torments.

Comment 22

[Shu-yu Guo [:shu]]

Attachment: Fix redeclaring catch parameters in eval.

This does not fix the generator case, because it is just so goddamn weird I'm
going to talk to people at TC39 next meeting.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 8785224 [details] [diff] [review]
Fix redeclaring catch parameters in eval.

Review of attachment 8785224 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/LexicalEnvironment/var-in-catch-body-annex-b-eval-destructuring.js
@@ +1,2 @@
> +assertThrowsInstanceOf(() => evaluate(`try { throw {} } catch ({e}) { var e; }`), SyntaxError);
> +assertThrowsInstanceOf(() => evaluate(`try { throw {} } catch ({e}) { eval('var e'); }`), SyntaxError);

Pretty sure this needs a reftest skip-if in the case where this is running in the browser.  Might also be worth a loop to try out Function and eval handling of this, too.  And parseModule, which you'll have to typeof-test for.

::: js/src/tests/ecma_6/LexicalEnvironment/var-in-catch-body-annex-b-eval-for-of.js
@@ +1,1 @@
> +assertThrowsInstanceOf(() => evaluate(`

reftest skip-if, and also maybe test out Function/eval/parseModule as well.

::: js/src/vm/EnvironmentObject.cpp
@@ +3230,5 @@
>      }
>  
> +    if (env->isSyntactic() && !env->isGlobal() &&
> +        (env->scope().kind() == ScopeKind::Catch ||
> +         env->scope().kind() == ScopeKind::SimpleCatch))

Maybe worth an inline function to test for both catch-kind variants.

@@ -3249,5 @@
>  js::CheckEvalDeclarationConflicts(JSContext* cx, HandleScript script,
>                                    HandleObject scopeChain, HandleObject varObj)
>  {
> -    // We don't need to check body-level lexical bindings for conflict. Eval
> -    // scripts always execute under their own lexical scope.

lol :-(

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0212f61c24
Fix redeclaring catch parameters in eval. (r=Waldo)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c0212f61c24