Closure-compiler: Numbers 2^31 < x < 2^31-1 should not raise BITWISE_OPERAND_OUT_OF_RANGE

[haberman]

Take this input:

function toUint32(x) {
  if (x < 0) {
    x += 0x100000000;  // 2^32
  }
  return x;
}

console.log(toUint32(~~0xe0000000).toString(16));

Closure Compiler returns the error:

hello.js:12: WARNING - Operand out of range, bitwise operation will lose information: NUMBER 3.758096384E9 12 [source_file: hello.js]
console.log(toUint32(~~0xe0000000).toString(16));

And yet the output of this code is e0000000, which illustrates that no information is in fact lost.

I think this warning should only fire for numbers > 2^32 - 1, not > 2^31 - 1.

[haberman]

Ping? This seems like a clear bug that has a simple solution.

[MatrixFrog]

Essentially it's an off-by-one error? Might make a good first bug for someone.

[brad4d]

I tried to reproduce the error and couldn't.
Here's my attempt.

https://closure-compiler-debugger.appspot.com/#input0%3Dfunction%2520toUint32(x)%2520%257B%250A%2520%2520if%2520(x%2520%253C%25200)%2520%257B%250A%2520%2520%2520%2520x%2520%252B%253D%25200x100000000%253B%2520%2520%252F%252F%25202%255E32%250A%2520%2520%257D%250A%2520%2520return%2520x%253B%250A%257D%250A%250Aconsole.log(toUint32(~~0xe0000000).toString(16))%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_SYMBOLS%3D1%26CHECK_TYPES%3D1%26CLOSURE_PASS%3D1%26LANG_IN_IS_ES6%3D1%26MISSING_PROPERTIES%3D1%26PRESERVE_TYPE_ANNOTATIONS%3D1%26PRETTY_PRINT%3D1%26TRANSPILE%3D1

No warnings or errors generated.

[haberman]

Code that reports it is here: https://github.com/google/closure-compiler/blob/8759434583175dae0679013a6ea7f5f11a5d5b1d/src/com/google/javascript/jscomp/PeepholeFoldConstants.java#L426

Not sure how to trigger it in an open-source build, but I'm getting it every time with building with Blaze internally.

[brad4d]

OK, this does it

https://closure-compiler-debugger.appspot.com/#input0%3Dfunction%2520toUint32(x)%2520%257B%250A%2520%2520if%2520(x%2520%253C%25200)%2520%257B%250A%2520%2520%2520%2520x%2520%252B%253D%25200x100000000%253B%2520%2520%252F%252F%25202%255E32%250A%2520%2520%257D%250A%2520%2520return%2520x%253B%250A%257D%250A%250Aconsole.log(toUint32(~~0xe0000000).toString(16))%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_SYMBOLS%3D1%26CHECK_TYPES%3D1%26CLOSURE_PASS%3D1%26FOLD_CONSTANTS%3D1%26LANG_IN_IS_ES6%3D1%26MISSING_PROPERTIES%3D1%26PRESERVE_TYPE_ANNOTATIONS%3D1%26PRETTY_PRINT%3D1%26TRANSPILE%3D1

[dimvar]

@brad4d do you want to take this? Integer.MAX_VALUE is 2^32 - 1, so we'd need to multiply it by 2 before checking for warnings.
https://docs.oracle.com/javase/7/docs/api/java/lang/Integer.html#MAX_VALUE

[haberman]

Integer.MAX_VALUE is 2^32 - 1

I think you mean 2^31 - 1?

so we'd need to multiply it by 2 before checking for warnings.

(2^31 - 1) * 2 is 4294967294. The correct value to compare against is 2^32 - 1, which is 4294967295. So please don't use Integer.MAX_VALUE * 2. It is probably better to specify the constant's full value directly (or write (1 << 32) - 1).

[dimvar]

Oh right, I was a bit sloppy :) Thanks Joshua.

[blickly]

Although in Java, both Integer.MAX_VALUE * 2 and (1 << 32) - 1 are ints, and will wrap around. Probably need to force a long with something like (1L << 32) - 1

[brad4d]

The relevant part of the spec is:
https://tc39.github.io/ecma262/#sec-bitwise-not-operator-runtime-semantics-evaluation

I think we should define these constants and use them for checking bounds on arguments to bitwise operators.
static final double BITWISE_SAFE_MIN = - (1L << 32);
static final double BITWISE_SAFE_MAX = (1L << 33) - 1;

No, the "33" above is not a typo.
It's to cover the fact that hex constants are always positive, so
0xffffffff => 4294967295 (not -1)
Bit operations on this value will still behave as expected.

It doesn't seem worth attempting to guarantee the value was actually specified as a hex or octal constant, though.

[haberman]

I don't follow your reasoning. 2^33 - 1 is 0x1ffffffff. The following program illustrates that bitwise not will lose data with this constant:

function toUint32(x) {
  if (x < 0) {
    x += 0x100000000;  // 2^32
  }
  return x;
}

console.log(toUint32(~~0x1ffffffff).toString(16));

Prints: ffffffff (not the original value of 1ffffffff).

Likewise, -(2^32) is -4294967296. But this program illustrates that bitwise not on this constant will lose data:

console.log(~~(-4294967296))

This prints 0, not the original value of -4294967296.

I really think the correct bounds are:

• -2^31 (Integer.MIN_VALUE)
• 2^32 - 1 ((1L << 32) - 1)

[brad4d]

Sorry, off-by-one error. _sigh_
very well then

static final double BITWISE_SAFE_MIN = - Math.pow(2, 31);
static final double BITWISE_SAFE_MAX = Math.pow(2, 32) - 1;

[shicks]

I think it depends on how the number is used. If you're using a number as
signed then the safe max is 2^31-1, not 2^32-1, isn't it?

On Thu, Aug 4, 2016 at 2:16 PM, Bradford C. Smith [email protected]
wrote:

Sorry, off-by-one error. _sigh_
very well then

static final double BITWISE_SAFE_MIN = - Math.pow(2, 31);
static final double BITWISE_SAFE_MAX = Math.pow(2, 32) - 1;

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/google/closure-compiler/issues/1852#issuecomment-237686059,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIyG0O1nM4BsuhMVojLq872Fve3g63Sks5qcla2gaJpZM4I4tX_
.

[haberman]

If you're using a number as signed then the safe max is 2^31-1, not 2^32-1, isn't it?

That is the bound that the current code checks. It throws false alarms, because it is unaware that we will be treating the number as unsigned later.

If you can guarantee that it will never throw a false alarm, then great. :)

Otherwise I would suggest that 2^32 - 1 is a more reasonable bound to check.

[ChadKillingsworth]

A false positive, but suppressible warning is preferred to losing data in my book.

[dimvar]

I actually prefer less configuration if possible. So, in this case, I'd rather check for the higher bound and miss warning in some cases.

[concavelenz]

I agree with Chad. I believe the behavior is correct.

[haberman]

Consider that the surrounding context could be:

// The >>> operator will coerce its argument to uint32.
var x = ~0xe0000000 >>> 0;

It seems absurd to warn about this when some JavaScript operators (like >>>) implicitly convert their argument to Uint32.

If you apply this approach to other operators, you would also warn for -1 >>> 0, and certainly 0.1 + 0.2.

I think noisy warnings are harmful. If I lose faith that warnings are reasonable, I will just tune them out and suppress them on the slightest suspicion that they are noise.

[brad4d]

FYI, I have a change in review for this.
When it is submitted the acceptable boundaries will be as follows.

// Maximum value - treat as unsigned 32 bits
4294967295 // 2^32 - 1 == 0xffffffff unsigned

// Minimum value - treat as signed 32 bits
-2147483648 // - 2^31 == 0x80000000 signed

These are OK because in 2's complement all bits above the 32nd are either all 0's (non-negative/unsigned) or all 1's (signed negative). Thus no information is lost by the conversion to 32bits.
This is not true for values beyond this range.

Bear in mind that these limits are only enforced when literal values are used.
The compiler won't notice if you assign a value outside of this range to a variable and then use that in a bitwise operation.

[brad4d]

Just had a discussion with @concavelenz about this. I'll try to summarize our positions.

1. With the exception of >>>, JavaScript always treats the result of bitwise operations as signed numbers. This leads to surprising behavior for values >= 2^31. In particular apparent no-op bitwise operations actually re-interpret the values as signed 32-bit values, making them negative.

0x80000000 == 2147483648 // 2^31 0x80000000 | 0 == -2147483648 0x80000000 & 0xffffffff == -2147483648 ~~0x80000000 == -2147483648 0x80000000 << 0 == -2147483648 0x80000000 >> 0 == -2147483648 0x80000000 >>> 0 == 2147483648 // unsigned shift preserves positive value

1. This frequently surprises developers, because:
   
   
   
   • The common way to represent bit values with hex literals always gives non-negative values.
   
   
   • JavaScript numbers can be much larger than 2^32 - 1, so it's reasonable to expect a 32-bit
     
     value you've been operating on to be extended with 0's on the left, and thus be positive when
     
     you do arithmetic on it.
   
   

@concavelenz says:

1. If we're going to warn about literal values being out of range for bit operations, we should warn about any value that would end up changing unexpectedly to negative when an apparent no-op is applied to it. By this criteria, the current behavior is correct.
2. OTOH, the pass that is currently doing this checking is an optimization pass, and arguably shouldn't be throwing out warnings. By this criteria, it would be fine to just drop the checks here and possibly add them elsewhere.

@brad4d says:

1. We should warn about literal values used in bitwise operations that will actually lose information when converted to 32 bits. Re-interpretation as a signed value is not losing information.
   
   
   
   • It's just annoying a developer to complain about e.g. 0x80101111 because it happens to have the high bit set.
   
   
   • However, a developer should be warned if they actually specify a value that is too large, e.g. 0x100000000 (extra zero typo?)
   
   
2. Perhaps this check should move to an error checking phase, but it's worth having it here until that's done.
3. Perhaps we should consider warning if the result of a bitwise operation is negative and is later used in an arithmetic operation. But that's outside the scope of this bug.

[concavelenz]

Josh, why isn't an suppression appropriate for your use case? If you didn't have the call to toUint32, it would be incorrect.

[haberman]

The code I posted was just intended to be a repro. (btw in retrospect, just using >>> 0 would have better than writing ToUint32()).

The actual code is here: https://github.com/google/protobuf/blob/master/js/binary/utils_test.js#L186

I really don't like that the warning claims that information is lost, when the output has 32 full bits of precision. This code works fine (with no lost information), and yet this warning has been firing on the codebase for months. When I finally dug into it, I realized that the warning was just overzealous, which made me unhappy.

[brad4d]

@haberman, as a corollary to @concavelenz's question, I'd be interested to know whether the warning actually led you discover the need for the toUint32() method.

Side note: While working on this I've found that x >>> 0 is probably the simplest way to force x to be interpreted as a 32-bit unsigned integer.

[haberman]

@brad4d: The warning didn't help me in any way. I inherited maintenance of a codebase where this warning was firing on some pre-existing code. For a while, the warning made me feel vague anxiety that the code might be broken, but I didn't have time to look into it. One day I decided to dig into the problem, and I discovered that the code was fine and it was the warning that was broken. The code did not require any changes whatsoever -- the warning was just noise. I feel unhappy about the unsettling feeling the warning gave me for months.

[ChadKillingsworth]

I would say that the unsettled feeling was completely appropriate. And a comment like so would have made it go away:

/**
 * @suppress {whateverClassThisIs} the toUInt32 call here protects
 *    us from this unsafe manipulation
 */

[brad4d]

Next push to github should include a change removing the troublesome warnings.

[haberman]

Thank you!