[module] Track script "module code" status

test/cctest/test-api.cc

Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index e2773ef3373bed5afbbf4b0a2e7a7c5a82ba84bb..8758eb5c459ffc680b9b5d53f91ab6b9837230af 100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -23604,6 +23604,32 @@ TEST(ScriptPositionInfo) {
   CHECK(!script1->GetPositionInfo(220384, &info, script1->NO_OFFSET));
 }
 
+TEST(ScriptIsModule) {
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+  const char* url = "http://www.foo.com/foo.js";
+  v8::ScriptOrigin origin(v8_str(url), v8::Integer::New(isolate, 1));
+  Local<Script> localScript;
+  i::Handle<i::SharedFunctionInfo> fnInfo;
+
+  v8::ScriptCompiler::Source script_source1(v8_str("void 1;\n"), origin);
+  localScript = v8::ScriptCompiler::Compile(env.local(), &script_source1)
+                    .ToLocalChecked();
+  fnInfo = i::Handle<i::SharedFunctionInfo>::cast(
+      v8::Utils::OpenHandle(*localScript->GetUnboundScript()));
+  i::Handle<i::Script> globalScript(i::Script::cast(fnInfo->script()));
+  CHECK_EQ(false, globalScript->is_module());
+
+  v8::ScriptCompiler::Source script_source2(v8_str("void 2;\n"), origin);

[mike3, 2016/06/12 19:54:48]

A distinct Source object for each test case is necessary to avoid hitting the
compilation cache. I expect this is an indication that CompilationCache should
also be aware of the `is_module` flag, but I wanted to confirm with you before
making that change.

[vogelheim, 2016/06/14 15:20:50]

This is a good catch! I hadn't thought of that... After discussing with Adam:
Yes, the CompilationCache needs to depend on is_module.

Since, at present, this isn't being used, the easiest implementation would be to
just check for !is_module() when the compilation cache is queried - that'd
preserve correctness. We could then add that flag to the compilation cache when
modules are fully implemented.

--------------

[We briefly discussed how to add it, whether we could either merge that flag
into the LanguageMode, or if we should introduce some sort of
AllFlagsThatAffectCompilationStruct. The agreement was that the former doesn't
make sense since those are orthogonal; the latter might make sense if we
expected more of these flags in the future, but we don't. Hence just adding a
boolean to the compilation cache lookup would likely be best & easiest.]

[mike3, 2016/06/19 16:59:00]

Much as I enjoy seeing my commit count increase (petty, I know), I don't mind
putting in the extra effort to get it right the first time.

When it comes to LanguageMode, I don't believe "non-strict", "strict" and
"module" are orthogonal. "module" is a superset of "strict" in terms of
capabilities, so even though the specification terminology makes a distinction,
there isn't a practical benefit in allowing "strictness" to vary independently
from "moduleness". In fact, it is likely more convenient to couple the
definitions like this because it means you never have to worry about entering
an invalid "non-strict module" state.

Implementing this might not be so bad, either. The global `is_strict` function
seems to be the primary interface for checking strictness, so if that were
updated to return `true` for module code, then that might cover most cases.

Of course, I remain ignorant of huge sections of the code base, and I expect
I'm over-simplifying things.

+  localScript = v8::ScriptCompiler::CompileModule(env.local(), &script_source2)
+                    .ToLocalChecked();
+  fnInfo = i::Handle<i::SharedFunctionInfo>::cast(
+      v8::Utils::OpenHandle(*localScript->GetUnboundScript()));
+  i::Handle<i::Script> moduleScript(i::Script::cast(fnInfo->script()));
+  CHECK_EQ(true, moduleScript->is_module());
+}
+
 void CheckMagicComments(Local<Script> script, const char* expected_source_url,
                         const char* expected_source_mapping_url) {
   if (expected_source_url != NULL) {