[WIP] Support for shorthand object literal notation. #2432

AnthonyCalandra · 2018-07-20T20:25:37Z

This pull request adds support for object literal shorthand notation (properties and methods). In code:

let foo = 1;
let bar = { foo }; // Equivalent to: { foo: foo }
let baz = {
  fn() {
    return 123;
  }
}; // Equivalent to: { fn: function() { return 123; } }

JerryScript-DCO-1.0-Signed-off-by: Anthony Calandra [email protected]

AnthonyCalandra · 2018-07-20T20:30:28Z

Currently there is one bug that I'm aware of. The following will throw a ReferenceError when the correct behavior is to throw a SyntaxError:

try {
  var a = { true, false, null };
  assert(false);
} catch (e) {
  assert(e instanceof SyntaxError);
}

One obvious way I can fix this is by checking to see if the literal is reserved/keyword when I know I have shorthand notation. Looks like there currently isn't a helper function available to do this, so I would probably pull out what is in lexer_parse_identifier. Any alternative ideas?

Also, it looks like I'm failing two test262 tests. I think the second one is just a side-effect of failing the first one, so the first test is here. I've copied that file over locally and ran it using my local build. The resulting bytecode looks identical to the result when I run on master:

--- Script parsing start ---

  [  1] CBC_PUSH_LITERAL idx:0->string(Test262: This statement should not be evaluated.)
  [  0] CBC_THROW
  [  1] CBC_PUSH_NUMBER_POS_BYTE number:1
  [  0] CBC_ASSIGN_SET_IDENT_BLOCK idx:1->ident(x)

--- Script parsing end ---


//////////////////////////////////////////////////////////////////////////////
//CHECK#1
y={x;};
~~~~^
Script Error: SyntaxError: Expected ':' token. [line: 19, column: 5]

At this point I can't figure out why the test would fail. Any ideas?

zherczeg · 2018-07-23T06:18:51Z

var a = { true:true }; is valid but var a = { true } is not? Is it restricted to what kind of identifiers?

AnthonyCalandra · 2018-07-23T11:48:44Z

@zherczeg Yes that's correct. No reserved words or keywords allowed.

zherczeg · 2018-07-23T13:01:44Z

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

@@ -1343,7 +1342,6 @@ lexer_check_left_paren (parser_context_t *context_p) /**< context */
  return (context_p->source_p < context_p->source_end_p
          && context_p->source_p[0] == (uint8_t) LIT_CHAR_LEFT_PAREN);
 } /* lexer_check_left_paren */
-#endif /* !CONFIG_DISABLE_ES2015_CLASS */


Why do you remove this check? Isn't this an ES6 feature?

It came from the ES6 classes pull request but I also use it for checking if we have a method (it checks to see if there is a left parenthesis after the identifier). If I keep the ifdef around it then if the user disables classes I won't have this function available.

I mean this shorthand notation isn/t an es6 feature? Then we might need to change the ifdef.

Done. I'm planning on adding computed properties at some point, so all these new language features to object initializers could fit nicely into this macro.

zherczeg · 2018-07-23T13:02:53Z

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

-            && context_p->source_p[0] != LIT_CHAR_COLON)
+            && context_p->source_p[0] != LIT_CHAR_COLON
+            && context_p->source_p[0] != LIT_CHAR_RIGHT_BRACE
+            && context_p->source_p[0] != LIT_CHAR_COMMA)


What about the pre scanner?

Sorry I'm not sure I understand this comment.

I mean the pre-scanner which checks JS code early. It is used by switch, for-in, etc.

zherczeg · 2018-07-23T13:04:56Z

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

@@ -666,23 +666,58 @@ parser_parse_object_literal (parser_context_t *context_p) /**< context */
                                         literal_index,
                                         PARSER_OBJECT_PROPERTY_VALUE);

-      lexer_next_token (context_p);


Why do you remove lexer_next_token?

I didn't really remove it, I just moved it down into the if statement. If we have the shorthand notation, I cant skip this early because I need to create a literal object from the current token.

You have to create a literal regardless what is after the identifier, doesn't it?

Currently that's what I'm doing, but I won't emit bytecode if I see what's after the identifier makes the expression ill-formed. I thought maybe emitting the bytecode was why I'm failing the test262 test but it didn't make the test pass. Doing it this way though, I match what master outputs for this test.

AnthonyCalandra · 2018-07-24T00:54:25Z

Update: Fixed the issue with keywords/reserved words as identifiers not throwing SyntaxError.

Also, I've noticed that running test262 with all ES2015 features enabled fail five tests (two of them are from these changes). Looking at the code, we only pull in from their ECMAScript 5.1 branch, which means we only test for that particular version? Should we fix the failures or leave them? For context on what my PR breaks, refer to my first comment^.

zherczeg

When we add new code, it is not just a technical problem, it is also a quality maintaining problem. This seemingly simple task is actually quite a big challenge (even for me!), since you need to figure out a way how to make this part of the existing code and existing design principles. That requires a lot of thinking on implementation variations and selecting the best one.

Unfortunately I don't have time thinking on this task, but I hope you will find a nicer solution. Just ask @rerobika how many hours we spent on discussing the class implementation. And how many times he had to refactor his code. Many times even I throw away my own code and restart it from scratch because it does not look quality enough.

zherczeg · 2018-07-24T06:25:09Z

jerry-core/config.h

 # define CONFIG_DISABLE_ES2015_PROMISE_BUILTIN
+# define CONFIG_DISABLE_ES2015_SHORTHAND_OBJ_LITERAL_NOTATION


I would prefer a single guard for all object literal extensions. Perhaps CONFIG_DISABLE_ES2015_OBJ_LITERAL is enough.

zherczeg · 2018-07-24T06:35:24Z

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

+ *         false - otherwise
+ */
+bool
+lexer_is_identifier_keyword (parser_context_t *context_p) /**< context */


There is a similar code for this. It would be good to not duplicate that.

You need to be smart here, invent a way which can be effectively optimized by tail merging or just "reparse" the string or whatever.

My updated patch doesn't yet address this; still thinking on it.

zherczeg · 2018-07-24T06:39:06Z

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

+        bool not_end_of_literal = (context_p->source_p < context_p->source_end_p
+          && context_p->source_p[0] != LIT_CHAR_COLON
+          && context_p->source_p[0] != LIT_CHAR_RIGHT_BRACE
+          && context_p->source_p[0] != LIT_CHAR_COMMA);


Align these lines after the left bracket.

zherczeg · 2018-07-24T06:41:13Z

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

+      {
+        /* Else, we have something that isn't a valid object literal. */
+        parser_raise_error (context_p, PARSER_ERR_COLON_EXPECTED);
+      }


I don't really like this implementation. It is chaotic. You probably need a helper such as lexer_check_arrow, and add a switch.

zherczeg · 2018-07-24T06:46:42Z

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

+          && context_p->source_p[0] != LIT_CHAR_COLON);
+#endif /* CONFIG_DISABLE_ES2015_SHORTHAND_OBJ_LITERAL_NOTATION */
+
+        if (not_end_of_literal)


Please add test for the pre-parser such as:

switch (0) { default: var a = { foo, foo() {}, bar:5 } }

This should throw a ReferenceError because foo isn't defined initially. Is that intended?

The main reason of this test to check, whether the scanner is able to pre-scan the new syntax element. As it seems it can so the thrown Reference Error is correct.

AnthonyCalandra · 2018-07-24T12:41:20Z

Thanks @zherczeg. I will come back to this PR with a different approach.

AnthonyCalandra · 2018-08-17T18:29:53Z

@zherczeg I have tried to make the patch less chaotic, please take a look if my design looks reasonable this time. I still can't figure out why I'm failing those test262 tests (refer to my second post)... Any ideas?

rerobika

Getting closer, only a few remarks.

rerobika · 2018-08-21T11:29:38Z

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

@@ -610,49 +610,78 @@ parser_parse_object_literal (parser_context_t *context_p) /**< context */

  while (true)
  {
-    lexer_expect_object_literal_id (context_p, LEXER_OBJ_IDENT_NO_OPTS);
+    lexer_expect_object_literal_id (context_p, LEXER_OBJ_IDENT_OBJ_METHOD);


Shouldn't we keep the original flag when the short hand literal notation is disabled?

My proposal:

#ifndef CONFIG_DISABLE_ES2015_OBJ_LITERAL lexer_expect_object_literal_id (context_p, LEXER_OBJ_IDENT_OBJ_METHOD); #else /* CONFIG_DISABLE_ES2015_OBJ_LITERAL */ lexer_expect_object_literal_id (context_p, LEXER_OBJ_IDENT_NO_OPTS); #endif /* !CONFIG_DISABLE_ES2015_OBJ_LITERAL */

rerobika · 2018-08-21T11:30:28Z

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

+#else /* !CONFIG_DISABLE_ES2015_OBJ_LITERAL */
+    bool token_type_is_method = (context_p->token.type == LEXER_PROPERTY_GETTER
+                                 || context_p->token.type == LEXER_PROPERTY_SETTER);
+#endif /* CONFIG_DISABLE_ES2015_OBJ_LITERAL */


How about this?

bool token_type_is_method = (context_p->token.type == LEXER_PROPERTY_GETTER || context_p->token.type == LEXER_PROPERTY_SETTER); #ifndef CONFIG_DISABLE_ES2015_OBJ_LITERAL token_type_is_method = token_type_is_method || context_p->token.type == LEXER_PROPERTY_METHOD); #endif /* CONFIG_DISABLE_ES2015_OBJ_LITERAL */

rerobika · 2018-08-21T12:00:04Z

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

+                                         && context_p->token.type != LEXER_COMMA);
+#else /* !CONFIG_DISABLE_ES2015_OBJ_LITERAL */
+        bool not_end_of_property_name = context_p->token.type != LEXER_COLON;
+#endif /* CONFIG_DISABLE_ES2015_OBJ_LITERAL */


Comment mismatch ditto.

rerobika · 2018-08-21T12:01:22Z

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

+#else /* !CONFIG_DISABLE_ES2015_OBJ_LITERAL */
+        bool not_end_of_literal = (context_p->source_p < context_p->source_end_p
+                                   && context_p->source_p[0] != LIT_CHAR_COLON);
+#endif /* CONFIG_DISABLE_ES2015_OBJ_LITERAL */


Comment mismatch, it should be:

#ifndef CONFIG_DISABLE_ES2015_OBJ_LITERAL ... #else /* CONFIG_DISABLE_ES2015_OBJ_LITERAL */ ... #endif /* !CONFIG_DISABLE_ES2015_OBJ_LITERAL */

zherczeg · 2018-08-21T14:31:52Z

I would like to help you a bit @AnthonyCalandra, so I started to implement computed properties. This patch contains all around changes, which should simplify your work. The patch has not been finished yet.

zherczeg · 2018-08-21T14:32:42Z

Forgot to add the patch number: it is #2481.

JerryScript-DCO-1.0-Signed-off-by: Anthony Calandra [email protected]

AnthonyCalandra · 2018-08-29T01:29:58Z

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

@@ -260,7 +260,6 @@ parser_parse_array_literal (parser_context_t *context_p) /**< context */
  }
 } /* parser_parse_array_literal */

-#ifdef CONFIG_DISABLE_ES2015_OBJECT_INITIALIZER


@zherczeg I noticed that everything in this macro were needed for my code so I removed the ifdefs here. I'm guessing this change in your computed properties PR was to save binary size?

AnthonyCalandra · 2018-08-29T01:30:57Z

Rebased and resolved conflicts that arose due to computed properties addition.

EDIT: Nevermind, lots of stuff is broken and I don't know why... I might have to abandon this PR -- I just don't have time to work on this anymore and the code is starting to turn into macro soup.

zherczeg · 2018-08-29T06:34:02Z

Since there are two things in this patch (function and keyword) maybe splitting it into two patches could help. When a patch is hard to maintain, I usually just put it away and use it for inspiration and redo the work from scratch.

Anyway let us know if you want to work on this, because we also need these features and we have to work on it if you don't have time. Sorry for the inconvenience. Unfortunately the parser is by far the most challenging part of the engine from development perspective.

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch 2 times, most recently from 4f2b16f to 089ac6a Compare July 22, 2018 02:07

AnthonyCalandra changed the title ~~[WIP] Support for shorthand object literal notation.~~ Support for shorthand object literal notation. Jul 22, 2018

zherczeg requested changes Jul 23, 2018

View reviewed changes

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch 5 times, most recently from 1565ca9 to 4a2a11d Compare July 24, 2018 00:50

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch 2 times, most recently from 82c2666 to 922111a Compare July 24, 2018 01:41

zherczeg requested changes Jul 24, 2018

View reviewed changes

AnthonyCalandra changed the title ~~Support for shorthand object literal notation.~~ [WIP] Support for shorthand object literal notation. Jul 24, 2018

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch from 922111a to f08ad99 Compare August 4, 2018 17:22

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch 2 times, most recently from 33a803e to 469d361 Compare August 17, 2018 18:28

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch 2 times, most recently from 80ae0cb to ef2ef79 Compare August 17, 2018 18:57

akosthekiss added parser Related to the JavaScript parser ES2015 Related to ES2015 features labels Aug 17, 2018

rerobika requested changes Aug 21, 2018

View reviewed changes

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch from ef2ef79 to 9366d38 Compare August 22, 2018 18:37

Support for shorthand object literal notation.

40c5751

JerryScript-DCO-1.0-Signed-off-by: Anthony Calandra [email protected]

AnthonyCalandra force-pushed the shorthand-obj-literal-notation branch from 9366d38 to 40c5751 Compare August 29, 2018 01:27

AnthonyCalandra commented Aug 29, 2018

View reviewed changes

AnthonyCalandra closed this Sep 8, 2018

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed

		# define CONFIG_DISABLE_ES2015_PROMISE_BUILTIN
		# define CONFIG_DISABLE_ES2015_SHORTHAND_OBJ_LITERAL_NOTATION

[WIP] Support for shorthand object literal notation. #2432

[WIP] Support for shorthand object literal notation. #2432

Uh oh!

Conversation

AnthonyCalandra commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnthonyCalandra commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg commented Jul 23, 2018

Uh oh!

AnthonyCalandra commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

AnthonyCalandra Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnthonyCalandra commented Jul 24, 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!

AnthonyCalandra Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zherczeg Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

AnthonyCalandra commented Jul 24, 2018

Uh oh!

AnthonyCalandra commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rerobika 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!

AnthonyCalandra commented Jul 20, 2018 •

edited

Loading

AnthonyCalandra commented Jul 20, 2018 •

edited

Loading

AnthonyCalandra commented Jul 23, 2018 •

edited

Loading

AnthonyCalandra Jul 23, 2018 •

edited

Loading

AnthonyCalandra Aug 17, 2018 •

edited

Loading

zherczeg Jul 24, 2018 •

edited

Loading

AnthonyCalandra commented Aug 17, 2018 •

edited

Loading

AnthonyCalandra Aug 29, 2018 •

edited

Loading

AnthonyCalandra commented Aug 29, 2018 •

edited

Loading