enhance `global_defs` by alexlamsl · Pull Request #1469 · mishoo/UglifyJS

alexlamsl · 2017-02-07T17:10:19Z

This is an alternative approach to #1467, incorporating the feature to substitute AST_Node when supplied via command-line (API also supported, but a bit messier).

This PR also has the advantage of not relying on #1425 thus not requiring unsafe to function for expanded property syntax, i.e. process.engine="V8" now works with default compress options.

Included (slightly modified) all the tests from #1467, added cli mocha tests for #1467 (comment)

/cc @kzc

kzc · 2017-02-07T21:01:55Z

test/compress/global_defs.js

+        console.log(++CONFIG.VALUE);
+        console.log(++CONFIG["VAL" + "UE"]);
+        console.log(++DEBUG[CONFIG.VALUE]);
+        CONFIG.VALUE.FOO = "bar";


This leads to a nonsensical replacement 42..FOO - could you replace it with something that works - be it another variable or an object literal or something?

Fixed - now behaves the same way as #1467.

Works for me.

kzc · 2017-02-07T21:13:24Z

If this PR incorporates the same tests as PR #1467 and addresses the same issues and does not rely on unsafe, then this PR would be preferable.

kzc · 2017-02-07T21:17:22Z

I noticed that this works:

$ echo 'console.log(baz);' | bin/uglifyjs --define console.log=println -c
println(baz);

but the reverse does not:

$ echo 'println(baz);' | bin/uglifyjs --define println=console.log -c
undefined:8001
                && (!self.scope.uses_with || !compressor.find_parent(AST_With))) {
                               ^

TypeError: Cannot read property 'uses_with' of undefined

It's it's too difficult to fix we can drop the arbitrary node replacement feature altogether and just stick to replacing constants and literals.

kzc · 2017-02-07T21:19:39Z

@alexlamsl By the way, these PRs are really impressive. Just tell me when you think I'm asking for an unreasonable feature.

alexlamsl · 2017-02-07T23:49:04Z

@kzc as long as you are willing to test and point out areas of bugs / improvements, I'm happy to give it a go 😉

While making fixes to #1467, I noticed there is a fundamental difference between C={D:1} and C.D=1, i.e. the former express an absolute knowledge on C whereas the latter is only sure C has one defined property with a specific value.

So this is a reboot of the efforts, separating #1425 (best-effort basis) with global_defs (absolute substitution). Of course in some cases both features would be required to give a final optimal result.

Let me try and address the two issues you've mentioned above, and if successful, I'll close out #1467.

kzc · 2017-02-08T01:30:16Z

test/compress/issue-208.js

+    options = {
+        global_defs: { DEBUG: 0 }
+    }
+    input: {


Please add this to the beginning of input:

const DEBUG = true;

It should also generate a warning.

Could you also add OTHER: 1 to global_defs and the following line to the test:

var OTHER;

To ensure it also produces a warning.

It would generate a warning, plus killing all the positive cases below (because they are no longer AST_SymolRef.undeclared() 😅

I'll pick two OTHER variables as you suggested then.

You could name it something else. Just curious what happens to variable declarations that are set and not set.

Having const DEBUG = false; at the top of the file and overriding it with --define DEBUG=true (or vice versa) would be a common use case. It would work well with -c toplevel.

Overriding the definition would be ideal if you don't mind the extra work.

Okay, I'll go with the warning option then.

That's fine too.

Overriding the definition would be ideal

Should we be limiting this override to top-level-defined variables or at any scopes?

So this is an interesting case to consider:

global_defs: { "config.DEBUG" : 1 } var config = { engine: "voom!" }; if (config.DEBUG) { log(); }

Should we override the value and/or raise a warning, or something else altogether?

I've added those tests to this PR to reflect the current behaviour. Please leave comments on what you expect each case to be and we'll go from there?

kzc · 2017-02-08T02:28:13Z

test/compress/issue-208.js

        x = DEBUG;
    }
    expect: {
+        const ENV = 3;


overriding const ENV should warn.

See if d7e9eb9 works?

kzc · 2017-02-08T02:29:49Z

test/compress/issue-208.js

    }
    expect: {
+        const ENV = 3;
+        var FOO = 4;


overriding var FOO should warn.

One could argue in this specific case if it is set to the same value (4) it shouldn't - but that's too much work.

See if d7e9eb9 works?

kzc · 2017-02-08T02:34:28Z

test/compress/global_defs.js

    }
    expect: {
+        const FOO = { BAR: 0 };
+        console.log(FOO.BAR);


This is a tricky case. But since FOO is known to be global I think FOO.BAR should be changed to 0.

Had FOO been a local variable or parameter, then it would have been left as is.

So do you mean no changes to current behaviour is required for this case?

If FOO is global - which it is in this test case - then FOO.BAR should be changed to 0.

Ah I see - the FOO.BAR ➡️ 0 optimisation goes under #1425. I'll add unsafe here to enable it then.

Done 28c4d97

Interesting side effect. That wasn't my intention. Even if FOO was declared as const FOO = null; I was thinking that since FOO is global then FOO.BAR is fair game to be replaced with the global_defs setting.

I'm slightly confused now - FOO.BAR is defined as "moo" In global_defs, so do you mean you want that instead of 0?

PTAL at e88acb4 and see if that matches what you mean?

I think the new test and behavior is correct.

kzc · 2017-02-08T02:49:58Z

lib/compress.js

    }

+    (function (def){
+        AST_Node.DEFMETHOD("resolve_defines", function(compressor) {


To speed this up when global_defs are not defined:

if (!compressor.option("global_defs")) return null;

Done 00d93f3

kzc · 2017-02-08T02:52:23Z

lib/compress.js

+        });
+        def(AST_SymbolRef, function(compressor, suffix){
+            if (!this.undeclared()) return;
+            var name = this.name + suffix;


Done 85c3c60

kzc · 2017-02-08T02:53:03Z

lib/compress.js

+            if (!this.undeclared()) return;
+            var name = this.name + suffix;
+            var defines = compressor.option("global_defs");
+            if (defines && HOP(defines, name)) {


if (defines && HOP(defines, (name = this.name + suffix))) {

Done 85c3c60

alexlamsl · 2017-02-08T03:17:53Z

@kzc you may have discovered a bug / missing feature in #1469 (comment)

$ echo 'const a={b:0};console.log(a.b)' | bin/uglifyjs -c unsafe
const a={b:0};console.log(0);

$ echo 'const a=0;console.log(a)' | bin/uglifyjs -c
const a=0;console.log(a);

$ echo 'const a=0;console.log(a)' | bin/uglifyjs -c unsafe
const a=0;console.log(a);

kzc · 2017-02-08T04:00:09Z

test/compress/global_defs.js

+    }
+    input: {
+        function f(CONFIG) {
+            return CONFIG.VALUE;


add comment: // CONFIG not global - do not replace

Done 889e879

kzc · 2017-02-08T04:00:18Z

test/compress/global_defs.js

+        }
+        function g() {
+            var CONFIG = { VALUE: 1 };
+            return CONFIG.VALUE;


add comment: // CONFIG not global - do not replace

Done 889e879

kzc · 2017-02-08T04:01:38Z

test/compress/global_defs.js

+    }
+    input: {
+        function f(CONFIG) {
+            return CONFIG.VALUE;


add comment: // CONFIG not global - do not replace

Done 889e879

kzc · 2017-02-08T04:02:00Z

test/compress/global_defs.js

+        }
+        function g() {
+            var CONFIG = { VALUE: 1 };
+            return CONFIG.VALUE;


add comment: // CONFIG not global - do not replace

Done 889e879

alexlamsl · 2017-02-08T04:06:00Z

@kzc I've updated test/compress/issue-208.js in 7495875 to clarify the value of ENV - is that what you expect it to be?

(If it were to be overridden by global_defs, it would have been 10 as opposed to 30.)

alexlamsl · 2017-02-08T04:06:23Z

(Travis CI timed out.)

kzc · 2017-02-08T04:43:41Z

I think we have keep backwards compatibility with -c --define:

$ uglifyjs -V
uglify-js 2.7.5

$ echo 'f(ENV * 10);' | uglifyjs -c --define ENV=1
f(10);

$ echo 'const ENV = 3; f(ENV * 10);' | uglifyjs -c --define ENV=1
const ENV=3;f(30);

But this seems inconsistent to me. ENV is global in both cases, but only in the former case is it replaced.

So if we are to keep backwards compatibility then we can't have a global const DEBUG = false; overridden by -c --define. That's unfortunate because that would be a real use case.

What's the logic governing the current behavior in master?

alexlamsl · 2017-02-08T04:54:15Z

Both 2.7.5 and master are governed by this rule, which means a variable will never trigger global_defs if defined (local or global).

So this PR current is backward compatible with 2.7.5, with the addition of a new warning emitted on this case. I suppose if we ever go to 3.x then we can change this behaviour?

alexlamsl · 2017-02-08T04:57:32Z

FYI, global_defs is introduced in dde5b22, and the only change to that line since then was #208.

kzc · 2017-02-08T05:02:12Z

So this PR current is backward compatible with 2.7.5, with the addition of a new warning emitted on this case. I suppose if we ever go to 3.x then we can change this behaviour?

It's probably prudent to retain this behavior in 2.x and revisit the logic in 3.x.

If the variable is declared in 2.x, then no substitutions can take place at all - correct? Hmm. I wonder if any code in the wild would break since using the --define would have no effect, and as such would probably not be used.

alexlamsl · 2017-02-08T05:09:07Z

If the variable is declared in 2.x, then no substitutions can take place at all - correct?

Except for the expanded syntax, e.g. --define CONFIG.DEBUG=false - in this case any CONFIG.DEBUG will get overridden even if a global variable CONFIG is defined.

Hmm. I wonder if any code in the wild would break

I'm wondering about that too - the 2.7.5 behaviour is so limited it's either not used, or the results aren't optimal. But hey, may be we should bet on the safe side, release the current form into the wild, then wait and see if there are request for enhancements on the Issue Tracker?

kzc · 2017-02-08T05:15:49Z

I tend to agree. A case could easily be made to allow --define overrides for declared globals. We don't have to wait for another user to request it since it's clearly a useful scenario.

@avdg, @rvanvelzen Do you have an opinion on this?

alexlamsl · 2017-02-09T03:26:15Z

Squashed all the previous commits, and added one to demonstrate the potential breaking change we've discussed.

FWIW, I'd vote towards including the change as it seems to be the expected behaviour, but I'm fine with it either way.

kzc · 2017-02-09T03:45:13Z

LGTM

avdg · 2017-02-09T11:05:16Z

LGTM unless there are coverage holes (which is a hard question anyway now especially without cover analysis).

But there are many tests in already :-)

- support arrays, objects & AST_Node - support `"a.b":1` on both cli & API - emit warning if variable is modified - override top-level variables fixes mishoo#1416 closes mishoo#1198

- support arrays, objects & AST_Node - support `"a.b":1` on both cli & API - emit warning if variable is modified - override top-level variables fixes mishoo#1416 closes mishoo#1198 closes mishoo#1469

alexlamsl changed the title ~~enhance global_defs~~ [WIP] enhance global_defs Feb 7, 2017

alexlamsl force-pushed the global_defs-obj branch from dfaedc7 to e36e622 Compare February 7, 2017 17:14

kzc reviewed Feb 7, 2017

View reviewed changes

alexlamsl mentioned this pull request Feb 8, 2017

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

Closed

kzc reviewed Feb 8, 2017

View reviewed changes

alexlamsl closed this Feb 8, 2017

alexlamsl reopened this Feb 8, 2017

alexlamsl force-pushed the global_defs-obj branch from c7c7218 to af34265 Compare February 9, 2017 03:13

alexlamsl changed the title ~~[WIP] enhance global_defs~~ enhance global_defs Feb 9, 2017

This was referenced Feb 9, 2017

Next release after v2.7.5 #1383

Closed

2.8.0 staging #1485

Merged

enhance global_defs

7c4bf9d

- support arrays, objects & AST_Node - support `"a.b":1` on both cli & API - emit warning if variable is modified - override top-level variables fixes mishoo#1416 closes mishoo#1198

alexlamsl force-pushed the global_defs-obj branch from 1f52b51 to 7c4bf9d Compare February 18, 2017 11:24

alexlamsl closed this in e275148 Feb 23, 2017

alexlamsl deleted the global_defs-obj branch February 24, 2017 00:23

This was referenced Feb 24, 2017

dead-code=true not removing unused function #21

Closed

Nested global_defs #954

Closed

Define constants in objects? #132

Closed

Update uglify gruntjs/grunt-contrib-uglify#453

Merged

Conversation

alexlamsl commented Feb 7, 2017

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 Feb 7, 2017

Uh oh!

kzc commented Feb 7, 2017

Uh oh!

kzc commented Feb 7, 2017

Uh oh!

alexlamsl commented Feb 7, 2017

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 Feb 8, 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!

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 Feb 8, 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!

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!

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 commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

kzc Feb 8, 2017 •

edited

Loading

kzc Feb 8, 2017 •

edited

Loading

alexlamsl commented Feb 8, 2017 •

edited

Loading

alexlamsl commented Feb 8, 2017 •

edited

Loading

kzc commented Feb 8, 2017 •

edited

Loading