Private fields

src/parsing/parser.cc

Index: src/parsing/parser.cc
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
index 4d4bc8fb89b802ba22c7ef34accb6f2c05e64e2b..0b6666e6234d682dbb83f9235a69c16ecd7a9a5b 100644
--- a/src/parsing/parser.cc
+++ b/src/parsing/parser.cc
@@ -163,7 +163,7 @@ Expression* Parser::CallClassFieldInitializer(Scope* scope,
 }
 
 Expression* Parser::RewriteSuperCall(Expression* super_call) {
-  if (!allow_harmony_class_fields()) {
+  if (!(allow_harmony_class_fields() || allow_harmony_private_class_fields())) {
     return super_call;
   }
   // TODO(bakkot) The whole charade with the do expression can be replaced by a
@@ -584,6 +584,7 @@ Parser::Parser(ParseInfo* info)
   set_allow_harmony_restrictive_generators(FLAG_harmony_restrictive_generators);
   set_allow_harmony_trailing_commas(FLAG_harmony_trailing_commas);
   set_allow_harmony_class_fields(FLAG_harmony_class_fields);
+  set_allow_harmony_private_class_fields(FLAG_harmony_private_class_fields);
   for (int feature = 0; feature < v8::Isolate::kUseCounterFeatureCount;
        ++feature) {
     use_counts_[feature] = 0;
@@ -3365,6 +3366,37 @@ DoExpression* Parser::ParseDoExpression(bool* ok) {
   return expr;
 }
 
+const AstRawString* ClassPrivateFieldVariableName(
+    AstValueFactory* ast_value_factory, const AstRawString* raw_name) {
+  std::string name(reinterpret_cast<const char*>(raw_name->raw_data()),
+                   raw_name->length());  // TODO(bakkot) check complicated names

[Dan Ehrenberg, 2016/09/10 04:30:28]

What's the issue here? Non-latin1 names?

[bakkot, 2016/09/13 01:05:38]

Non-one-byte names, in particular, although it looks like this particular line
is ok.

+  name = "#" + name;

[Dan Ehrenberg, 2016/09/10 04:30:28]

We don't concatenate strings in the parser like this generally.

[bakkot, 2016/09/13 01:05:38]

I'm not sure it can be avoided; we need an AstRawString here.

+  return ast_value_factory->GetOneByteString(
+      name.c_str());  // TODO(bakkot) sometimes two-byte probably, though maybe
+                      // doesn't matter

[Dan Ehrenberg, 2016/09/10 04:30:28]

Yeah, you can in fact have identifier characters which don't fit in latin1 in JS

[bakkot, 2016/09/13 01:05:38]

The reason I think it might not matter is that the variables using these names
are totally synthetic. It's valuable to have the name match the field name only
if the name will be visible somewhere; currently it is, because of the
ReferenceError thrown at runtime when referring to an unbound name, but that can
change.

If I understand correctly, this will convert two-byte names to one-byte, which
will mangle the name in a deterministic way, and it'll function correctly. But
this may not be the case. If not, I'm not sure how to deal with this.

+}
+
+Property* Parser::ParsePrivateFieldReference(Expression* base, bool* ok) {
+  // PrimaryExpression ::
+  //     PrivateName
+  int pos = peek_position();
+
+  Expect(Token::HASH, CHECK_OK);
+  const AstRawString* property_name =
+      ParseIdentifierName(CHECK_OK);  // TODO(bakkot) more complex names
+  const AstRawString* symbol_var_name =
+      ClassPrivateFieldVariableName(ast_value_factory(), property_name);
+
+  ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(2, zone());
+  args->Add(base, zone());
+  args->Add(NewUnresolved(symbol_var_name), zone());
+  Expression* call = factory()->NewCallRuntime(
+      Runtime::kThrowIfMissingPrivateField, args, pos);
+
+  Expression* prop = NewUnresolved(symbol_var_name);
+  return factory()->NewProperty(call, prop, pos);

[Dan Ehrenberg, 2016/09/10 04:30:28]

Great logic here, ends up being very elegant, but long-term I think we want
ThrowIfMissingPrivateField to be either part of the load/store IC (a flag passed
into that), or, it looks like we could pretty easily make a code stub for this
runtime function (based on CodeStubAssembler::TryHasOwnProperty). Include a TODO
explaining the need for optimization here.

[bakkot, 2016/09/13 01:05:38]

Added a TODO.

+}
+
 void Parser::ParseArrowFunctionFormalParameterList(
     ParserFormalParameters* parameters, Expression* expr,
     const Scanner::Location& params_loc, Scanner::Location* duplicate_loc,
@@ -4244,6 +4276,7 @@ PreParser::PreParseResult Parser::ParseLazyFunctionBodyWithPreParser(
     SET_ALLOW(harmony_async_await);
     SET_ALLOW(harmony_trailing_commas);
     SET_ALLOW(harmony_class_fields);
+    SET_ALLOW(harmony_private_class_fields);
 #undef SET_ALLOW
   }
   PreParser::PreParseResult result = reusable_preparser_->PreParseLazyFunction(
@@ -4415,6 +4448,10 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
   ZoneList<ClassLiteral::Property*>* properties = NewClassPropertyList(4);
   ZoneList<Expression*>* instance_field_initializers =
       new (zone()) ZoneList<Expression*>(0, zone());
+  ZoneList<Variable*>* private_field_symbol_vars =
+      new (zone()) ZoneList<Variable*>(0, zone());
+  ZoneList<Variable*>* private_field_name_vars =
+      new (zone()) ZoneList<Variable*>(0, zone());
   FunctionLiteral* constructor = nullptr;
   bool has_seen_constructor = false;
   Variable* static_initializer_var = nullptr;
@@ -4429,12 +4466,13 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
   while (peek() != Token::RBRACE) {
     if (Check(Token::SEMICOLON)) continue;
     FuncNameInferrer::State fni_state(fni_);
+    const AstRawString* private_name = nullptr;
     bool is_computed_name = false;  // Classes do not care about computed
                                     // property names here.
     ExpressionClassifier property_classifier(this);
-    ClassLiteral::Property* property =
-        ParseClassPropertyDefinition(&checker, has_extends, &is_computed_name,
-                                     &has_seen_constructor, CHECK_OK);
+    ClassLiteral::Property* property = ParseClassPropertyDefinition(
+        &checker, has_extends, &private_name, &is_computed_name,
+        &has_seen_constructor, CHECK_OK);
     RewriteNonPattern(CHECK_OK);
     impl()->AccumulateFormalParameterContainmentErrors();
 
@@ -4445,8 +4483,31 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
           name != nullptr ? name : ast_value_factory()->empty_string());
     } else {
       if (property->kind() == ClassLiteralProperty::FIELD) {
-        DCHECK(allow_harmony_class_fields());
-        if (property->is_static()) {
+        if (private_name != nullptr) {
+          DCHECK(allow_harmony_private_class_fields());
+          DCHECK(property->key() == nullptr);
+          DCHECK(!property->is_static());
+
+          const AstRawString* symbol_var_name =
+              ClassPrivateFieldVariableName(ast_value_factory(), private_name);
+          Variable* symbol_var = scope()->DeclareLocal(
+              symbol_var_name, CONST, kCreatedInitialized,
+              Variable::NORMAL);  // TODO(bakkot) flags, Declare vs DeclareLocal

[Dan Ehrenberg, 2016/09/10 04:30:28]

I'm confused, where's the actual declaration AST node made? This creates a
rather strange AST, I think. I think Declare is what you want generally.

[bakkot, 2016/09/13 01:05:38]

Switched to that. This has the side effect of making duplicate property name
declaration an early SyntaxError: `class { #a; #a; }` produces "SyntaxError:
Identifier '#a' has already been declared". This isn't quite per spec, but
sounds like it probably should be:
https://github.com/tc39/proposal-private-fields/issues/48

+          private_field_symbol_vars->Add(symbol_var, zone());
+
+          const AstRawString* name_var_name = ClassFieldVariableName(
+              true, ast_value_factory(), instance_field_initializers->length());
+          Variable* name_var = scope()->DeclareLocal(
+              name_var_name, CONST, kCreatedInitialized,
+              Variable::NORMAL);  // TODO(bakkot) flags, Declare vs DeclareLocal
+          private_field_name_vars->Add(name_var, zone());
+
+          instance_field_initializers->Add(property->value(), zone());
+
+          // Does not add to properties because no action is required from the
+          // backend during class definition.
+        } else if (property->is_static()) {
+          DCHECK(allow_harmony_class_fields());
           if (static_initializer_var == nullptr) {
             static_initializer_var =
                 NewTemporary(ast_value_factory()->empty_string());
@@ -4463,7 +4524,9 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
           Expression* call = factory()->NewCallRuntime(Runtime::kInlineCall,
                                                        args, kNoSourcePosition);
           property->set_value(call);
+          properties->Add(property, zone());
         } else {
+          DCHECK(allow_harmony_class_fields());
           // if (is_computed_name) { // TODO(bakkot) figure out why this is
           // necessary for non-computed names in full-codegen
           ZoneList<Expression*>* to_name_args =
@@ -4487,9 +4550,11 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
           if (!ok) return nullptr;
           instance_field_initializers->Add(property->value(), zone());
           property->set_value(factory()->NewVariableProxy(name_var));
+          properties->Add(property, zone());
         }
+      } else {
+        properties->Add(property, zone());
       }
-      properties->Add(property, zone());
     }
 
     DCHECK_NOT_NULL(fni_);
@@ -4500,7 +4565,8 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
   int end_pos = scanner()->location().end_pos;
 
   bool has_instance_fields = instance_field_initializers->length() > 0;
-  DCHECK(!has_instance_fields || allow_harmony_class_fields());
+  DCHECK(!has_instance_fields || allow_harmony_class_fields() ||
+         allow_harmony_private_class_fields());
   bool has_default_constructor = constructor == nullptr;
   if (has_default_constructor) {
     constructor = DefaultConstructor(name, has_extends, has_instance_fields,
@@ -4519,6 +4585,31 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
     proxy->var()->set_initializer_position(end_pos);
   }
 
+  DCHECK(private_field_symbol_vars->length() ==
+         private_field_name_vars->length());
+  DCHECK(!allow_harmony_private_class_fields() ||
+         private_field_symbol_vars->length() == 0);
+  for (int i = 0; i < private_field_symbol_vars->length(); ++i) {

[Dan Ehrenberg, 2016/09/10 04:30:28]

A comment explaining the desugaring you're doing?

[bakkot, 2016/09/13 01:05:38]

Done.

+    VariableProxy* symbol_var_proxy =
+        factory()->NewVariableProxy(private_field_symbol_vars->at(i));
+    VariableProxy* name_var_proxy =
+        factory()->NewVariableProxy(private_field_name_vars->at(i));
+
+    ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(1, zone());
+    args->Add(factory()->NewUndefinedLiteral(kNoSourcePosition),
+              zone());  // TODO(bakkot) name
+    Expression* name =
+        factory()->NewCallRuntime(Runtime::kCreatePrivateSymbol, args, pos);
+
+    Assignment* inner_assignment = factory()->NewAssignment(
+        Token::INIT, name_var_proxy, name, kNoSourcePosition);
+    Assignment* outer_assignment = factory()->NewAssignment(
+        Token::INIT, symbol_var_proxy, inner_assignment, kNoSourcePosition);
+    do_block->statements()->Add(
+        factory()->NewExpressionStatement(outer_assignment, kNoSourcePosition),
+        zone());
+  }
+
   ClassLiteral* class_literal = factory()->NewClassLiteral(
       proxy, extends, constructor, properties, pos, end_pos);
 
@@ -4534,7 +4625,7 @@ Expression* Parser::ParseClassLiteral(const AstRawString* name,
                                    class_literal, kNoSourcePosition),
           pos),
       zone());
-  if (allow_harmony_class_fields() &&
+  if ((allow_harmony_class_fields() || allow_harmony_private_class_fields()) &&
       (has_instance_fields ||
        (extends != nullptr && !has_default_constructor))) {
     // Default constructors for derived classes without fields will not try to