csharpsquid:S2183 - Bit shifting - False positive and wrong explanation

SonarQube
,
1

The following (simplified code) code gives a (false positive) SQ bug:
C#
byte result = LargeNumber >> 8;
// In case the example makes no sense to you: In the real code, masks are used to select the right bits.

The explanation given bij SQ is wrong:

Since an int is a 32-bit variable, shifting by more than +/-31 is confusing at best and an error at worst. Shifting an int by 32 is the same as shifting it by 0, and shifting it by 33 is the same as shifting it by 1.

If you shift an int 32 times, the result is zero. It is NOT the same as shifting it 0 times (which will leave the value intact). Furthermore, shifting 33 is not the same as shifting it one time. Shifting it by 33 will give the same result as shifting it 32 times; the variable will be 0.

Summary

  • Right shifting an int32 8 times and storing it in a byte should not yield a SQ S2183 bug.
  • The explanation for the SQ S2183 bug is not correct.

EDIT
Mhhh… If found something unexpected in C#:
UInt16 test = 0xFFFF;
UInt16 test1 = (UInt16)(test >> 1);
UInt16 test2 = (UInt16)(test >> 16);
UInt16 test3 = (UInt16)(test >> 17);
Console.WriteLine($“Test: {test}”);
Console.WriteLine($“Test1: {test1}”);
Console.WriteLine($“Test2: {test2}”);
Console.WriteLine($“Test3: {test3}”);

UInt32 foe = 0xFFFFFFFF;
UInt32 foe1 = foe >> 1;
UInt32 foe2 = foe >> 32;
UInt32 foe3 = foe >> 33;
Console.WriteLine($"Foe: {foe}");
Console.WriteLine($"Foe1: {foe1}");
Console.WriteLine($"Foe2: {foe2}");
Console.WriteLine($"Foe3: {foe3}");

Console output:
Test: 65535
Test1: 32767
Test2: 0
Test3: 0
Foe: 4294967295
Foe1: 2147483647
Foe2: 4294967295
Foe3: 2147483647

So for UInt32, the SQ explanation is correct. For UInt16, it is not… :confused:

2

Hi @Yellow,

I apologize for the terribly delayed answer. You are right, the 16bit integers seem to behave differently when shifted more than length-1 times than 32bit integers. I could not find the exact reason but I made a few tests and I think my explanation of this behavior is good enough.

The documentation of the >> operator talks only about 32bit and 64bit numbers and it says that only the lowest 5 or 6 bits of the count operand (for 32bit and 64bit numbers correspondingly) are used when shifting.

This means (and your tests are confirming it) that the runtime clears the other bits before doing the operation:

// Int32
x >> 1 // x >> 0b0000_0000_0000_0001
x >> 33 // x >> (0b0000_0000_0010_0001 & 0b0000_0000_0001_1111) --> x >> 0b0000_0000_0000_0001
//                                     ^^^^^^^^^^^^^^^^^^^^^^^ mask the operand first

It is similar for 64bit integers…

As for the 8bit and 16bit integers, it seems (I could not find documentation on this) that they are first cast to 32bit and then a 32bit operation is performed (e.g. shifted by max 31). This assumption seems to be supported by the fact that the result of the shifting of a 16bit number is a 32bit number (and you need to cast it to store it in a 16bit variable), and also if you execute the following code it passes (8bit integers work the same way too):

UInt16 test = 0xFFFF;
UInt16 test1 = (UInt16)(test >> 1);
UInt16 test3 = (UInt16)(test >> 33);
Assert.AreEqual(test1, test3); // pass

I created a ticket to fix the rule and we will implement it in the near future.