Refactor how directives e.g. strictness are handled for lazy

Summary:
The current approach of handling directives limits what we can do,
and the logic of computing strictness is duplicated and confusing.

The goal of this diff is to first achieve parity with the previous
strictness behaviors, and then migrate/implement other/new
directives to this new approach in upper diffs.

### Status Quo

Parser is reponsible to set strictness to ESTree Node
1. preParse records strictness for every function
2. lazyParse recovers info from preParse to maintain the invariant
that parser can still set the strictness correctly (D25318752)

Sematic validator then recompute the strictness again and
verify/update the ESTree Node!
1. for non-lazy fn, it scan its body node to set FC
2. for lazy fn, it reuses the node strictness set by lazyParse to
set FC (D25318752)

### Proposed Changes

Parser is no longer responsible for setting strictness to node.
It needs to know strictness only for the parsing purpose.
1. preParse collects seen directives for every function
2. lazyParse reconstruct the directive nodes in lazy function body

Semantic validator is the source of truth for compute strictness
(and any other directive information) going forward.

(Note: this ignores all push blocking failures!)

Reviewed By: avp

Differential Revision: D29035883

fbshipit-source-id: fb79e5d1750c338cd3d3d7dcbf4659c24edf87e8

include/hermes/Parser/PreParser.h

@@ -9,10 +9,12 @@
 #define HERMES_PARSER_PREPARSER_H
 
 #include "llvh/ADT/DenseMap.h"
+#include "llvh/ADT/SmallString.h"
 #include "llvh/ADT/SmallVector.h"
 #include "llvh/Support/SMLoc.h"
 
 namespace hermes {
+
 namespace parser {
 using llvh::SMLoc;
 
@@ -40,6 +42,11 @@ struct PreParsedFunctionInfo {
 
   /// Whether or not the function is strict.
   bool strictMode;
+
+  /// List of directives.
+  /// Note that we can't use \c UniqueString* here since they are
+  /// arena-allocated and reclaimed between parse passes.
+  llvh::SmallVector<llvh::SmallString<24>, 1> directives{};
 };
 
 /// Per buffer information from preparsing.

lib/AST/SemanticValidator.cpp

@@ -70,9 +70,6 @@ bool SemanticValidator::doFunction(Node *function, bool strict) {
 }
 
 void SemanticValidator::visit(ProgramNode *node) {
-#ifndef NDEBUG
-  strictnessIsPreset_ = node->strictness != Strictness::NotSet;
-#endif
   FunctionContext newFuncCtx{this, astContext_.isStrictMode(), node};
 
   scanDirectivePrologue(node->_body);
@@ -637,10 +634,9 @@ void SemanticValidator::visitFunction(
   // arrow functions).
   if (auto *bodyNode = dyn_cast<ESTree::BlockStatementNode>(body)) {
     if (bodyNode->isLazyFunctionBody) {
-      // If it is a lazy function body, then scanning the directive prologue
-      // won't accomplish anything. Set the strictness directly via the
-      // stored strictness (which was populated based on preparsing data).
-      curFunction()->strictMode = ESTree::isStrict(node->strictness);
+      // If it is a lazy function body, then the directive nodes in the body are
+      // fabricated without location, so don't set useStrictNode.
+      scanDirectivePrologue(bodyNode->_body);
     } else {
       useStrictNode = scanDirectivePrologue(bodyNode->_body);
     }
@@ -900,12 +896,6 @@ void SemanticValidator::validateAssignmentTarget(const Node *node) {
 
 void SemanticValidator::updateNodeStrictness(FunctionLikeNode *node) {
   auto strictness = ESTree::makeStrictness(curFunction()->strictMode);
-  // Only verify the strictness if there are no errors. Otherwise it is not
-  // possible to ensure that it is correct.
-  assert(
-      (sm_.getErrorCount() || !strictnessIsPreset_ ||
-       node->strictness == strictness) &&
-      "Preset strictness is different from detected strictness");
   node->strictness = strictness;
 }
 

lib/AST/SemanticValidator.h

@@ -80,16 +80,6 @@ class SemanticValidator {
   /// the AST. False if we just want to validate the AST.
   bool compile_;
 
-#ifndef NDEBUG
-  /// Our parser detects strictness and initializes the flag in every node,
-  /// but if we are reading an external AST, we must look for "use strict" and
-  /// initialize the flag ourselves here.
-  /// For consistency we always perform the detection, but in debug mode we also
-  /// want to ensure that our results match what the parser generated. This
-  /// flag indicates whether strictness is preset or not.
-  bool strictnessIsPreset_{false};
-#endif
-
   /// The maximum AST nesting level. Once we reach it, we report an error and
   /// stop.
   static constexpr unsigned MAX_RECURSION_DEPTH =
@@ -235,7 +225,7 @@ class SemanticValidator {
   void
   visitFunction(FunctionLikeNode *node, Node *id, NodeList &params, Node *body);
 
-  /// Scan a list of directives in the beginning of a program of function
+  /// Scan a list of directives in the beginning of a program or function
   /// (see ES5.1 4.1 - a directive is a statement consisting of a single
   /// string literal).
   /// Update the flags in the function context to reflect the directives. (We
@@ -265,9 +255,7 @@ class SemanticValidator {
   /// (used by elision).
   void validateAssignmentTarget(const Node *node);
 
-  /// A debugging method to set the strictness of a function-like node to
-  /// the curent strictness, asserting that it doesn't change if it had been
-  /// preset.
+  /// Set the strictness of a function-like node to the current strictness.
   void updateNodeStrictness(FunctionLikeNode *node);
 
   /// Get the LabelDecorationBase depending on the node type.

lib/Parser/JSParserImpl-flow.cpp

@@ -530,7 +530,7 @@ Optional<ESTree::Node *> JSParserImpl::parseDeclareClassFlow(SMLoc start) {
   advance(JSLexer::GrammarContext::Type);
 
   // NOTE: Class definition is always strict mode code.
-  SaveStrictMode saveStrictMode{this};
+  SaveStrictModeAndSeenDirectives saveStrictMode{this};
   setStrictMode(true);
 
   if (!need(

lib/Parser/JSParserImpl-ts.cpp

@@ -776,7 +776,7 @@ Optional<ESTree::Node *> JSParserImpl::parseTSNamespaceDeclaration() {
 
   while (!check(TokenKind::r_brace)) {
     auto optMember =
-        parseStatementListItem(Param{}, false, AllowImportExport::Yes, members);
+        parseStatementListItem(Param{}, AllowImportExport::Yes, members);
     if (!optMember)
       return None;
   }

lib/Parser/JSParserImpl.cpp

@@ -295,6 +295,7 @@ bool JSParserImpl::eatSemi(bool optional) {
 }
 
 void JSParserImpl::processDirective(UniqueString *directive) {
+  seenDirectives_.push_back(directive);
   if (directive == useStrictIdent_)
     setStrictMode(true);
   if (directive == useStaticBuiltinIdent_)
@@ -313,7 +314,7 @@ bool JSParserImpl::recursionDepthExceeded() {
 
 Optional<ESTree::ProgramNode *> JSParserImpl::parseProgram() {
   SMLoc startLoc = tok_->getStartLoc();
-  SaveStrictMode saveStrict{this};
+  SaveStrictModeAndSeenDirectives saveStrictModeAndSeenDirectives{this};
   ESTree::NodeList stmtList;
 
   if (!parseStatementList(
@@ -328,7 +329,6 @@ Optional<ESTree::ProgramNode *> JSParserImpl::parseProgram() {
       startLoc,
       endLoc,
       new (context_) ESTree::ProgramNode(std::move(stmtList)));
-  program->strictness = ESTree::makeStrictness(isStrictMode());
   return program;
 }
 
@@ -467,7 +467,7 @@ Optional<ESTree::FunctionLikeNode *> JSParserImpl::parseFunctionHelper(
     return None;
   }
 
-  SaveStrictMode saveStrictMode{this};
+  SaveStrictModeAndSeenDirectives saveStrictModeAndSeenDirectives{this};
 
   // Grammar context to be used when lexing the closing brace.
   auto grammarContext =
@@ -510,7 +510,6 @@ Optional<ESTree::FunctionLikeNode *> JSParserImpl::parseFunctionHelper(
     if (!body)
       return None;
 
-    node->strictness = ESTree::makeStrictness(isStrictMode());
     return setLocation(startLoc, body.getValue(), node);
   }
 
@@ -531,7 +530,6 @@ Optional<ESTree::FunctionLikeNode *> JSParserImpl::parseFunctionHelper(
         predicate,
         isGenerator,
         isAsync);
-    decl->strictness = ESTree::makeStrictness(isStrictMode());
     node = decl;
   } else {
     auto *expr = new (context_) ESTree::FunctionExpressionNode(
@@ -543,7 +541,6 @@ Optional<ESTree::FunctionLikeNode *> JSParserImpl::parseFunctionHelper(
         predicate,
         isGenerator,
         isAsync);
-    expr->strictness = ESTree::makeStrictness(isStrictMode());
     node = expr;
   }
   return setLocation(startLoc, body, node);
@@ -666,6 +663,15 @@ Optional<ESTree::Node *> JSParserImpl::parseStatement(Param param) {
 #undef _RET
 }
 
+llvh::SmallVector<llvh::SmallString<24>, 1> JSParserImpl::copySeenDirectives()
+    const {
+  llvh::SmallVector<llvh::SmallString<24>, 1> copies;
+  for (UniqueString *directive : seenDirectives_) {
+    copies.emplace_back(directive->str());
+  }
+  return copies;
+}
+
 Optional<ESTree::BlockStatementNode *> JSParserImpl::parseFunctionBody(
     Param param,
     bool eagerly,
@@ -686,7 +692,20 @@ Optional<ESTree::BlockStatementNode *> JSParserImpl::parseFunctionBody(
       // Emulate parsing the "use strict" directive in parseBlock.
       setStrictMode(functionInfo.strictMode);
 
-      auto *body = new (context_) ESTree::BlockStatementNode({});
+      // PreParse collected directives idents into \c PreParsedFunctionInfo,
+      // iterate on them and fabricate directive nodes into the body node so
+      // the semantic validator can scan them back.
+      ESTree::NodeList stmtList;
+      for (const llvh::SmallString<24> &directive : functionInfo.directives) {
+        auto *strLit = new (context_)
+            ESTree::StringLiteralNode(lexer_.getIdentifier(directive));
+        auto *dirStmt = new (context_)
+            ESTree::ExpressionStatementNode(strLit, strLit->_value);
+        stmtList.push_back(*dirStmt);
+      }
+
+      auto *body =
+          new (context_) ESTree::BlockStatementNode(std::move(stmtList));
       body->isLazyFunctionBody = true;
       body->paramYield = paramYield_;
       body->paramAwait = paramAwait_;
@@ -700,8 +719,8 @@ Optional<ESTree::BlockStatementNode *> JSParserImpl::parseFunctionBody(
     return None;
 
   if (pass_ == PreParse) {
-    preParsed_->functionInfo[(*body)->getStartLoc()] =
-        PreParsedFunctionInfo{(*body)->getEndLoc(), isStrictMode()};
+    preParsed_->functionInfo[(*body)->getStartLoc()] = PreParsedFunctionInfo{
+        (*body)->getEndLoc(), isStrictMode(), copySeenDirectives()};
   }
 
   return body;
@@ -760,7 +779,6 @@ Optional<ESTree::Node *> JSParserImpl::parseDeclaration(Param param) {
 
 bool JSParserImpl::parseStatementListItem(
     Param param,
-    bool parseDirectives,
     AllowImportExport allowImportExport,
     ESTree::NodeList &stmtList) {
   if (checkDeclaration()) {
@@ -843,8 +861,7 @@ Optional<bool> JSParserImpl::parseStatementList(
   }
 
   while (!check(TokenKind::eof) && !checkN(until, otherUntil...)) {
-    if (!parseStatementListItem(
-            param, parseDirectives, allowImportExport, stmtList)) {
+    if (!parseStatementListItem(param, allowImportExport, stmtList)) {
       return None;
     }
   }
@@ -2518,7 +2535,7 @@ Optional<ESTree::Node *> JSParserImpl::parsePropertyAssignment(bool eagerly) {
   SMLoc startLoc = tok_->getStartLoc();
   ESTree::NodePtr key = nullptr;
 
-  SaveStrictMode saveStrictMode{this};
+  SaveStrictModeAndSeenDirectives saveStrictModeAndSeenDirectives{this};
 
   bool computed = false;
   bool generator = false;
@@ -2615,7 +2632,6 @@ Optional<ESTree::Node *> JSParserImpl::parsePropertyAssignment(bool eagerly) {
           /* predicate */ nullptr,
           false,
           false);
-      funcExpr->strictness = ESTree::makeStrictness(isStrictMode());
       funcExpr->isMethodDefinition = true;
       setLocation(startLoc, block.getValue(), funcExpr);
 
@@ -2719,7 +2735,6 @@ Optional<ESTree::Node *> JSParserImpl::parsePropertyAssignment(bool eagerly) {
           /* predicate */ nullptr,
           false,
           false);
-      funcExpr->strictness = ESTree::makeStrictness(isStrictMode());
       funcExpr->isMethodDefinition = true;
       setLocation(startLoc, block.getValue(), funcExpr);
 
@@ -2900,7 +2915,6 @@ Optional<ESTree::Node *> JSParserImpl::parsePropertyAssignment(bool eagerly) {
         /* predicate */ nullptr,
         generator,
         async);
-    funcExpr->strictness = ESTree::makeStrictness(isStrictMode());
     funcExpr->isMethodDefinition = true;
     setLocation(startLoc, optBody.getValue(), funcExpr);
 
@@ -4202,7 +4216,7 @@ Optional<ESTree::ClassDeclarationNode *> JSParserImpl::parseClassDeclaration(
     Param param) {
   assert(check(TokenKind::rw_class) && "class must start with 'class'");
   // NOTE: Class definition is always strict mode code.
-  SaveStrictMode saveStrictMode{this};
+  SaveStrictModeAndSeenDirectives saveStrictModeAndSeenDirectives{this};
   setStrictMode(true);
 
   SMLoc startLoc = advance().Start;
@@ -4258,7 +4272,7 @@ Optional<ESTree::ClassDeclarationNode *> JSParserImpl::parseClassDeclaration(
 Optional<ESTree::ClassExpressionNode *> JSParserImpl::parseClassExpression() {
   assert(check(TokenKind::rw_class) && "class must start with 'class'");
   // NOTE: A class definition is always strict mode code.
-  SaveStrictMode saveStrictMode{this};
+  SaveStrictModeAndSeenDirectives saveStrictModeAndSeenDirectives{this};
   setStrictMode(true);
 
   SMLoc start = advance().Start;
@@ -4798,7 +4812,6 @@ Optional<ESTree::Node *> JSParserImpl::parseClassElement(
           special == SpecialKind::Async ||
               special == SpecialKind::AsyncGenerator));
   assert(isStrictMode() && "parseClassElement should only be used for classes");
-  funcExpr->strictness = ESTree::makeStrictness(true);
   funcExpr->isMethodDefinition = true;
 
   if (special == SpecialKind::Get && funcExpr->_params.size() != 0) {
@@ -5030,7 +5043,7 @@ Optional<ESTree::Node *> JSParserImpl::parseArrowFunctionExpression(
   if (!reparseArrowParameters(leftExpr, paramList, isAsync))
     return None;
 
-  SaveStrictMode saveStrictMode{this};
+  SaveStrictModeAndSeenDirectives saveStrictModeAndSeenDirectives{this};
   ESTree::Node *body;
   bool expression;
 
@@ -5064,7 +5077,6 @@ Optional<ESTree::Node *> JSParserImpl::parseArrowFunctionExpression(
       expression,
       isAsync);
 
-  arrow->strictness = ESTree::makeStrictness(isStrictMode());
   return setLocation(startLoc, getPrevTokenEndLoc(), arrow);
 }
 

lib/Parser/JSParserImpl.h

@@ -110,6 +110,14 @@ class JSParserImpl {
     lexer_.setStrictMode(mode);
   }
 
+  llvh::SmallVector<UniqueString *, 1> &getSeenDirectives() {
+    return seenDirectives_;
+  }
+
+  /// Copy the seen directives from a vector of \c UniqueString* into a vector
+  /// of \c SmallString to be stored in \c PreParseFunctionInfo safely.
+  llvh::SmallVector<llvh::SmallString<24>, 1> copySeenDirectives() const;
+
   llvh::ArrayRef<StoredComment> getStoredComments() const {
     return lexer_.getStoredComments();
   }
@@ -213,6 +221,13 @@ class JSParserImpl {
   /// This is used when checking if `await` is a valid Identifier name.
   bool paramAwait_{false};
 
+  /// Appended when the parser has seen an directive being visited in the
+  /// current function scope (It's intended to be used with
+  /// `SaveStrictModeAndSeenDirectives`).
+  /// This is used to store directives for lazy functions in the preParse pass,
+  /// so we can recover directive nodes back in the lazyParse pass.
+  llvh::SmallVector<UniqueString *, 1> seenDirectives_{};
+
 #if HERMES_PARSE_JSX
   /// Incremented when inside a JSX tag and decremented when leaving it.
   /// Used to know whether to lex JS values or JSX text.
@@ -633,7 +648,6 @@ class JSParserImpl {
 
   bool parseStatementListItem(
       Param param,
-      bool parseDirectives,
       AllowImportExport allowImportExport,
       ESTree::NodeList &stmtList);
 
@@ -1260,16 +1274,21 @@ class JSParserImpl {
       ESTree::IdentifierNode *ident);
 #endif
 
-  /// RAII to save and restore the current setting of "strict mode".
-  class SaveStrictMode {
+  /// RAII to save and restore the current setting of "strict mode" and
+  /// "seen directives".
+  class SaveStrictModeAndSeenDirectives {
     JSParserImpl *const parser_;
-    const bool oldValue_;
+    const bool oldStrictMode_;
+    const unsigned oldSeenDirectiveSize_;
 
    public:
-    SaveStrictMode(JSParserImpl *parser)
-        : parser_(parser), oldValue_(parser->isStrictMode()) {}
-    ~SaveStrictMode() {
-      parser_->setStrictMode(oldValue_);
+    explicit SaveStrictModeAndSeenDirectives(JSParserImpl *parser)
+        : parser_(parser),
+          oldStrictMode_(parser->isStrictMode()),
+          oldSeenDirectiveSize_(parser->getSeenDirectives().size()) {}
+    ~SaveStrictModeAndSeenDirectives() {
+      parser_->setStrictMode(oldStrictMode_);
+      parser_->getSeenDirectives().resize(oldSeenDirectiveSize_);
     }
   };