Track usage of "this" and "arguments" in Scope

test/cctest/test-parsing.cc

Index: test/cctest/test-parsing.cc
diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc
index cf037ea141a988b0f31ba32de6c27f858a95a97b..c2c38acc3a45a5003e77b48df24bcd305a6f4e02 100644
--- a/test/cctest/test-parsing.cc
+++ b/test/cctest/test-parsing.cc
@@ -915,6 +915,106 @@ static int Utf8LengthHelper(const char* s) {
 }
 
 
+TEST(ScopeUsesThisAndArguments) {
+  static const struct {
+    const char* prefix;
+    const char* suffix;
+  } surroundings[] = {
+    { "function f() {", "}" },
+    { "function f() { \"use strict\";", "}" },
+    { "\"use strict\"; function f() {", "}" },
+    { "f = () => {", "}" },
+    { "f = () => { \"use strict\";", "}" },

[rossberg, 2014/10/15 12:56:32]

It can't harm to keep these variants, you never know...

[aperez, 2014/10/15 17:09:51]

The issue with keeping those is that making the function
strict enables block scoping, so the expected test results
vary for some the cases below, for example in thi one:

   { "if (foo()) { this.f() }",
     true, false, false, false },

It would generate those two inputs:

   function () { if (foo()) { this.f() } }
   function () { "use strict"; if (foo()) { this.f() } }

For the first one “uses_this” is expected to be “true”
(and “inner_uses_this” expected to be “false”), but for
the second one it has to be “false” because the usage
of “this” is in the block scope of the “if”...

This is why I have removed the prefixes which introduce
add the “"use strict";” and added some extra cases at the
end of “source_data” that enable strict mode.

+    { "\"use strict\"; f = () => {", "}" },
+  };
+
+  static const struct {
+    const char* body;
+    bool uses_this;
+    bool uses_arguments;
+    bool inner_uses_this;
+    bool inner_uses_arguments;
+  } source_data[] = {
+    { "",
+      false, false, false, false },
+    { "return this;",
+      true, false, false, false },
+    { "return arguments;",
+      false, true, false, false },
+    { "return arguments[0];",
+      false, true, false, false },
+    { "return this + arguments[0];",
+      true, true, false, false },
+    { "return () => this;",
+      false, false, true, false },
+    { "var x = function () { this.foo = 42 };",

[arv (Not doing code reviews), 2014/09/04 16:41:21]

Why do we need to track the this reference inside a function expression?

[rossberg, 2014/09/10 06:33:56]

Indeed, this seems wrong. Although it should be true for an arrow function.

+      false, false, true, false },
+    { "this.foo = 42;",
+      true, false, false, false },
+    { "this.foo();",
+      true, false, false, false },
+    { "if (foo()) { this.f() }",
+      true, false, false, false },
+    { "if (arguments.length) { this.f() }",
+      true, true, false, false },
+    { "while (true) { this.f() }",
+      true, false, false, false },
+    { "if (true) { while (true) this.foo(arguments) }",
+      true, true, false, false },
+    { "var f = function () { return this }",
+      false, false, true, false },
+    { "var f = function () { return arguments[0] }",
+      false, false, false, true },
+  };

[rossberg, 2014/09/10 06:33:56]

We probably need some tests with block scopes as well, e.g.

  "while (true) { let x; this, arguments; }"  =>  false, false, true, true

+
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::Factory* factory = isolate->factory();
+
+  v8::HandleScope handles(CcTest::isolate());
+  v8::Handle<v8::Context> context = v8::Context::New(CcTest::isolate());
+  v8::Context::Scope context_scope(context);
+
+  isolate->stack_guard()->SetStackLimit(GetCurrentStackPosition() - 128 * 1024);
+
+  for (unsigned j = 0; j < arraysize(surroundings); ++j) {
+    for (unsigned i = 0; i < arraysize(source_data); ++i) {
+      int kProgramByteSize = i::StrLength(surroundings[j].prefix) +
+                             i::StrLength(surroundings[j].suffix) +
+                             i::StrLength(source_data[i].body);
+      i::ScopedVector<char> program(kProgramByteSize + 1);
+      i::SNPrintF(program, "%s%s%s", surroundings[j].prefix,
+                  source_data[i].body, surroundings[j].suffix);
+      i::Handle<i::String> source =
+          factory->NewStringFromUtf8(i::CStrVector(program.start()))
+              .ToHandleChecked();
+      i::Handle<i::Script> script = factory->NewScript(source);
+      i::CompilationInfoWithZone info(script);
+      i::Parser::ParseInfo parse_info = {isolate->stack_guard()->real_climit(),
+                                         isolate->heap()->HashSeed(),
+                                         isolate->unicode_cache()};
+      i::Parser parser(&info, &parse_info);
+      parser.set_allow_arrow_functions(true);
+      info.MarkAsGlobal();
+      parser.Parse();
+      CHECK(i::Rewriter::Rewrite(&info));
+      CHECK(i::Scope::Analyze(&info));
+      CHECK(info.function() != NULL);
+
+      i::Scope* scope = info.function()->scope();
+      CHECK(scope->is_global_scope());
+      CHECK_EQ(1, scope->inner_scopes()->length());
+
+      i::Scope* inner_scope = scope->inner_scopes()->at(0);
+      CHECK_EQ(source_data[i].uses_this, inner_scope->uses_this());
+      CHECK_EQ(source_data[i].uses_arguments, inner_scope->uses_arguments());
+      CHECK_EQ(source_data[i].inner_uses_this, inner_scope->inner_uses_this());
+      CHECK_EQ(source_data[i].inner_uses_arguments,
+               inner_scope->inner_uses_arguments());
+    }
+  }
+}
+
+
 TEST(ScopePositions) {
   v8::internal::FLAG_harmony_scoping = true;