Implement ES2015 module system #2599

vincedani · 2018-11-16T13:47:51Z

This patch is the first part of the ES2015 module system implementation.

The language specification can be found here

The implemented feature is documented here

JerryScript-DCO-1.0-Signed-off-by: Daniel Vince vinced@inf.u-szeged.hu

rerobika

In general, please add the missing:

Function
Function argument
Structure
Structure member
documentations.

Also, please try the avoid the layer violations and please use the JERRY_ASSERT macro to ensure prerequisites at certain points.

jerry-core/config.h

jerry-core/ecma/operations/ecma-lex-env.c

jerry-core/jcontext/jcontext.h

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

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

jerry-core/vm/vm.c

jerry-core/ecma/operations/ecma-lex-env.c

jerry-core/ecma/operations/ecma-lex-env.h

jerry-core/include/jerryscript-port.h

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

galpeter · 2019-01-11T10:41:49Z

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

@@ -308,7 +313,8 @@ parser_parse_enclosed_expr (parser_context_t *context_p) /**< context */
 * Parse var statement.
 */
 static void
-parser_parse_var_statement (parser_context_t *context_p) /**< context */
+parser_parse_var_statement (parser_context_t *context_p, /**< context */
+                            lexer_literal_t *variable_name) /**< variable name */


What if we return with that value?

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

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

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

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

jerry-port/default/default-io.c

tests/jerry/es2015/module-imported.js

tools/build.py

tools/run-tests.py

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

.travis.yml

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

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

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

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

docs/14.MODULE-SYSTEM.md

jerry-core/config.h

jerry-core/ecma/operations/ecma-lex-env.h

jerry-core/jcontext/jcontext.h

jerry-core/module/module.c

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

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

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

rerobika

LGTM (informal)

zherczeg · 2019-02-26T13:46:56Z

docs/14.MODULE-SYSTEM.md

+# ES6 module support for JerryScript
+
+The module system allows users to write import and export statements in scripts. Therefore the logic of the application could be separated in custom modules.
+The standard's relevant part can be found [here]( https://www.ecma-international.org/ecma-262/6.0/#sec-modules).


1 space before http

zherczeg · 2019-02-26T13:51:37Z

jerry-core/config.h

@@ -50,6 +50,12 @@
 # define CONFIG_DISABLE_ES2015_TYPEDARRAY_BUILTIN
 #endif /* CONFIG_DISABLE_ES2015 */

+#if (defined (CONFIG_DISABLE_ES2015) || !defined (JERRY_ENABLE_MODULE_SYSTEM))


I would add this to the ifdef CONFIG_DISABLE_ES2015 above.

zherczeg · 2019-02-26T13:58:37Z

jerry-core/module/module.c

+  for (uint16_t i = 0; i < export_node_p->module_request_count; i++)
+  {
+    parser_module_names_t *next_p = current_p->next_p;
+    ecma_string_t *import_name_p = ecma_new_ecma_string_from_utf8 (current_p->local_name.value_p,


Why are these not strings? Magic strings could be used and no conversion would be needed here.

zherczeg · 2019-02-26T14:00:22Z

jerry-core/ecma/operations/ecma-lex-env.c

+  JERRY_ASSERT (lex_env_p != NULL);
+  JERRY_ASSERT (ecma_is_lexical_environment (lex_env_p));
+
+  ecma_module_lex_envs_t *new_wrapper_p;


new_module_lex_env.

zherczeg · 2019-02-26T14:02:08Z

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

+typedef struct parser_module_names
+{
+  parser_module_utf8_string_t import_name; /**< local name of the import - export item */
+  parser_module_utf8_string_t local_name;  /**< import name of the import - export item */


Is this a local->import map? If yes, what about using objects for this (for faster property search).

Thanks for your review, I’ve updated the patch.

import { a as b } from 'module.js';

If a specific module (module.js) has been run, its lexical environment is being checked. If that lexical environment contains a property with the given name (a), then a new property is created in the parent context with the imported name (b) and assigned with the value of a.

If objects are being used for the local name -> import name mapping, the key should be the module path. This object should contain as many key-value pairs as many import items were in the import statement.

I think in this scenario objects won’t worth it, because we now have 1 property search per import item (in the lexical environment), then we should have min. 2 (in the object too).

zherczeg · 2019-02-26T14:09:06Z

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

@@ -90,6 +90,8 @@ typedef enum
  PARSER_ERR_LEFT_BRACE_EXPECTED,                     /**< left brace expected */
  PARSER_ERR_RIGHT_PAREN_EXPECTED,                    /**< right paren expected */
  PARSER_ERR_RIGHT_SQUARE_EXPECTED,                   /**< right square expected */
+  PARSER_ERR_LEFT_PAREN_MULTIPLY_LITERAL_EXPECTED,    /**< left paren or multiply or literal expected */


Isn't this module specific?

LaszloLango · 2019-02-27T08:31:45Z

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

+
+    if (current_p != NULL && current_length == import_name_p->prop.length)
+    {
+      if (memcmp (current_p, import_name_p->u.char_p, current_length) == 0)


Merge these two if statements

LaszloLango · 2019-02-27T08:33:04Z

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

+  while (stored_imports != NULL)
+  {
+    if (stored_imports->script_path.length == module_node_p->script_path.length
+       && memcmp (stored_imports->script_path.value_p,


wrong indentation

LaszloLango · 2019-02-27T08:33:57Z

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

+                                bool is_import_item) /**< given item is import item */
+{
+  if (is_import_item &&
+      parser_module_check_for_duplicates (context_p, module_node_p, import_name_p))


Move && to this line

LaszloLango · 2019-02-27T08:34:16Z

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

+
+  parser_module_context_t *parent_context_p = JERRY_CONTEXT (module_top_context_p);
+  if ((parent_context_p == NULL || parent_context_p->exports_p == NULL || parent_context_p->imports_p == NULL)
+    && module_context_p->exports_p != NULL)


wrong indentation

LaszloLango · 2019-02-27T08:35:29Z

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

+
+    if (context_p->token.type == LEXER_RIGHT_BRACE
+        || (context_p->token.type == LEXER_LITERAL
+        && lexer_compare_raw_identifier_to_current (context_p, "from", 4)))


wrong indentation

LaszloLango · 2019-02-27T08:44:07Z

jerry-port/default/default-io.c

+ * @return the source of the file
+ */
+uint8_t *
+jerry_port_module_read_source (const char *file_name_p, /**< file name */


The documentation update is missing for the new port API functions. BTW, why are these needed? Might be better to put the port API changes into a separate PR.

Thanks for the review, I’ve updated the port API documentation. The port functions are necessary for the modules, because without these Jerry could not open custom files (source files). For this reason, the changes can’t be separated into another PR.

rtakacs · 2019-03-05T11:57:16Z

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

+
+    int request_count = exports_p->module_request_count + module_node_p->module_request_count;
+
+    if (request_count < MAX_IMPORT_COUNT)


if ((uint16_t) request_count < MAX_IMPORT_COUNT) // error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]

rtakacs · 2019-03-05T11:57:25Z

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

+      stored_imports->module_names_p = module_node_p->module_names_p;
+
+      int request_count = stored_imports->module_request_count + module_node_p->module_request_count;
+      if (request_count < MAX_IMPORT_COUNT)


if ((uint16_t) request_count < MAX_IMPORT_COUNT) // error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]

rtakacs · 2019-03-05T13:05:04Z

Other notes:

jerry-core/module/module.c should be renamed to module_import.c (or something like this). An other module.c already exist in the jerry-ext folder.
The Makefile of the NuttX target doesn't like if a filename is used multiple times (see nuttx-stm32f4/Makefile#L47 - All the archive files are copied from JerryScript to NuttX. The copied archives are extracted to get the object files. If there are two module.o files (in different archive files) the (jerry-core)module.o will be overwritten by the (jerry-ext)module.o file). This causes linker errors.
The targets/nuttx-stm32f4/jerry-main.c implements jerry-port-log. This functions is also implemented in jerry-port/default/default-io.c. Since both files are compiled, there is a linker error that Travis shows. (I've removed jerry-port-log from jerry-main.c to solve the linker error.)
Test files contain paths (tests/jerry/...) that should be opened. These paths can be different on boards - e.g. js-remote-test mounts tests/jerry/ folder as /test/.

vincedani · 2019-03-07T16:32:11Z

Other notes:

jerry-core/module/module.c should be renamed to module_import.c (or something like this). An other module.c already exist in the jerry-ext folder.
The Makefile of the NuttX target doesn't like if a filename is used multiple times (see nuttx-stm32f4/Makefile#L47 - All the archive files are copied from JerryScript to NuttX. The copied archives are extracted to get the object files. If there are two module.o files (in different archive files) the (jerry-core)module.o will be overwritten by the (jerry-ext)module.o file). This causes linker errors.

The targets/nuttx-stm32f4/jerry-main.c implements jerry-port-log. This functions is also implemented in jerry-port/default/default-io.c. Since both files are compiled, there is a linker error that Travis shows. (I've removed jerry-port-log from jerry-main.c to solve the linker error.)

Test files contain paths (tests/jerry/...) that should be opened. These paths can be different on boards - e.g. js-remote-test mounts tests/jerry/ folder as /test/.

Thank you @rtakacs for your review. I've moved and renamed the module.{h, c} as you suggested.

The paths in the test files are adjusted to the ./tools/runners/run-test-suite.sh, which runs on Travis too. I think we should figure out how to adjust these paths when the the js-remote-test environment is running.

zherczeg

Only minor changes. Good for a first patch.

zherczeg · 2019-03-13T13:38:26Z

jerry-core/ecma/base/ecma-module.c

+                   size_t path_size, /**< length of the path */
+                   const char *source_p, /**< module source */
+                   size_t source_size, /**< length of the source */
+                   parser_module_node_t *module_node_p) /**< module node */


Misaligned arguments

zherczeg · 2019-03-13T13:38:48Z

jerry-core/ecma/base/ecma-module.c

+                                              path_length,
+                                              (const char *) buffer_p,
+                                              size,
+                                              current_p);


Ditto. Please fix all of them.

JerryScript-DCO-1.0-Signed-off-by: Daniel Vince vinced@inf.u-szeged.hu

LaszloLango

LGTM

vincedani changed the title ~~[WIP] ES6 module system~~ Implement ES2015 module system Jan 10, 2019

rerobika requested changes Jan 10, 2019

View reviewed changes

galpeter reviewed Jan 11, 2019

View reviewed changes

rerobika requested changes Jan 17, 2019

View reviewed changes

rerobika requested changes Jan 18, 2019

View reviewed changes

vincedani commented Jan 18, 2019

View reviewed changes

jerry-core/parser/js/js-parser-module.c Outdated Show resolved Hide resolved

vincedani commented Jan 18, 2019

View reviewed changes

jerry-core/parser/js/js-parser-module.c Show resolved Hide resolved

rerobika requested changes Feb 13, 2019

View reviewed changes

rerobika requested changes Feb 14, 2019

View reviewed changes

rerobika approved these changes Feb 20, 2019

View reviewed changes

zherczeg requested changes Feb 26, 2019

View reviewed changes

LaszloLango requested changes Feb 27, 2019

View reviewed changes

rtakacs requested changes Mar 5, 2019

View reviewed changes

vincedani mentioned this pull request Mar 7, 2019

[Part II] Implement ES2015 module system #2792

Merged

zherczeg approved these changes Mar 13, 2019

View reviewed changes

Part I: Implement ES2015 module system.

e3a8cc4

JerryScript-DCO-1.0-Signed-off-by: Daniel Vince vinced@inf.u-szeged.hu

LaszloLango approved these changes Mar 18, 2019

View reviewed changes

LaszloLango merged commit 4123f35 into jerryscript-project:master Mar 18, 2019

LaszloLango mentioned this pull request Mar 18, 2019

Migration Guide #2419

Merged

zherczeg mentioned this pull request Mar 19, 2019

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

Closed


		int request_count = exports_p->module_request_count + module_node_p->module_request_count;

		if (request_count < MAX_IMPORT_COUNT)

Implement ES2015 module system #2599

Implement ES2015 module system #2599

Uh oh!

Conversation

vincedani commented Nov 16, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

vincedani commented Nov 16, 2018 •

edited

Loading

rtakacs Mar 5, 2019 •

edited

Loading

rtakacs Mar 5, 2019 •

edited

Loading