Class fields, part 3 (backends)

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index 53ef7d95ff187ea6dd69dd4775f526c23d788ba3..168499a171a44972cf95017620682fdd063ec91d 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -1492,6 +1492,18 @@ void BytecodeGenerator::VisitClassLiteral(ClassLiteral* expr) {
       .LoadNamedProperty(literal, prototype_string(), feedback_index(slot))
       .StoreAccumulatorInRegister(prototype);
 
+  // TODO(bakkot) This shouldn't be necessary; I just couldn't figure out how to
+  // invoke the function literal with appropriate home object and receiver
+  // (mainly because of FeedbackVector check failures which are totally opaque
+  // to me)

[Dan Ehrenberg, 2016/09/10 04:06:54]

Seems OK; it'd be good to note that this is motivated by semantics under
discussion at https://github.com/tc39/proposal-class-public-fields/issues/50 .
From off-line discussion, sounds like you have reasons for structuring it this
way, so maybe reword the comment to explain what it's doing exactly (rather than
"shouldn't be necessary") and how it ties into the parser. It'd also help if you
wrote out the desugaring in pseudo-JS. The feedback vector seems to be used
analogously to elsewhere, so no need to comment on that.

[bakkot, 2016/09/12 23:05:15]

Added a more reasonable comment mentioning that this is only necessary to
implement a particular semantics which are not what is currently specified.

+  VariableProxy* static_initializer_proxy = expr->static_initializer_proxy();
+  if (static_initializer_proxy != nullptr) {
+    Variable* variable = static_initializer_proxy->var();
+    builder()->LoadAccumulatorWithRegister(literal);
+    VisitVariableAssignment(variable, Token::INIT,  // TODO(bakkot) tokens

[Dan Ehrenberg, 2016/09/10 04:06:54]

What is this TODO?

[bakkot, 2016/09/12 23:05:15]

Oh, I just wasn't sure if Token::INIT or Token::ASSIGN was proper here. Looking
more into the code, it doesn't appear to matter.

+                            FeedbackVectorSlot::Invalid());
+  }
+
   VisitClassLiteralProperties(expr, literal, prototype);
   builder()->CallRuntime(Runtime::kToFastProperties, literal, 1);
   // Assign to class variable.
@@ -1546,6 +1558,18 @@ void BytecodeGenerator::VisitClassLiteralProperties(ClassLiteral* expr,
   for (int i = 0; i < expr->properties()->length(); i++) {
     ClassLiteral::Property* property = expr->properties()->at(i);
 
+    if (property->kind() == ClassLiteral::Property::FIELD &&
+        !property->is_static()) {
+      // This just does the ToName call on the field name and stores it in a

[Dan Ehrenberg, 2016/09/10 04:06:54]

Was Melissa O'Neill at Mudd when you went there? One thing that she told me that
really stuck was to never write just 'this', always write 'this <something>',
otherwise what you're saying becomes unintelligible.

It'd be much clearer to say something like, "the parser overloads the value of
this property with a variable which is used to store the possibly-computed name
of this property; desugar the result to assign the resulting key to the variable
for later access by the generated initializer function". If you could also write
out what the desugaring is in pseudo-JS, that would help.

[bakkot, 2016/09/12 23:05:15]

I've written a more reasonable if slightly wordy comment here also.

+      // variable (a proxy for which is the value() of this property)
+      DCHECK(property->value()->IsVariableProxy());
+      Variable* variable = property->value()->AsVariableProxy()->var();
+      VisitForAccumulatorValue(property->key());
+      VisitVariableAssignment(variable, Token::INIT,
+                              FeedbackVectorSlot::Invalid());
+      continue;
+    }
+
     // Set-up receiver.
     Register new_receiver = property->is_static() ? literal : prototype;
     if (new_receiver != old_receiver) {
@@ -1594,7 +1618,12 @@ void BytecodeGenerator::VisitClassLiteralProperties(ClassLiteral* expr,
         break;
       }
       case ClassLiteral::Property::FIELD: {
-        UNREACHABLE();
+        DCHECK(property->is_static());
+        builder()
+            ->LoadLiteral(Smi::FromInt(property->NeedsSetFunctionName()))
+            .StoreAccumulatorInRegister(set_function_name);
+        builder()->CallRuntime(Runtime::kDefineDataPropertyInLiteral, receiver,
+                               5);
         break;
       }
     }