[parser] Skipping inner funcs: Fix hoisting.

The catch variable is a special VAR-mode variable which is not in a declaration
scope. Normally creating such a variable is not possible with DeclareVariable,
but Parser bypasses it by calling DeclareLocal directly (which doesn't have the
hoisting check).

PreParser used to cut corners and declare the catch variable as a LET-mode
variable to prevent hoisting.

But since LET and VAR variables behave differently when deciding whether they
block sloppy block function hoisting, that approach doesn't fly.

BUG=v8:5516,chromium:771474

Change-Id: Ic6f5f4996416c9fa59132725c8b0b6b570c72f48
Reviewed-on: https://chromium-review.googlesource.com/700634
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48308}

src/ast/scopes.cc

@@ -1207,12 +1207,7 @@ Variable* Scope::DeclareVariableName(const AstRawString* name,
   }
   DCHECK(!is_with_scope());
   DCHECK(!is_eval_scope());
-  // Unlike DeclareVariable, DeclareVariableName allows declaring variables in
-  // catch scopes: Parser::RewriteCatchPattern bypasses DeclareVariable by
-  // calling DeclareLocal directly, and it doesn't make sense to add a similar
-  // bypass mechanism for PreParser.
-  DCHECK(is_declaration_scope() || (IsLexicalVariableMode(mode) &&
-                                    (is_block_scope() || is_catch_scope())));
+  DCHECK(is_declaration_scope() || IsLexicalVariableMode(mode));
   DCHECK(scope_info_.is_null());
 
   // Declare the variable in the declaration scope.
@@ -1238,6 +1233,19 @@ Variable* Scope::DeclareVariableName(const AstRawString* name,
   }
 }
 
+void Scope::DeclareCatchVariableName(const AstRawString* name) {
+  DCHECK(!already_resolved_);
+  DCHECK(GetDeclarationScope()->is_being_lazily_parsed());
+  DCHECK(is_catch_scope());
+  DCHECK(scope_info_.is_null());
+
+  if (FLAG_preparser_scope_analysis) {
+    Declare(zone(), name, VAR);
+  } else {
+    variables_.DeclareName(zone(), name, VAR);
+  }
+}
+
 void Scope::AddUnresolved(VariableProxy* proxy) {
   DCHECK(!already_resolved_);
   DCHECK(!proxy->is_resolved());

src/ast/scopes.h

@@ -201,6 +201,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
 
   // The return value is meaningful only if FLAG_preparser_scope_analysis is on.
   Variable* DeclareVariableName(const AstRawString* name, VariableMode mode);
+  void DeclareCatchVariableName(const AstRawString* name);
 
   // Declarations list.
   ThreadedList<Declaration>* declarations() { return &decls_; }

src/parsing/preparser.h

@@ -1012,11 +1012,7 @@ class PreParser : public ParserBase<PreParser> {
       if (catch_name == nullptr) {
         catch_name = ast_value_factory()->dot_catch_string();
       }
-      // Unlike in the parser, we need to declare the catch variable as LET
-      // variable, so that it won't get hoisted out of the scope. (Parser uses
-      // DeclareLocal instead of DeclareVariable to prevent hoisting.) Another
-      // solution would've been to add DeclareLocalName just for this purpose.
-      catch_info->scope->DeclareVariableName(catch_name, LET);
+      catch_info->scope->DeclareCatchVariableName(catch_name);
 
       if (catch_info->pattern.variables_ != nullptr) {
         for (auto variable : *catch_info->pattern.variables_) {

test/cctest/parsing/test-preparser.cc

@@ -462,6 +462,9 @@ TEST(PreParserScopeAnalysis) {
       {"if (true) { function *f1() {} }"},
       {"if (true) { async function f1() {} }"},
 
+      // (Potentially sloppy) block function shadowing a catch variable.
+      {"try { } catch(var1) { if (true) { function var1() {} } }"},
+
       // Simple parameters.
       {"var1", ""},
       {"var1", "var1;"},

test/mjsunit/skipping-inner-functions.js

@@ -338,3 +338,17 @@ TestSkippableFunctionInForOfHeaderAndBody();
   }
   lazy();
 })();
+
+(function TestSloppyBlockFunctionShadowingCatchVariable() {
+  // Regression test for
+  // https://bugs.chromium.org/p/chromium/issues/detail?id=771474
+  function lazy() {
+    try {
+    } catch (my_var) {
+      if (false) {
+        function my_var() { }
+      }
+    }
+  }
+  lazy();
+})();