disallow collapse_vars constant replacement in for-in statements

[kzc]

A more comprehensive fix for: #1537

[kzc]

Have to rename the commit comment from for-of to for-in.

[kzc]

Just to clarify, this PR is to be applied to master but will fix the harmony branch for-of collapse_vars constant replacement issue once master is (eventually) merged into harmony.

[alexlamsl]

Do you mean to remove/revert this part like you do in #1537 (comment)?

[kzc]

I've changed this patch a few times. I'm not happy with it. Might have a potential issue on harmony with pathological ES6 input. I'll revisit it tomorrow.

[alexlamsl]

Just a quick note to say that when reduce_vars is enabled, any AST_Symbol in AST_ForIn or AST_Destructuring, its corresponding SymbolDef (i.e. symbol.definition()) would have a field fixed === false.

I haven't studied it rigorously yet, but I reckon collapse_vars can currently only apply to a subset of sites where reduce_vars is deemed safe.

[kzc]

I reckon collapse_vars can currently only apply to a subset of sites where reduce_vars is deemed safe.

Indeed that's true for constant values.

The collapse_vars fix in master is fine for ES5. I just want to fix this one specific esoteric thing in collapse_vars for harmony whether or not reduce_vars is enabled.

[alexlamsl]

Keeping them separate is good, I agree. I'm just thinking that given that collape_vars is working fine on master, whether the problem is with AST_Destructuring rather than AST_ForIn.

[kzc]

whether the problem is with AST_Destructuring rather than AST_ForIn

Now that you mention it, AST_Destructuring will also require a harmony specific fix for collapse_vars. And I'm pretty sure I know of a new way to break reduce_vars on harmony with array destructuring because of the way the harmony AST is designed:

$ echo 'var x = 1, y = 2; [x] = [y];' | bin/uglifyjs -c collapse_vars=0,reduce_vars=1
var x=1,y=2;[1]=[2];

Sure enough. AST_Array can be regular array or it can be used for destructuring. There might be a flag on the object to differentiate it, but it really ought to be a different class altogether - AST_ArrayDestructuring.

Compress transforms are so broken on harmony in general related to let/const scopes that ES6 users would be well advised to disable compress and mangle altogether. A short-term harmony fix could be to only optimize functions that do not have ES6-specific nodes.

[alexlamsl]

You learn something new every day, they say. It's gonna be fun, they say. 🙄

Jokes aside, I think your suggestions on blanket blocking AST_Destructuring makes sense. I'll take care of the reduce_vars case in a moment.

[kzc]

@alexlamsl Maybe we should abandon harmony altogether? There are no active harmony maintainers addressing these key compress and mangle issues related to let/const scope. Uglify transforms were written with ES5 in mind, it's non-trivial to adapt to ES6+.

[alexlamsl]

I don't understand ES6 enough to work on it, so you won't get much push-back from me for abandoning it per se.

The question is to whether keep the branch around just for merges between releases. But #1460 might make even that difficult, if we are making 3.x happen.

[kzc]

At the very least we should disable certain compress options on harmony by default - unused, collapse_vars, reduce_vars, and probably a half dozen others.

[alexlamsl]

Would you mind putting together the list of known bad cases for harmony?

I'd put them in say test/compress/known-bad.js then disable all the relevant options by default under lib/compress.js in a new PR.

[kzc]

Would you mind putting together the list of known bad cases for harmony?

I'm not exaggerating when I say nearly every compress transform breaks ES6 code. Just dig through the [ES6] issues. Mangle is also broken related to destructuring. I don't have the time to catalog all the ES6 breakage.

Harmony does work without compress and mangle as far as I know - except for complex import and export statements which are not implemented.

[alexlamsl]

Harmony does work without compress and mangle as far as I know

... wait, what is uglify-js if you can't compress or mangle? 😰

[kzc]

wait, what is uglify-js if you can't compress and mangle?

Evidently, some people are able to use harmony successfully with a subset of ES6 features - simple classes and methods I imagine. Anything with destructuring is use at own risk.

Edit: the harmony ES6 template string support works well too.

[alexlamsl]

Ah, so the problem is with default options and people aren't aware what options to turn on/off for their own specific use case.

It's not a showstopper per se as every option can be disabled regardless of their default values. I guess it's then just an argument of whether it's worth to effort to diverge from master by explicitly disabling some options.

In that case I'd vote for status quo, as it's less work for future merges, and the harmony branch is pretty much in alpha stage, so it's not unreasonable to expect people to know about a few more knobs.

[kzc]

It's not a showstopper per se as every option can be disabled regardless of their default values.

True enough.

I'd vote for status quo, as it's less work for future merges

+1

[kzc]

@alexlamsl I was wrong about harmony array destructuring - it also uses AST_Destructuring on the LHS:

$ echo '[x] = [y];' | bin/uglifyjs --dump-ast
{
  "_class": "AST_Toplevel",
  "body": [
    {
      "_class": "AST_SimpleStatement",
      "body": {
        "_class": "AST_Assign",
        "left": {
          "_class": "AST_Destructuring",
          "names": [
            {
              "_class": "AST_SymbolRef",
              "name": "x"
            }
          ],
          "is_array": true
        },
        "operator": "=",
        "right": {
          "_class": "AST_Array",
          "elements": [
            {
              "_class": "AST_SymbolRef",
              "name": "y"
            }
          ]
        }
      }
    }
  ]
}

--dump-ast is from #769 - used for debugging. Don't want to add overhead to the existing AST by merging it into master.

[kzc]

This PR is intended for master and will be merged into harmony eventually.

See #1537 (comment) for how to fix collapse_vars with harmony destructuring with additional harmony-specific test cases.

[alexlamsl]

Probably unrelated to this PR:

$ echo 'var a="";for(a in {}){var b=2;f(b/5*8)}' | uglifyjs -c
var a="";for(a in{}){var b=2;f(3.2)}

$ echo 'var a="";for(a in {}){var b=2;f(b/5*8)}' | uglifyjs -c reduce_vars=false
WARN: Collapsing constant b [-:1,32]
var a="";for(a in{}){f(2/5*8)}

Since #1459 would happen before collapse_vars, so reduce_vars would take precedence if enabled. But notice how collapse_vars removed a top-level variable.

[kzc]

But notice how collapse_vars removed a top-level variable.

That's correct - as designed. collapse_vars wasn't a default option before so it was an opt-in.

Let's see if anyone complains. If they do, you can add a check for the toplevel compress option before replace.

[kzc]

notice how collapse_vars removed a top-level variable

I don't see the toplevel var a being removed in your examples - although collapse_vars would certainly collapse toplevel vars if the situation arose. b is collapsed but it is local.

I have no issue with you having collapse_vars respect -c toplevel if you want - but you'd have to update all the tests in test/compress/collapse_vars.js accordingly to add toplevel: true to compress options, which is an implicit assumption in many tests.

We could remove collapse_vars' ability to replace constant variables in favor of reduce_vars but I don't know if that would negatively impact collapse_vars' effectiveness and/or efficiency.

[alexlamsl]

b is collapsed but it is local.

I thought for-loops don't form scopes?

$ node
> b
ReferenceError: 'b' is undefined
   at Global code (repl:1:1)
   ...
> for (var a in {}) {var b = 2}
undefined
> b
undefined

I have no issue with you having collapse_vars respect -c toplevel if you want

Nah - I think your "let's see if anyone complains" is probably fine for this case. I think if we were to change anything in collapse_vars we'll need a thorough review to optimise synergies between different rules, and we can pick this up then.

I ran into this just because I was testing some trivial cases for this PR.

[kzc]

I thought for-loops don't form scopes?

They shouldn't - for vars at least. I see what you mean now.

Hmm. Odd. It might have something to do with this:

https://github.com/mishoo/UglifyJS2/blob/ce54c9cceef68b78be7cc429988df26add904d9b/lib/compress.js#L484-L486

At least the output is not incorrect. We can revisit later if we decide to have collapse_vars respect -c toplevel.

[kzc]

@alexlamsl I dislike unknown behaviors, so I named all the anonymous functions in collapse_single_use_vars() and confirmed it was indeed the collapse_child_block code above:

$ echo 'var a="";for(a in {}){var b=2;f(b*5)}' | jx bin/uglifyjs -c reduce_vars=false

replace_var
collapse_constant
_
anonymous
_
do_list
doit
MAP
do_list
anonymous
_
anonymous
_
collapse_single_use_vars
collapse_child_block
collapse_single_use_vars
tighten_body
anonymous
OPT
anonymous
_
anonymous
time_it
replenish
read_whole_file
emit
endReadable
_tickCallback

WARN: Collapsing constant b [-:1,32]
var a="";for(a in{}){f(5*2)}

I used jx from the (now defunct) jxcore project which was a Node.JS 0.11.x port to the SpiderMonkey VM which gives a better stack trace than Node/V8.

The function _ calls you see above are from:

https://github.com/mishoo/UglifyJS2/blob/ce54c9cceef68b78be7cc429988df26add904d9b/lib/transform.js#L57

[alexlamsl]

@kzc thanks for the detailed investigation - something to bear in mind when we revisit this later 😉

I did follow jxcore before - shame that we are still pretty much stuck with a single VM implementation.

[kzc]

I did follow jxcore before - shame that we are still pretty much stuck with a single VM implementation.

Indeed. Microsoft had to fake out a V8 C++ API shim in order to get their Chakra port of Node.JS to work.

I've heard that the former jxcore lead is now working at Microsoft on the Chakra engine.

[alexlamsl]

Indeed. Microsoft had to fake out a V8 C++ API shim in order to get their Chakra port of Node.JS to work.

... which is what I'm using for development. So apologies in advance if I hit some node-chakracore specific bug and confused everyone in the future 😉

I've heard that the former jxcore lead is now working at Microsoft on the Chakra engine.

Good to know 👍

[kzc]

node-chakracore - how good is that port? Any deficiencies or incompatibilities of note?

How's the uglify speed compared to Node/V8 for a large file like math.js?

[alexlamsl]

For the work I do I rarely run into issues, the latest being nodejs/node-chakracore#170 but those are always questionable use of "feature" IMHO.

>nvs use node
PATH -= C:\ProgramData\nvs\chakracore\7.0.0-pre10\x64
PATH += C:\ProgramData\nvs\node\7.7.1\x64

>node -v
v7.7.1

>node bin\uglifyjs math.js -o min.js -mc warnings=false,unsafe,unsafe_comps,unsafe_math,unsafe_proto,keep_fargs=false,pure_getters --stats
Timing information (compressed 1 files):
- parse: 0.593s
- scope: 1.531s
- squeeze: 14.891s
- mangle: 0.125s
- generate: 0.407s

>nvs use chakracore
PATH -= C:\ProgramData\nvs\node\7.7.1\x64
PATH += C:\ProgramData\nvs\chakracore\7.0.0-pre10\x64

>node -v
v7.0.0-nightly201612169538b4aa4f

>node bin\uglifyjs math.js -o min.js -mc warnings=false,unsafe,unsafe_comps,unsafe_math,unsafe_proto,keep_fargs=false,pure_getters --stats
Timing information (compressed 1 files):
- parse: 0.481s
- scope: 1.370s
- squeeze: 17.138s
- mangle: 0.136s
- generate: 0.481s

>nvs use chakracore-nightly
PATH -= C:\ProgramData\nvs\chakracore\7.0.0-pre10\x64
PATH += C:\ProgramData\nvs\chakracore-nightly\8.0.0-nightly20170303fd355edcd9\x64

>node -v
v8.0.0-nightly20170303fd355edcd9

>node bin\uglifyjs math.js -o min.js -mc warnings=false,unsafe,unsafe_comps,unsafe_math,unsafe_proto,keep_fargs=false,pure_getters --stats
Timing information (compressed 1 files):
- parse: 0.462s
- scope: 1.388s
- squeeze: 16.731s
- mangle: 0.132s
- generate: 0.515s

Not that far behind in terms of performance either.