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

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.

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.

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.

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.

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.

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.

The check in bindVar: scopeStmt->type == StmtType::CATCH Won't work for the second case, because the |scopeStmt| is BLOCK, not CATCH.

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.

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.

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?

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

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?

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.

Fixed by bug 1263355.

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

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

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")}`.)

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

(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?

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

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

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 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 :-(

Pushed by shu@rfrn.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0212f61c24 Fix redeclaring catch parameters in eval. (r=Waldo)