Implement new Function.prototype.toString and fix CreateDynamicFunction parsing

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index 5c03ae4546cb40f8ac3d47e60541d45c18a722ba..835d4b2ea5e14276faeb99cb6fc8fb12c169b5a3 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -519,7 +519,8 @@ Parser::Parser(ParseInfo* info)
       total_preparse_skipped_(0),
       temp_zoned_(false),
       log_(nullptr),
-      preparsed_scope_data_(info->preparsed_scope_data()) {
+      preparsed_scope_data_(info->preparsed_scope_data()),
+      parameters_end_pos_(info->parameters_end_pos()) {
   // Even though we were passed ParseInfo, we should not store it in
   // Parser - this makes sure that Isolate is not accidentally accessed via
   // ParseInfo during background parsing.
@@ -2742,6 +2743,7 @@ Parser::LazyParsingResult Parser::SkipFunction(
     int* materialized_literal_count, int* expected_property_count,
     bool is_inner_function, bool may_abort, bool* ok) {
   DCHECK_NE(kNoSourcePosition, function_scope->start_position());
+  DCHECK_EQ(kNoSourcePosition, parameters_end_pos_);
   if (produce_cached_parse_data()) CHECK(log_);
 
   DCHECK_IMPLIES(IsArrowFunction(kind),
@@ -3122,8 +3124,34 @@ ZoneList<Statement*>* Parser::ParseFunction(
 
   if (IsResumableFunction(kind)) PrepareGeneratorVariables();
 
+  int expected_parameters_end_pos = parameters_end_pos_;
+  if (expected_parameters_end_pos != kNoSourcePosition) {
+    // This is the first function encountered in a CreateDynamicFunction eval.
+    parameters_end_pos_ = kNoSourcePosition;
+    // The function name should have been ignored, giving us the empty string
+    // here.
+    DCHECK_EQ(function_name, ast_value_factory()->empty_string());
+  }
+
   ParserFormalParameters formals(function_scope);
   ParseFormalParameterList(&formals, CHECK_OK);
+  if (expected_parameters_end_pos != kNoSourcePosition) {
+    // Check for '(' or ')' shenanigans in the parameter string for dynamic
+    // functions.
+    int position = peek_position();
+    if (position < expected_parameters_end_pos) {
+      ReportMessageAt(Scanner::Location(position, position + 1),
+                      MessageTemplate::kArgStringTerminatesParametersEarly);
+      *ok = false;
+      return nullptr;
+    } else if (position > expected_parameters_end_pos) {

[Dan Ehrenberg, 2017/02/13 17:32:32]

Optional: This branch will always be taken if we reach this line. It'd be nicer
to have a DCHECK to assert that it's true rather than a test.

[jwolfe, 2017/02/13 21:31:05]

The case where position == expected_parameters_end_pos is the success case, so
all three branches are possible here. Perhaps it would be more optimal to first
check if the two are equal to so that we do only 1 check in the common case.
That would look like this:

if (position != expected_parameters_end_pos) {
  // error
  if (position > expected_parameters_end_pos) {
  } else {
  }
}
// success

Are optimizations that subtle something we should be worrying about?

+      ReportMessageAt(Scanner::Location(expected_parameters_end_pos - 2,
+                                        expected_parameters_end_pos),
+                      MessageTemplate::kUnexpectedEndOfArgString);
+      *ok = false;
+      return nullptr;
+    }
+  }
   Expect(Token::RPAREN, CHECK_OK);
   int formals_end_position = scanner()->location().end_pos;
   *num_parameters = formals.num_parameters();