Optimizing away unused function parameter produces redundant comparison !(void 0 >= b)

[juj]

Consider the following function:

/**
 * Scans array of non-zero elements in range [start, end[
 * and returns the index where value x is found,
 * or -1 otherwise.
 * You can omit 'end' parameter to scan the range [start, inf[
 * Or you can also omit 'start' parameter to scan whole array
 * @param {number} x
 * @param {number=} start
 * @param {number=} end
 * @return {number}
 */
function find(arr, x, start, end) {
  for(var i = start|0; !(i >= end); ++i) {
    if (arr[i] == x) {
      return i;
    }
    if (!arr[i]) {
      break;
    }
  }
   return -1;
}

here the intent is that the function can be called with optional end parameter, or optional both start+end parameters.

This function find() is provided as sort of a generic library function, and its implementation does not know which of the three forms (start + end, only start, no start or end) of the function the user of the library will be using. But Closure does! (or at least it tries)

So if application developer never called this function by providing an end parameter, it would be great to have Closure optimize out the check !(i >= end) completely, because end is always undefined, so when i is known to be a number, !(i >= end) is always true.

If application developers calls this with

var a = new Uint8Array(4);
a[0] = 1;
a[1] = 1;
a[2] = 2;
a[3] = 1;

console.log(find(a, 2, 0));
console.log(find(a, 1, 1));

then Closure does recognize that end is always undefined, and it is very much on board with the {number=} annotation that suppresses it from issuing any warnings, but instead of resolving the result, it generates something unexpected:

function a(e, f) {
  for (var d = c, b = f | 0; !(void 0 <= b); ++b) {
    if (d[b] == e) {
      return b;
    }
    if (!d[b]) {
      break;
    }
  }
  return -1;
}

void 0 is an abbreviation for undefined, void 0 <= b returns always false, so !(false) is always true, so !(void 0 <= b) would be redundant here.

Any chance it would be possible for Closure to do this kind of optimization? This is something that occurs in practice in Emscripten C/C++ <-> JS UTF-8 string marshalling functions, which get generated to practically every program compiled with Emscripten, and I am trying to make sure that these primitive exposed functions carry absolutely minimum weight to generated code.

[blickly]

This sounds like a reasonable peephole optimization.

Created internal issue b/123711094 to track.

[juj]

Here is the actual form extracted from Emscripten, if that helps as a test case:

// ==ClosureCompiler==
// @output_file_name default.js
// @compilation_level ADVANCED_OPTIMIZATIONS
// @formatting pretty_print
// ==/ClosureCompiler==

var UTF8Decoder = new TextDecoder("utf8");

/**
 * @param {number} idx
 * @param {number=} maxBytesToRead
 * @return {string}
 */
function UTF8ArrayToString(u8Array, idx, maxBytesToRead) {
 var endIdx = idx + maxBytesToRead;
 var endPtr = idx;
 while (u8Array[endPtr] && !(endPtr >= endIdx)) ++endPtr;
 return UTF8Decoder.decode(u8Array.subarray ? u8Array.subarray(idx, endPtr) : new Uint8Array(u8Array.slice(idx, endPtr)));
}

/**
 * @param {number} ptr
 * @param {number=} maxBytesToRead
 * @return {string}
 */
function UTF8ToString(ptr, maxBytesToRead) {
 return ptr ? UTF8ArrayToString(HEAPU8, ptr, maxBytesToRead) : "";
}

// test:
var HEAPU8 = new Uint8Array(10);
HEAPU8[1] = 44;
HEAPU8[2] = 45;
console.log(UTF8ToString(1+Math.random()|0));

Oddly when I run in Closure compiler service, this generates

var a = new TextDecoder("utf8"), e = new Uint8Array(10);
e[1] = 44;
e[2] = 45;
console.log(function(b, f) {
  if (b) {
    var d = b + f;
    for (var c = b; e[c] && !(c >= d);) {
      ++c;
    }
    d = a.decode(e.subarray ? e.subarray(b, c) : new Uint8Array(e.slice(b, c)));
  } else {
    d = "";
  }
  return d;
}(1 + Math.random() | 0));

i.e. it does not optimize away function parameter f, although when I run locally with Emscripten, it does (although I don't call the function with Math.random() in Emscripten, the scenario is a bit different)

In Emscripten I get the following:

function f(a) {
    if (a) {
        for (var e = g, l = a + NaN, d = a; e[d] && !(d >= l);) ++d;
        a = c.decode(e.subarray ? e.subarray(a, d) : new Uint8Array(e.slice(a, d)))
    } else a = "";
    return a
}

so the omitted function parameter got correctly turned to undefined, and then var endIdx = idx + maxBytesToRead; was computed to get idx + undefined == NaN, and the NaN got substituted in place of endIdx, but expansion did not continue further. I.e. l = a+NaN -> l == NaN -> && !(d >= l) could go away as redundant since it's always true. So final form could be

function f(a) {
    if (a) {
        for (var e = g, d = a; e[d];) ++d;
        a = c.decode(e.subarray ? e.subarray(a, d) : new Uint8Array(e.slice(a, d)))
    } else a = "";
    return a
}

(the effect of #3201 shows up here, after that this could further be:)

function f(a) {
    if (a) {
        for (var d = a; g[d];) ++d;
        a = c.decode(g.subarray ? g.subarray(a, d) : new Uint8Array(g.slice(a, d)))
    } else a = "";
    return a
}

[juj]

Seeing this same issue when looking at Emscripten's closure optimization today.

Input code

var UTF8Decoder = new TextDecoder('utf8');

/**
 * @param {number} ptr
 * @param {number=} maxBytesToRead
 * @return {string}
 */
function UTF8ToString(ptr, maxBytesToRead) {
  if (!ptr) return '';
  var maxPtr = ptr + maxBytesToRead;
  for(var end = ptr; !(end >= maxPtr) && HEAPU8[end];) ++end;
  return UTF8Decoder.decode(HEAPU8.subarray(ptr, end));
}

that is only ever getting called with one argument, i.e. UTF8ToString(ptr);, and maxBytesToRead is never defined.

That results in generated code

function k(b) {
    if (!b) return "";
    for (var c = b + NaN, a = b; !(a >= c) && l[a];) ++a;
    return h.decode(l.subarray(b, a))
}

whereas it would further optimize by hand to

function k(b) {
    if (!b) return "";
    for (var a = b; l[a];) ++a;
    return h.decode(l.subarray(b, a))
}

Oddly, if I try to generate a small test case in online closure compiler web page, closure wants to optimize the test code even less:

https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250Avar%2520UTF8Decoder%2520%253D%2520new%2520TextDecoder('utf8')%253B%250A%250A%252F**%250A%2520*%2520%2540param%2520%257Bnumber%257D%2520ptr%250A%2520*%2520%2540param%2520%257Bnumber%253D%257D%2520maxBytesToRead%250A%2520*%2520%2540return%2520%257Bstring%257D%250A%2520*%252F%250Afunction%2520UTF8ToString(ptr%252C%2520maxBytesToRead)%2520%257B%250A%2520%2520if%2520(!ptr)%2520return%2520''%253B%250A%2520%2520var%2520maxPtr%2520%253D%2520ptr%2520%252B%2520maxBytesToRead%253B%250A%2520%2520for(var%2520end%2520%253D%2520ptr%253B%2520!(end%2520%253E%253D%2520maxPtr)%2520%2526%2526%2520HEAPU8%255Bend%255D%253B)%2520%252B%252Bend%253B%250A%2520%2520return%2520UTF8Decoder.decode(HEAPU8.subarray(ptr%252C%2520end))%253B%250A%257D%250A%250Avar%2520HEAPU8%2520%253D%2520new%2520Uint8Array(%255B0%252C65%252C66%252C67%252C0%255D)%253B%250Aconsole.log(UTF8ToString(Date.now()%253F1%253A2))%253B%250A

and produces

function(a,e){if(!a)return"";for(var f=a+e,b=a;!(b>=f)&&d[b];)++b;return c.decode(d.subarray(a,b))}

(i.e. it did not even attempt to take away maxBytesToRead)