Uglifyjs: `reduce_vars` doesn't replace single-use class definitions as it does with functions

[kzc]

Bug report or feature request?

feature request

ES5 or ES6+ input?

ES6

Uglify version (uglifyjs -V)

uglify-es 3.1.6

JavaScript input

Would like single-use class definitions to be converted to class expressions:

$ echo 'class Foo{}; console.log(Foo.name);' | bin/uglifyjs -c toplevel,keep_fnames
class Foo{}console.log(Foo.name);

as is currently done for function definitions:

$ echo 'function Bar(){}; console.log(Bar.name);' | bin/uglifyjs -c toplevel,keep_fnames
console.log(function Bar(){}.name);

i.e., for the first example I'd like to see this output:

console.log(class Foo{}.name);

Doing so would allow further optimization of programs involving single-use class definitions and hoist_props with #2415:

$ cat test.js
class Foo {}
var o1 = { c: Foo };
console.log(o1.c.name);

function Bar() {}
var o2 = { f: Bar };
console.log(o2.f.name);

$ bin/uglifyjs test.js -c toplevel,keep_fnames,hoist_props,passes=9 -b
class Foo {}

var o1 = {
    c: Foo
};

console.log(o1.c.name), console.log(function Bar() {}.name);

[kzc]

@alexlamsl I have no idea where to look in reduce_vars to implement this, so no PR is pending unfortunately.

[kzc]

Implementing this issue might inadvertently break some code relying on names in class definitions being retained, so we might want to bump the minor version number.

This feature request also brings to light the problem of a lack of a keep_classnames option in compress, with compress keep_fnames being responsible for both function and class names. Class and function name retention is a mess - it's an all or nothing proposition.

So for a given input:

$ cat names.js
console.log(class C1{}, class C2{}, function F1(){}, function F2(){});

there is no middle ground between this:

$ cat names.js | bin/uglifyjs -mc
console.log(class{},class{},function(){},function(){});

and this:

$ cat names.js | bin/uglifyjs -m keep_fnames,keep_classnames -c keep_fnames
console.log(class C1{},class C2{},function F1(){},function F2(){});

Well, one could do this:

$ cat names.js | bin/uglifyjs -m reserved=[C1,F2] -c keep_fnames
console.log(class C1{},class c{},function c(){},function F2(){});

but that's not great either - in order to retain certain class/function expression names you have to keep useless mangled names for the rest.

Ideally keep_fnames and keep_classnames should optionally accept a RegExp to decide which classes and functions not to disturb. So for example one could specify:

$ cat names.js | bin/uglifyjs --keep-fnames /2/ --keep-classnames /1/ -mc
console.log(class C1{},class{},function(){},function F2(){});

[kzc]

It'd be nice if --keep-fnames and --keep-classnames would be able to accept either a RegExp or an array of valid names.

[alexlamsl]

Let's keep those -keep-*names stuff in a separate issue report :wink:

As for your original example above - taking a smaller step forward:

$ echo 'var Foo=class{}; console.log(Foo.name);' | bin/uglifyjs -c toplevel
console.log(class{}.name);

can be achieved by the following patch:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -2263,9 +2263,8 @@ merge(Compressor.prototype, {
                     return false;
             return true;
         }
-        def(AST_Node, return_false);
-        def(AST_Constant, return_true);
-        def(AST_Function, function(){
+
+        function all_refs_local() {
             var self = this;
             var result = true;
             self.walk(new TreeWalker(function(node) {
@@ -2280,7 +2279,12 @@ merge(Compressor.prototype, {
                 }
             }));
             return result;
-        });
+        }
+
+        def(AST_Node, return_false);
+        def(AST_Constant, return_true);
+        def(AST_ClassExpression, all_refs_local);
+        def(AST_Function, all_refs_local);
         def(AST_Unary, function(){
             return this.expression.is_constant_expression();
         });

[kzc]

Let's keep those -keep-*names stuff in a separate issue report

I'll create an issue for it.

[alexlamsl]

I'll create an issue for it.

Ninja-ed you with #2418 :ghost:

[kzc]

In the patch shouldn't the following be added to is_constant_expression?

+        def(AST_RegExp, return_false);

[alexlamsl]

In the patch shouldn't the following be added to is_constant_expression?

Not really, as you can't shove a variable into a RegExp literal - at least not in ES5 anyway.

[kzc]

When is is_constant_expression used?

[alexlamsl]

When is is_constant_expression used?

1) reduce_vars to see if single_use variable is safe to be moved (across scopes)
2) inline to see if function() { return <<value>> } can be flattened into <<value>>

[kzc]

Okay then - I'll take your word for it that RegExp is not an issue. Was thinking of the while-loop mutating RegExp match case.

[alexlamsl]

Was thinking of the while-loop mutating RegExp match case.

That is taken care of by the fact that such AST_SymbolRef won't be counted as a single reference.

[alexlamsl]

Here's the full example from this issue:

$ echo 'class Foo{} console.log(Foo.name)' | uglifyjs -c toplevel,keep_fnames
console.log(class Foo{}.name);

And here's the full patch that makes it work:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -383,7 +383,7 @@ merge(Compressor.prototype, {
                         }
                     }
                 }
-                if (node instanceof AST_Defun) {
+                if (node instanceof AST_DefClass || node instanceof AST_Defun) {
                     var d = node.name.definition();
                     if (compressor.exposed(d) || safe_to_read(d)) {
                         d.fixed = false;
@@ -2263,9 +2263,8 @@ merge(Compressor.prototype, {
                     return false;
             return true;
         }
-        def(AST_Node, return_false);
-        def(AST_Constant, return_true);
-        def(AST_Function, function(){
+
+        function all_refs_local() {
             var self = this;
             var result = true;
             self.walk(new TreeWalker(function(node) {
@@ -2280,7 +2279,12 @@ merge(Compressor.prototype, {
                 }
             }));
             return result;
-        });
+        }
+
+        def(AST_Node, return_false);
+        def(AST_Constant, return_true);
+        def(AST_ClassExpression, all_refs_local);
+        def(AST_Function, all_refs_local);
         def(AST_Unary, function(){
             return this.expression.is_constant_expression();
         });
@@ -4450,13 +4454,16 @@ merge(Compressor.prototype, {
             && is_lhs(self, compressor.parent()) !== self) {
             var d = self.definition();
             var fixed = self.fixed_value();
+            if (fixed instanceof AST_DefClass) {
+                d.fixed = fixed = make_node(AST_ClassExpression, fixed, fixed);
+            }
             if (fixed instanceof AST_Defun) {
                 d.fixed = fixed = make_node(AST_Function, fixed, fixed);
             }
             if (compressor.option("unused")
                 && fixed
                 && d.references.length == 1
-                && (d.single_use || is_func_expr(fixed)
+                && (d.single_use || (is_func_expr(fixed) || fixed instanceof AST_ClassExpression)
                     && !(d.scope.uses_arguments && d.orig[0] instanceof AST_SymbolFunarg)
                     && !d.scope.uses_eval
                     && compressor.find_parent(AST_Scope) === fixed.parent_scope)) {

[kzc]

@alexlamsl Notice how the issue was renamed due to my confusion over the name of the option involved? Basically we need to make options for all permutations of the words "reduce", "hoist", "vars" and "properties" - reduce_props anyone?

[alexlamsl]

@kzc so _that's_ why - I sincerely thought you had a good reason to suspect hoist_vars should be responsible for making this issue work.

And then I rushed out the patch above forgetting to enquire on this curious point. :sweat_smile: