Reviewers: Dan Ehrenberg, rossberg,
Message:
On 2015/09/03 00:10:29, rossberg wrote:
Hm, I don't quite understand this CL. It seems to special-case for-loops,
but
there are many other constructs that could introduce conflicting bindings.
Shouldn't these conflicts be detected in a more central place?
Specifically, I
think you want to invoke something similar to the existing
CheckConflictingDeclarations in the function that parses the `catch`
phrase.
We will also have to deal with let-/const-bindings, but the for-of case is
particularly awkward because B.3.5 specifies that var-/for-/for-in-bindings
are fine.
I wasn't sure if there was a better way to make sure for-of acts correctly
given
those constraints.
Description:
Do not permit binding over catch parameters in for-of
This is banned by the ES6 spec (see sections 13.15.1
and B.3.5). When done inside an eval, throw a runtime
SyntaxError. We should get an early SyntaxError otherwise.
[email protected]
LOG=N
BUG=v8:4231
Please review this at https://codereview.chromium.org/1318023004/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+49, -13 lines):
M src/parser.h
M src/parser.cc
A + test/mjsunit/es6/try-catch-for-of.js
Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index
d993d20595de80567f267e8410dfe77edcbe450c..51d665ad5596c98062ee5bf392e12e360f44aedc
100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3617,13 +3617,13 @@ Statement* Parser::ParseForStatement(ZoneList<const
AstRawString*>* labels,
DCHECK(parsing_result.declarations.length() == 1);
Block* init_block = nullptr;
+ const AstRawString* name = parsing_result.SingleName();
// special case for legacy for (var/const x =.... in)
if (!IsLexicalVariableMode(parsing_result.descriptor.mode) &&
parsing_result.declarations[0].initializer != nullptr) {
VariableProxy* single_var = scope_->NewUnresolved(
- factory(), parsing_result.SingleName(), Variable::NORMAL,
- each_beg_pos, each_end_pos);
+ factory(), name, Variable::NORMAL, each_beg_pos,
each_end_pos);
init_block = factory()->NewBlock(
nullptr, 2, true, parsing_result.descriptor.declaration_pos);
init_block->AddStatement(
@@ -3636,6 +3636,16 @@ Statement* Parser::ParseForStatement(ZoneList<const
AstRawString*>* labels,
zone());
}
+ // For some reason, we can't bind over catch parameters in for-of.
+ if (mode == ForEachStatement::ITERATE &&
+ IsDeclaredAsCatchParameter(scope_, name)) {
+
ParserTraits::ReportMessageAt(parsing_result.first_initializer_loc,
+ MessageTemplate::kVarRedeclaration,
+ name);
+ *ok = false;
+ return nullptr;
+ }
+
// Rewrite a for-in/of statement of the form
//
// for (let/const/var x in/of e) b
@@ -4816,6 +4826,13 @@ void Parser::CheckConflictingVarDeclarations(Scope*
scope, bool* ok) {
}
}
+bool Parser::IsDeclaredAsCatchParameter(Scope* scope,
+ const AstRawString* name) {
+ for (; scope != NULL; scope = scope->outer_scope())
+ if (scope->is_catch_scope() && scope->LookupLocal(name)) return true;
+ return false;
+}
+
//
----------------------------------------------------------------------------
// Parser support
Index: src/parser.h
diff --git a/src/parser.h b/src/parser.h
index
effa562e2bc9d25862bd37e8d57ac6ae9f87ee57..9964e20d005e27d633989830116dba4b6983ae3f
100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -1134,6 +1134,11 @@ class Parser : public ParserBase<ParserTraits> {
// hoisted over such a scope.
void CheckConflictingVarDeclarations(Scope* scope, bool* ok);
+ // Catch statements introduce some odd scoping constraints. You can't
put a
+ // for-of statement that tries to bind to the same identifier as the
+ // CatchParameter, for example. (see B.3.5)
+ bool IsDeclaredAsCatchParameter(Scope* scope, const AstRawString* name);
+
// Parser support
VariableProxy* NewUnresolved(const AstRawString* name, VariableMode
mode);
Variable* Declare(Declaration* declaration,
Index: test/mjsunit/es6/try-catch-for-of.js
diff --git a/test/mjsunit/d8-worker-spawn-worker.js
b/test/mjsunit/es6/try-catch-for-of.js
similarity index 73%
copy from test/mjsunit/d8-worker-spawn-worker.js
copy to test/mjsunit/es6/try-catch-for-of.js
index
a114d8587e0ebc7095565cfebb3634e7fdefbbe7..560aa3118a287a90148ae5b1f3365bc685602c01
100644
--- a/test/mjsunit/d8-worker-spawn-worker.js
+++ b/test/mjsunit/es6/try-catch-for-of.js
@@ -25,16 +25,30 @@
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-if (this.Worker) {
- var workerScript =
- `var w = new Worker('postMessage(42)');
- onmessage = function(parentMsg) {
- w.postMessage(parentMsg);
- var childMsg = w.getMessage();
- postMessage(childMsg);
- };`;
+// Test that we throw syntax errors when users attempt to shadow
+// try-catch bindings with for-of statements.
- var w = new Worker(workerScript);
- w.postMessage(9);
- assertEquals(42, w.getMessage());
+function testEarlyError(s) {
+ assertThrows(function() {
+ eval("(function() { " + s + " })")
+ }, SyntaxError);
+}
+
+testEarlyError("try {} catch (e) { for (var e of []) {} }");
+
+assertThrows(function () {
+ try {
+ throw null;
+ } catch (e) {
+ eval("for (var e of []) {}");
+ }
+}, SyntaxError);
+
+// For-in statements should be fine, though.
+
+try {} catch (f) { for (var f in []) {} }
+try {
+ throw null;
+} catch (g) {
+ eval("for (var g in []) {}");
}
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
