Add strict mode, sloppy mode and strong mode UseCounters

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index b54172f72c21cc6ccf320ed04877227804a5d134..8388bd13c3a4b00f0fbf54dc95f970a8210afbec 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -346,8 +346,8 @@ FunctionLiteral* Parser::DefaultConstructor(bool call_super, Scope* scope,
   FunctionKind kind = call_super ? FunctionKind::kDefaultSubclassConstructor
                                  : FunctionKind::kDefaultBaseConstructor;
   Scope* function_scope = NewScope(scope, FUNCTION_SCOPE, kind);
-  function_scope->SetLanguageMode(
-      static_cast<LanguageMode>(language_mode | STRICT));
+  SetLanguageMode(function_scope,
+                  static_cast<LanguageMode>(language_mode | STRICT));
   // Set start and end position to the same value
   function_scope->set_start_position(pos);
   function_scope->set_end_position(pos);
@@ -1049,6 +1049,8 @@ FunctionLiteral* Parser::DoParseProgram(ParseInfo* info) {
     FunctionState function_state(&function_state_, &scope_, scope,
                                  kNormalFunction, &function_factory);
 
+    // Don't count the mode in the use counters--give the program a chance

[adamk, 2015/11/04 00:53:54]

This doesn't sound right to me. I suspect it means we wouldn't measure a strict
eval as strict (see compiler.cc for where ParseInfo::language_mode_ is set).

[Dan Ehrenberg, 2015/11/04 17:55:35]

I think that's OK--the exact counting doesn't matter, it just matters whether we
have gotten that the page includes strict code and no sloppy code. I think it'd
get that right in this case.

+    // to enable script/module-wide strict/strong mode below.
     scope_->SetLanguageMode(info->language_mode());
     ZoneList<Statement*>* body = new(zone()) ZoneList<Statement*>(16, zone());
     bool ok = true;
@@ -1191,7 +1193,7 @@ FunctionLiteral* Parser::ParseLazy(Isolate* isolate, ParseInfo* info,
     if (shared_info->is_arrow()) {
       Scope* scope =
           NewScope(scope_, FUNCTION_SCOPE, FunctionKind::kArrowFunction);
-      scope->SetLanguageMode(shared_info->language_mode());
+      SetLanguageMode(scope, shared_info->language_mode());
       scope->set_start_position(shared_info->start_position());
       ExpressionClassifier formals_classifier;
       ParserFormalParameters formals(scope);
@@ -1336,13 +1338,11 @@ void* Parser::ParseStatementList(ZoneList<Statement*>* body, int end_token,
           // Strong mode implies strict mode. If there are several "use strict"
           // / "use strong" directives, do the strict mode changes only once.
           if (is_sloppy(scope_->language_mode())) {
-            scope_->SetLanguageMode(
-                static_cast<LanguageMode>(scope_->language_mode() | STRICT));
+            EnableLanguageMode(STRICT);

[adamk, 2015/11/03 22:23:15]

I was expecting this function to be the only place touched in this patch. I
guess it depends on what exactly we're trying to measure: the use of directives,
or just the existence of strict code. I was expecting the former (which would
exclude, e.g., classes from this measurement).

[Dan Ehrenberg, 2015/11/04 17:55:35]

I think it'd be more useful to measure how often strict code happens. This would
help guide both our optimization and language design efforts. Not sure exactly
why we'd care about the explicit declarations.

[adamk, 2015/11/04 20:18:53]

I was assuming we were trying to measure developer intentions, I guess. Maybe
rossberg's a better reviewer for this patch, given it was his idea originally to
add a use strict counter? It seems there are a variety of reasons we might want
these counters and what we want to measure with them, and different ideas would
result in different approaches.

           }
 
           if (use_strong_found) {
-            scope_->SetLanguageMode(
-                static_cast<LanguageMode>(scope_->language_mode() | STRONG));
+            EnableLanguageMode(STRONG);
             if (IsClassConstructor(function_state_->kind())) {
               // "use strong" cannot occur in a class constructor body, to avoid
               // unintuitive strong class object semantics.
@@ -1378,11 +1378,18 @@ void* Parser::ParseStatementList(ZoneList<Statement*>* body, int end_token,
           // incremented after parsing is done.
           ++use_counts_[v8::Isolate::kUseAsm];
           scope_->SetAsmModule();
+        } else {
+          // Should not change mode, but will increment UseCounter
+          // if appropriate. Ditto usages below.
+          EnableLanguageMode(SLOPPY);

[adamk, 2015/11/04 00:53:54]

I think the weird thing about this, to me, is that it's a no-op except for the
use-counter. Same goes for the EnableLanguageMode things below.

[Dan Ehrenberg, 2015/11/04 17:55:35]

Well, I could write the code instead like

else if (is_sloppy(language_mode_)) {
  ++use_counters[kSloppyMode];
}

Just a matter of taste which is cleaner. I thought this way was cleaner (it's
definitely less code, and I though the comment cleared it up) but I see how it
can be viewed otherwise.

         }
       } else {
         // End of the directive prologue.
         directive_prologue = false;
+        EnableLanguageMode(SLOPPY);
       }
+    } else {
+      EnableLanguageMode(SLOPPY);
     }
 
     body->Add(stat, zone());
@@ -1459,8 +1466,7 @@ void* Parser::ParseModuleItemList(ZoneList<Statement*>* body, bool* ok) {
   //    ModuleItem*
 
   DCHECK(scope_->is_module_scope());
-  scope_->SetLanguageMode(
-      static_cast<LanguageMode>(scope_->language_mode() | STRICT));
+  EnableLanguageMode(STRICT);
 
   while (peek() != Token::EOS) {
     Statement* stat = ParseModuleItem(CHECK_OK);
@@ -4183,7 +4189,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
                           declaration_scope != original_declaration_scope)
                      ? NewScope(declaration_scope, FUNCTION_SCOPE, kind)
                      : NewScope(scope_, FUNCTION_SCOPE, kind);
-  scope->SetLanguageMode(language_mode);
+  SetLanguageMode(scope, language_mode);
   ZoneList<Statement*>* body = NULL;
   int arity = -1;
   int materialized_literal_count = -1;
@@ -4415,7 +4421,7 @@ void Parser::SkipLazyFunctionBody(int* materialized_literal_count,
       total_preparse_skipped_ += scope_->end_position() - function_block_pos;
       *materialized_literal_count = entry.literal_count();
       *expected_property_count = entry.property_count();
-      scope_->SetLanguageMode(entry.language_mode());
+      SetLanguageMode(scope_, entry.language_mode());

[adamk, 2015/11/04 00:53:54]

Why isn't this EnableLanguageMode()?

[Dan Ehrenberg, 2015/11/04 17:55:35]

Because it doesn't bitor the language mode with the scope_'s previous mode, and
I didn't read enough of the surrounding code to know whether or not I can prove
that that doesn't matter.

       if (entry.uses_super_property()) scope_->RecordSuperPropertyUsage();
       if (entry.calls_eval()) scope_->RecordEvalCall();
       return;
@@ -4451,7 +4457,7 @@ void Parser::SkipLazyFunctionBody(int* materialized_literal_count,
   total_preparse_skipped_ += scope_->end_position() - function_block_pos;
   *materialized_literal_count = logger.literals();
   *expected_property_count = logger.properties();
-  scope_->SetLanguageMode(logger.language_mode());
+  SetLanguageMode(scope_, logger.language_mode());

[adamk, 2015/11/04 00:53:54]

Same here as above, seems like this is EnableLanguageMode.

I think this is the reason that "Set" and "Enable" aren't helpful to me as names
for these operations.

[Dan Ehrenberg, 2015/11/04 17:55:35]

EnableLanguageMode means "the language mode should be at least this strong"--it
bitor's the language mode together with the scope's mode. The code takes about
two lines and it's a little verbose. I was thinking of making a single-argument
version of SetLanguageMode which got the scope from scope_ but it seemed
unnecessary.

   if (logger.uses_super_property()) {
     scope_->RecordSuperPropertyUsage();
   }
@@ -4744,7 +4750,7 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
   if (!parameters.is_simple) {
     DCHECK_NOT_NULL(inner_scope);
     DCHECK_EQ(body, inner_block->statements());
-    scope_->SetLanguageMode(inner_scope->language_mode());
+    SetLanguageMode(scope_, inner_scope->language_mode());
     Block* init_block = BuildParameterInitializationBlock(parameters, CHECK_OK);
     DCHECK_NOT_NULL(init_block);
 
@@ -4848,8 +4854,7 @@ ClassLiteral* Parser::ParseClassLiteral(const AstRawString* name,
 
   Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);
   BlockState block_state(&scope_, block_scope);
-  scope_->SetLanguageMode(
-      static_cast<LanguageMode>(scope_->language_mode() | STRICT));
+  EnableLanguageMode(STRICT);
   scope_->SetScopeName(name);
 
   VariableProxy* proxy = NULL;
@@ -6386,5 +6391,27 @@ Expression* Parser::SpreadCallNew(Expression* function,
 
   return factory()->NewCallRuntime(Context::REFLECT_CONSTRUCT_INDEX, args, pos);
 }
+
+
+void Parser::SetLanguageMode(Scope* scope, LanguageMode mode) {
+  v8::Isolate::UseCounterFeature feature;
+  if (is_sloppy(mode))
+    feature = v8::Isolate::kSloppyMode;
+  else if (is_strict(mode))
+    feature = v8::Isolate::kStrictMode;
+  else if (is_strong(mode))

[adamk, 2015/11/03 22:23:15]

This has to go before is_strict, since strong mode implies strict mode.

[Dan Ehrenberg, 2015/11/04 17:55:36]

Yeah, this needs to be fixed.

[Dan Ehrenberg, 2015/11/04 20:27:53]

Fixed

+    feature = v8::Isolate::kStrongMode;
+  else
+    UNREACHABLE();
+  ++use_counts_[feature];
+  scope->SetLanguageMode(mode);
+}
+
+
+void Parser::EnableLanguageMode(LanguageMode mode) {
+  SetLanguageMode(scope_,
+                  static_cast<LanguageMode>(scope_->language_mode() | mode));
+}
+
 }  // namespace internal
 }  // namespace v8