Njs: let variable declaration support.

[xeioex]

Initially let and const can be implemented as reference to var declarations.
See this for comparison.

let vs var intro

[hongzhidao]

@xeioex

Take a look.

1. Removed unnecessary njs_parser_var_expression().
2. Making njs_parser_var_statement() more generic.
   https://gist.github.com/hongzhidao/02ec604eb243db631dc392b01f2c2667

See the comment.

diff -r f41e47c8ac96 njs/njs_parser_expression.c
--- a/njs/njs_parser_expression.c   Tue Mar 26 00:42:39 2019 +0800
+++ b/njs/njs_parser_expression.c   Fri Apr 05 20:56:20 2019 +0800
@@ -219,6 +219,8 @@ njs_parser_var_expression(njs_vm_t *vm,
         return token;
     }

+    /* The return token will never be NJS_TOKEN_ASSIGNMENT. */
+
     for ( ;; ) {
         switch (token) {

BTW, it's better to merge these after adding let.

[xeioex]

@hongzhidao

BTW, it's better to merge these after adding let.

what do you mean? The patches look good. I plan to commit them. Thanks.

[hongzhidao]

@hongzhidao

BTW, it's better to merge these after adding let.

what do you mean? The patches look good. I plan to commit them. Thanks.

Well, it's ok. No problem.

[hongzhidao]

@drsm @xeioex

Can you help check the rules of let?

My thoughts.

1. Variables declared by let have their scope in the block.
2. In each scope, let should be used at the top, this means:
   a) Reference can't be used before let declaration with the same variable. (throw is not defined)
   b) var declaration cant be used before let declaration with the same variable. (throw has been declared)

Here's the patch.
https://gist.github.com/hongzhidao/7b8ffeb12db7e10bc3d142f16a3db138

Welcome to test, thanks :)

[drsm]

@hongzhidao

node:

if (false) let y = 1;
           ^^^
SyntaxError: Lexical declaration cannot appear in a single-statement context

[hongzhidao]

@hongzhidao

node:

if (false) let y = 1;
           ^^^
SyntaxError: Lexical declaration cannot appear in a single-statement context

Fixed.

In JS, let declaration can't appear in a single-statement context.

What is left. (on it)

1. unit tests

[drsm]

@hongzhidao
is there any TDZ ?

function x() {
    var t = 1;
    if (true) {
        let t = t + 1;
    }
}
x();

$ node tdz.js 
        let t = t + 1;
                ^
ReferenceError: t is not defined

[hongzhidao]

@drsm good reporting.

You are right, I need to review the design again.

[hongzhidao]

@drsm @xeioex

Added let variable declaration support.
https://gist.github.com/hongzhidao/7b8ffeb12db7e10bc3d142f16a3db138

Welcome to test, thanks again.

[drsm]

@hongzhidao
looks good.
but still no TDZ, and i'm not sure about it.
because, this should compile, but won't run:

root@node:~# node --use-strict
> function x() { var x = 1; if (true) { let x = x + 1; } }
undefined
> x()
Thrown:
ReferenceError: x is not defined
    at x (repl:1:47)

TDZ is determined at run-time stage:

function f() {
    console.log(x);
}
f();
let x = 1;

```
$ node tdz.js
/home/drsm/work/njs/tdz.js:2
console.log(x);
^
ReferenceError: x is not defined

```js
function f() {
    console.log(x);
}
let x = 1;
f();
// OK

[drsm]

@hongzhidao @xeioex
maybe use NJS_INVALID value type for variables that are inside TDZ, and throw ReferenceError when they are accessed in that state.
or an another value type should be introduced, as typeof tdzvar must throw also.

[hongzhidao]

TDZ is determined at run-time stage:

Is TDZ introduced by let declaration?

maybe use NJS_INVALID value type for variables that are inside TDZ, and throw ReferenceError when they are accessed in that state.

I'm not sure, since in njs, variables and references are processed completely at compile phase.
Maybe it's a good idea.

function f() {
console.log(x);
}
f();
let x = 1;

It breaks the design of variable. On it.

[hongzhidao]

@xeioex

f();

let x = 1;

function f() {
    console.log(x);
}

ReferenceError: x is not defined

I think it totally change the mechanism of variable, it requires variables to be generated at runtime.
Seems interesting.

In JS.

"use strict"

function foo() {
  b = 1;
  var a;
  let a;
}

This works well since foo is not called.

[drsm]

@hongzhidao

Is TDZ introduced by let declaration?

yes.

I'm not sure, since in njs, variables and references are processed completely at compile phase.
Maybe it's a good idea.

>
for let and const AKA lexically declared variables, an exception should be thrown on any access when they are in uninitialized state (except initialization, that may fail also):

root@node:~# node --use-strict
> function f() { throw new Error('test'); }
undefined
> let x = f()
Thrown:
Error: test
    at f (repl:1:22)
> x
Thrown:
ReferenceError: x is not defined

the state check can be omitted in some situations.
for example, a variable which reside in current scope and we now that it is not initialized, when it accessed just generate throw instruction.
or, when we sure that a variable is initialized, generate code as if it was var declared.

[drsm]

@hongzhidao

so, let them be in the block (for simplicity):

{
f();

let x = 1;

function f() {
    console.log(x);
}

x++;
}

1. variable x is hoisted to top of the block and put into TDZ
2. function f() { is hoisted to top of the block (see #134), the variable f became lexically declared, and initialized to a function object generated.
   inside f() we have to check the state of x at run-time as it
   a) lexically declared
   b) came from an outer scope
   then generate console.log call.
3. function call is generated
4. initializer of x is generated and x became initialized.
5. generate increment statement, there is no need to check x's state

[xeioex]

@hongzhidao

I think it totally change the mechanism of variable, it requires variables to be generated at runtime.
Seems interesting.

Can't we avoid generating variables at runtime using @drsm idea?

use NJS_INVALID value type for variables that are inside TDZ, and throw ReferenceError when they are accessed in that state.

[hongzhidao]

@hongzhidao

I think it totally change the mechanism of variable, it requires variables to be generated at runtime.
Seems interesting.

Can't we avoid generating variables at runtime using @drsm idea?

use NJS_INVALID value type for variables that are inside TDZ, and throw ReferenceError when they are accessed in that state.

On it.

[xeioex]

@hongzhidao

Added let variable declaration support.
https://gist.github.com/hongzhidao/7b8ffeb12db7e10bc3d142f16a3db138

test262 diff:
https://gist.github.com/xeioex/3ef25182a3e9a018ee466c5aab9e9ee4

- - Passed 6785 tests (25.0%)
- - Failed 20334 tests (75.0%)
+ - Passed 6831 tests (25.2%)
+ - Failed 20288 tests (74.8%)

[xeioex]

@hongzhidao @drsm

Variables declared by let have their scope in the block.
In each scope, let should be used at the top, this means:
a) Reference can't be used before let declaration with the same variable. (throw is not defined)
b) var declaration cant be used before let declaration with the same variable. (throw has been declared)

Agree.

A tricky behavior which is also not covered in the patch (not sure we should do it right now).

// language/statements/let/syntax/let-iteration-variable-is-freshly-allocated-for-each-iteration-multi-let-binding
// In a normal for statement the iteration variable is freshly allocated for each iteration.
"use strict";
let a = [];
for (let i = 0; i < 5; ++i) {
  a.push(function () { return i; });
}

for (let k = 0; k < 5; ++k) {
  if (k !== a[k]()) {
      throw new Error("k !== a[k]");
  }
}

[hongzhidao]

@xeioex

function f() {
    x = 1;
}

Now, It cant's pass in njs. Do we make it pass, since it does in JS?

[xeioex]

@hongzhidao

Now, It cant's pass in njs. Do we make it pass, since it does in JS?

Not sure, I think we can leave it as is (as compilation rather than runtime exception). We can return to it later if it is really important.

[drsm]

@hongzhidao

function f() {
    x = 1;
}

Now, It cant's pass in njs. Do we make it pass, since it does in JS?

I think no.

TypeScript does not allow this for example:

root@node:~# ts-node
> function f() { x = 1; }
[eval].ts:1:16 - error TS2304: Cannot find name 'x'.

1 function f() { x = 1; }
                 ~

[drsm]

@xeioex @hongzhidao

test262 diff:
https://gist.github.com/xeioex/3ef25182a3e9a018ee466c5aab9e9ee4

- - Passed 6785 tests (25.0%)
- - Failed 20334 tests (75.0%)
+ - Passed 6831 tests (25.2%)
+ - Failed 20288 tests (74.8%)

how about an idea of adding const without read-only semantics, i think it will fix some tests?

[drsm]

also:

In JS.

"use strict"

function foo() {
  b = 1;
  var a;
  let a;
}

This works well since foo is not called.

looks like a bug in nodejs, plain V8 fails there:

eshost -m -s -x 'function f() { var a; let a; }'
#### ch


#### jsc
[native code]
asyncFunctionResume@[native code]
[native code]
promiseReactionJob@[native code]
SyntaxError: Cannot declare a let variable twice: 'a'.

#### sm

SyntaxError: redeclaration of var a:

#### v8

SyntaxError: Identifier 'a' has already been declared

#### xs

SyntaxError: duplicate variable a

BTW, information about eshost tool may be found there

[hongzhidao]

My thoughts.

1. We can't determine whether reference is OK at compile phase, see the below.

foo();
let x;
function foo() {
    x = 1;              // Reference error.
}

let x;
foo();
function foo() {
    x = 1;              // OK
}



md5-0d5fe7c27a524d4c2b9f2471ddd72386



function foo() {
    x = 1;             
}



md5-8a90215b5ada4d86add5c86ca6709874



function f() { var a; let a; }  // throw SyntaxError

One more question.
Does ReferenceError mean it's throwed at runtime phase?

[drsm]

@hongzhidao

1. We can't determine whether reference is OK at compile phase, see the below.

Agree.

>> let x; foo(); function foo() { x = 1; return x; }
shell:foo
# we have to check the state of 5648FF603070 there, something like:
+ 00000 CHECK            5648FF603070
00000 MOVE              5648FF603070 5648FF6030A0
00032 RETURN            5648FF603070
shell:main
00000 FUNCTION FRAME    5648FF603080 0
00032 FUNCTION CALL     5648FF603090
00056 STOP              5648FF6030C0

1. The same as 1, I think the below OK. (tried in chrome, and typescript is a strongly-typed language)

No. It's not about types, it's about name resolution. We can determine that the variable x doesn't exist in the outer scopes and throw ReferenceError at compile phase. So, there is no changes from the current behavior.

1. We can avoid duplicate declaration at parsing phase.

Yes, actually, we have to.

One more question.
Does ReferenceError mean it's throwed at runtime phase?

Dynamic name resolution was in JS from the beginning, because of global this:

$ node
> function lookupx() { return x; }
undefined
> lookupx()
ReferenceError: x is not defined
    at lookupx (repl:1:22)
> this.x = 42
42
> lookupx()
42
> delete this.x
true
> lookupx()
ReferenceError: x is not defined
    at lookupx (repl:1:22)
> var x = 42
undefined
> lookupx()
42

So, yes, in general, ReferenceError should be thrown at runtime.

And, after the let and const were introduced, there is a mess, that's why there is no global this in the ES6 module code:

$ node --use-strict
> function lookupx() { return x; }
undefined
> lookupx()
ReferenceError: x is not defined
    at lookupx (repl:1:22)
> this.x = 42
42
> lookupx()
42
> delete this.x
true
> lookupx()
ReferenceError: x is not defined
    at lookupx (repl:1:22)
> var x = 43;
undefined
> lookupx()
43
> this.x
43
> delete this.x
TypeError: Cannot delete property 'x' of #<Object>
> this.x = 44;
44
> lookupx()
44
> let x = 45
SyntaxError: Identifier 'x' has already been declared
> function lookupz() { return z; }
undefined
> this.z = 46
46
> let z = 47
undefined
> lookupz()
47
> this.z
46
> z
47
> function lookupy() { return y; }
undefined
> this.y = 48
48
> lookupy()
48
> const y = 49
undefined
> lookupy()
49

[hongzhidao]

Thanks. Will try to introduce njs_vmcode_name_t.

cat /tmp/test.js && ./build/njs -d /tmp/test.js

var x;
x = x + 1;
test.js:main
00000 NAME              0141
00024 ADD               0141 0141 1234930
00064 NAME              0141
00088 STOP              0141

[drsm]

Some thoughts.

There should be a flag that indicate at the compile phase, is the variable initialized or not.
And for var declared variables it should be set to true, as they are always initialized to undefined.

But for let or const variables while no initializer code generated, it set to false (TDZ).
This flag will determine should we generate a NAME instruction before accessing the variable or not.

As initializer can't be branched (if (y) let x = 1 is not allowed), then after it is generated we can access a variable without any checks.

Is it possible?

the NAME instruction handler will be:
if (type == NJS_INVALID) { throw ReferenceError("undefined variable"); }
the name of the variable can be omitted, i think, for performance reasons,
as it just a guard instruction, and correct code will never trigger the exception.

[hongzhidao]

Seems good, it's an optimization.

Now, I plan to change ReferenceError from compile phase to runtime phase without breaking anything.
Then it'll be easier to introduce let.

But another feature needs to be finished, we need to show the error line at runtime. @xeioex

[hongzhidao]

@xeioex @drsm

Take a look again, it's still a draft, welcome to discuss the design.
https://gist.github.com/hongzhidao/8f6bdffa627f00e0c5830b8075151e3f

Main idea.

1. There may be an initialized value at parsing phase with a variable.
   a) for let declaration is invalid, for var declaration is undefined
   b) the default value of references is &var->value.
2. Introduced njs_vmcode_name_t, may can be imroved by @drsm idea.
3. Other details like typeof have not processed yet. Check the design first.

@drsm welcome to test, thanks.

But another feature needs to be finished, we need to show the error line at runtime. @xeioex

need help.

[drsm]

deleted

[hongzhidao]

Updated.
https://gist.github.com/hongzhidao/8f6bdffa627f00e0c5830b8075151e3f

the NAME instruction handler will be:
if (type == NJS_INVALID) { throw ReferenceError("undefined variable"); }
the name of the variable can be omitted, i think, for performance reasons,
as it just a guard instruction, and correct code will never trigger the exception.

In njs_generate_name().

if (var->type != NJS_VARIABLE_LET) {
+        return NXT_OK;
+    }

[drsm]

so, there is about 30% performance loss (on pure math):

$ cat lexical.js 
var i;

function one() {
    console.time();
    var x = 1;
    var y = 1;
    var z = 1;
    function mix() {
        z = x + y;
        x = (z + 1)/(x + 1);
        y = (z + 1)/(y + 1);
    }

    for (i = 0; i < 1000000; ++i) {
        mix();
    }
    console.timeEnd();
    console.log('one', x, y, z);
}

function two() {
    console.time();
    let x = 1;
    let y = 1;
    let z = 1;
    function mix() {
        z = x + y;
        x = (z + 1)/(x + 1);
        y = (z + 1)/(y + 1);
    }

    for (i = 0; i < 1000000; ++i) {
        mix();
    }
    console.timeEnd();
    console.log('two', x, y, z);
}

for (var j = 0; j < 5; ++j) {
    one();
    two();
    console.log('\n');
}

lexical.js:mix
00000 ADD               0015 0025 0035
00040 ADD               0004 0015 5557B65629C0
00080 ADD               0014 0025 5557B65629C0
00120 DIVIDE            0025 0004 0014
00160 ADD               0014 0015 5557B65629C0
00200 ADD               0004 0035 5557B65629C0
00240 DIVIDE            0035 0014 0004
00280 RETURN            5557B65629D0
...
lexical.js:mix
00000 NAME              0025 0001 5557B6562A30
00040 NAME              0035 0001 5557B6562A40
00080 ADD               0015 0025 0035
00120 NAME              0015 0001 5557B6562A50
00160 NAME              0015 0001 5557B6562A60
00200 ADD               0004 0015 5557B65629C0
00240 NAME              0025 0001 5557B6562A70
00280 ADD               0014 0025 5557B65629C0
00320 DIVIDE            0025 0004 0014
00360 NAME              0025 0001 5557B6562A80
00400 NAME              0015 0001 5557B6562A90
00440 ADD               0014 0015 5557B65629C0
00480 NAME              0035 0001 5557B6562AA0
00520 ADD               0004 0035 5557B65629C0
00560 DIVIDE            0035 0014 0004
00600 NAME              0035 0001 5557B6562AB0
00640 RETURN            5557B65629D0
...
default: 106.978494ms
one 1.618033988749895 1.618033988749895 3.23606797749979
default: 140.294661ms
two 1.618033988749895 1.618033988749895 3.23606797749979

if any IO is involved, there will be no difference.
great!

[xeioex]

@hongzhidao @drsm

so, there is about 30% performance loss (on pure math)

30% seems large. Can't we somehow optimize the usage of NAME instructions?

For example, we can track for each var + scope pair that NAME instruction was already generated.

[drsm]

@xeioex

30% seems large. Can't we somehow optimize the usage of NAME instructions?

It is specially crafted near the worst case scenario, and yes it can be optimized.

[hongzhidao]

@xeioex

30% seems large. Can't we somehow optimize the usage of NAME instructions?

It is specially crafted near the worst case scenario, and yes it can be optimized.

On it :)

[hongzhidao]

@xeioex @drsm

Updated.
https://gist.github.com/hongzhidao/8f6bdffa627f00e0c5830b8075151e3f

Take a look again. It's a draft yet, but I think it's close to ready. I'll check the details again.

More unit tests are welcome. @drsm thanks for your great help.

But another feature needs to be finished, we need to show the error line at runtime.

@xeioex, it's better to make it as a separate improvement.

[xeioex]

@hongzhidao

But another feature needs to be finished, we need to show the error line at runtime.

Do you mean to show not only a function where exception happened but also the exact line?

[hongzhidao]

@xeioex

ou mean to show not only a function where exception happened but also the exact line?

Yes.

{ nxt_string("let a = a + 1;"),
      nxt_string("ReferenceError: \"a\" is not defined") },

Now it's unable to show what line throwed at runtime.

[hongzhidao]

@xeioex @drsm

Updated. (including typeof TDZ)
https://gist.github.com/hongzhidao/8f6bdffa627f00e0c5830b8075151e3f

Tomorrow I'll split simplified typeof into a separate patch.

[xeioex]

@hongzhidao

Refactored variable reference.

is already committed.

[drsm]

@hongzhidao
Great work!

here we can eliminate NAME at all, as unneeded:

$ cat lexical2.js 
function one() {
    let a = 1;
    let b = 2;
    let c = 3;
    return a + b + c;
}

function two() {
    var a = 1;
    var b = 2;
    var c = 3;
    return a + b + c;
}

for (var j = 0; j < 5; ++j) {
    var i, acc = 0;

    console.time()
    for (i = 0; i < 10000000; ++i) {
        acc += one();
    }
    console.timeEnd();

    acc = 0;
    console.time()
    for (i = 0; i < 10000000; ++i) {
        acc += two();
    }
    console.timeEnd();
    console.log('');
}

there is about 5% performance loss, but the test is completely synthetic
and i'm not sure it's worth the effort:

lexical2.js:one
00000 MOVE              0004 55E79DE14E80
00032 MOVE              0014 55E79DE14E90
00064 MOVE              0024 55E79DE14EA0
00096 NAME              0004 0001 55E79DE14EB0
00136 NAME              0014 0001 55E79DE14EC0
00176 ADD               0034 0004 0014
00216 NAME              0024 0001 55E79DE14ED0
00256 ADD               0034 0034 0024
00296 RETURN            0034
lexical2.js:two
00000 MOVE              0004 55E79DE14E80
00032 MOVE              0014 55E79DE14E90
00064 MOVE              0024 55E79DE14EA0
00096 ADD               0034 0004 0014
00136 ADD               0034 0034 0024
00176 RETURN            0034

default: 954.699855ms
default: 906.061375ms

default: 948.708216ms
default: 919.134684ms

also:

$ cat lexical3.js 
for (let i = 0; i < 10; ++i) {
    setImmediate(() => console.log(i));
}

[drsm]

the main problem:

$ cat lexical5.js 
var acc = 0;
var tdz = false;
for (var i = 0; i < 5; ++i) {
    if (tdz) {
        acc = x;
        tdz = false;
    }
    let x = 4;
    tdz = true;
}
console.log(acc);

the x variable should be set to uninitialized state when control reaches the start of the block.
or maybe uninitilize it when we're leaving the block.

[drsm]

del

[hongzhidao]

@drsm

Try this, please.
https://gist.github.com/hongzhidao/87f43203282892695d3c3ba9264da0fc

@xeioex

I want to unify the return value of these functions. njs_value_index, njs_variable_index, njs_variable_typeof.

@@ -349,7 +368,7 @@ njs_variable_index(njs_vm_t *vm, njs_par
         return var->index;
     }

-    return NJS_INDEX_ERROR;
+    return NJS_INDEX_NONE;
 }

If OK, I plan to split it into a separate patch (included in simplified typeof patch).

[drsm]

@hongzhidao

var acc = 0;
var tdz = false;
for (var i = 0; i < 5; ++i) {
    if (true) {
        if (tdz) {
            acc = x;
            tdz = false;
        }
        let x = 4;
        tdz = true;
    }
}
console.log(acc);

or just:

 cat lexical5.js 
var acc = 0;
var tdz = false;
for (var i = 0; i < 5; ++i) {
    {
        if (tdz) {
            acc = x;
            tdz = false;
        }
        let x = 4;
        tdz = true;
    }
}
console.log(acc);

[hongzhidao]

Well, I think the key point is the BLOCK. Thanks.
I think it's not hard to resolve, but I need deep into block first, including in parser.

@drsm
BTW, do you think the priority of the issue https://github.com/nginx/njs/issues/134 is higher than the current ticket?

[drsm]

Well, I think the key point is the BLOCK. Thanks.

but if there is no outer loop, then there is no need to protect the block from reentry

[drsm]

@xeioex @hongzhidao

BTW, do you think the priority of the issue #134 is higher than the current ticket?

I think that is better to merge #134 with the current one, and add const without readonly semantics there.
So all parts of lexical scoping puzzle will be in one place.

[xeioex]

@hongzhidao

I want to unify the return value of these functions. njs_value_index, njs_variable_index, njs_variable_typeof.

Looks good to me.

[hongzhidao]

@xeioex

Simplified typeof operation.

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1555675429 -28800
# Node ID a6c82ddff460a0d19dd24f6189ce8252954ec41b
# Parent  6d7a4fb82b25ad918ebee7ab7cb5de70b2b7fa15
Simplified typeof operation.

diff -r 6d7a4fb82b25 -r a6c82ddff460 njs/njs_generator.c
--- a/njs/njs_generator.c   Thu Apr 18 20:51:53 2019 +0300
+++ b/njs/njs_generator.c   Fri Apr 19 20:03:49 2019 +0800
@@ -575,7 +575,7 @@ njs_generate_builtin_object(njs_vm_t *vm
     njs_vmcode_object_copy_t  *copy;

     index = njs_variable_index(vm, node);
-    if (nxt_slow_path(index == NJS_INDEX_ERROR)) {
+    if (nxt_slow_path(index == NJS_INDEX_NONE)) {
         return NXT_ERROR;
     }

@@ -600,7 +600,7 @@ njs_generate_variable(njs_vm_t *vm, njs_
     njs_index_t  index;

     index = njs_variable_index(vm, node);
-    if (nxt_slow_path(index == NJS_INDEX_ERROR)) {
+    if (nxt_slow_path(index == NJS_INDEX_NONE)) {
         return NXT_ERROR;
     }

@@ -622,7 +622,7 @@ njs_generate_var_statement(njs_vm_t *vm,
     lvalue = node->left;

     index = njs_variable_index(vm, lvalue);
-    if (nxt_slow_path(index == NJS_INDEX_ERROR)) {
+    if (nxt_slow_path(index == NJS_INDEX_NONE)) {
         return NXT_ERROR;
     }

@@ -2128,6 +2128,17 @@ njs_generate_typeof_operation(njs_vm_t *
     if (expr->token == NJS_TOKEN_NAME) {
         expr->index = njs_variable_typeof(vm, expr);

+        if (expr->u.reference.variable) {
+            ret = njs_generate_variable(vm, generator, expr);
+            if (nxt_slow_path(ret != NXT_OK)) {
+                return NXT_ERROR;
+            }
+
+        } else {
+            expr->index = njs_value_index(vm, &njs_value_undefined,
+                                          generator->runtime);
+        }
+
     } else {
         ret = njs_generator(vm, generator, node->left);
         if (nxt_slow_path(ret != NXT_OK)) {
@@ -2794,7 +2805,7 @@ njs_generate_try_statement(njs_vm_t *vm,
         /* A "try/catch" case. */

         catch_index = njs_variable_index(vm, node->left);
-        if (nxt_slow_path(catch_index == NJS_INDEX_ERROR)) {
+        if (nxt_slow_path(catch_index == NJS_INDEX_NONE)) {
             return NXT_ERROR;
         }

@@ -2853,7 +2864,7 @@ njs_generate_try_statement(njs_vm_t *vm,
             /* A try/catch/finally case. */

             catch_index = njs_variable_index(vm, node->left->left);
-            if (nxt_slow_path(catch_index == NJS_INDEX_ERROR)) {
+            if (nxt_slow_path(catch_index == NJS_INDEX_NONE)) {
                 return NXT_ERROR;
             }

@@ -3039,7 +3050,7 @@ njs_generate_import_statement(njs_vm_t *
     expr = node->right;

     index = njs_variable_index(vm, lvalue);
-    if (nxt_slow_path(index == NJS_INDEX_ERROR)) {
+    if (nxt_slow_path(index == NJS_INDEX_NONE)) {
         return NXT_ERROR;
     }

diff -r 6d7a4fb82b25 -r a6c82ddff460 njs/njs_variable.c
--- a/njs/njs_variable.c    Thu Apr 18 20:51:53 2019 +0300
+++ b/njs/njs_variable.c    Fri Apr 19 20:03:49 2019 +0800
@@ -349,7 +349,7 @@ njs_variable_index(njs_vm_t *vm, njs_par
         return var->index;
     }

-    return NJS_INDEX_ERROR;
+    return NJS_INDEX_NONE;
 }


diff -r 6d7a4fb82b25 -r a6c82ddff460 njs/njs_vm.c
--- a/njs/njs_vm.c  Thu Apr 18 20:51:53 2019 +0300
+++ b/njs/njs_vm.c  Fri Apr 19 20:03:49 2019 +0800
@@ -993,8 +993,6 @@ njs_vmcode_post_decrement(njs_vm_t *vm,
 njs_ret_t
 njs_vmcode_typeof(njs_vm_t *vm, njs_value_t *value, njs_value_t *invld)
 {
-    nxt_uint_t  type;
-
     /* ECMAScript 5.1: null, array and regexp are objects. */

     static const njs_value_t  *types[NJS_TYPE_MAX] = {
@@ -1034,10 +1032,7 @@ njs_vmcode_typeof(njs_vm_t *vm, njs_valu
         &njs_string_object,
     };

-    /* A zero index means non-declared variable. */
-    type = (value != NULL) ? value->type : NJS_UNDEFINED;
-
-    vm->retval = *types[type];
+    vm->retval = *types[value->type];

     return sizeof(njs_vmcode_2addr_t);
 }

Then I plan to fix #134 first.

[hongzhidao]

@drsm

I'm kind of confused about BLOCK.

In JS, functions can only be declared at top level or inside a block.

if (1) function foo() {}  // error
if (1) { function foo() {} } // ok

So, is there block here?

if (1) x

If no.

the let variables should be set to uninitialized state when control reaches the start of the block.

var t = false;
for (let i = 0; i < 3; i++) {
  {  // set let variables invalid value in this scope. right?
  if (t) x
  let x;
  t = true;
  }
}

What we need to do is calling the right scope.

static nxt_int_t
+njs_generate_loop_body(njs_vm_t *vm, njs_generator_t *generator,
+    njs_parser_node_t *parent)
+{
+    njs_index_t         index;
+    njs_parser_node_t   *node;
+    nxt_lvlhsh_each_t   lhe;
+    njs_vmcode_move_t   *move;
+    njs_parser_scope_t  *scope;
+
+    if (parent != NULL) {
+        scope = parent->scope;
+
+        nxt_lvlhsh_each_init(&lhe, &njs_variables_hash_proto);
+
+        for ( ;; ) {
+            node = nxt_lvlhsh_each(&scope->references, &lhe);
+
+            if (node == NULL) {
+                break;
+            }
+
+            if (node->u.reference.variable->type == NJS_VARIABLE_LET) {
+                index = njs_value_index(vm, &njs_value_invalid,
+                                        generator->runtime);
+                njs_generate_code_move(generator, move, node->index, index);
+            }
+        }
+    }
+
+    return njs_generator(vm, generator, parent);
+}

[drsm]

@hongzhidao

So, is there block here?

if (1) x

no, there is no block

the let variables should be set to uninitialized state when control reaches the start of the block.

var t = false;
for (let i = 0; i < 3; i++) {
  {  // set let variables invalid value in this scope. right?
  if (t) x
  let x;
  t = true;
  }
}

yes, every lexically declared variable has a block which owns it.
blocks for i and x are different in this case.

[hongzhidao]

every lexically declared variable has a block which owns it.

Can you say more about what is lexically declared variable? It'll be helpful for me.

Now there are some types of variable.
let x;
var y;
function f() {}
(function foo() {})

[hongzhidao]

@xeioex

Added let variable declaration support. (based on the latest release)
https://gist.github.com/hongzhidao/87f43203282892695d3c3ba9264da0fc

@drsm, I think the patch ready now, welcome to test. Thanks again.

I think that is better to merge #134 with the current one, and add const without readonly semantics there. So all parts of lexical scoping puzzle will be in one place.

There are three features: let, const, scope function. They can be implemented one by one.

Left: (on it)

let a = [];
for (let i = 0; i < 5; ++i) {
  a.push(function () { return i; });
}

for (let k = 0; k < 5; ++k) {
  if (k !== a[k]()) {
      throw new Error("k !== a[k]");
  }
}

@drsm

var a = [];

for (let i = 0; i < 5; ++i) {
  a.push(function () { return i; });
}

a[0]();  // why is the value 0, not 5?

[drsm]

@hongzhidao

why is the value 0, not 5?

for (let i=... and for (let i in ... have a different semantics from for (var counterparts.
On every loop iteration a new variable is created, but with the same name and value. So if the previous value is bound to some closure it is left untouched.

for (let i = 0, j = 1, k = 2; i < 10; ++i) {
    setImmediate(() => console.log(i, j, k));
    i += 2;
    j += i;
    k += j;
}

{
    let i = 0, j = 1, k = 2;
    let _fun = (_i, _j, _k) => {
        setImmediate(() => console.log(_i, _j, _k));
        _i += 2;
        _j += _i;
        _k += _j;

        i = _i;
        j = _j;
        k = _k;
    }
    for (; i < 10; ++i) _fun(i, j, k);
}

[hongzhidao]

@drsm

We need translate for statement into loop + call?

1. var

for (var i = 1; i < 10; i++) {
    console.log(i);
}

=>

var i = 1;
function _func(i) {
    console.log(i);
}
for (; i < 10; i++) _func(i);

1. let

for (let i = 1; i < 10; i++) {
    console.log(i);
}

=>

let i = 1;
function _func(_i) {
    console.log(_i);
    i = _i;
}
for (; i < 10; i++) _func();

[igorsysoev]

On 19 Apr 2019, at 18:25, hongzhidao [e-mail] wrote:

We need translate for statement into loop + call?

SetImmediate() creates function closure.

If a function closure with captured variables declared via "let" is created inside
loop we need closure scope for every variable. I thought a lot how to make efficient
and still have no idea.

--
Igor Sysoev

[drsm]

@hongzhidao

We need translate for statement into loop + call?

the case for (var i = 1; i < 10; i++) is OK
the case for (let i = 1; i < 10; i++) should be fixed.
babel do it that way.

[drsm]

@hongzhidao

Thank you for the patch!

Added let variable declaration support. (based on the latest release)
https://gist.github.com/hongzhidao/87f43203282892695d3c3ba9264da0fc

@drsm, I think the patch ready now, welcome to test. Thanks again.

>

Problem with catch parameter:

13.15.1 Static Semantics: Early Errors

It is a Syntax Error if any element of the BoundNames of CatchParameter also occurs in the LexicallyDeclaredNames of Block.

so, this should fail:

try { } catch (e) { let e; }

BTW, the spec declares plural BoundNames there because of Destructuring assignment which is not supported

try { throw { a: 1, b: 2, c: 3 }; } catch ({a,b,c}) { console.log(a, b, c); }

The switch statement needs some fixes:

• It should create a block scope

> let x = 2; switch(true) { default: let x = 1; } x
2

• TDZ is also there:

> switch(true) { case false: let x = 1; default: x = 2; }
Thrown:
ReferenceError: x is not defined

[drsm]

@hongzhidao

Can you say more about what is lexically declared variable? It'll be helpful for me.

Now there are some types of variable.
let x;
var y;
function f() {}
(function foo() {})

lexically declared are:

• let x;
• const c = 1;
• import x from ...
• { function f() {} }

[xeioex]

@drsm

Regarding let + for loop + closures. In your experience as a JavaScript programmer is it a critical issue for let variables support?

I see the following alternatives:
1) Leave it as is (no support), until there is serious practical need. It is a tricky thing to do right and efficient. @igorsysoev agrees. We can specify the deviation from the specs in the document similar to this.
2) We need an optimized version for the case when there is no closure captures inside the loop.

[drsm]

@xeioex

Regarding let + for loop + closures. In your experience as a JavaScript programmer is it a critical issue for let variables support?

No, I think it is not critical.

Simple workaround for the most common usage case:

for (let i = 0; i < 10; ++i) {
    const x = i;
    setImmediate(() => console.log(x));
}

Actually i didn't ever imagine how many side-effects there may be.

+1 to leave it as is.

[drsm]

@xeioex

Oh, I think, I get it wrong.
And the problem is not only about a for loop counter,
but anything let declared inside a loop.

var i = 0;
do {
    let x = i++;
    setImmediate(() => console.log(x));
} while (i < 10);

then things get worse, this widely used.

[drsm]

@xeioex @hongzhidao

Can we construct a wrapper function in parallel while parsing a loop body?
And if nothing is captured, just throw it away.

[hongzhidao]

@drsm

What about the idea?

let x, y, z;
+ (function(x, y, z) {   // (i). Add this after let statement that should be inner block scope.
...
let m, n;
+ (function(m, n) { // the same as (i)
...
+ })(m, n); // the same as (ii)
+ })(x, y, z); // (ii). Add this before block end
}

If OK, I'll try it with below.

1. Add (function(x, y, z) { after let statement.

nxt_queue_init(&scope->lets);
nxt_queue_insert_head(&scope->lets, [x, y, z]);
+ (function(x, y, z) {

1. In njs_parser_block_end().

loop names in scope->lets do
   + }(names[i], names[i+1],...names[n]);
end

[hongzhidao]

@drsm

Can we construct a wrapper function in parallel while parsing a loop body?
And if nothing is captured, just throw it away.

I think it's OK, we can add tokens to lexer like this. @xeioex

+njs_lexer_token_add(njs_vm_t *vm, njs_lexer_t *lexer, njs_token_t token,
+    uint32_t token_line, nxt_str_t *text, double number)
+{
+    njs_lexer_token_t  *lt;
+
+    lt = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_lexer_token_t));
+    if (nxt_slow_path(lt == NULL)) {
+        return NXT_ERROR;
+    }
+
+    lt->token = token;
+    lt->token_line = token_line;
+    lt->text = *text;
+    lt->key_hash = nxt_djb_hash(text->start, text->length);
+    lt->number = number;
+
+    nxt_queue_insert_tail(&lexer->preread, &lt->link);
+
+    return NXT_OK;
+}

[drsm]

@drsm

What about the idea?

let x, y, z;
+ (function(x, y, z) {   // (i). Add this after let statement that should be inner block scope.
...
let m, n;
+ (function(m, n) { // the same as (i)
...
+ })(m, n); // the same as (ii)
+ })(x, y, z); // (ii). Add this before block end
}

If OK, I'll try it with below.

Yes IIFE with an arrow function would work, I think.

[hongzhidao]

@drsm

Welcome to test, thanks.
https://gist.github.com/hongzhidao/430dc73da7dea3db7e1eedaac1091597
BTW, unit tests are welcome also.

@xeioex take a look. (draft design)
I introduced njs_lexer_token_add() for enhancing parser.

[hongzhidao]

@xeioex @drsm

Done in:

1. Improved njs_lexer_rollback().
2. Making parser more generic.
3. Added let variable declaration support.
   https://gist.github.com/hongzhidao/882500514e191280b88db5c143306073

[xeioex]

@hongzhidao

Added let variable declaration support.

this patch introduced an issue:

>if(1) {console.log("dfggf" + (typeof (x)))}
Segmentation fault (core dumped)

cat test.js

while(function f(){}){    
  if(typeof(f) === "function") {
    break; 
  }
}

> njs test.js
Segmentation fault

[xeioex]

@hongzhidao

Improved njs_lexer_rollback().

Committed thanks.

Making parser more generic.

Committed only the part relevant for parser, I think njs_vmcode_name and njs_lexer_token_add code should be added with the let patch.

[hongzhidao]

deleted.

[hongzhidao]

@xeioex

Added let variable declaration support.
https://gist.github.com/hongzhidao/882500514e191280b88db5c143306073

@drsm welcome to test.

[drsm]

@hongzhidao

looks fine, except this and a loop counter.

Is it possible to transform this:

for (let i = 0; i < 10; ++i) { setImmediate(() => console.log(i)); i += 1; }

into

{ let _i; for (_i = 0; _i < 10; ++_i) { ((i) => { setImmediate(() => console.log(i)); i += 1; _i = i; })(_i); } }

?

[hongzhidao]

@drsm

Is it possible to transform this:

for (let i = 0; i < 10; ++i) { setImmediate(() => console.log(i)); i += 1; }

into

{ let _i; for (_i = 0; _i < 10; ++_i) { ((i) => { setImmediate(() => console.log(i)); i += 1; _i = i; })(_i); } }

?

No, it's too complicated.

How about processing this, arguments like arrow-function?

[xeioex]

@hongzhidao

How about this idea in dealing with let loop variables? Just an idea, not 100% sure it is a workable solution.

> for (let a = 0; a < 10; a++) {console.log(a)}
shell:main
// 1)  we need a function frame (or a block scope??) around the loop to have uniq closures scope 
// frame->closures[n], vm->scopes[NJS_SCOPE_CLOSURE + n]
00000 MOVE              1AED960 1AED970
00032 JUMP              +160
00056 METHOD FRAME      1AE3DC0 1AED980 1
00096 MOVE              0002 1AED960
00128 FUNCTION CALL     1AED990
00152 POST INC          1AED990 1AED960 1AED960
00192 LESS              1AED990 1AED960 1AED9A0
// 2) here we need a new instruction which would allocate a new closures block (frame->closures[n], vm->scopes[NJS_SCOPE_CLOSURE + n]),
// but ONLY if let var is captured here, so ordinary loop would be fast
00232 JUMP IF TRUE      1AED990 18446744073709551440
00264 STOP              1AED9B0

Some thoughts:
1) Currently, the only easy way for us to realloc variable values in runtime is to use closures blocks.
Because other scopes in vm->scopes[] are harder to swap.
2) In is also important to do this ONLY if let var is captured, because simple for (let a = 0; a < 100000; a++) {} can consume a lot of memory. In the future closures will be GCed. But, for now they are not.

[hongzhidao]

@xeioex

1. I think the essence of let variables is TDZ. With the example bellow.

for (...) {
   ...
   let x, y, z;   // noticed that this appears inside for loop.
   /* can be regarded as:
    ((x, y, z) => { ... })(x, y, z);
    what I mean is that do you think of this case? a function frame + allocate a new closures block
  */
}

1. > simple for (let a = 0; a < 100000; a++) {} can consume a lot of memory.

If use the design you mentioned, does it only allocate one closure? frame->closures[n], vm->scopes[NJS_SCOPE_CLOSURE + n]

1. > looks fine, except this and a loop counter.

This can be easily fixed, we can use arrow-function instead of function expression.

BTW, I'm on vacation these days, will continue this next week.

[xeioex]

@hongzhidao

what I mean is that do you think of this case?

I mean something like (to avoid too many function frames creation/destroing)

(let x = 0; i < 10; i++) {...}
->

((x) => { for (var x; i < 10; i++) }) {
...
//at the end of the loop: new instruction
// which would realloc the arrow function arguments
}(0)

If use the design you mentioned, does it only allocate one closure?

I am suggesting to detect the case when let variable is captured inside the loop (in compile time) and only in this case implement the logic with closures (using new instruction), otherwise treat let as a var.

BTW, I'm on vacation these days, will continue this next week.

I am also on vacation. Have a nice week.

[hongzhidao]

@xeioex

for (var i = 0; i < 10;) {
    setImmediate(() => {
        console.log(i);
    })
    i++;
}

test.js:anonymous
00000 METHOD FRAME      21A7F20 21B20E0 1
00040 MOVE              0002 0141       // i is 0141
00072 FUNCTION CALL     0004
00096 RETURN            21B20F0
test.js:main
00000 MOVE              0141 21B20D0
00032 JUMP              +184
00056 OBJECT COPY       0161 0151
00088 FUNCTION FRAME    0161 1
00120 FUNCTION          0002 21B1400
00152 FUNCTION CALL     0161
00176 POST INC          0161 0141 0141
00216 LESS              0161 0141 21B2110
00256 JUMP IF TRUE      0161 18446744073709551416
00288 STOP              21B20F0

I can't still find a way to archive it (let + closure).

2) here we need a new instruction which would allocate a new closures block (frame->closures[n], vm->scopes[NJS_SCOPE_CLOSURE + n]),

Notice that frame->closures[n] is created at function calling (the argument of setImmediate), not in for loop.

[xeioex]

@hongzhidao

I am currently on fuzzers issues, cannot dive deeper into the let + for loops. You may want to choose a different task for now.

[hongzhidao]

OK.

[xeioex]

BTW, Babel

1. no capture

for (let i =0; i < 10; i++) {
  console.log(i);
}

->

for (var i =0; i < 10; i++) {
  console.log(i);
}

1. capture

for (let i =0; i < 10; i++) {
  setTimeout(()=>{console.log(i)}, 0)
}

for (let i =0; i < 10; i++) {
  for (let j = 0; j < 10; j++) {
         let k = j + 1;
    setTimeout(()=>{console.log(i, j, k)}, 0)
  }
}

->

"use strict";

var _loop = function _loop(i) {
  setTimeout(function () {
    console.log(i);
  }, 0);
};

for (var i = 0; i < 10; i++) {
  _loop(i);
}

var _loop = function _loop(i) {
  var _loop2 = function _loop2(j) {
    var k = j + 1;
    setTimeout(function () {
      console.log(i, j, k);
    }, 0);
  };

  for (var j = 0; j < 10; j++) {
    _loop2(j);
  }
};

for (var i = 0; i < 10; i++) {
  _loop(i);
}

[lexborisov]

@drsm @xeioex @hongzhidao @jirutka

Take a look.

Two patches for let support:

1. Scopes refactoring.
2. Introduced let implementation.

Known bugs:
CLI if declaration let failed.

>> let x = x + 1;
Thrown:
ReferenceError: cannot access to variable before initialization
    at main (shell:1)

>> x
undefined

diff.

[drsm]

@lexborisov

Hi Alexander!

Thank you for the patch!

Known bugs:

Here is some more:

$ build/njs -c 'let x = 2; switch(true) { default: let x = 1; } x'
Thrown:
SyntaxError: "x" has already been declared in string:1
$ build/njs -c 'try { } catch (e) { let e; }'

[xeioex]

@drsm BTW, https://github.com/nginx/njs/issues/267 was also fixed.

[drsm]

just another weird case that shouldn't fail:

var x = function fn() { let fn; };

BTW:
```
$ node --use-strict
Welcome to Node.js v14.16.1.
Type ".help" for more information.

var x = function fn(x = fn) { return x; }
undefined
x()
[Function: fn]
var x = function fn(fn = fn, x = fn) { return x; }
undefined
x()
Uncaught ReferenceError: Cannot access 'fn' before initialization
at fn (REPL3:1:26)
var x = function fn(fn, x = fn) { return x; }
undefined
x()
undefined
var x = function fn(x = fn, fn) { return x; }
undefined
x()
Uncaught ReferenceError: Cannot access 'fn' before initialization
at fn (REPL7:1:25)
x(1)
1
x(undefined)
Uncaught ReferenceError: Cannot access 'fn' before initialization
at fn (REPL7:1:25)
x(undefined, 2)
Uncaught ReferenceError: Cannot access 'fn' before initialization
at fn (REPL7:1:25)
```

[drsm]

something goes wrong:

$ build/njs -c 'let y = () => y; y()'
Thrown:
ReferenceError: cannot access to variable before initialization
    at anonymous (string:1)
    at main (string:1)

[lexborisov]

@drsm

Artem, thanks for testing!

Found bugs fixed:

1. Scopes refactoring.
2. Introduced let implementation.

[drsm]

@lexborisov

this looks broken in the latest patch:

let x = (() => x)()

[lexborisov]

@drsm

Yes, it didn't work out to check during code generation. Now at runtime.
Fixed.

[drsm]

>> let let;
undefined
>> 

should fail in strict mode, actually:

$ node --use-strict
Welcome to Node.js v14.16.1.
Type ".help" for more information.
> let let;
let let;
    ^^^

Uncaught SyntaxError: Unexpected strict mode reserved word

[lexborisov]

@drsm

Thanks, fixed.

[drsm]

didn't get it

[lexborisov]

This already been removed.

[drsm]

@lexborisov

This already been removed.

OK, thanks.

just found an another useless corner case:

> let x = (x, 0)
Uncaught ReferenceError: Cannot access 'x' before initialization

[lexborisov]

@drsm

Thanks,
fixed, and let x = x too.

[drsm]

var break_tdz = false;

for (var i = 0; i < 10; ++i) {
    if (break_tdz) {
        x = 2;
    }
    break_tdz = true;
    let x = 1;
}