Add comments about lazy parsing and lazy compilation.

src/parser.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 3955 matching lines @@
       VariableMode fvar_mode = is_extended_mode() ? CONST_HARMONY : CONST;
       fvar = new(zone()) Variable(scope_,
          function_name, fvar_mode, true /* is valid LHS */,
          Variable::NORMAL, kCreatedInitialized, Interface::NewConst());
       VariableProxy* proxy = factory()->NewVariableProxy(fvar);
       VariableDeclaration* fvar_declaration = factory()->NewVariableDeclaration(
           proxy, fvar_mode, scope_, RelocInfo::kNoPosition);
       scope_->DeclareFunctionVar(fvar_declaration);
     }
 
-    // Determine whether the function will be lazily compiled.
-    // The heuristics are:
+    // Determine if the function can be parsed lazily. Lazy parsing is different
+    // from lazy compilation; we need to parse more eagerly than we compile.
+
+    // We can only parse lazily if we also compile lazily. The heuristics for
+    // lazy compilation are:
     // - It must not have been prohibited by the caller to Parse (some callers
     //   need a full AST).
     // - The outer scope must allow lazy compilation of inner functions.
     // - The function mustn't be a function expression with an open parenthesis
     //   before; we consider that a hint that the function will be called
     //   immediately, and it would be a waste of time to make it lazily
     //   compiled.
     // These are all things we can know at this point, without looking at the
     // function itself.
-    bool is_lazily_compiled = (mode() == PARSE_LAZILY &&
-                               scope_->AllowsLazyCompilation() &&
-                               !parenthesized_function_);
+
+    // In addition, we need to distinguish between these cases:
+    // (function foo() {
+    //   bar = function() { return 1; }
+    //  })();
+    // and
+    // (function foo() {
+    //   var a = 1;
+    //   bar = function() { return a; }
+    //  })();
+
+    // Now foo will be parsed eagerly and compiled eagerly (optimization: assume
+    // parenthesis before the function means that it will be called
+    // immediately). The inner function *must* be parsed eagerly to resolve the
+    // possible reference to the variable in foo's scope. However, it's possible
+    // that it will be compiled lazily.
+
+    // To make this additional case work, both Parser and PreParser implement a
+    // logic where only top-level functions will be parsed lazily.
+    bool is_lazily_parsed = (mode() == PARSE_LAZILY &&
+                             scope_->AllowsLazyCompilation() &&
+                             !parenthesized_function_);
     parenthesized_function_ = false;  // The bit was set for this function only.
 
-    if (is_lazily_compiled) {
+    if (is_lazily_parsed) {
       int function_block_pos = position();
       FunctionEntry entry;
       if (pre_parse_data_ != NULL) {
         // If we have pre_parse_data_, we use it to skip parsing the function
         // body.  The preparser data contains the information we need to
         // construct the lazy function.
         entry = pre_parse_data()->GetFunctionEntry(function_block_pos);
         if (entry.is_valid()) {
           if (entry.end_pos() <= function_block_pos) {
             // End position greater than end of stream is safe, and hard
             // to check.
             ReportInvalidPreparseData(function_name, CHECK_OK);
           }
           scanner()->SeekForward(entry.end_pos() - 1);
 
           scope->set_end_position(entry.end_pos());
           Expect(Token::RBRACE, CHECK_OK);
           isolate()->counters()->total_preparse_skipped()->Increment(
               scope->end_position() - function_block_pos);
           materialized_literal_count = entry.literal_count();
           expected_property_count = entry.property_count();
           scope_->SetLanguageMode(entry.language_mode());
         } else {
-          is_lazily_compiled = false;
+          is_lazily_parsed = false;

[Sven Panne, 2014/02/19 09:51:11]

Mind explosion... o_O Can we add a comment here, too? I don't have the slightest
clue why we set this to false here...

[marja, 2014/02/19 12:19:47]

This is just an error case, where we do have preparse data but fail to get
information for this particular function from it. So this is a safety net to
bail out and parse eagerly in that case. I think it's not important that we
parse eagerly, but we might as well do the "else" branch below ("partially parse
the function without building the AST") in that error case. I don't think
there's any deeper wisdom here. But ofc I cannot be 100 % sure since I didn't
write the code and there is no comment.. ;)

There *used* to be a comment in this branch but it was removed by
https://codereview.chromium.org/8662037/diff/11001/src/parser.cc . The comment
was: "There is no preparser data for the function, we will not lazily compile
after all.", but if that's true, it's pretty bad. That would mean PreParser
knows something about lazy compilation that Parser doesn't and that's just wrong
(Parser should always be the more clever one).

Another piece of code archaeology: https://codereview.chromium.org/7565003 added
the original logic, and this CL actually claims that 1) PreParser and Parser
*might* do the lazy analysis differently (huh?) 2) PreParser should be regarded
as an authority if they differ (why on earth?) and 3) if PreParser thinks
something is lazy, and Parser think it's not, all further laziness will fail
since we never advance the preparse data. (This is true.)

Okay, now my mind is blown, too. o.O If even the code authors are unsure whether
the PreParser laziness analysis and Parser laziness analysis can produce
different results, and only one of the 2 directions is handled properly, it's
pretty bad.

Anyhow, I added a comment reflecting my current understanding of how this works,
not trying to make claims for which I'm unsure if they're true.

What we'd really need is to put some kind of metric here, to see if this branch
is ever hit on the wild.

         }
       } else {
         // With no preparser data, we partially parse the function, without
         // building an AST. This gathers the data needed to build a lazy
         // function.
         SingletonLogger logger;
         PreParser::PreParseResult result = LazyParseFunctionLiteral(&logger);
         if (result == PreParser::kPreParseStackOverflow) {
           // Propagate stack overflow.
           set_stack_overflow();
@@ skipping 16 matching lines @@
         scope->set_end_position(logger.end());
         Expect(Token::RBRACE, CHECK_OK);
         isolate()->counters()->total_preparse_skipped()->Increment(
             scope->end_position() - function_block_pos);
         materialized_literal_count = logger.literals();
         expected_property_count = logger.properties();
         scope_->SetLanguageMode(logger.language_mode());
       }
     }
 
-    if (!is_lazily_compiled) {
+    if (!is_lazily_parsed) {
+      // Everything inside an eagerly parsed function will be parsed eagerly
+      // (see comment above).
       ParsingModeScope parsing_mode(this, PARSE_EAGERLY);
       body = new(zone()) ZoneList<Statement*>(8, zone());
       if (fvar != NULL) {
         VariableProxy* fproxy = scope_->NewUnresolved(
             factory(), function_name, Interface::NewConst());
         fproxy->BindTo(fvar);
         body->Add(factory()->NewExpressionStatement(
             factory()->NewAssignment(fvar_init_op,
                                      fproxy,
                                      factory()->NewThisFunction(pos),
@@ skipping 1301 matching lines @@
       ASSERT(info()->isolate()->has_pending_exception());
     } else {
       result = ParseProgram();
     }
   }
   info()->SetFunction(result);
   return (result != NULL);
 }
 
 } }  // namespace v8::internal