[WIP][ES6] Function declarations should be block scoped in ES2015 #1953

OndrejSpanel · 2017-05-16T13:14:22Z

PR for #1952

The change is straighforward, function declaration is now handled like DefClass, no function scope search is performed.

The only expected change was I had to simplify one test case for dead code elimination which assumed functions are function scoped.

It might be nice to add some test for the case fixed by this, the only test I can think of is parsing AST, peforming figure_out_scope() and checking if symbol scopes match expectation.

kzc · 2017-05-16T14:59:22Z

@OndrejSpanel Could you please make a test case using console.log that is actually runnable on node 6+ so we can verify the results? The test case should output different results without this PR.

kzc · 2017-05-16T15:31:20Z

@alexlamsl It would be nice to run ES6 tests with expect_stdout but we have a problem in that they'd only work with node 6 and later versions. Can the expect_stdout test functionality be extended to run ES6 tests with an optional minimum major node version needed to run the test - i.e.: node: 6. If the host version of node is less than that number then the output is not run in the test sandbox, and only the expect / expect_exact output is verified.

If the node version is differently numbered in ChakraNode than Node/V8 we'd need a different scheme.

alexlamsl · 2017-05-16T16:43:48Z

@OndrejSpanel thanks for the PR - please be patient as we may need to enhance our test framework a little to enable thorough testing on harmony.

@kzc can we safely assumes that Node.js which aren't ES6-compliant would fail to parse the input code from test/compress/?

If so, we can avoid process.version dances and have a transparent way to use expect_stdout by simply skipping this part when we encounter SyntaxError from sandbox.run_code(input_code).

kzc · 2017-05-16T16:53:45Z

can we safely assumes that Node.js which aren't ES6-compliant would fail to parse the input code from test/compress/?

Not necessarily. Block scope issues for functions could parse fine in ES5, but produce the wrong result. We have a few harmony tickets along those lines.

You raise a good point - we should also factor ecma version into the tests as well.

we can avoid process.version dances and have a transparent way to use expect_stdout by simply skipping this part when we encounter SyntaxError from sandbox.run_code(input_code).

There could be inadvertent syntax errors from incorrectly constructed tests that we should not skip.

I'd prefer to keep the test logic explicit with a minimum node major version. Either it passes or fails with no doubt.

alexlamsl · 2017-05-16T17:00:40Z

@kzc thanks for the detailed analysis - your suggestion of simply specifying a minimum Node.js version makes sense.

node-chakracore isn't production-ready yet, so we can safely ignore that complication for now - we'll cross that bridge when the time comes, before that I can always keep the broken pieces to myself 👻

You raise a good point - we should also factor ecma version into the tests as well.

We should be able to specify beautify: { ecma: 6 } in those tests, so that may cover that.

alexlamsl · 2017-05-16T17:06:15Z

@OndrejSpanel @kzc Travis failures in Node.js 0.10 and 0.12 for this and #1954 are caused by a recent npm update. I have merged the relevant fix onto harmony now, so if you rebase the PRs on top that should sort that out.

kzc · 2017-05-16T22:33:14Z

@alexlamsl If you look at the npm pages for uglify-js and uglify-es in the upper left you see an icon saying "build failing":

https://www.npmjs.com/package/uglify-js
https://www.npmjs.com/package/uglify-es

I suspect that the following line in the README no longer works - can you please remove it?

[![Build Status](https://travis-ci.org/mishoo/UglifyJS2.svg)](https://travis-ci.org/mishoo/UglifyJS2)

Martii · 2017-05-17T01:30:24Z

Since ES6 was enabled mainly into nodejs@4.x and above I would recommend dumping the tests in prior versions of nodejs... unless you plan on using the --harmony switch for CI.

alexlamsl · 2017-05-17T02:02:30Z

@kzc I think the badge is working just fine - there is a test time-out on harmony. I have restarted that so we should get back to green in a moment.

kzc · 2017-05-17T02:10:53Z

I think the badge is working just fine - there is a test time-out on harmony. I have restarted that so we should get back to green in a moment.

But what's the point of it? What is it showing exactly? It's not release status - it's the status the last commit?

alexlamsl · 2017-05-17T02:19:25Z

But what's the point of it? What is it showing exactly? It's not release status - it's the status the last commit?

Good point - never thought about that.

In fact, a npm badge is probably more informative than the current CI one.

alexlamsl · 2017-05-17T02:34:09Z

I suspect that the following line in the README no longer works - can you please remove it?

Done in 0813c53 😉

OndrejSpanel · 2017-05-18T07:52:10Z

The test case should output different results without this PR.

I am not sure if I am able to do this. I can create a mocha test verifying the AST and symbol scopes (something similar to what I did in https://github.com/mishoo/UglifyJS2/blob/89e94b2ca431b31ffda41d9f51e11c541aa9b626/test/mocha/accessorTokens-1492.js before), but I am afraid I am not enough familiar with JS to be able to create an example where this changes results in a different compressed result, which I think is what you want.

alexlamsl · 2017-05-18T08:01:00Z

@OndrejSpanel would this help?

$ cat test.js
function f() {
    return 1;
}
if (console.log(f())) {
    function f() {
        return 2;
    }
} else {
    function f() {
        return 3;
    }
}

$ nvs use node/0.10
$ node test.js
3

$ nvs use node/0.12
$ node test.js
3

$ nvs use node/4
$ node test.js
3

$ nvs use node/6
$ node test.js
1

$ nvs use node/7
$ node test.js
1

OndrejSpanel · 2017-05-18T08:02:21Z

I will try that once #1967 is merged.

alexlamsl · 2017-05-22T05:02:29Z

Possibly related issues:
#1903
#1807
#1672
#1666
#1664
#1358
#1317

alexlamsl · 2017-05-23T07:49:03Z

test/compress/dead-code.js

            console.log("unreachable");
            var foo;
-            function bar() {}


This actually flags an issue with the code changes in this PR. Consider the following:

while (console.log(bar)) { function bar() {} }

Passing it to Node.js 7 actually prints undefined instead of throwing ReferenceError: bar is not defined.

Does minify() need a top level ecma option to specify whether to parse/compress in ecma: 5 or ecma: 6 mode? output.js already has ecma: 6 support to print ES6 properties.

parse() doesn't have ecma mode switches AFAICT.

parse() doesn't have ecma mode switches AFAICT.

I appreciate that, but is there a need for ecma in parse and compress? Is there ambiguity that could only be resolved via such an explicit option?

Interesting idea. While adding complexity to lib/parse.js on the harmony branch sounds worrying, we may end up needing to do so if ECMAScript breaks forward-compatibility, i.e. code valid in the older version is no longer accepted in a latter version.

IMHO parse() serves a slightly different purpose than acorn, in that we don't need it to throw stringent syntax errors rather than obeying GIGO.

alexlamsl · 2017-05-23T10:06:03Z

This is fun...

console.log(f);
if (console.log(f)) {
    function f() {}
}
while (console.log(f)) {
  function f() {}
}
console.log(f);

gives:

undefined
undefined
undefined
undefined

But flipping the if...

console.log(f);
if (!console.log(f)) {
    function f() {}
}
while (console.log(f)) {
  function f() {}
}
console.log(f);

gives:

undefined
undefined
[Function: f]
[Function: f]

It's almost as if function f(){} is just var f = function(){}; under ES6+

kzc · 2017-05-23T12:04:59Z

It's almost as if function f(){} is just var f = function(){}; under ES6+

FF53 produces the same results, not just V8.

OndrejSpanel · 2017-05-23T12:10:49Z

The tests you are doing, are they strict mode or not? I think the change should probably be applied in strict mode only.

kzc · 2017-05-23T12:15:12Z

If "use strict" is used, the tests above do indeed produce the error: ReferenceError: f is not defined

OndrejSpanel · 2017-05-23T12:17:48Z

I am not sure, but what you describe seems to me to behave like "web extensions" (B.3.3 Block-Level Function Declarations Web Legacy Compatibility Semantics) described in http://www.ecma-international.org/ecma-262/6.0/index.html#sec-additional-ecmascript-features-for-web-browsers

See also https://stackoverflow.com/a/31461615/16673

alexlamsl · 2017-05-23T12:21:47Z

By default the codebase operates in normal mode unless it explicitly calls TreeWalker.has_directive("use strict") for conditional execution of strict-mode-only code paths.

alexlamsl · 2017-05-25T08:39:44Z

So here's my dilemma:

$ cat test.js
function f() {
    g();
    if (0) {
        function g() {};
    }
}
f();

$ nvs use node/4
$ node test.js

$ nvs use node/6
$ node test.js
test.js:2
    g();
    ^

TypeError: g is not a function
    at f

Essentially, we have an issue whereby on master we compress test.js into:

function f() {
    g();
    function g() {};
}

But on harmony, the compression that would produce identical runtime result is:

function f() {
    g();
    var g;
}

So that means if I were to "fix" it for harmony, it will never produce a result that works on JavaScript engines supported by master.

alexlamsl · 2017-05-25T08:50:03Z

Next time when anybody finds IE8 catch variable weird, rest assured ES6 has got your back:

$ cat test.js
console.log(typeof f);
if (console.log(typeof f)) {
    // unreachable
    f();
} else {
    console.log(typeof f);
    function f() {}
    console.log(typeof f);
}
console.log(typeof f);

$ nvs use node/4
$ node test.js
1 'function'
2 'function'
3 'function'
4 'function'
5 'function'

$ nvs use node/6
$ node test.js
1 'undefined'
2 'undefined'
3 'function'
4 'function'
5 'function'

The definition is "global" within the block scope, but sequential outside of that. 🙈

kzc · 2017-05-25T13:42:41Z

It's a bit more sane in strict mode for both ES5 and ES6 JS engines.

$ cat strict_test.js 

"use strict";
console.log(typeof f);
if (console.log(typeof f)) {
    // unreachable
    f();
} else {
    console.log(typeof f);
    function f() {}
    console.log(typeof f);
}
console.log(typeof f);

ES5 JS engine:

$ node421 strict_test.js 
undefined
undefined
function
function
undefined

ES6 JS engine:

$ node690 strict_test.js 
undefined
undefined
function
function
undefined

It appears that uglify master does not behave correctly in strict mode - it applies non-strict ES5 function scope behavior in all cases:

$ bin/uglifyjs -V
uglify-js 3.0.11

$ cat strict_test.js | bin/uglifyjs -cm | node421
function
function
function
function
function

$ cat strict_test.js | bin/uglifyjs -cm | node690
function
function
function
function
function

kzc · 2017-05-25T14:25:06Z

It appears that uglify master does not behave correctly in strict mode - it applies non-strict ES5 function scope behavior in all cases

To be fair, declaring functions in block scope is not valid ES5. Using an older ES5 mozilla JS engine:

$ jx -jsv
Mozilla SpiderMonkey v34

$ jx strict_test.js 
SyntaxError: in strict mode code, functions may be declared only at top level or immediately within another function (strict_test.js 8:13)

It's undefined behavior at best.

kzc · 2017-05-25T14:36:36Z

Perhaps both master and harmony branches should adopt the rule "if a given function declaration is subject to 'use strict' then never hoist it".

I noticed that although hoist_funs=true often produces smaller output, this optimization will often make gzipped uglify output larger compared to hoist_funs=false.

alexlamsl · 2017-05-25T17:37:44Z

hoist_funs aside, #1953 (comment) is about dead code elimination. I just tried adding "use strict"; and both node/4 and node/6 behaves the same way as node/6 without "use strict";

So I think the fix is to add a check for has_directive("use strict") and turn AST_Defun into AST_Var within extract_declarations_from_unreachable_code() on master, then merging that onto harmony removing the check and making it unconditionally output AST_Var.

kzc · 2017-05-25T18:06:40Z

hoist_funs aside, #1953 (comment) is about dead code elimination.

The two subjects are intricately linked. DCE is subject to scope, which directly influences the conditions to hoist.

Function declarations should be block scoped in ES2015 (#1952)

8223bf6

OndrejSpanel changed the title ~~Function declarations should be function scoped in ES2015 (#1952)~~ Function declarations should be block scoped in ES2015 May 16, 2017

This was referenced May 17, 2017

make expect_stdout node version specific #1963

Merged

Harmony v3.0.8 #1967

Merged

alexlamsl mentioned this pull request May 23, 2017

[ES6] fix destructuring bugs in mangle and compress #1992

Merged

alexlamsl reviewed May 23, 2017

View reviewed changes

alexlamsl changed the title ~~Function declarations should be block scoped in ES2015~~ [WIP][ES6] Function declarations should be block scoped in ES2015 May 25, 2017

alexlamsl mentioned this pull request May 25, 2017

[ES6] export function name incorrectly dropped by compress #2001

Closed

This was referenced May 26, 2017

fix dead_code on block-scoped function under "use strict" #2006

Merged

fix hoist_funs on block-scoped function under "use strict" #2013

Merged

kzc mentioned this pull request Jul 22, 2017

Harmony - Function scope wrong #1952

Closed

alexlamsl closed this Jul 23, 2017

[WIP][ES6] Function declarations should be block scoped in ES2015 #1953

[WIP][ES6] Function declarations should be block scoped in ES2015 #1953

Conversation

OndrejSpanel commented May 16, 2017 • edited

kzc commented May 16, 2017

kzc commented May 16, 2017

alexlamsl commented May 16, 2017

kzc commented May 16, 2017

alexlamsl commented May 16, 2017

alexlamsl commented May 16, 2017

kzc commented May 16, 2017

Martii commented May 17, 2017

alexlamsl commented May 17, 2017

kzc commented May 17, 2017

alexlamsl commented May 17, 2017

alexlamsl commented May 17, 2017

OndrejSpanel commented May 18, 2017

alexlamsl commented May 18, 2017

OndrejSpanel commented May 18, 2017

alexlamsl commented May 22, 2017 • edited

alexlamsl May 23, 2017

Choose a reason for hiding this comment

kzc May 23, 2017

Choose a reason for hiding this comment

alexlamsl May 23, 2017

Choose a reason for hiding this comment

kzc May 23, 2017

Choose a reason for hiding this comment

kzc May 23, 2017

Choose a reason for hiding this comment

alexlamsl May 23, 2017

Choose a reason for hiding this comment

alexlamsl commented May 23, 2017

kzc commented May 23, 2017

OndrejSpanel commented May 23, 2017

kzc commented May 23, 2017 • edited

OndrejSpanel commented May 23, 2017 • edited

alexlamsl commented May 23, 2017

alexlamsl commented May 25, 2017

alexlamsl commented May 25, 2017

kzc commented May 25, 2017 • edited

kzc commented May 25, 2017 • edited

kzc commented May 25, 2017

alexlamsl commented May 25, 2017

kzc commented May 25, 2017

OndrejSpanel commented May 16, 2017 •

edited

alexlamsl commented May 22, 2017 •

edited

kzc commented May 23, 2017 •

edited

OndrejSpanel commented May 23, 2017 •

edited

kzc commented May 25, 2017 •

edited

kzc commented May 25, 2017 •

edited