Add comments about lazy parsing and lazy compilation.

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 409059778a6a5f44606f6d0bed20af710cb797ec..534a37b08fdad890d1fb89c5f5f264a60026a17a 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3973,8 +3973,11 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
       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.
@@ -3984,12 +3987,31 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     //   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) {
@@ -4013,7 +4035,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
           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
@@ -4050,7 +4072,9 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
       }
     }
 
-    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) {