Uglifyjs: [ES6][regression] Destructuring failing

[joaovieira]

Bug report or feature request?
Bug report

ES5 or ES6+ input?
ES6

uglify-js version (uglifyjs -V)
v3.0.0

JavaScript input - ideally as small as possible.
The uglifyjs CLI command executed or minify() options used.

var result = UglifyJS.minify("let [a] = [1];");

JavaScript output produced and/or the error or warning.

TypeError: expr.name.definition is not a function
    at get_lhs (eval at <anonymous> (.../node_modules/uglify-js/tools/node.js:21:1), <anonymous>:7899:41)
    at collapse (eval at <anonymous> (.../node_modules/uglify-js/tools/node.js:21:1), <anonymous>:7793:31)
    ...

Using latest harmony branch. I believe it was working on v2.x and flagged as [regresssion]. Feel free to update it otherwise.

[joaovieira]

If it helps, it started in https://github.com/mishoo/UglifyJS2/commit/b4c18f6b83501e56f8518db2e454be95e941b803, which is the massive merge 😁 both parents are good.

[kzc]

The problem is that expr.name is not guaranteed to be an AST_Symbol on the harmony branch:

$ echo 'let [a] = [1];' | bin/uglifyjs -o ast
{
  "_class": "AST_Toplevel",
  "body": [
    {
      "_class": "AST_Let",
      "definitions": [
        {
          "_class": "AST_VarDef",
          "name": {
            "_class": "AST_Destructuring",
            "names": [
              {
                "_class": "AST_SymbolLet",
                "name": "a"
              }
            ],
            "is_array": true
          },
          "value": {
            "_class": "AST_Array",
            "elements": [
              {
                "_class": "AST_Number",
                "value": 1
              }
            ]
          }
        }
      ]
    }
  ]
}

This patch seems to work:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -799,7 +799,7 @@ merge(Compressor.prototype, {
             }

             function get_lhs(expr) {
-                if (expr instanceof AST_VarDef) {
+                if (expr instanceof AST_VarDef && expr.name instanceof AST_Symbol) {
                     var def = expr.name.definition();
                     if (def.orig.length > 1
                         || def.references.length == 1 && (!def.global || compressor.toplevel(def))) {

[alexlamsl]

@kzc I see you are enjoying --output ast a bit there :wink:

Thanks for the patch - I'll spin up a PR for this in a minute. :+1:

[kzc]

I see you are enjoying --output ast a bit there

No doubt. Sure beats maintaining and applying a separate AST dump patch.

I noticed that since errors are now captured by minify() we no longer see stack dumps in the CLI for bugs like this issue.

[alexlamsl]

I noticed that since errors are now captured by minify() we no longer see stack dumps in the CLI for bugs like this issue.

Indeed - they are now captured and only get its .message printed. Before it is actually an uncaught exception which terminates the process.

[kzc]

The stack dump was useful in diagnosing errors. I had to hack minify.js to throw the error to see what's going on in the CLI. Maybe --verbose should print the stack trace - or just emit the trace by default?

[alexlamsl]

just emit the trace by default?

I'll do that if you find it helpful - I was under the false assumption that nobody reads it :sweat_smile:

[kzc]

I was under the false assumption that nobody reads it

I've seen stack traces in CLI bug reports in the past. It's particularly useful when they don't provide a test case.

[joaovieira]

I also found it easier to debug when throwing the error automatically, rather than "evaluating" the result. It's more natural. The error is also a stack trace, which is most commonly found in catch blocks (as it was).

Anyway, thanks for the patch and the PR 👍