S2197: modulo of unsigned integer can never be negative (FP)

Checking the following piece of code:

public override string ToString()
{
    var sb = new StringBuilder();

    for (byte row = 0; row < Row.Count; row++)
    {
        for (byte col = 0; col < Column.Count; col++)
        {
            if (col % 4 == 3 && col < 19) // FP, col is unsigned.
            {
                sb.Append('|');
            }
        }
        if (row % 4 == 3 && row < 15) // FP, col is unsigned.
        {
            sb.AppendLine("----+----+----+----+----");
        }
    }
    return sb.ToString();
}

You could argue that row and col also could not be negative for that is another debate. I did not check if this can trigger for ushort, uint, or `ulong, but that should obviously not be forgotten.

Version: SonarAnalyzer.CSharp v.9.32.0.97167
Language: C#

I confirm this as an FP. It works as expected for ushort, uint, and ulong, but byte was overlooked in the implementation. I created a ticket, and we will tackle it in a future hardening sprint.

1 Like

If an iteration variable is changed to byte to avoid S2197, I don’t like it. The idiomatic way to iterate is using int (if foreach cannot be used), and using smaller types may cause unexpected bugs (if the value does not fit) and may interfere with the JIT’s ability to optimize the code.

row in for (int row = 0; row < Row.Count; row++) (without writes to row in the loop) could be ascertained non-negative using an analysis similar to the JIT’s bounds checking elimination. Otherwise, I think marking false positive would be better than using byte.

@Jiles Tjoelker Although it is besides the topic, byte was not used to avoid S2197, but because of bit mask operations on other unsigned numbers (I just stripped a lot of code to minimize the reproduction steps). As mentioned in my own report I agree with you that this should also not report on int’s.