fix bugs with getter/setter (#1926)

- `reduce_vars`
- `side_effects`
- property access for object
- `AST_SymbolAccessor` as key names

enhance `test/ufuzz.js`
- add object getter & setter
  - property assignment to setter
  - avoid infinite recursion in setter
- fix & adjust assignment operators
  - 50% `=`
  - 25% `+=`
  - 2.5% each for the rest
- avoid "Invalid array length"
- fix `console.log()`
  - bypass getter
  - curb recursive reference
- deprecate `-E`, always report runtime errors

lib/ast.js

@@ -685,8 +685,8 @@ var AST_Object = DEFNODE("Object", "properties", {
 var AST_ObjectProperty = DEFNODE("ObjectProperty", "key value", {
     $documentation: "Base class for literal object properties",
     $propdoc: {
-        key: "[string] the property name converted to a string for ObjectKeyVal.  For setters and getters this is an arbitrary AST_Node.",
+        key: "[string] the property name converted to a string for ObjectKeyVal.  For setters and getters this is an AST_SymbolAccessor.",
-        value: "[AST_Node] property value.  For setters and getters this is an AST_Function."
+        value: "[AST_Node] property value.  For setters and getters this is an AST_Accessor."
     },
     _walk: function(visitor) {
         return visitor._visit(this, function(){

lib/compress.js

@@ -368,6 +368,13 @@ merge(Compressor.prototype, {
                     pop();
                     return true;
                 }
+                if (node instanceof AST_Accessor) {
+                    var save_ids = safe_ids;
+                    safe_ids = Object.create(null);
+                    descend();
+                    safe_ids = save_ids;
+                    return true;
+                }
                 if (node instanceof AST_Binary
                     && (node.operator == "&&" || node.operator == "||")) {
                     node.left.walk(tw);
@@ -1220,12 +1227,12 @@ merge(Compressor.prototype, {
                 && !node.expression.has_side_effects(compressor);
     }
 
-    // may_eq_null()
+    // may_throw_on_access()
-    // returns true if this node may evaluate to null or undefined
+    // returns true if this node may be null, undefined or contain `AST_Accessor`
     (function(def) {
-        AST_Node.DEFMETHOD("may_eq_null", function(compressor) {
+        AST_Node.DEFMETHOD("may_throw_on_access", function(compressor) {
             var pure_getters = compressor.option("pure_getters");
-            return !pure_getters || this._eq_null(pure_getters);
+            return !pure_getters || this._throw_on_access(pure_getters);
         });
 
         function is_strict(pure_getters) {
@@ -1237,7 +1244,12 @@ merge(Compressor.prototype, {
         def(AST_Undefined, return_true);
         def(AST_Constant, return_false);
         def(AST_Array, return_false);
-        def(AST_Object, return_false);
+        def(AST_Object, function(pure_getters) {
+            if (!is_strict(pure_getters)) return false;
+            for (var i = this.properties.length; --i >=0;)
+                if (this.properties[i].value instanceof AST_Accessor) return true;
+            return false;
+        });
         def(AST_Function, return_false);
         def(AST_UnaryPostfix, return_false);
         def(AST_UnaryPrefix, function() {
@@ -1246,33 +1258,33 @@ merge(Compressor.prototype, {
         def(AST_Binary, function(pure_getters) {
             switch (this.operator) {
               case "&&":
-                return this.left._eq_null(pure_getters);
+                return this.left._throw_on_access(pure_getters);
               case "||":
-                return this.left._eq_null(pure_getters)
+                return this.left._throw_on_access(pure_getters)
-                    && this.right._eq_null(pure_getters);
+                    && this.right._throw_on_access(pure_getters);
               default:
                 return false;
             }
         })
         def(AST_Assign, function(pure_getters) {
             return this.operator == "="
-                && this.right._eq_null(pure_getters);
+                && this.right._throw_on_access(pure_getters);
         })
         def(AST_Conditional, function(pure_getters) {
-            return this.consequent._eq_null(pure_getters)
+            return this.consequent._throw_on_access(pure_getters)
-                || this.alternative._eq_null(pure_getters);
+                || this.alternative._throw_on_access(pure_getters);
         })
         def(AST_Sequence, function(pure_getters) {
-            return this.expressions[this.expressions.length - 1]._eq_null(pure_getters);
+            return this.expressions[this.expressions.length - 1]._throw_on_access(pure_getters);
         });
         def(AST_SymbolRef, function(pure_getters) {
             if (this.is_undefined) return true;
             if (!is_strict(pure_getters)) return false;
             var fixed = this.fixed_value();
-            return !fixed || fixed._eq_null(pure_getters);
+            return !fixed || fixed._throw_on_access(pure_getters);
         });
     })(function(node, func) {
-        node.DEFMETHOD("_eq_null", func);
+        node.DEFMETHOD("_throw_on_access", func);
     });
 
     /* -----[ boolean/negation helpers ]----- */
@@ -1813,11 +1825,11 @@ merge(Compressor.prototype, {
             return any(this.elements, compressor);
         });
         def(AST_Dot, function(compressor){
-            return this.expression.may_eq_null(compressor)
+            return this.expression.may_throw_on_access(compressor)
                 || this.expression.has_side_effects(compressor);
         });
         def(AST_Sub, function(compressor){
-            return this.expression.may_eq_null(compressor)
+            return this.expression.may_throw_on_access(compressor)
                 || this.expression.has_side_effects(compressor)
                 || this.property.has_side_effects(compressor);
         });
@@ -2370,6 +2382,7 @@ merge(Compressor.prototype, {
             var args = trim(this.args, compressor, first_in_statement);
             return args && make_sequence(this, args);
         });
+        def(AST_Accessor, return_null);
         def(AST_Function, return_null);
         def(AST_Binary, function(compressor, first_in_statement){
             var right = this.right.drop_side_effect_free(compressor);
@@ -2437,11 +2450,11 @@ merge(Compressor.prototype, {
             return values && make_sequence(this, values);
         });
         def(AST_Dot, function(compressor, first_in_statement){
-            if (this.expression.may_eq_null(compressor)) return this;
+            if (this.expression.may_throw_on_access(compressor)) return this;
             return this.expression.drop_side_effect_free(compressor, first_in_statement);
         });
         def(AST_Sub, function(compressor, first_in_statement){
-            if (this.expression.may_eq_null(compressor)) return this;
+            if (this.expression.may_throw_on_access(compressor)) return this;
             var expression = this.expression.drop_side_effect_free(compressor, first_in_statement);
             if (!expression) return this.property.drop_side_effect_free(compressor, first_in_statement);
             var property = this.property.drop_side_effect_free(compressor);

lib/parse.js

@@ -1310,10 +1310,15 @@ function parse($TEXT, options) {
             var type = start.type;
             var name = as_property_name();
             if (type == "name" && !is("punc", ":")) {
+                var key = new AST_SymbolAccessor({
+                    start: S.token,
+                    name: as_property_name(),
+                    end: prev()
+                });
                 if (name == "get") {
                     a.push(new AST_ObjectGetter({
                         start : start,
-                        key   : as_atom_node(),
+                        key   : key,
                         value : create_accessor(),
                         end   : prev()
                     }));
@@ -1322,7 +1327,7 @@ function parse($TEXT, options) {
                 if (name == "set") {
                     a.push(new AST_ObjectSetter({
                         start : start,
-                        key   : as_atom_node(),
+                        key   : key,
                         value : create_accessor(),
                         end   : prev()
                     }));

lib/scope.js

@@ -360,11 +360,6 @@ AST_Symbol.DEFMETHOD("unmangleable", function(options){
     return this.definition().unmangleable(options);
 });
 
-// property accessors are not mangleable
-AST_SymbolAccessor.DEFMETHOD("unmangleable", function(){
-    return true;
-});
-
 // labels are always mangleable
 AST_Label.DEFMETHOD("unmangleable", function(){
     return false;

test/compress/dead-code.js

@@ -131,3 +131,19 @@ try_catch_finally: {
         "1",
     ]
 }
+
+accessor: {
+    options = {
+        side_effects: true,
+    }
+    input: {
+        ({
+            get a() {},
+            set a(v){
+                this.b = 2;
+            },
+            b: 1
+        });
+    }
+    expect: {}
+}

test/compress/pure_getters.js

@@ -119,3 +119,62 @@ chained: {
         a.b.c;
     }
 }
+
+impure_getter_1: {
+    options = {
+        pure_getters: "strict",
+        side_effects: true,
+    }
+    input: {
+        ({
+            get a() {
+                console.log(1);
+            },
+            b: 1
+        }).a;
+        ({
+            get a() {
+                console.log(1);
+            },
+            b: 1
+        }).b;
+    }
+    expect: {
+        ({
+            get a() {
+                console.log(1);
+            },
+            b: 1
+        }).a;
+        ({
+            get a() {
+                console.log(1);
+            },
+            b: 1
+        }).b;
+    }
+    expect_stdout: "1"
+}
+
+impure_getter_2: {
+    options = {
+        pure_getters: true,
+        side_effects: true,
+    }
+    input: {
+        // will produce incorrect output because getter is not pure
+        ({
+            get a() {
+                console.log(1);
+            },
+            b: 1
+        }).a;
+        ({
+            get a() {
+                console.log(1);
+            },
+            b: 1
+        }).b;
+    }
+    expect: {}
+}

test/compress/reduce_vars.js

@@ -2508,3 +2508,32 @@ issue_1922_2: {
     }
     expect_stdout: "1"
 }
+
+accessor: {
+    options = {
+        evaluate: true,
+        reduce_vars: true,
+        toplevel: true,
+    }
+    input: {
+        var a = 1;
+        console.log({
+            get a() {
+                a = 2;
+                return a;
+            },
+            b: 1
+        }.b, a);
+    }
+    expect: {
+        var a = 1;
+        console.log({
+            get a() {
+                a = 2;
+                return a;
+            },
+            b: 1
+        }.b, a);
+    }
+    expect_stdout: "1 1"
+}

test/mocha/accessorTokens-1492.js

@@ -16,7 +16,7 @@ describe("Accessor tokens", function() {
                 assert.equal(node.start.pos, 12);
                 assert.equal(node.end.endpos, 46);
 
-                assert(node.key instanceof UglifyJS.AST_SymbolRef);
+                assert(node.key instanceof UglifyJS.AST_SymbolAccessor);
                 assert.equal(node.key.start.pos, 16);
                 assert.equal(node.key.end.endpos, 22);
 

test/mocha/getter-setter.js

@@ -71,7 +71,7 @@ describe("Getters and setters", function() {
         var fail = function(data) {
             return function (e) {
                 return e instanceof UglifyJS.JS_Parse_Error &&
-                    e.message === "Invalid getter/setter name: " + data.operator;
+                    e.message === "Unexpected token: operator (" + data.operator + ")";
             };
         };
 

test/sandbox.js

@@ -1,14 +1,16 @@
 var vm = require("vm");
 
-function safe_log(arg) {
+function safe_log(arg, level) {
     if (arg) switch (typeof arg) {
       case "function":
         return arg.toString();
       case "object":
         if (/Error$/.test(arg.name)) return arg.toString();
         arg.constructor.toString();
-        for (var key in arg) {
+        if (level--) for (var key in arg) {
-            arg[key] = safe_log(arg[key]);
+            if (!Object.getOwnPropertyDescriptor(arg, key).get) {
+                arg[key] = safe_log(arg[key], level);
+            }
         }
     }
     return arg;
@@ -48,7 +50,9 @@ exports.run_code = function(code) {
         ].join("\n"), {
             console: {
                 log: function() {
-                    return console.log.apply(console, [].map.call(arguments, safe_log));
+                    return console.log.apply(console, [].map.call(arguments, function(arg) {
+                        return safe_log(arg, 3);
+                    }));
                 }
             }
         }, { timeout: 5000 });

test/ufuzz.js

@@ -48,7 +48,6 @@ var STMT_COUNT_FROM_GLOBAL = true; // count statement depth from nearest functio
 var num_iterations = +process.argv[2] || 1/0;
 var verbose = false; // log every generated test
 var verbose_interval = false; // log every 100 generated tests
-var verbose_error = false;
 var use_strict = false;
 for (var i = 2; i < process.argv.length; ++i) {
     switch (process.argv[i]) {
@@ -58,9 +57,6 @@ for (var i = 2; i < process.argv.length; ++i) {
       case '-V':
         verbose_interval = true;
         break;
-      case '-E':
-        verbose_error = true;
-        break;
       case '-t':
         MAX_GENERATED_TOPLEVELS_PER_RUN = +process.argv[++i];
         if (!MAX_GENERATED_TOPLEVELS_PER_RUN) throw new Error('Must generate at least one toplevel per run');
@@ -103,7 +99,6 @@ for (var i = 2; i < process.argv.length; ++i) {
         console.log('<number>: generate this many cases (if used must be first arg)');
         console.log('-v: print every generated test case');
         console.log('-V: print every 100th generated test case');
-        console.log('-E: print generated test case with runtime error');
         console.log('-t <int>: generate this many toplevels per run (more take longer)');
         console.log('-r <int>: maximum recursion depth for generator (higher takes longer)');
         console.log('-s1 <statement name>: force the first level statement to be this one (see list below)');
@@ -192,12 +187,33 @@ var ASSIGNMENTS = [
     '=',
     '=',
     '=',
+    '=',
+    '=',
+    '=',
+    '=',
+
+    '=',
+    '=',
+    '=',
+    '=',
+    '=',
+    '=',
+    '=',
+    '=',
+    '=',
+    '=',
 
-    '==',
-    '!=',
-    '===',
-    '!==',
     '+=',
+    '+=',
+    '+=',
+    '+=',
+    '+=',
+    '+=',
+    '+=',
+    '+=',
+    '+=',
+    '+=',
+
     '-=',
     '*=',
     '/=',
@@ -207,7 +223,8 @@ var ASSIGNMENTS = [
     '<<=',
     '>>=',
     '>>>=',
-    '%=' ];
+    '%=',
+];
 
 var UNARY_SAFE = [
     '+',
@@ -359,7 +376,6 @@ function createFunction(recurmax, inGlobal, noDecl, canThrow, stmtDepth) {
     // avoid "function statements" (decl inside statements)
     else if (inGlobal || rng(10) > 0) s += 'var ' + createVarName(MANDATORY) + ' = ' + name + '(' + createArgs() + ');';
 
-
     return s;
 }
 
@@ -406,7 +422,7 @@ function getLabel(label) {
     return label && " L" + label;
 }
 
-function createStatement(recurmax, canThrow, canBreak, canContinue, cannotReturn, stmtDepth) {
+function createStatement(recurmax, canThrow, canBreak, canContinue, cannotReturn, stmtDepth, target) {
     ++stmtDepth;
     var loop = ++loops;
     if (--recurmax < 0) {
@@ -414,10 +430,11 @@ function createStatement(recurmax, canThrow, canBreak, canContinue, cannotReturn
     }
 
     // allow to forcefully generate certain structures at first or second recursion level
-    var target = 0;
+    if (target === undefined) {
         if (stmtDepth === 1 && STMT_FIRST_LEVEL_OVERRIDE >= 0) target = STMT_FIRST_LEVEL_OVERRIDE;
         else if (stmtDepth === 2 && STMT_SECOND_LEVEL_OVERRIDE >= 0) target = STMT_SECOND_LEVEL_OVERRIDE;
         else target = STMTS_TO_USE[rng(STMTS_TO_USE.length)];
+    }
 
     switch (target) {
       case STMT_BLOCK:
@@ -636,7 +653,7 @@ function _createExpression(recurmax, noComma, stmtDepth, canThrow) {
                 strictMode()
             );
             if (instantiate) for (var i = rng(4); --i >= 0;) {
-                if (rng(2)) s.push('this.' + getDotKey() + createAssignment() + _createBinaryExpr(recurmax, noComma, stmtDepth, canThrow) + ';');
+                if (rng(2)) s.push('this.' + getDotKey(true) + createAssignment() + _createBinaryExpr(recurmax, noComma, stmtDepth, canThrow) + ';');
                 else  s.push('this[' + createExpression(recurmax, COMMA_OK, stmtDepth, canThrow) + ']' + createAssignment() + _createBinaryExpr(recurmax, noComma, stmtDepth, canThrow) + ';');
             }
             s.push(
@@ -689,19 +706,19 @@ function _createExpression(recurmax, noComma, stmtDepth, canThrow) {
             ") || " + rng(10) + ").toString()[" +
             createExpression(recurmax, COMMA_OK, stmtDepth, canThrow) + "] ";
       case p++:
-        return createArrayLiteral(recurmax, COMMA_OK, stmtDepth, canThrow);
+        return createArrayLiteral(recurmax, stmtDepth, canThrow);
       case p++:
-        return createObjectLiteral(recurmax, COMMA_OK, stmtDepth, canThrow);
+        return createObjectLiteral(recurmax, stmtDepth, canThrow);
       case p++:
-        return createArrayLiteral(recurmax, COMMA_OK, stmtDepth, canThrow) + '[' +
+        return createArrayLiteral(recurmax, stmtDepth, canThrow) + '[' +
             createExpression(recurmax, COMMA_OK, stmtDepth, canThrow) + ']';
       case p++:
-        return createObjectLiteral(recurmax, COMMA_OK, stmtDepth, canThrow) + '[' +
+        return createObjectLiteral(recurmax, stmtDepth, canThrow) + '[' +
             createExpression(recurmax, COMMA_OK, stmtDepth, canThrow) + ']';
       case p++:
-        return createArrayLiteral(recurmax, COMMA_OK, stmtDepth, canThrow) + '.' + getDotKey();
+        return createArrayLiteral(recurmax, stmtDepth, canThrow) + '.' + getDotKey();
       case p++:
-        return createObjectLiteral(recurmax, COMMA_OK, stmtDepth, canThrow) + '.' + getDotKey();
+        return createObjectLiteral(recurmax, stmtDepth, canThrow) + '.' + getDotKey();
       case p++:
         var name = getVarName();
         return name + ' && ' + name + '[' + createExpression(recurmax, COMMA_OK, stmtDepth, canThrow) + ']';
@@ -713,7 +730,7 @@ function _createExpression(recurmax, noComma, stmtDepth, canThrow) {
     return _createExpression(recurmax, noComma, stmtDepth, canThrow);
 }
 
-function createArrayLiteral(recurmax, noComma, stmtDepth, canThrow) {
+function createArrayLiteral(recurmax, stmtDepth, canThrow) {
     recurmax--;
     var arr = "[";
     for (var i = rng(6); --i >= 0;) {
@@ -746,18 +763,56 @@ var KEYS = [
     "3",
 ].concat(SAFE_KEYS);
 
-function getDotKey() {
+function getDotKey(assign) {
-    return SAFE_KEYS[rng(SAFE_KEYS.length)];
+    var key;
+    do {
+        key = SAFE_KEYS[rng(SAFE_KEYS.length)];
+    } while (assign && key == "length");
+    return key;
 }
 
-function createObjectLiteral(recurmax, noComma, stmtDepth, canThrow) {
+function createAccessor(recurmax, stmtDepth, canThrow) {
-    recurmax--;
+    var namesLenBefore = VAR_NAMES.length;
-    var obj = "({";
+    var s;
-    for (var i = rng(6); --i >= 0;) {
+    var prop1 = getDotKey();
-        var key = KEYS[rng(KEYS.length)];
+    if (rng(2) == 0) {
-        obj += key + ":(" + createExpression(recurmax, COMMA_OK, stmtDepth, canThrow) + "), ";
+        s = [
+            'get ' + prop1 + '(){',
+            strictMode(),
+            createStatements(2, recurmax, canThrow, CANNOT_BREAK, CANNOT_CONTINUE, CAN_RETURN, stmtDepth),
+            createStatement(recurmax, canThrow, CANNOT_BREAK, CANNOT_CONTINUE, CAN_RETURN, stmtDepth, STMT_RETURN_ETC),
+            '},'
+        ].join('\n');
+    } else {
+        var prop2;
+        do {
+            prop2 = getDotKey();
+        } while (prop1 == prop2);
+        s = [
+            'set ' + prop1 + '(' + createVarName(MANDATORY) + '){',
+            strictMode(),
+            createStatements(2, recurmax, canThrow, CANNOT_BREAK, CANNOT_CONTINUE, CAN_RETURN, stmtDepth),
+            'this.' + prop2 + createAssignment() + _createBinaryExpr(recurmax, COMMA_OK, stmtDepth, canThrow) + ';',
+            '},'
+        ].join('\n');
     }
-    return obj + "})";
+    VAR_NAMES.length = namesLenBefore;
+    return s;
+}
+
+function createObjectLiteral(recurmax, stmtDepth, canThrow) {
+    recurmax--;
+    var obj = ['({'];
+    for (var i = rng(6); --i >= 0;) {
+        if (rng(20) == 0) {
+            obj.push(createAccessor(recurmax, stmtDepth, canThrow));
+        } else {
+            var key = KEYS[rng(KEYS.length)];
+            obj.push(key + ':(' + createExpression(recurmax, COMMA_OK, stmtDepth, canThrow) + '),');
+        }
+    }
+    obj.push('})');
+    return obj.join('\n');
 }
 
 function createNestedBinaryExpr(recurmax, noComma, stmtDepth, canThrow) {
@@ -787,7 +842,7 @@ function _createSimpleBinaryExpr(recurmax, noComma, stmtDepth, canThrow) {
         return canThrow && rng(10) == 0 ? expr : '(' + assignee + ' && ' + expr + ')';
       case 4:
         assignee = getVarName();
-        expr = '(' + assignee + '.' + getDotKey() + createAssignment()
+        expr = '(' + assignee + '.' + getDotKey(true) + createAssignment()
             + _createBinaryExpr(recurmax, noComma, stmtDepth, canThrow) + ')';
         return canThrow && rng(10) == 0 ? expr : '(' + assignee + ' && ' + expr + ')';
       default:
@@ -992,7 +1047,7 @@ for (var round = 1; round <= num_iterations; round++) {
             }
         }
         if (verbose || (verbose_interval && !(round % INTERVAL_COUNT)) || !ok) log(options);
-        else if (verbose_error && typeof original_result != "string") {
+        else if (typeof original_result != "string") {
             console.log("//=============================================================");
             console.log("// original code");
             try_beautify(original_code, original_result);