Inner classes should not finalize the parent's class heritage environment #2686

rerobika · 2019-01-09T10:06:19Z

This patch is the proper fix for #2667, since #2269 did not fix the problem entirely.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

LaszloLango · 2019-01-09T14:48:21Z

#2269 did not fix the problem entirely.

What is the remained issue? Is it possible to add a new test case for it?

rerobika · 2019-01-09T20:41:56Z

@LaszloLango I've just added the new test for it, but the fix became a bit more complicated.

akosthekiss · 2019-01-14T23:46:25Z

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

  PARSER_CLASS_HAS_SUPER = (1u << 21),        /**< class has super reference */
-  PARSER_CLASS_STATIC_FUNCTION = (1u << 22),  /**< this function is a static class method */
-  PARSER_CLASS_SUPER_PROP_REFERENCE = (1u << 23),  /**< super property call or assignment */
+  PARSER_CLASS_IMPLICIT_SUPER = (1u << 22),   /**< super property call or assignment */


comment seems to be copy-pasted from PARSER_CLASS_SUPER_PROP_REFERENCE

akosthekiss · 2019-01-14T23:51:52Z

jerry-core/ecma/base/ecma-globals.h

                                             *   in sync with PARSER_CLASS_CONSTRUCTOR) */
  ECMA_PARSE_HAS_SUPER = (1u << 3), /**< the current context has super reference */
-  ECMA_PARSE_HAS_STATIC_SUPER = (1u << 4), /**< the current context is a static class method */
+  ECMA_PARSE_HAS_IMPL_SUPER = (1u << 4), /**< the current context has super implicit super reference */


one too many supers in comment

akosthekiss · 2019-01-14T23:56:46Z

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

-      JERRY_ASSERT (ecma_get_object_prototype (func_obj_p) == NULL);
+#ifndef JERRY_NDEBUG
+      ecma_object_t *prototype_obj_p = ecma_builtin_get (ECMA_BUILTIN_ID_OBJECT_PROTOTYPE);
+      JERRY_ASSERT (ecma_get_object_prototype (func_obj_p) == prototype_obj_p);


Merge these lines into a single statement to avoid the need for preprocessor guards and temporary variables.

JERRY_ASSERT (ecma_get_object_prototype (func_obj_p) == ecma_builtin_get (ECMA_BUILTIN_ID_OBJECT_PROTOTYPE));

akosthekiss · 2019-01-14T23:58:07Z

jerry-core/vm/vm.c

          {
-            super_class_p = ecma_create_object (NULL, 0, ECMA_OBJECT_TYPE_GENERAL);
+            ecma_object_t *prototype_obj_p = ecma_builtin_get (ECMA_BUILTIN_ID_OBJECT_PROTOTYPE);
+            super_class_p = ecma_create_object (prototype_obj_p, 0, ECMA_OBJECT_TYPE_GENERAL);


Merge these lines.

super_class_p = ecma_create_object (ecma_builtin_get (ECMA_BUILTIN_ID_OBJECT_PROTOTYPE), 0, ECMA_OBJECT_TYPE_GENERAL);

akosthekiss · 2019-01-15T00:02:33Z

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

  bool super_called = false;
-  uint32_t status_flags = PARSER_IS_FUNCTION | PARSER_IS_CLOSURE | (context_p->status_flags & PARSER_CLASS_HAS_SUPER);
+  uint32_t status_flags = PARSER_IS_FUNCTION | PARSER_IS_CLOSURE;
+  status_flags |= (context_p->status_flags & (PARSER_CLASS_HAS_SUPER | PARSER_CLASS_IMPLICIT_SUPER));


Now that the expression is broken out into a separate statement, the outer parentheses are unnecessary.

status_flags |= context_p->status_flags & ...

Exactly, the only problem with the single statement that it reaches the line length limit, but the outer parentheses can be removed.

akosthekiss · 2019-01-15T00:04:32Z

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

      literal_p->u.bytecode_p = parser_parse_function (context_p, constructor_status_flags);
      literal_p->type = LEXER_FUNCTION_LITERAL;
-      parser_emit_cbc_literal (context_p, PARSER_TO_EXT_OPCODE (CBC_EXT_SET_CLASS_LITERAL), context_p->literal_count);
+      parser_emit_cbc_literal (context_p, PARSER_TO_EXT_OPCODE (CBC_EXT_SET_CLASS_LITERAL), result_index);


I don't think that this change is necessary. Where else is result_index used?

It should be. That was currently an unrevealed bug, since the parser_parse_function can overwrite the literal_object and increase the literal count as well. But thanks for the reminding I'll add a test case for it.

I think I see what I've missed here: is it parser_parse_function that may update context_p->literal_count and that's why it should be saved? If so, it's OK. But I think it would be better not to inject result_index in the middle of literal_p->field assignments. It could be simple moved two lines upwards.

Exactly. Okay it can me moved up.

akosthekiss · 2019-01-15T00:07:41Z

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


-  if (context_p->token.type == LEXER_KEYW_EXTENDS)
+  bool create_class_env = (bool) (context_p->token.type == LEXER_KEYW_EXTENDS
+                                  || context_p->status_flags & PARSER_CLASS_HAS_SUPER);


Hmm. I think this should rather be written as

(context_p->token.type == LEXER_KEYW_EXTENDS) || (bool) (context_p->status_flags & PARSER_CLASS_HAS_SUPER)

The == results a bool, the || results a bool. It's only the & that needs casting.

rerobika · 2019-01-15T09:19:02Z

@akosthekiss Thank for the review I've updated the PR also rebased it to the latest master.

zherczeg

I wouldn't say it is worth to support this likely unintentional bug.

zherczeg · 2019-01-15T09:56:29Z

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


-  if (context_p->token.type == LEXER_KEYW_EXTENDS)
+  bool create_class_env = (context_p->token.type == LEXER_KEYW_EXTENDS
+                           || context_p->status_flags & PARSER_CLASS_HAS_SUPER);


I would enclose this expression into brackets.

Thanks! I've forgotten it.

akosthekiss

I've spotted some style issues in the test file. The rest LGTM

tests/jerry/es2015/class-inheritance-inner-class.js

This patch is the proper fix for jerryscript-project#2667, since jerryscript-project#2269 did not fix the problem entirely. JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

rerobika · 2019-01-16T08:08:52Z

@akosthekiss Thanks for the review, I've fixed the related style issues.

akosthekiss

LGTM

zherczeg

LGTM

rerobika force-pushed the fix_issue_2667 branch from 257d751 to 9e3ceb6 Compare January 9, 2019 20:40

rerobika force-pushed the fix_issue_2667 branch from 9e3ceb6 to 57c236c Compare January 9, 2019 20:44

rerobika mentioned this pull request Jan 10, 2019

Fix super keyword parsing for arrow functions #2664

Merged

akosthekiss requested changes Jan 15, 2019

View reviewed changes

rerobika force-pushed the fix_issue_2667 branch from 57c236c to 4078926 Compare January 15, 2019 09:18

zherczeg requested changes Jan 15, 2019

View reviewed changes

rerobika force-pushed the fix_issue_2667 branch 2 times, most recently from 75105c3 to a91e6ec Compare January 15, 2019 10:52

akosthekiss reviewed Jan 15, 2019

View reviewed changes

Fix inner classes in class heritage environment

51622f7

This patch is the proper fix for jerryscript-project#2667, since jerryscript-project#2269 did not fix the problem entirely. JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

rerobika force-pushed the fix_issue_2667 branch from a91e6ec to 51622f7 Compare January 16, 2019 08:08

akosthekiss approved these changes Jan 16, 2019

View reviewed changes

zherczeg approved these changes Jan 16, 2019

View reviewed changes

akosthekiss added bug Undesired behaviour ES2015 Related to ES2015 features labels Jan 16, 2019

akosthekiss merged commit 86e60dd into jerryscript-project:master Jan 16, 2019

rerobika deleted the fix_issue_2667 branch February 28, 2019 11:11

Inner classes should not finalize the parent's class heritage environment #2686

Inner classes should not finalize the parent's class heritage environment #2686

Uh oh!

Conversation

rerobika commented Jan 9, 2019

Uh oh!

LaszloLango commented Jan 9, 2019

Uh oh!

rerobika commented Jan 9, 2019

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 Jan 15, 2019

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!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rerobika commented Jan 16, 2019

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants