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

Harmony - Function scope wrong #1952

Closed
OndrejSpanel opened this issue May 16, 2017 · 6 comments
Closed

Harmony - Function scope wrong #1952

OndrejSpanel opened this issue May 16, 2017 · 6 comments

Comments

@OndrejSpanel
Copy link
Contributor

When I analyse AST and symbol information of the following JS, the symbol C is considered global:

if (true) {
        function C() {}
}

Additionally, in the following code both Cs are considered to be the same symbol:

if (true) {
        function C() {}
}
if (true) {
        function C() {}
}

When I use var or class instead of function, the scoping seems correct, it is only seen with functions. When I wrap the function in IIFE, as follows, it is no longer global:

(function(){
    if (true) {
        function C() {}
    }
})()

I think in ES2015 function declaration should be block scoped, not function scoped (see http://2ality.com/2015/02/es6-scoping.html):

Function declarations…

  • are block-scoped, like let.

Can someone confirm the issue and perhaps give me some hint where this could be fixed? I would be glad to develop a PR for that if confirmed.

@alexlamsl
Copy link
Collaborator

While I cannot answer the question regarding ES6, I am fairly certain that the scope assignment happens within AST_Toplevel.figure_out_scope(), so any changes should be around these lines:

https://github.com/mishoo/UglifyJS2/blob/81243c4e71552b39ab0f704bc812788269cd93e2/lib/scope.js#L199-L210

@OndrejSpanel
Copy link
Contributor Author

I have tried to confirm how this should work. I have found a StackOverflow question What are the precise semantics of block-level functions in ES6? about this, which IIUC says the behaviour I expect is guranteed only in strict mode. I am not sure if and how is Uglify distinguishing strict/non strict code and what is desired behaviour here. Should the new behaviour be applied only in the strict mode?

@kzc
Copy link
Contributor

kzc commented May 16, 2017

Should the new behaviour be applied only in the strict mode?

In an ideal world we'd like it to work as the ES spec dictates, but since the spec is difficult to interpret we tend just accept whatever behavior node and Chrome have, and assume they've done their homework. So if you can test a few examples in node with and without "use strict" you'll have your answer.

@alexlamsl alexlamsl added the bug label May 16, 2017
@fabiosantoscode
Copy link
Contributor

@kzc I've been able to verify that node does block-scope function declarations when 'use strict' is applied

fabio@thinkpad ♥  cat index.js 

{
  function f(){}
}

console.log(f)

fabio@thinkpad ♥  node index.js 
[Function: f]
fabio@thinkpad ♥  vim index.js
[...]
fabio@thinkpad ♥  cat index.js 
'use strict'

{
  function f(){}
}

console.log(f)

fabio@thinkpad ♥  node index.js 
/home/fabio/bin/index.js:7
console.log(f)
            ^

ReferenceError: f is not defined

@kzc
Copy link
Contributor

kzc commented Jul 22, 2017

Thanks @fabiosantoscode.

When the same test code you supplied is run through uglify-es with the options -c --toplevel | node it produces the same results as above.

I think #1952 and #1953 can be closed?

@alexlamsl
Copy link
Collaborator

Thanks @fabiosantoscode @kzc for confirmation.

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

No branches or pull requests

4 participants