augment evaluate to extract within objects by alexlamsl · Pull Request #1425 · mishoo/UglifyJS

alexlamsl · 2017-01-15T10:46:49Z

This replaces and extends the existing "string".length => 6 optimisation, and is also hidden behind unsafe compress option flag.

Basically this enables evaluate to navigate into object property on a best effort basis, and bails out if the final form isn't constant.

Allows for constants hidden inside configuration objects to propagate, which is often encountered in the field. Does not address #1416 directly but may form part of the final solution.

avdg · 2017-01-15T11:12:25Z

I don't see any reason to have this optimization under safe flag on long term if the feature is well maintained and done on literals only.

But to be sure, there need some tests on literals containing non-constant expressions or variable access (since access on variables can be modified in js in very surprising ways)

alexlamsl · 2017-01-15T11:14:32Z

@avdg I concur - though since "string".length was behind unsafe, I erred on the safe side. 😛

avdg · 2017-01-15T11:18:49Z

I think it's worth doing an investigation on that if time permits.

avdg

Looking good on my end.

alexlamsl · 2017-01-15T11:25:13Z

@avdg let's leave that for another PR, as I need incremental understanding on all the corner cases before I'm comfortable enough to tackle the general case 😅

Thanks for the review!

avdg · 2017-01-15T11:27:51Z

No worries patch is fine as is and understanding risk takes time. So I understand the precaution.

avdg · 2017-01-15T12:12:38Z

The "string".length thingy came from 8511e80 (#501)

kzc · 2017-01-15T14:48:29Z

lib/compress.js

-                    return str.length;
+            if (compressor.option("unsafe")) {
+                var val = ev(this.expression, compressor);
+                if (HOP(val, this.property)) {


Before calling HOP() must check if first argument is an object literal:

$ echo 'a = null.p;' | bin/uglifyjs -c unsafe undefined:6244 if (ex !== def) throw ex; ^ TypeError: Cannot convert undefined or null to object at hasOwnProperty (<anonymous>)

We want to preserve bad javascript code as is. Garbage in, garbage out.

Hmm, your example seems to work over here: 😓

C:\UglifyJS2>echo a = null.p; | node bin\uglifyjs -c unsafe a=null.p; C:\UglifyJS2>node > var minify = require('uglify-js').minify; undefined > minify('a = null.p;', { fromString: true, compress: { unsafe: true } }); { code: 'a=null.p;', map: null }

Regardless, do you suggest I follow this line to fix this up?

@kzc fixed, added some tests for that as well

It just shows that hasOwnProperty is implemented differently on different node versions or platforms.

I suggest something like this:

if (val instanceof AST_Object && HOP(val, this.property)) {

Sorry, val instanceof AST_Object doesn't work here.

@kzc except val is an actual plain object at this point, not an AST_Node?

Just to be clear - does my latest commit address your concern? 😅

Maybe this is sufficient:

if (val && HOP(val, this.property)) {

I think that works.

@kzc done in b363786 - thanks!

kzc · 2017-01-15T15:09:30Z

@avdg If this PR is ever merged to harmony, then an ev() would have to be done on the key expression for computed properties for side effects. Not an issue in ES5/master as key is side effect free AFAIK.

avdg · 2017-01-15T15:17:54Z

uh yeah, if we get more of these patches that behave totally different on harmony, it would be better that I would have repo access, so I can quickly fix the incompatibles, I don't think @rvanvelzen would be able to do this alone, though I don't want to do this alone myself (as it impacts user code in the end).

For now I might want to delay this pr until I finish my other pr's I'm currently working on, so we have patches for harmony as well.

alexlamsl · 2017-01-15T15:32:43Z

@avdg @kzc if you are concerned with harmony, I had a quick look and I think if I do if (prop instanceof AST_ObjectKeyVal && typeof prop.key === "string") { ... } that should make it safe for both branches?

... and may be move that def(AST_Object, ...) block up or down to avoid possible merge conflict with the def(AST_TemplateString, ...) which is new on the harmony branch.

avdg · 2017-01-15T15:52:55Z

I don't know if we should get any issues for harmony actually, but I'm working on something very related (AST_ArrayElement) so I might trip some wires and complicate code merging (but I might also not create these kind of conflicts).

Concerning computed properties, these are formatted var foo = {['foo' + 'bar']: "foobar!"} but I don't worry about optimizing these cases (yet). The worst case would be if that code simply doesn't work anymore with any optimization patches.

That said, we might need to keep track of optimizations that have to be implemented on harmony later. I do keep a list for few issues myself, but that's a list only I can access to as that is the project board on https://github.com/avdg/UglifyJS2/projects/

kzc · 2017-01-15T15:55:23Z

I think if I do if (prop instanceof AST_ObjectKeyVal && typeof prop.key === "string") { ... } that should make it safe for both branches?

Is typeof prop.key === "string" sufficient to avoid computed properties? We might want to use this on master anyway.

alexlamsl · 2017-01-15T15:56:36Z

@kzc that would be my understanding, as anything other than a constant key would be an AST_Node

kzc · 2017-01-15T16:02:41Z

Just to clarify my comment - in harmony is prop always guaranteed to be of type AST_ObjectKeyVal?

avdg · 2017-01-15T16:05:37Z

~~I don't think the ast documentation is up to date for AST_Object~~

edit: @kzc is more correct than me, everything is an AST_ObjectProperty for now.

kzc · 2017-01-15T16:13:10Z

on harmony:

var AST_ObjectProperty = DEFNODE("ObjectProperty", "key value", {
    $documentation: "Base class for literal object properties",
    $propdoc: {
        key: "[string|AST_Node] the property name converted to a string for ObjectKeyVal. For setters, getters and computed property this is an arbitrary AST_Node",

Just if (typeof prop.key === "string") should be sufficient I think.

kzc · 2017-01-15T16:16:56Z

If a computed prop is a constant expression reduced to a string literal in compress, then typeof prop.key === "string" would still work on a subsequent compress pass would it not?

alexlamsl · 2017-01-15T16:18:30Z

@kzc updated prop.key check as per request

kzc · 2017-01-15T16:22:20Z

lib/compress.js

            for (var i = this.properties.length; --i >= 0;) {
                var prop = this.properties[i];
-                if (prop instanceof AST_ObjectKeyVal) {
+                if (typeof prop.key === "string") {


I don't think that's advisable because it could skip a side effect for key/value in a computed prop or a situation where there's a non-string literal key.

On both master and harmony ev() always has to be evaluated for prop.value in case it throws (indicating a side effect).

@kzc would you mind giving me an example of what you mean? 😅

prop.key could be a Number literal with a value with a side effect.

Let's not worry about harmony for this PR. It complicates things.

Within this loop I'm constructing an object which I can evaluate the values of, filtering out all the others.

The idea is to keep the evaluation process forward, and then at the end see if we hit anything constant. If we don't (or there isn't a key match due to its leaf fails to evaluate), we just bail out.

Now that I'm thinking out loud, I think try-catch-ing the ev(prop.value, compressor); here would make sense as I want to skip over those failures.

@kzc ah, good point - I'll revert back to checking AST_ObjectKeyVal then.

I think try-catch-ing the ev(prop.value, compressor); here would make sense as I want to skip over those failures.

No, you must not catch those exceptions - they indicate a side effect and must be allowed to be thrown.

ev() always has to be evaluated on the prop.value regardless of the key.

For ES5/master the key is always constant AFAIK, so ev() does not have to be run on the key. But in harmony (if supported) then ev() must always be run on AST_Node prop.key values in case it throws.

I'll revert back to checking AST_ObjectKeyVal then

I don't think that check is needed at all.

@kzc thanks for bearing with me - I think I get your point now.

Can you check if the tests in the latest commit is good enough to cover this path?

kzc · 2017-01-15T16:54:40Z

lib/compress.js

-                if (prop instanceof AST_ObjectKeyVal) {
-                    val[prop.key] = ev(prop.value, compressor);
-                }
+                val[prop.key] = ev(prop.value, compressor);


I think that's the correct way to do it for ES5/master.

Can you please add a comment above this live that harmony/ES6 will additionally have to run ev() on prop.key which will throw on side effects?

@alexlamsl Will the following line work in ES5/master?

val[ev(prop.key, compressor)] = ev(prop.value, compressor);

If so, maybe that's the answer for both branches.

Sorry for the the stream of thoughts. Just thinking out loud. Implementing this optimization is more complicated than it first appears.

@kzc not directly - but I can do:

var key = prop.key; if (key instanceof AST_Node) { key = ev(key, compressor); } val[key] = ev(prop.value, compressor);

would that be acceptable?

@kzc no worries! I set aside this weekend to tackle this. I run into this case often enough to want to work on it. 😃

kzc · 2017-01-15T17:25:01Z

@alexlamsl I think your "ensure compatibility with harmony branch" fix is okay, but I found a new problem...

expected:

$ echo 'console.log({a:1, a:2}.a);' | node
2

without your PR:

$ echo 'console.log({a:1, a:2}.a);' | uglifyjs -c unsafe | node
2

with your PR:

$ echo 'console.log({a:1, a:2}.a);' | bin/uglifyjs -c unsafe | node
1

Could you please prepend [WIP] to this issue's title while we mull over other potential issues?

alexlamsl · 2017-01-15T17:39:43Z

@kzc fix pushed, title updated.

kzc · 2017-01-15T17:39:55Z

@alexlamsl Could you please add a few tests with integer and floating point keys - with and without side effects in the value? A key of 0 in particular may prove interesting.

kzc · 2017-01-15T17:48:10Z

test/compress/evaluate.js

+        console.log(
+            ({a:{b:1},a:1}) + 1,
+            2,
+            ({a:{b:1},a:1}).b + 1,


Is 1..b the correct expected result?

Hard to compare the input with expected because of the length of the tests. Would you mind splitting all the various types of tests into separate named tests - each testing one particular facet? i.e., object_literal_repeated_props, prop_on_null, object_literal_with_side_effect_in_prop_value, etc.

@kzc indeed this is getting incomprehensible - I'm splitting them out as we speak 😅

1..b is correct as far as I can tell - one dot would cause syntax error as it confuses with numeral literals.

Cool. Thanks.

The test names I gave are just examples - please use whatever test names you think are appropriate.

kzc · 2017-01-15T17:51:46Z

lib/compress.js

        def(AST_Object, function(compressor){
            var val = {};
-            for (var i = this.properties.length; --i >= 0;) {
+            for (var i = 0, l = this.properties.length; i < l; i++) {


This is a personal preference I admit, but I'd prefer l not be used as a variable name because it looks like 1. Could you please use len instead?

alexlamsl · 2017-01-15T18:24:43Z

@kzc nice suggestion for trying out numbers as property keys - as they cannot be AST_Dot, this exposes a previous issue with evaluate() not optimising for AST_Sub.

I suppose this wasn't an issue with ES5 as things like "..."["len" + "gth"] will eventually be optimised into "...".length and feed into evaluate() all the same.

Also split out the tests, added new ones, and refactored the eye-sore variable as per request.

kzc · 2017-01-15T18:58:19Z

this exposes a previous issue with evaluate() not optimising for AST_Sub.

Yeah, I just happened to coincidentally notice that during testing this PR as well. ;-)

The tests are much clearer now. Thanks.

I don't have the time to further review this PR right now unfortunately. Sometimes better to wait on a complex PR like this. Other unforeseen cases usually come up.

kzc · 2017-01-16T16:21:02Z

the complication is with taking care of all the corner cases - the actual code changes are fairly minimal

No doubt. Javascript is a big collection of corner cases.

kzc · 2017-01-16T16:23:36Z

a < b

Be warned - there be dragons in Javascript inequality statements. Talk about corner cases.

alexlamsl · 2017-01-16T16:27:01Z

By a < b I mean turning var a = 3; console.log(a < b) to console.log(3 < b) - I hope that's safe? 👻

alexlamsl · 2017-01-16T16:29:47Z

Anyway, give me a shout when new error cases are found and I'll work on them some more. Otherwise I'll leave this PR here for you to merge at your leisure 😎

avdg · 2017-01-16T16:32:20Z

I guess that specific example is fine, but variable access can have weird stuff installed (rarely used though). Things like https://github.com/tc39/test262/blob/master/test/language/expressions/less-than-or-equal/S11.8.3_A2.3_T1.js

kzc · 2017-01-16T16:33:07Z

@alexlamsl I think this PR is in good shape but it would be wise to wait a week before removing the [WIP] just in case someone finds some breakage.

alexlamsl · 2017-01-16T16:35:45Z

@avdg thanks for the pointer - I'll try to keep that in mind for my next adventure!

@kzc sounds like a good plan.

avdg · 2017-01-16T16:35:53Z

[wip] just means that there are planned changes to take into account. If there is nothing planned, [wip] isn't necessary. There is still a lot that can go wrong before (and sadly enough just after) merging.

kzc · 2017-01-16T16:36:57Z

Regarding inequality comparisons, see unsafe_comps in compress.js.

kzc · 2017-01-16T16:39:50Z

If there is nothing planned, [wip] isn't necessary.

I personally take the bug report frequency into account before removing [WIP] on complicated far-reaching enhancements. If no reports for a while, it's usually safer.

But because this feature is mostly behind the unsafe option, it's less of a risk.

kzc · 2017-01-17T03:26:39Z

@alexlamsl Here's a new bug in code I found in the wild:

$ echo 'Object({toString: 0} + "");' | bin/uglifyjs -c unsafe=true
undefined:6259
                if (ex !== def) throw ex;
                                ^

TypeError: Cannot convert object to primitive value

Edit: some background... It's part of a function that checks whether an object is a host object in IE8 and earlier, so the author presumably wouldn't want it optimized away. But they wouldn't likely want to use the -c unsafe=true flag on such code.

alexlamsl · 2017-01-17T06:25:43Z

@kzc interesting hack - fixed 😉

kzc · 2017-01-17T15:52:35Z

Assuming no additional bugs are found please squash PR into a single commit when [WIP] is removed. Thanks.

- gated by `unsafe` - replaces previous optimisation specific to String.length - "123"[0] => 1 - [1, 2, 3][0] => 1 - [1, 2, 3].length => 3 - does not apply to objects with overridden prototype functions

alexlamsl · 2017-01-20T05:23:33Z

rebased & squashed

rvanvelzen · 2017-01-26T11:14:12Z

This is looking very good, thanks a lot for the hard work. :)

alexlamsl · 2017-02-15T09:46:34Z

If I'm correct this should resolve #787

avdg approved these changes Jan 15, 2017

View reviewed changes

kzc reviewed Jan 15, 2017

View reviewed changes

alexlamsl force-pushed the unsafe-evaluate branch from 3bd8ddc to b363786 Compare January 15, 2017 16:37

kzc reviewed Jan 15, 2017

View reviewed changes

alexlamsl changed the title ~~augment evaluate to extract within objects~~ [WIP] augment evaluate to extract within objects Jan 15, 2017

kzc reviewed Jan 15, 2017

View reviewed changes

alexlamsl force-pushed the unsafe-evaluate branch from 4026a26 to d2a4f29 Compare January 17, 2017 06:28

This was referenced Jan 17, 2017

add missing LHS cases which global_defs should avoid #1426

Merged

optimise binary operands with evaluate() #1427

Merged

augment evaluate to extract within objects

af0a5fb

- gated by `unsafe` - replaces previous optimisation specific to String.length - "123"[0] => 1 - [1, 2, 3][0] => 1 - [1, 2, 3].length => 3 - does not apply to objects with overridden prototype functions

alexlamsl force-pushed the unsafe-evaluate branch from d2a4f29 to af0a5fb Compare January 20, 2017 05:21

alexlamsl changed the title ~~[WIP] augment evaluate to extract within objects~~ augment evaluate to extract within objects Jan 20, 2017

alexlamsl mentioned this pull request Jan 20, 2017

Next release after v2.7.5 #1383

Closed

rvanvelzen merged commit 0d7d491 into mishoo:master Jan 26, 2017

alexlamsl deleted the unsafe-evaluate branch January 26, 2017 11:36

This was referenced Feb 6, 2017

[WIP] allow global_defs to pass in objects and arrays #1467

Closed

enhance global_defs #1469

Closed

This was referenced Feb 24, 2017

Constant evaluation is dumb #787

Closed

Remove property aliases of objects if used only once within identical scope #634

Closed

fix evaluate on object getter & setter #1515

Merged

Conversation

alexlamsl commented Jan 15, 2017

Uh oh!

avdg commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexlamsl commented Jan 15, 2017

Uh oh!

avdg commented Jan 15, 2017

Uh oh!

avdg left a comment

Choose a reason for hiding this comment

Uh oh!

alexlamsl commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avdg commented Jan 15, 2017

Uh oh!

avdg commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexlamsl Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Jan 15, 2017

Uh oh!

avdg commented Jan 15, 2017

Uh oh!

alexlamsl commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avdg commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzc commented Jan 15, 2017

Uh oh!

alexlamsl commented Jan 15, 2017

Uh oh!

kzc commented Jan 15, 2017

Uh oh!

avdg commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzc commented Jan 15, 2017

Uh oh!

kzc commented Jan 15, 2017

Uh oh!

alexlamsl commented Jan 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avdg commented Jan 15, 2017 •

edited

Loading

alexlamsl commented Jan 15, 2017 •

edited

Loading

avdg commented Jan 15, 2017 •

edited

Loading

alexlamsl Jan 15, 2017 •

edited

Loading

alexlamsl commented Jan 15, 2017 •

edited

Loading

avdg commented Jan 15, 2017 •

edited

Loading

avdg commented Jan 15, 2017 •

edited

Loading

kzc Jan 15, 2017 •

edited

Loading

kzc Jan 15, 2017 •

edited

Loading