Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

OndrejSpanel
Copy link
Contributor

@OndrejSpanel OndrejSpanel commented May 16, 2017

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.

@OndrejSpanel OndrejSpanel changed the title Function declarations should be function scoped in ES2015 (#1952) Function declarations should be block scoped in ES2015 May 16, 2017
@kzc
Copy link
Contributor

kzc commented May 16, 2017

@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
Copy link
Contributor

kzc commented May 16, 2017

@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
Copy link
Collaborator

@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
Copy link
Contributor

kzc commented May 16, 2017

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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Contributor

kzc commented May 16, 2017

@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
Copy link
Contributor

Martii commented May 17, 2017

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
Copy link
Collaborator

@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
Copy link
Contributor

kzc commented May 17, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

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

Done in 0813c53 😉

@OndrejSpanel
Copy link
Contributor Author

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
Copy link
Collaborator

@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
Copy link
Contributor Author

I will try that once #1967 is merged.

@alexlamsl
Copy link
Collaborator

alexlamsl commented May 22, 2017

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

console.log("unreachable");
var foo;
function bar() {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acorn has different ecma parsing modes:

usage: acorn [--ecma3|--ecma5|--ecma6|--ecma7|...|--ecma2015|--ecma2016|...]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

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
Copy link
Contributor

kzc commented May 23, 2017

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

FF53 produces the same results, not just V8.

@OndrejSpanel
Copy link
Contributor Author

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

@kzc
Copy link
Contributor

kzc commented May 23, 2017

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

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented May 23, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

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 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
Copy link
Collaborator

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
Copy link
Contributor

kzc commented May 25, 2017

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
Copy link
Contributor

kzc commented May 25, 2017

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
Copy link
Contributor

kzc commented May 25, 2017

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
Copy link
Collaborator

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
Copy link
Contributor

kzc commented May 25, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants