Njs: setImmediate() is broken inside module

[drsm]

$ cat bug.js

export default { bug: true };

setImmediate(function() {
    console.log('immediate');
});
$ build/njs
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> import bug from 'bug.js'
ReferenceError: "setImmediate" is not defined in bug.js:0
>> bug
ReferenceError: "bug" is not defined in shell:1
>> setImmediate
Segmentation fault

[hongzhidao]

@drsm thanks.

Try the patch please.

diff -r 53dc000e8c14 njs/njs_variable.c
--- a/njs/njs_variable.c    Sat Mar 02 20:31:10 2019 +0800
+++ b/njs/njs_variable.c    Sat Mar 23 15:16:55 2019 +0800
@@ -448,6 +448,7 @@ static njs_ret_t
 njs_variable_reference_resolve(njs_vm_t *vm, njs_variable_reference_t *vr,
     njs_parser_scope_t *node_scope)
 {
+    njs_variable_t      *var;
     nxt_lvlhsh_query_t  lhq;
     njs_parser_scope_t  *scope, *previous;

@@ -460,7 +461,15 @@ njs_variable_reference_resolve(njs_vm_t

     for ( ;; ) {
         if (nxt_lvlhsh_find(&scope->variables, &lhq) == NXT_OK) {
-            vr->variable = lhq.value;
+            var = lhq.value;
+
+            if (previous && previous->module
+                && var->value.type != NJS_FUNCTION)
+            {
+                goto not_found;
+            }
+
+            vr->variable = var;

             if (scope->type == NJS_SCOPE_SHIM) {
                 scope = previous;
@@ -488,16 +497,20 @@ njs_variable_reference_resolve(njs_vm_t
             return NXT_OK;
         }

-        if (scope->module || scope->parent == NULL) {
+        if (scope->parent == NULL) {
             /* A global scope. */
-            vr->scope = scope;
-
-            return NXT_DECLINED;
+            goto not_found;
         }

         previous = scope;
         scope = scope->parent;
     }
+
+not_found:
+
+    vr->scope = scope;
+
+    return NXT_DECLINED;
 }


diff -r 53dc000e8c14 njs/test/module/normal.js
--- a/njs/test/module/normal.js Sat Mar 02 20:31:10 2019 +0800
+++ b/njs/test/module/normal.js Sat Mar 23 15:16:55 2019 +0800
@@ -32,4 +32,12 @@ if (lib1_2.get() != 1) {
     console.log("failed!");
 }

+if (isNaN() === false) {
+    console.log("failed!");
+}
+
+if (isNaN(123) === true) {
+    console.log("failed!");
+}
+
 console.log("passed!");

[xeioex]

@hongzhidao, BTW it does not fail without the patch (will do), otherwise looks good.

+if (isNaN() === false) {
+    console.log("failed!");
+}
+
+if (isNaN(123) === true) {
+    console.log("failed!");
+}
+

[xeioex]

@hongzhidao

+            var = lhq.value;
+
+            if (previous && previous->module
+                && var->value.type != NJS_FUNCTION)
+            {
+                goto not_found;
+            }
+
+            vr->variable = var;

BTW, why only NJS_FUNCTION? we also have global objects, like JSON, Math.

[hongzhidao]

@hongzhidao

+            var = lhq.value;
+
+            if (previous && previous->module
+                && var->value.type != NJS_FUNCTION)
+            {
+                goto not_found;
+            }
+
+            vr->variable = var;

BTW, why only NJS_FUNCTION? we also have global objects, like JSON, Math.

You are right. Help rework. @xeioex

/* TODO: once */
    switch (type) {
    case NJS_OBJECT:
        index = node->token - NJS_TOKEN_FIRST_OBJECT;
        var->value.data.u.object = &vm->shared->objects[index];
        break;

    case NJS_FUNCTION:
        index = node->token - NJS_TOKEN_FIRST_FUNCTION;
        var->value.data.u.function = &vm->shared->functions[index];
        break;

    default:
        return NXT_ERROR;
    }

    var->value.type = type;
    var->value.data.truth = 1;

[xeioex]

@hongzhidao

BTW, this https://gist.github.com/xeioex/83f8a91447f075e29d220e9bc54647b8
passed all tests as well.

[hongzhidao]

@xeioex

But global variables are not allowed to access in modules. (On it)

[xeioex]

@hongzhidao

But global variables are not allowed to access in modules

Agree. But it looks like a separate issue. Also, just type check is not enough.

@drsm
The global functions should be part of a global this object. If this object is not available in module code, isn't it means that setImmediate() and other function should not be unavailable as well?

[hongzhidao]

@xeioex

Take a look first.

diff -r aaf68d999559 njs/njs_variable.c
--- a/njs/njs_variable.c    Sat Mar 23 14:57:52 2019 +0300
+++ b/njs/njs_variable.c    Sat Mar 23 22:28:45 2019 +0800
@@ -448,6 +448,7 @@ static njs_ret_t
 njs_variable_reference_resolve(njs_vm_t *vm, njs_variable_reference_t *vr,
     njs_parser_scope_t *node_scope)
 {
+    njs_variable_t      *var;
     nxt_lvlhsh_query_t  lhq;
     njs_parser_scope_t  *scope, *previous;

@@ -460,7 +461,22 @@ njs_variable_reference_resolve(njs_vm_t

     for ( ;; ) {
         if (nxt_lvlhsh_find(&scope->variables, &lhq) == NXT_OK) {
-            vr->variable = lhq.value;
+            var = lhq.value;
+
+            if (previous && previous->module) {
+                /*
+                 * builtin objects and functions are allowed to access in
+                 * modules.
+                 */
+
+                if (var->value.type != NJS_OBJECT
+                    && var->value.type != NJS_FUNCTION)
+                {
+                    goto not_found;
+                }
+            }
+
+            vr->variable = var;

             if (scope->type == NJS_SCOPE_SHIM) {
                 scope = previous;
@@ -488,16 +504,19 @@ njs_variable_reference_resolve(njs_vm_t
             return NXT_OK;
         }

-        if (scope->module || scope->parent == NULL) {
+        if (scope->parent == NULL) {
             /* A global scope. */
-            vr->scope = scope;
-
-            return NXT_DECLINED;
+            goto not_found;
         }

         previous = scope;
         scope = scope->parent;
     }
+
+not_found:
+
+    vr->scope = scope;
+    return NXT_DECLINED;
 }


diff -r aaf68d999559 njs/test/module/normal.js
--- a/njs/test/module/normal.js Sat Mar 23 14:57:52 2019 +0300
+++ b/njs/test/module/normal.js Sat Mar 23 22:28:45 2019 +0800
@@ -6,30 +6,36 @@ import crypto from 'crypto';
 var h = crypto.createHash('md5');
 var hash = h.update('AB').digest('hex');

+var fails = 0;
 if (lib1.hash() != hash) {
-    console.log("failed!");
+    fails++;
 }

 if (lib2.hash() != hash) {
-    console.log("failed!");
+    fails++;
 }

 if (lib1.get() != 0) {
-    console.log("failed!");
+    fails++;
 }

 if (lib1_2.get() != 0) {
-    console.log("failed!");
+    fails++;
 }

 lib1.inc();

 if (lib1.get() != 1) {
-    console.log("failed!");
+    fails++;
 }

 if (lib1_2.get() != 1) {
-    console.log("failed!");
+    fails++;
 }

-console.log("passed!");
+if (JSON.stringify({}) != "{}") {
+    fails++;
+}
+
+setImmediate(console.log,
+             fails ? "failed: " + fails : "passed!");

[hongzhidao]

@xeioex

Tried in chrome.

// file2.js
import './file1.js';

// file1.js
console.log(isNaN());
console.log(Math);
console.log(this);

file1.js:3 Math {abs: ƒ, acos: ƒ, acosh: ƒ, asin: ƒ, asinh: ƒ, …}
file1.js:4 undefined

And normal global variables are not allowed to access in modules.

[xeioex]

And normal global variables are not allowed to access in modules.

So, we also need a test for this.

[hongzhidao]

And normal global variables are not allowed to access in modules.

So, we also need a test for this.

On it.

[xeioex]

@hongzhidao

BTW, how do you import a file in chrome?

// in Developer tools -> console
> import f from './file1.js'
VM136:1 Uncaught SyntaxError: Unexpected identifier

[hongzhidao]

@xeioex

cat test.html (note that add type with 'module' value)

<script type="module" src="file2.js"></script>

cat file2.js

import './file1.js';

cat file1.js

console.log(isNaN());
console.log(Math);
console.log(this);

[xeioex]

@hongzhidao

cat test.html:

<script>
var my = 'WAKA';
console.log(this); 
</script>

<script type="module">
console.log(isNaN());
console.log(Math);
console.log(my);
console.log(this); 
</script>

output:

Window {postMessage: ƒ, blur: ƒ, focus: ƒ, close: ƒ, parent: Window, …}
true
Math {abs: ƒ, acos: ƒ, acosh: ƒ, asin: ƒ, asinh: ƒ, …}
WAKA
undefined

It seems only global this is unavailable, but everything else is available from the global scope.

[hongzhidao]

@xeioex

It seems only global this is unavailable, but everything else is available from global scope.

Yes. It does in chrome.

I think I misunderstand the usage of variable accessing in modules.
See this. It seems global variables are allowed to access in modules.
http://exploringjs.com/es6/ch_modules.html#_benefit-variable-checking

It only says that the top level scope of variables in modules is a local scope.
If yes, your patch is right enough.

@drsm suggestions are welcome.

[xeioex]

@hongzhidao

It only says that the top level scope of variables in modules is a local scope.

Agree. So, let's wait for @drsm response, otherwise I plan to commit https://gist.github.com/xeioex/83f8a91447f075e29d220e9bc54647b8

[hongzhidao]

@xeioex

I think we can commit it first, then I'll recommit the patch of arrow function.
This ticket can be kept.

In next week.

1. Review the patch of arrow function.
2. Start the feature of template literal.
3. Improve modules feature.

[xeioex]

@drsm

Please close the ticket if it is OK.

[drsm]

deleted

[drsm]

@xeioex @hongzhidao

$ cat bug.js 
export default {
    f: function() {
        console.log('f: ', typeof namespace_pollution);
    }
};

setImmediate(function() {
    console.log('immediate: ', typeof namespace_pollution);
});
$ build/njs
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> var namespace_pollution = 123;
undefined
>> import bug from 'bug.js';
undefined
'immediate: ' 'number'
>> bug.f()
'f: ' 'number'
undefined
>>

and

$ cat bug.js 
export default {
    f: function() {
        console.log('f: ', typeof namespace_pollution, namespace_pollution);
    }
};
setImmediate(function() {
    console.log('immediate: ', typeof namespace_pollution, namespace_pollution);
});

$ build/njs
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> import bug from 'bug.js';
ReferenceError: "namespace_pollution" is not defined in bug.js:8
>> var namespace_pollution = 123;
Segmentation fault

[drsm]

@xeioex @hongzhidao

cat test.html:

<script>
var my = 'WAKA';
console.log(this); 
</script>

<script type="module">
console.log(isNaN());
console.log(Math);
console.log(my);
console.log(this); 
</script>

output:

Window {postMessage: ƒ, blur: ƒ, focus: ƒ, close: ƒ, parent: Window, …}
true
Math {abs: ƒ, acos: ƒ, acosh: ƒ, asin: ƒ, asinh: ƒ, …}
WAKA
undefined

It seems only global this is unavailable, but everything else is available from the global scope.

I'm unable to reproduce that in the latest nodejs 'cause the file is executed as a module
and import is unavailable from REPL:

# node --experimental-modules test.mjs
(node:803) ExperimentalWarning: The ESM module loader is experimental.
1.0 this: undefined, test: undefined
1.1 this: undefined, test: undefined
# node
> var a = 123;
undefined
> this.a
123
> import x from './test.mjs';
Thrown:
import x from './test.mjs';
       ^

SyntaxError: Unexpected identifier
> 
# cat test.mjs 
console.log(`1.0 this: ${typeof this}, test: ${typeof test}`);

setImmediate(() => {
    console.log(`1.1 this: ${typeof this}, test: ${typeof test}`);
});
# node --version
v11.12.0

maybe, the best way is to get rid of the global mutable scope completely, so every file is executed as a module with it's own "global" scope. not sure yet.

[hongzhidao]

deleted.

[drsm]

@hongzhidao with the patch above:

$ cat bug.js 
export default {
    f: function() {
        console.log('f: ', typeof gtest, gtest);
    }
};
$ build/njs
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> import bug from 'bug.js';
ReferenceError: "gtest" is not defined in bug.js:3
>> bug
ReferenceError: "bug" is not defined in shell:1
>> import bug from 'bug.js';
undefined
>> bug
undefined
>>

[hongzhidao]

@drsm

Fixed modules reset in accumulative mode.

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1553421943 -28800
# Node ID e1e6c0ca6b50b592a77880c3e1a7f004eb3d30a9
# Parent  81303df0fd67647cada20e23f60b9aaf9e7e5e92
Fixed modules reset in accumulative mode.

diff -r 81303df0fd67 -r e1e6c0ca6b50 njs/njs.c
--- a/njs/njs.c Sat Mar 23 16:39:40 2019 +0300
+++ b/njs/njs.c Sun Mar 24 18:05:43 2019 +0800
@@ -226,6 +226,10 @@ njs_vm_compile(njs_vm_t *vm, u_char **st
         return NJS_ERROR;
     }

+    if (vm->modules != NULL && vm->options.accumulative) {
+        njs_modules_reset(vm);
+    }
+
     parser = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_parser_t));
     if (nxt_slow_path(parser == NULL)) {
         return NJS_ERROR;
diff -r 81303df0fd67 -r e1e6c0ca6b50 njs/njs_module.c
--- a/njs/njs_module.c  Sat Mar 23 16:39:40 2019 +0300
+++ b/njs/njs_module.c  Sun Mar 24 18:05:43 2019 +0800
@@ -37,6 +37,37 @@ static njs_module_t *njs_module_add(njs_
 static nxt_int_t njs_module_insert(njs_vm_t *vm, njs_module_t *module);


+void
+njs_modules_reset(njs_vm_t *vm)
+{
+    nxt_str_t           *name;
+    nxt_uint_t          i;
+    njs_module_t        **item, *module;
+    nxt_lvlhsh_query_t  lhq;
+
+    item = vm->modules->start;
+
+    for (i = 0; i < vm->modules->items; i++) {
+        module = *item;
+
+        if (!module->function.native) {
+            name = &module->name;
+
+            lhq.key = *name;
+            lhq.key_hash = nxt_djb_hash(name->start, name->length);
+            lhq.proto = &njs_modules_hash_proto;
+            lhq.pool = vm->mem_pool;
+
+            (void) nxt_lvlhsh_delete(&vm->modules_hash, &lhq);
+        }
+
+        item++;
+    }
+
+    nxt_array_reset(vm->modules);
+}
+
+
 nxt_int_t
 njs_module_load(njs_vm_t *vm)
 {
@@ -63,22 +94,14 @@ njs_module_load(njs_vm_t *vm)
         } else {
             ret = njs_vm_invoke(vm, &module->function, NULL, 0, module->index);
             if (ret == NXT_ERROR) {
-                goto done;
+                return ret;
             }
         }

         item++;
     }

-    ret = NXT_OK;
-
-done:
-
-    if (vm->options.accumulative) {
-        nxt_array_reset(vm->modules);
-    }
-
-    return ret;
+    return NXT_OK;
 }


diff -r 81303df0fd67 -r e1e6c0ca6b50 njs/njs_module.h
--- a/njs/njs_module.h  Sat Mar 23 16:39:40 2019 +0300
+++ b/njs/njs_module.h  Sun Mar 24 18:05:43 2019 +0800
@@ -16,6 +16,7 @@ typedef struct {
 } njs_module_t;


+void njs_modules_reset(njs_vm_t *vm);
 nxt_int_t njs_module_load(njs_vm_t *vm);
 nxt_int_t njs_parser_module(njs_vm_t *vm, njs_parser_t *parser);
 njs_ret_t njs_module_require(njs_vm_t *vm, njs_value_t *args,

Thanks again.

@drsm I need more work, the patch is still not ideal.

[drsm]

@hongzhidao thanks!

[hongzhidao]

@drsm @xeioex

What I'm not sure is, in accumulative mode, will modules only run once while import module many times?
Such as.

> import m from 'm.js';  // first time it runs
> import m from 'm.js';  // does it run again?
...

[drsm]

@hongzhidao
REPL will run it again.
I think it is ok.
BTW, I can't find any other implementation which supports import in REPL.

[hongzhidao]

@hongzhidao
REPL will run it again.
I think it is ok.
BTW, I can't find any other implementation which supports import in REPL.

If yes, I think the patch above can be considered.