njs: bind() segfault.

@drsm @xeioex

-Werror=unused-but-set-variable

Good gcc option, we can add into njs. And I think it’s also needed in UNIT 😃

@hongzhidao

the problem arise when we assign a value to the variable generated by the function definition:

>> function f(a) { var q = g; function g(b) { return a + b; }; return q; }; f(1)(2);
3
>> function f(a) { var q = g; var g = 1; function g(b) { return a + b; }; return q; }; f(1)(2);
Segmentation fault
>> function f(a) { var g = g; function g(b) { return a + b; }; return g; }; f(1)(2);
Segmentation fault

the code generated is somewhat different (but should be identical):

>> function f(a) { var q = g; g = 1; function g(b) { return a + b; }; return q; }; f(1)(2);
shell:g
00000 ADD               0004 0015 0013
00040 RETURN            0004
shell:f
00000 MOVE              0015 0013
**00032 OBJECT COPY       0004 0014**
00064 MOVE              0014 556462282A60
00096 RETURN            0004
shell:main
00000 FUNCTION FRAME    556462282A50 1
00032 MOVE              0002 556462282A60
00064 FUNCTION CALL     5564622A3780
00088 FUNCTION FRAME    5564622A3780 1
00120 MOVE              0002 5564622A3790
00152 FUNCTION CALL     5564622A3780
00176 STOP              5564622A3780

3
>> function f(a) { var q = g; var g = 1; function g(b) { return a + b; }; return q; }; f(1)(2);
shell:g
00000 ADD               0004 0015 0013
00040 RETURN            0004
shell:f
00000 MOVE              0015 0013
**00032 MOVE              0004 0014**
00064 MOVE              0014 556462282A60
00096 RETURN            0004
shell:main
00000 FUNCTION FRAME    556462282A50 1
00032 MOVE              0002 556462282A60
00064 FUNCTION CALL     5564622A38B0
00088 FUNCTION FRAME    5564622A38B0 1
00120 MOVE              0002 5564622A3790
00152 FUNCTION CALL     5564622A38B0
00176 STOP              5564622A38B0

Segmentation fault

test:

(function(a) { var g = f; var f = 1; function f() { return a; } return g; })(42)()

 * explain why do we need function->closure?
 * what's the diffenence of function->closures and vm->active_frame->closures?

vm->active_frame->closures is array of parent functions scopes that are referenced in nested functions. function->closure means that the function has closures (function->closures) captured on function creation.

When parent function calls nested function, the nested function should use current active frame closures. When parent function creates closure from nested function (by returning it or by assigning it to variable or property) the nested function should capture current active frame closures. And when the nested closure function will later be called after parent function will complete, the nested closure function should use the captured closures.

@hongzhidao

Can you try it again? It seems work well.

I mean this patch:

# HG changeset patch
# User hongzhidao <hongzhidao@gmail.com>
# Date 1555098777 -28800
# Node ID 76c58f358b013f51f6183f75750bb1b825055d27
# Parent  ace6f73dff8d51f79cde1c14aa17d26bb3fb70f0
Simplified shim variable and scope.

We plan to process it later, it’s not clear now.

@drsm

Can you try it again? It seems work well.

changeset:   892:48d2c60ec069
tag:         tip
user:        hongzhidao <hongzhidao@gmail.com>
date:        Sat Apr 13 23:38:53 2019 +0800
summary:     Fixed function declaration.

changeset:   891:ace6f73dff8d
user:        hongzhidao <hongzhidao@gmail.com>
date:        Sat Apr 13 01:11:49 2019 +0800
summary:     Making parser hoist more generic.

./build/njs
interactive njs 0.3.1

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

>> if (true) { var i = 0; ++i } typeof i
'number'

@xeioex

Fixed segfault.

# HG changeset patch
# User hongzhidao <hongzhidao@gmail.com>
# Date 1555169933 -28800
# Node ID 48d2c60ec0690a1a784ae630d93f88af9461169d
# Parent  ace6f73dff8d51f79cde1c14aa17d26bb3fb70f0
Fixed function declaration.

diff -r ace6f73dff8d -r 48d2c60ec069 njs/njs_variable.c
--- a/njs/njs_variable.c	Sat Apr 13 01:11:49 2019 +0800
+++ b/njs/njs_variable.c	Sat Apr 13 23:38:53 2019 +0800
@@ -65,6 +65,11 @@ njs_variable_add(njs_vm_t *vm, njs_parse

     if (nxt_lvlhsh_find(&scope->variables, &lhq) == NXT_OK) {
         var = lhq.value;
+
+        if (type == NJS_VARIABLE_FUNCTION) {
+            var->type = type;
+        }
+
         return var;
     }

diff -r ace6f73dff8d -r 48d2c60ec069 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c	Sat Apr 13 01:11:49 2019 +0800
+++ b/njs/test/njs_unit_test.c	Sat Apr 13 23:38:53 2019 +0800
@@ -6413,6 +6413,16 @@ static njs_unit_test_t  njs_test[] =
     { nxt_string("function f() { var a = 1; function baz() { return a; } return baz; } f().bind()()"),
       nxt_string("1") },

+    { nxt_string("function f() { var t = 1; function baz() { return t; } return baz; }"
+                 "f().bind()();"),
+      nxt_string("1") },
+
+    { nxt_string("(function(a) { var s = typeof g, q = g; var g = 1; s += typeof g; function g(b) { return a + b }; return q; })(1)(2)"),
+      nxt_string("3")},
+
+    { nxt_string("(function(a) { var g = f; var f = 1; function f() { return a; } return g; })(42)()"),
+      nxt_string("42") },
+
     { nxt_string("function f(a, b) { return a + b }"
                  "f(3,4) === f.bind()(3,4)"),
       nxt_string("true") },

BTW, it seems the code related to shim and function declaration can be simplified, it is a separate topic. I think this ticket can be closed. Finally, the answer to the question is helpful, thanks. Why not need to call njs_generate_children_indexes_release in njs_generate_object_dest_index?

@hongzhidao

Simplified shim variable and scope. https://gist.github.com/hongzhidao/c79d454849f93cd9d7ea4adeef482802

BTW, this patch breaks the following test: https://gist.github.com/xeioex/10d915edfbf7b5395dcf22fcb7fc1aa2 (The test itself is at the bottom, everything else is test harness).

It fixes the following test: https://gist.github.com/64099ab9ef7f8f5f63a8e0771766c98f Everything else is not changed.

@xeioex

Done in. Making njs_parser_node_t more generic. Making parser hoist more generic. Simplified shim variable. Fixed function declaration.

1. Making njs_parser_node_t more generic.

2. Making parser hoist more generic. https://gist.github.com/hongzhidao/1733a63d20b4accc2490ba35ea51bc92

3. Simplified shim variable and scope. https://gist.github.com/hongzhidao/c79d454849f93cd9d7ea4adeef482802 Check whether this looks clear, please.

@hongzhidao

njs/njs_variable.c: In function ‘njs_variable_reference_resolve’:
njs/njs_variable.c:452:34: error: variable ‘previous’ set but not used [-Werror=unused-but-set-variable]
     njs_parser_scope_t  *scope, *previous;

@hongzhidao

Here are patches. https://gist.github.com/hongzhidao/b670852214c946add89c53749893c24e

Thanks in the pipeline. BTW, what is wrong with node->label? 😃

It’s OK, but I want to re-use it with node->name for function name without introducing newly field.

@xeioex take a look.

Simplified function creation. https://gist.github.com/hongzhidao/397fdb1611101fe088535f5d97ed7f09

Now.

1. Fixed segfault.
2. Introduced hoist in parser.
3. Eliminate function creation in parser.
4. Remove shim variable type and shim scope type.
5. Remove NJS_TOKEN_FUNCTION_EXPRESSION.

Left question.

1. Why not need to call njs_generate_children_indexes_release in njs_generate_object_dest_index?

@hongzhidao

// test.js
var g = function f() { }

njs -d test.js

test.js:anonymous   (i)
00000 RETURN            14862A0
test.js:main
00000 FUNCTION          0141 1485C80
00032 STOP              14862A0

See (i), do you agree it’s an anonymous function? Not test.js:f.

no. it is a named function expression. but, as for now, Function.prototype.name is not supported. and the name f is not bound to any variable in the module scope of test.js.

I think imports should go first, as they are just links from one module’s scope to another (in general). and we cannot rebind them:

OK, This can belongs to module improvement.

@hongzhidao

Does function expression hoist too? And shim expression? var g = function f(a) { return (a > 1) ? a * f(a - 1) : 1 };

no, it’s just a statement that generates a function object. the name f in the example above is bound to the scope of the function object created:

>> var f = 'abc', g = function f(a) { return (a > 1) ? a * f(a - 1) : 1 }; (typeof f) + g(3);
'string6'

@hongzhidao

BTW, we can hoist function declaration if necessary like import statement.

var name; and function name() {} should be hoisted.

https://tc39.github.io/ecma262/#sec-variable-statement

A var statement declares variables that are scoped to the running execution context’s VariableEnvironment. Var variables are created when their containing Lexical Environment is instantiated and are initialized to undefined when created. Within the scope of any VariableEnvironment a common BindingIdentifier may appear in more than one VariableDeclaration but those declarations collectively define only one variable. A variable defined by a VariableDeclaration with an Initializer is assigned the value of its Initializer’s AssignmentExpression when the VariableDeclaration is executed, not when the variable is created.

https://tc39.github.io/ecma262/#sec-block-static-semantics-toplevelvardeclarednames

NOTE At the top level of a function or script, inner function declarations are treated like var declarations.

Thanks. So it seems we needn’t consider variable hoist, since all variables belongs to scope->variables in njs.

@hongzhidao

Another issue (coredump).

how is that different from #126 (comment) example?

1. bind bug.

function foo() {
    var t = 2;

    function baz() {
        t = 3;
    }

    baz.bind()()
}

foo();

1. Notice that variable g is declared before function declaration. It has no concern with bind.

function f(a) { var g; function g(b) { return a + b } return g }
var k = f('a');
k('b')

I looks like to me, it should also be function->closures, but the following test fails for example

No, it should depend on function->closure flag. If it is set, then use function->closures, otherwise vm->active->frame->closures.