bug with nested cross-scope function substitutions #2449

natew · 2017-11-07T01:05:27Z

Bug report or feature request?

Bug

ES5 or ES6+ input?

ES5

Uglify version (uglifyjs -V)

uglify-webpack-plugin 1.0.1

JavaScript input

https://github.com/Qix-/color-convert/blob/43835c2bba69ae52d2e2ca84a182a86dd86f34ac/index.js

The uglifyjs CLI command executed or minify() options used.

No customization

JavaScript output or error produced.

Runtime error:

The text was updated successfully, but these errors were encountered:

natew · 2017-11-07T01:09:01Z

Seems the fix was to do this:

https://github.com/Qix-/color-convert/pull/49

kzc · 2017-11-07T01:57:41Z

What's the issue with this uglify-es output exactly?

$ bin/uglifyjs -V
uglify-es 3.1.8

-m -c -b

$ curl -s https://raw.githubusercontent.com/Qix-/color-convert/43835c2bba69ae52d2e2ca84a182a86dd86f34ac/index.js | bin/uglifyjs -m -c -b
function wrapRaw(e) {
    var n = function(n) {
        return void 0 === n || null === n ? n : (arguments.length > 1 && (n = Array.prototype.slice.call(arguments)), 
        e(n));
    };
    return "conversion" in e && (n.conversion = e.conversion), n;
}

function wrapRounded(e) {
    var n = function(n) {
        if (void 0 === n || null === n) return n;
        arguments.length > 1 && (n = Array.prototype.slice.call(arguments));
        var r = e(n);
        if ("object" == typeof r) for (var o = r.length, t = 0; t < o; t++) r[t] = Math.round(r[t]);
        return r;
    };
    return "conversion" in e && (n.conversion = e.conversion), n;
}

var conversions = require("./conversions"), route = require("./route"), convert = {}, models = Object.keys(conversions);

models.forEach(function(e) {
    convert[e] = {}, Object.defineProperty(convert[e], "channels", {
        value: conversions[e].channels
    }), Object.defineProperty(convert[e], "labels", {
        value: conversions[e].labels
    });
    var n = route(e);
    Object.keys(n).forEach(function(r) {
        var o = n[r];
        convert[e][r] = wrapRounded(o), convert[e][r].raw = wrapRaw(o);
    });
}), module.exports = convert;

--toplevel -m -c -b

$ curl -s https://raw.githubusercontent.com/Qix-/color-convert/43835c2bba69ae52d2e2ca84a182a86dd86f34ac/index.js | bin/uglifyjs --toplevel -m -c -b
var e = require("./conversions"), n = require("./route"), r = {};

Object.keys(e).forEach(function(o) {
    r[o] = {}, Object.defineProperty(r[o], "channels", {
        value: e[o].channels
    }), Object.defineProperty(r[o], "labels", {
        value: e[o].labels
    });
    var t = n(o);
    Object.keys(t).forEach(function(e) {
        var n = t[e];
        r[o][e] = function(e) {
            var n = function(n) {
                if (void 0 === n || null === n) return n;
                arguments.length > 1 && (n = Array.prototype.slice.call(arguments));
                var r = e(n);
                if ("object" == typeof r) for (var o = r.length, t = 0; t < o; t++) r[t] = Math.round(r[t]);
                return r;
            };
            return "conversion" in e && (n.conversion = e.conversion), n;
        }(n), r[o][e].raw = function(e) {
            var n = function(n) {
                return void 0 === n || null === n ? n : (arguments.length > 1 && (n = Array.prototype.slice.call(arguments)), 
                e(n));
            };
            return "conversion" in e && (n.conversion = e.conversion), n;
        }(n);
    });
}), module.exports = r;

alexlamsl · 2017-11-07T06:05:26Z

I've tried both index.js and route.js from that project and they seem to produce the correct results.

Looking at the screenshot above, var t=function(){for(var e={},t=r.length,o=0;... does not even resemble the original input of:

var wrappedFn = function (args) {
    if (args === undefined || args === null) {
        return args;
    }

    if (arguments.length > 1) {
        args = Array.prototype.slice.call(arguments);
    }

    var result = fn(args);

    // we're assuming the result is an array here.
    // see notice in conversions.js; don't use box types
    // in conversion functions.
    if (typeof result === 'object') {
        for (var len = result.length, i = 0; i < len; i++) {
            result[i] = Math.round(result[i]);
        }
    }

    return result;
};

So I'll check with the tool which pre-processed those files before feeding into uglify-es.

natew · 2017-11-07T16:17:33Z

I’m just using the published version and it’s a runtime error. Did you see the ticket i linked to? Seems to be the duplicate line that causes it.

…

On Mon, Nov 6, 2017 at 10:05 PM Alex Lam S.L. ***@***.***> wrote: I've tried both index.js and route.js from that project and they seem to produce the correct results. Looking at the screenshot above, var t=function(){for(var e={},t=r.length,o=0;... does not even resemble the original input of: var wrappedFn = function (args) { if (args === undefined || args === null) { return args; } if (arguments.length > 1) { args = Array.prototype.slice.call(arguments); } var result = fn(args); // we're assuming the result is an array here. // see notice in conversions.js; don't use box types // in conversion functions. if (typeof result === 'object') { for (var len = result.length, i = 0; i < len; i++) { result[i] = Math.round(result[i]); } } return result; }; So I'll check with the tool which pre-processed those files before feeding into uglify-es. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2449 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAvRAgv4fa9G6MXtVmog4cNRvHvgge-ks5sz_M2gaJpZM4QUHQ6> .

alexlamsl · 2017-11-07T16:38:04Z

Please provide an actual reproducible test case, using only uglify-js.

kzc · 2017-11-07T16:39:45Z

Remove uglify from your webpack config and post a link to the unminified bundle produced by webpack in a gist.

natew · 2017-11-07T19:24:57Z

Ok, so looks like the route.js file is the culprit from the ticket linked, so checking that.

Unminified:

/***/ "../../packages/ui/node_modules/color/node_modules/color-convert/route.js":
/*!**********************************************************************************************************!*\
  !*** /Users/nw/projects/motion/orbit/packages/ui/node_modules/color/node_modules/color-convert/route.js ***!
  \**********************************************************************************************************/
/*! dynamic exports provided */
/*! all exports used */
/***/ (function(module, exports, __webpack_require__) {

var conversions = __webpack_require__(/*! ./conversions */ "../../packages/ui/node_modules/color/node_modules/color-convert/conversions.js");

/*
	this function routes a model to all other models.

	all functions that are routed have a property `.conversion` attached
	to the returned synthetic function. This property is an array
	of strings, each with the steps in between the 'from' and 'to'
	color models (inclusive).

	conversions that are not possible simply are not included.
*/

// https://jsperf.com/object-keys-vs-for-in-with-closure/3
var models = Object.keys(conversions);

function buildGraph() {
	var graph = {};

	for (var len = models.length, i = 0; i < len; i++) {
		graph[models[i]] = {
			// http://jsperf.com/1-vs-infinity
			// micro-opt, but this is simple.
			distance: -1,
			parent: null
		};
	}

	return graph;
}

// https://en.wikipedia.org/wiki/Breadth-first_search
function deriveBFS(fromModel) {
	var graph = buildGraph();
	var queue = [fromModel]; // unshift -> queue -> pop

	graph[fromModel].distance = 0;

	while (queue.length) {
		var current = queue.pop();
		var adjacents = Object.keys(conversions[current]);

		for (var len = adjacents.length, i = 0; i < len; i++) {
			var adjacent = adjacents[i];
			var node = graph[adjacent];

			if (node.distance === -1) {
				node.distance = graph[current].distance + 1;
				node.parent = current;
				queue.unshift(adjacent);
			}
		}
	}

	return graph;
}

function link(from, to) {
	return function (args) {
		return to(from(args));
	};
}

function wrapConversion(toModel, graph) {
	var path = [graph[toModel].parent, toModel];
	var fn = conversions[graph[toModel].parent][toModel];

	var cur = graph[toModel].parent;
	while (graph[cur].parent) {
		path.unshift(graph[cur].parent);
		fn = link(conversions[graph[cur].parent][cur], fn);
		cur = graph[cur].parent;
	}

	fn.conversion = path;
	return fn;
}

module.exports = function (fromModel) {
	var graph = deriveBFS(fromModel);
	var conversion = {};

	var models = Object.keys(graph);
	for (var len = models.length, i = 0; i < len; i++) {
		var toModel = models[i];
		var node = graph[toModel];

		if (node.parent === null) {
			// no possible conversion, or this node is the source model.
			continue;
		}

		conversion[toModel] = wrapConversion(toModel, graph);
	}

	return conversion;
};



/***/ }),

And with uglify:

/*!**********************************************************************************************************!*\
  !*** /Users/nw/projects/motion/orbit/packages/ui/node_modules/color/node_modules/color-convert/route.js ***!
  \**********************************************************************************************************/
/*! dynamic exports provided */
/*! all exports used */
function(e,t,o){var n=o(/*! ./conversions */"../../packages/ui/node_modules/color/node_modules/color-convert/conversions.js"),r=Object.keys(n);e.exports=function(e){for(var t=function(e){var t=function(){for(var e={},t=r.length,o=0;o<t;o++)e[r[o]]={distance:-1,parent:null};return e}(),o=[e];for(t[e].distance=0;o.length;)for(var s=o.pop(),i=Object.keys(n[s]),l=i.length,u=0;u<l;u++){var a=i[u],d=t[a];-1===d.distance&&(d.distance=t[s].distance+1,d.parent=s,o.unshift(a))}return t}(e),o={},r=Object.keys(t),s=r.length,i=0;i<s;i++){var l=r[i];null!==t[l].parent&&(o[l]=function(e,t){for(var o=[t[e].parent,e],r=n[t[e].parent][e],s=t[e].parent;t[s].parent;)o.unshift(t[s].parent),r=function(e,t){return function(o){return t(e(o))}}(n[t[s].parent][s],r),s=t[s].parent;return r.conversion=o,r}(l,t))}return o}}

Error is Uncaught TypeError: Cannot read property 'length' of undefined at the r.length line.

Edit: Also did verify that it runs fine unminified, but errors minified.

alexlamsl · 2017-11-07T19:33:25Z

Not valid JavaScript:

Parse error at 2449.js:1,80
../packages/ui/node_modules/color/node_modules/color-convert/route.js":
                                                                      ^
ERROR: Unexpected token: punc (:)
    at JS_Parse_Error.get

Please provide an actual reproducible test case.

natew · 2017-11-07T19:40:21Z

Apologies for pasting it in sloppily, I realize OSS work is unforgiving and I'm not making it easier.

What's the best way to provide this as reproducible? I can get the output like this:

mkdir /tmp/test && cd /tmp/test
npm i color-convert uglify-es
npx uglify-es ./node_modules/color-convert/route.js

Is there a way to get uglify to bundle all the files so you can run it somewhere?

alexlamsl · 2017-11-07T19:44:47Z

AFAICT you are running uglify-es ./node_modules/color-convert/route.js, so if you can put that ./node_modules/color-convert/route.js file somewhere, e.g. here or Gist, then I can reproduce that command and inspect the result accordingly.

Alternatively and preferably, file a Pull Request that would fix the issue you have observed.

natew · 2017-11-07T19:51:52Z

Yea but given you said reproducible I wasn't sure if you wanted it live environment, or something.

The output of route.js is this, but it looks different.

var conversions=require("./conversions");var models=Object.keys(conversions);function buildGraph(){var graph={};for(var len=models.length,i=0;i<len;i++){graph[models[i]]={distance:-1,parent:null}}return graph}function deriveBFS(fromModel){var graph=buildGraph();var queue=[fromModel];graph[fromModel].distance=0;while(queue.length){var current=queue.pop();var adjacents=Object.keys(conversions[current]);for(var len=adjacents.length,i=0;i<len;i++){var adjacent=adjacents[i];var node=graph[adjacent];if(node.distance===-1){node.distance=graph[current].distance+1;node.parent=current;queue.unshift(adjacent)}}}return graph}function link(from,to){return function(args){return to(from(args))}}function wrapConversion(toModel,graph){var path=[graph[toModel].parent,toModel];var fn=conversions[graph[toModel].parent][toModel];var cur=graph[toModel].parent;while(graph[cur].parent){path.unshift(graph[cur].parent);fn=link(conversions[graph[cur].parent][cur],fn);cur=graph[cur].parent}fn.conversion=path;return fn}module.exports=function(fromModel){var graph=deriveBFS(fromModel);var conversion={};var models=Object.keys(graph);for(var len=models.length,i=0;i<len;i++){var toModel=models[i];var node=graph[toModel];if(node.parent===null){continue}conversion[toModel]=wrapConversion(toModel,graph)}return conversion};

I appreciate uglify and am happy to report the bug in as much detail as is necessary, but don't have the time to invest into learning it in order to fix this bug. I think I've verified the issue and here's the best psuedo-code that explains it that I can do:

var conversions = require('./conversions'); // not sure if necessary to require, could just be an object
var models = Object.keys(conversions);
var graph = {}

function doStuff() {
  for (var len = models.length, i = 0; i < len; i++) { // runtime error happens here in checking length
     console.log(models[i])
  }
}

module.exports = function () {
        var other = doStuff()
	var models = Object.keys(graph);  // bug triggers here in redefining models
	return models;
};

Seems valuable enough to have it documented here at the least. There's a PR for this library already that will avoid it.

shanshanzhu · 2017-11-08T22:18:13Z

I got this exactly same error.

line 41489, console.log(i) shows up correctly, but at line 41493, i is undefined
It shouldn't be closed

kzc · 2017-11-09T08:42:17Z

@alexlamsl Here's how to repro this bug. It's not a harmony-specific issue.

$ npm install [email protected]

# any browserify version would probably suffice
$ browserify --version
12.0.1

$ uglifyjs -V
uglify-js 3.1.8

$ cat col.js
console.log(require("color-convert").keyword.rgb("blue"));

$ browserify col.js | node
[ 0, 0, 255 ]

$ browserify col.js | uglifyjs -c -b | node
[stdin]:404
                    for (var graph = {}, len = models.length, i = 0; i < len; i++) graph[models[i]] = {
                                                     ^
TypeError: Cannot read property 'length' of undefined

It's a function inlining issue. A workaround is to disable the compress option reduce_vars:

$ browserify col.js | uglifyjs -c reduce_vars=0 -b | node
[ 0, 0, 255 ]

alexlamsl · 2017-11-09T10:02:03Z

@kzc thanks for the useful information – will have a look when I get home.

alexlamsl · 2017-11-09T10:51:30Z

Reduced test case:

var a = "PASS";
function f() {
    return a;
}
function g() {
    return f();
}
!function() {
    var a = "FAIL";
    if (a == a) console.log(g());
}();

$ cat test.js | node
PASS
$ uglify-js test.js --toplevel -c evaluate=0 | node
FAIL

$ uglify-js test.js --toplevel -c evaluate=0 -b bracketize
var a = "PASS";

!function() {
    var a = "FAIL";
    a == a && console.log(a);
}();

fixes mishoo#2449

fixes #2449

kzc · 2017-11-09T15:58:09Z

@alexlamsl Given your test case above, why did it apparently succeed in v3.1.8 with evaluate=true and reduce_vars=true (default compress options)?

$ bin/uglifyjs test.js --toplevel -c
var a="PASS";console.log(a);

alexlamsl · 2017-11-09T16:37:52Z

@kzc evaluate causes:

var a = "FAIL"; a == a && ...

to become:

var a = "FAIL"; true && ...

and then unused comes in:

true && ...

so there will no longer be double definitions of a to create conflicts.

kzc · 2017-11-09T16:53:58Z

I see. Thanks.

natew mentioned this issue Nov 7, 2017

Breaking Uglify Qix-/color-convert#50

Closed

alexlamsl added the harmony label Nov 7, 2017

alexlamsl closed this as completed Nov 7, 2017

alexlamsl reopened this Nov 7, 2017

alexlamsl closed this as completed Nov 7, 2017

alexlamsl added bug and removed harmony labels Nov 9, 2017

alexlamsl reopened this Nov 9, 2017

alexlamsl changed the title ~~[bug] uglify-es breaks on npm package 'color-convert'~~ bug with nested cross-scope function substitutions Nov 9, 2017

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Nov 9, 2017

fix multiple nested function substitutions

a16145c

fixes mishoo#2449

alexlamsl mentioned this issue Nov 9, 2017

fix multiple nested function substitutions #2458

Merged

askielboe mentioned this issue Nov 9, 2017

App force close on release mode: undefined is not an object (evaluating 'e.length') facebook/react-native#16745

Closed

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Nov 9, 2017

fix multiple nested function substitutions

07cb641

fixes mishoo#2449

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Nov 9, 2017

fix multiple nested function substitutions

634d1f6

fixes mishoo#2449

alexlamsl closed this as completed in #2458 Nov 9, 2017

alexlamsl added a commit that referenced this issue Nov 9, 2017

fix multiple nested function substitutions (#2458)

1127a2c

fixes #2449

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug with nested cross-scope function substitutions #2449

bug with nested cross-scope function substitutions #2449

natew commented Nov 7, 2017 •

edited

Loading

natew commented Nov 7, 2017

kzc commented Nov 7, 2017 •

edited

Loading

alexlamsl commented Nov 7, 2017

natew commented Nov 7, 2017 via email

alexlamsl commented Nov 7, 2017

kzc commented Nov 7, 2017

natew commented Nov 7, 2017 •

edited

Loading

alexlamsl commented Nov 7, 2017 •

edited

Loading

natew commented Nov 7, 2017

alexlamsl commented Nov 7, 2017

natew commented Nov 7, 2017 •

edited

Loading

shanshanzhu commented Nov 8, 2017 •

edited

Loading

kzc commented Nov 9, 2017

alexlamsl commented Nov 9, 2017

alexlamsl commented Nov 9, 2017

kzc commented Nov 9, 2017

alexlamsl commented Nov 9, 2017

kzc commented Nov 9, 2017

bug with nested cross-scope function substitutions #2449

bug with nested cross-scope function substitutions #2449

Comments

natew commented Nov 7, 2017 • edited Loading

natew commented Nov 7, 2017

kzc commented Nov 7, 2017 • edited Loading

alexlamsl commented Nov 7, 2017

natew commented Nov 7, 2017 via email

alexlamsl commented Nov 7, 2017

kzc commented Nov 7, 2017

natew commented Nov 7, 2017 • edited Loading

alexlamsl commented Nov 7, 2017 • edited Loading

natew commented Nov 7, 2017

alexlamsl commented Nov 7, 2017

natew commented Nov 7, 2017 • edited Loading

shanshanzhu commented Nov 8, 2017 • edited Loading

kzc commented Nov 9, 2017

alexlamsl commented Nov 9, 2017

alexlamsl commented Nov 9, 2017

kzc commented Nov 9, 2017

alexlamsl commented Nov 9, 2017

kzc commented Nov 9, 2017

natew commented Nov 7, 2017 •

edited

Loading

kzc commented Nov 7, 2017 •

edited

Loading

natew commented Nov 7, 2017 •

edited

Loading

alexlamsl commented Nov 7, 2017 •

edited

Loading

natew commented Nov 7, 2017 •

edited

Loading

shanshanzhu commented Nov 8, 2017 •

edited

Loading