Implement ES2015 class feature (part I.) #2404

rerobika · 2018-06-19T14:05:51Z

This patch is the first milestone of the implementation of this new language element.

Currently supported:

Class statement
Class expression
Static methods

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]

zherczeg

I worry about as snapshots, what happens if a snapshot has classes and it is executed by a jerry without snapshots.

zherczeg · 2018-06-20T06:38:42Z

jerry-core/ecma/operations/ecma-function-object.c

@@ -690,6 +701,10 @@ ecma_op_function_construct_simple_or_external (ecma_object_t *func_obj_p, /**< F
                                         arguments_list_len),
                  ret_value);

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  JERRY_CONTEXT (class_invoked_with_new) = false;


Who sets this value to false if ecma_op_function_call throws an error? Using a global value is risky since it always finds way to escape :(

This can be improved after #2414 lands.

zherczeg · 2018-06-20T06:39:46Z

jerry-core/parser/js/byte-code.h

@@ -549,6 +549,12 @@
              VM_OC_RESOURCE_NAME) \
  CBC_OPCODE (CBC_EXT_LINE, CBC_NO_FLAG, 0, \
              VM_OC_LINE) \
+  CBC_OPCODE (CBC_EXT_SET_UP_PROPERTY, CBC_HAS_LITERAL_ARG, -1, \


setup property :)

I would rather use CBC_EXT_SET_STATIC_PROPERTY since it is only used by static functions.

zherczeg · 2018-06-20T06:40:21Z

jerry-core/parser/js/byte-code.h

@@ -665,6 +671,9 @@ typedef enum
  CBC_CODE_FLAGS_ARROW_FUNCTION = (1u << 7), /**< this function is an arrow function */
  CBC_CODE_FLAGS_STATIC_FUNCTION = (1u << 8), /**< this function is a static snapshot function */
  CBC_CODE_FLAGS_DEBUGGER_IGNORE = (1u << 9), /**< this function should be ignored by debugger */
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  CBC_CODE_FLAGS_CLASS_CONSTRUCTOR = (1u << 10), /**< this function is a class constructor */


I would just use CBC_CODE_FLAGS_CONSTRUCTOR since we might use it later for other purposes.

zherczeg · 2018-06-20T06:41:21Z

jerry-core/parser/js/js-lexer.h

+ */
+static const lexer_lit_location_t lexer_get_literal =
+{
+  (const uint8_t *) "get", 3, LEXER_IDENT_LITERAL, false


I think get and set are already there.

zherczeg · 2018-06-20T06:44:22Z

jerry-core/parser/js/js-lexer.c

@@ -2238,6 +2259,8 @@ static const lexer_lit_location_t lexer_set_literal =
  (const uint8_t *) "set", 3, LEXER_IDENT_LITERAL, false
 };

+#endif /* CONFIG_DISABLE_ES2015_CLASS */


Just keep these for classes.

zherczeg · 2018-06-20T06:46:33Z

jerry-core/parser/js/js-lexer.c

@@ -531,7 +541,7 @@ lexer_parse_identifier (parser_context_t *context_p, /**< context */
      {
        if (keyword_p->type >= LEXER_FIRST_FUTURE_STRICT_RESERVED_WORD)
        {
-          if (context_p->status_flags & PARSER_IS_STRICT)
+          if (context_p->status_flags & PARSER_IS_STRICT && !LEXER_INSIDE_CLASS)


I have a feeling we should not require this.

zherczeg · 2018-06-20T06:52:48Z

jerry-core/parser/js/js-lexer.c

+        lexer_skip_spaces (context_p);
+
+        if (context_p->source_p < context_p->source_end_p
+            && context_p->source_p[0] != LIT_CHAR_COLON)


What is the purpose of this check. colons are allowed in classes?

zherczeg · 2018-06-20T06:53:49Z

jerry-core/parser/js/js-parser-expr.c

+
+  while (true)
+  {
+    if (!is_static)


First function cannot be static?

zherczeg · 2018-06-20T06:56:51Z

jerry-core/vm/vm.c

@@ -1046,8 +1046,14 @@ vm_loop (vm_frame_ctx_t *frame_ctx_p) /**< frame context */
          continue;
        }
        case VM_OC_SET_PROPERTY:
+        case VM_OC_SET_UP_PROPERTY:


Add a define around this, so this byte code is not executed when the feature is disabled.

rerobika · 2018-06-25T09:42:10Z

The patch is updated based on your thoughts except the class invoking part. I'm looking for a better solution to handle this problem.

zherczeg

Good patch, only some minor things remained.

zherczeg · 2018-07-10T13:18:13Z

jerry-core/api/jerry-snapshot.c

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  if (compiled_code_p->status_flags & CBC_CODE_FLAGS_CONSTRUCTOR)
+  {
+    globals_p->class_found = true;


This is not necessary true. However I would not add another flag for classes at the moment, so keep this code.

zherczeg · 2018-07-10T13:20:42Z

jerry-core/parser/js/js-lexer.c

@@ -2231,6 +2250,15 @@ lexer_expect_object_literal_id (parser_context_t *context_p, /**< context */
 {
  lexer_skip_spaces (context_p);

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+  /* Skip empty statements */
+  while (context_p->status_flags & PARSER_IS_CLASS && *context_p->source_p == LIT_CHAR_SEMICOLON)


Add brackets around the and operation.

zherczeg · 2018-07-10T13:21:36Z

jerry-core/parser/js/js-lexer.c

+      if (!must_be_identifier
+          && context_p->token.lit_location.length == 6
+          && (context_p->status_flags & PARSER_IS_CLASS)
+          && lexer_compare_raw_identifier_to_current (context_p, "static", 6))


Check (context_p->status_flags & PARSER_IS_CLASS) first.

zherczeg · 2018-07-10T13:22:45Z

jerry-core/parser/js/js-lexer.c

@@ -2297,6 +2335,15 @@ lexer_expect_object_literal_id (parser_context_t *context_p, /**< context */
      }
    }

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+    if (context_p->status_flags & PARSER_IS_CLASS


Brackets again.

zherczeg · 2018-07-10T13:24:10Z

jerry-core/parser/js/js-parser-expr.c

+        break;
+      }
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */
+


Cannot we return early form this function? Or not even call it?

True, calling it is unnecessary.

zherczeg · 2018-07-10T13:40:27Z

jerry-core/parser/js/js-parser-scanner.c

+
+        if (type == LEXER_RIGHT_BRACE && stack_top == SCAN_STACK_HEAD)
+        {
+          mode = SCAN_MODE_STATEMENT;


Cant be an expression?

True, it can.

zherczeg · 2018-07-10T13:44:07Z

jerry-core/parser/js/js-parser-util.c

+    }
+    case PARSER_ERR_CLASS_STATIC_PROPERTY_NAME_PROTOTYPE:
+    {
+      return "Classes may not have a static property named 'prototype'.";


called prototype

zherczeg · 2018-07-10T13:45:19Z

jerry-core/parser/js/js-parser.c

+    }
+    else
+    {
+      JERRY_DEBUG_MSG ("\n--- Function parsing start ---\n\n");


Does it look better with a ?: or the line is too long?

It looks better but the lines are too long :/

zherczeg · 2018-07-10T13:46:52Z

jerry-core/vm/vm.c

+          JERRY_ASSERT (byte_code_start_p[0] == CBC_EXT_OPCODE);
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+          opfunc_set_accessor (VM_OC_GROUP_GET_INDEX (opcode_data) == VM_OC_SET_GETTER,
+                               stack_top_p[(byte_code_start_p[1] > CBC_EXT_SET_SETTER) ? -2 : -1],


I would use a const int index just like above instead of duplicating the code.

zherczeg · 2018-07-10T13:47:28Z

jerry-core/vm/vm.c

@@ -1047,7 +1047,12 @@ vm_loop (vm_frame_ctx_t *frame_ctx_p) /**< frame context */
        }
        case VM_OC_SET_PROPERTY:
        {
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+          const int index = (byte_code_start_p[0] == CBC_EXT_OPCODE) ? -2 : -1;


Just create a const int index = - 1 and use the index for accessing the stack top.

zherczeg · 2018-07-12T08:07:12Z

jerry-core/parser/js/js-lexer.c

+        && !is_static_method
+        && lexer_compare_raw_identifier_to_current (context_p, "constructor", 11))
+    {
+      context_p->status_flags |= PARSER_CLASS_CONSTRUCTOR;


It would be good to introduce a virtual token here.

zherczeg · 2018-07-12T08:16:22Z

jerry-core/parser/js/js-parser-expr.c

+
+  while (true)
+  {
+    lexer_expect_object_literal_id (context_p, false);


Add a skip empty semicolons here (if not static).

zherczeg · 2018-07-12T08:28:56Z

jerry-core/parser/js/js-lexer.c

@@ -2297,6 +2338,16 @@ lexer_expect_object_literal_id (parser_context_t *context_p, /**< context */
      }
    }

+#ifndef CONFIG_DISABLE_ES2015_CLASS
+    if ((context_p->status_flags & PARSER_IS_CLASS)


add !must_be_identifier

zherczeg · 2018-07-12T08:36:41Z

jerry-core/parser/js/js-parser-expr.c

+ * Parse class as an object literal.
+ */
+void
+parser_parse_class_literal (parser_context_t *context_p, /**< context */


Make this static!

zherczeg · 2018-07-12T08:50:26Z

jerry-core/parser/js/js-parser-scanner.c

+          break;
+        }
+
+        if (type == LEXER_RIGHT_BRACE && stack_top == SCAN_STACK_HEAD)


switch(1) { default: (class A{} % 1) }

zherczeg · 2018-07-12T08:54:40Z

jerry-core/parser/js/js-parser-statm.c

+
+  parser_emit_cbc_literal (context_p, CBC_SET_PROPERTY, context_p->lit_object.index);
+
+  if (is_statement && class_ident_index != PARSER_MAXIMUM_NUMBER_OF_LITERALS)


class_ident_index != PARSER_MAXIMUM_NUMBER_OF_LITERALS unnecessary

zherczeg · 2018-07-12T09:01:50Z

jerry-core/parser/js/js-parser.c

@@ -2596,7 +2646,18 @@ parser_parse_function (parser_context_t *context_p, /**< context */
 #ifdef PARSER_DUMP_BYTE_CODE
  if (context_p->is_show_opcodes)
  {
+#ifndef CONFIG_DISABLE_ES2015_CLASS
+    if (context_p->status_flags & PARSER_CLASS_CONSTRUCTOR)


zherczeg · 2018-07-12T09:02:10Z

jerry-core/vm/vm.c

+          const int index = (byte_code_start_p[0] == CBC_EXT_OPCODE) ? -2 : -1;
+#else
+          const int index = -1;
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */


Newline after.

zherczeg · 2018-07-12T09:02:17Z

jerry-core/vm/vm.c

+          const int index = (byte_code_start_p[1] > CBC_EXT_SET_SETTER) ? -2 : -1;
+#else
+          const int index = -1;
+#endif /* !CONFIG_DISABLE_ES2015_CLASS */


rerobika · 2018-07-12T12:05:41Z

@zherczeg Thanks for the reviews, the PR is updated.

zherczeg · 2018-07-12T12:16:00Z

jerry-core/parser/js/js-parser.c

+    (is_constructor ? JERRY_DEBUG_MSG ("\n--- Class constructor parsing start ---\n\n")
+                    : JERRY_DEBUG_MSG ("\n--- Function parsing start ---\n\n"));
+
+#else


JERRY_DEBUG_MSG (is_constructor ? "\n--- Class constructor parsing start ---\n\n" : "\n--- Function parsing start ---\n\n");

Remove newline after it.

zherczeg · 2018-07-12T12:16:09Z

jerry-core/parser/js/js-parser.c

+    bool is_constructor = context_p->status_flags & PARSER_CLASS_CONSTRUCTOR;
+    (is_constructor ? JERRY_DEBUG_MSG ("\n--- Class constructor parsing end ---\n\n")
+                    : JERRY_DEBUG_MSG ("\n--- Function parsing end ---\n\n"));
+#else


zherczeg

LGTM

LaszloLango · 2018-07-12T13:31:03Z

jerry-core/parser/js/js-parser-expr.c

+    /* Class expression may contain an identifier. */
+    if (context_p->token.type == LEXER_LITERAL && context_p->token.lit_location.type == LEXER_IDENT_LITERAL)
+    {
+      //NOTE: If 'Function.name' will be supported, the current literal object must be set to 'name' property.


/* NOTE: If 'Function.name' will be supported, the current literal object must be set to 'name' property. */

Good catch, I haven't noticed it.

This patch is the first milestone of the implementation of this new language element. Currently supported: - Class statement - Class expression - Static methods JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]

LaszloLango

LGTM

This includes the following syntax: - Class statement. - Class expression. - Static methods. Author: Robert Fancsik <[email protected]> PR-URL: jerryscript-project/jerryscript#2404

zherczeg requested changes Jun 20, 2018

View reviewed changes

rerobika force-pushed the es2015_class branch from 5f36037 to 1342693 Compare June 25, 2018 09:36

zherczeg requested changes Jul 10, 2018

View reviewed changes

rerobika force-pushed the es2015_class branch 2 times, most recently from b20b452 to 523148c Compare July 11, 2018 13:56

zherczeg requested changes Jul 12, 2018

View reviewed changes

rerobika force-pushed the es2015_class branch from 523148c to 1a3d196 Compare July 12, 2018 12:01

zherczeg requested changes Jul 12, 2018

View reviewed changes

rerobika force-pushed the es2015_class branch from 1a3d196 to 906ab97 Compare July 12, 2018 12:27

zherczeg approved these changes Jul 12, 2018

View reviewed changes

LaszloLango requested changes Jul 12, 2018

View reviewed changes

Implement ES2015 class feature (part I.)

4277444

This patch is the first milestone of the implementation of this new language element. Currently supported: - Class statement - Class expression - Static methods JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]

rerobika force-pushed the es2015_class branch from 906ab97 to 4277444 Compare July 12, 2018 13:38

LaszloLango approved these changes Jul 12, 2018

View reviewed changes

yichoi merged commit 43aae19 into jerryscript-project:master Jul 12, 2018

yorkie mentioned this pull request Oct 25, 2018

jerry: add class basic support yodaos-project/ShadowNode#395

Merged

3 tasks

rerobika deleted the es2015_class branch February 28, 2019 11:18


		parser_emit_cbc_literal (context_p, CBC_SET_PROPERTY, context_p->lit_object.index);

		if (is_statement && class_ident_index != PARSER_MAXIMUM_NUMBER_OF_LITERALS)

Implement ES2015 class feature (part I.) #2404

Implement ES2015 class feature (part I.) #2404

Uh oh!

Conversation

rerobika commented Jun 19, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rerobika commented Jun 25, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rerobika commented Jul 12, 2018

Uh oh!