False Positive for rule S4070, unsigned zero complements not recognized as all option

Rules and Languages Report False-positive / False-negative...
1

Rather small issue, but also shouldn’t be too hard to fix by checking in the analyzer if the constant results in all bits being set

  • What language is this for?
    C#
  • Which rule?
    S4070
  • Why do you believe it’s a false-positive/false-negative?
    ~0U or 0xffffffff or equivalent constant value that results in all bits being set should be recognized as an All flags option
  • IDE version
    Microsoft Visual Studio Professional 2022 (64-bit) - Current
    Version 17.8.5
  • SonarLint version
    7.6.00.83110
  • How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots)
namespace Power.Interfaces.Enums
{
    using System;

    [Flags]
    public enum VariantProcessingFeatures : ulong
    {
        None = 0,
        ColorCompression = 1,
        All = 0xffffffffffffffff,
    }
}
4

Hi @JJoensuu
Thanks for reaching out.
The rule S4070 doesn’t consider 0xffffffffffffffff (a.k.a. ulong.MaxValue) as a combination of all values in the Flags enum, mainly for two reasons:

  • it breaks the property of Flags enums that the bitwise OR of all individual values (defined as powers of 2, i.e. values v for which a non-negative integer e exists such that v == 1 << e) should give the All value
    • that is, there is no undeclared individual value in the enum
  • it breaks the F and G modifiers of the ToString method when the bitwise OR of all individual values is passed

As an example, consider the following two enums, declared as [Flags]:

[Flags]
public enum X1 : ulong
{
    None = 0,
    A = 1 << 0,
    B = 1 << 1,
    All = A | B,
}

[Flags]
public enum X2 : ulong
{
    None = 0,
    A = 1 << 0,
    B = 1 << 1,
    All = ulong.MaxValue, // 0xffffffffffffffff
}

Here is how X1 and X2 behave:

Console.WriteLine((X1.A | X1.B) == X1.All);  // True
Console.WriteLine((X1.A | X1.B).ToString()); // All
Console.WriteLine((X1.All).ToString());      // All
Console.WriteLine((X1.All).ToString("G"));   // All
Console.WriteLine((X1.All).ToString("F"));   // All

Console.WriteLine((X2.A | X2.B) == X2.All);  // False
Console.WriteLine((X2.A | X2.B).ToString()); // A, B
Console.WriteLine((X2.All).ToString());      // All
Console.WriteLine((X2.All).ToString("G"));   // All
Console.WriteLine((X2.All).ToString("F"));   // All

While X1 respects the properties described above, X2 doesn’t.
As a result X2.A | X2.B is not X2.All, so there are least n undeclared values X2.C1, X2.C2, …, X2.Cn such that X2.A | X2.B | X2.C1 | X2.C2 | ... | X2.Cn = X2.All.
This would create potential bugs in code, such as the following:

void AMethodReceivingX2(X2 x2)
{
    if (x2 == X2.All) { /* Do something specific */ }
}

// ...
AMethodReceivingX2(X2.A | X2.B); // I passed all declared flags, yet it's not the same as X2.All!

Therefore, while it may be tempting to conceptually define All as ~None for convenience and as it simplifies future maintenance of the enum, we think it’s more correct to:

  • either not define the All flag explicitly, when not needed
  • or define it as the combination of all individual values, when it is needed

So, in your specific scenario, I would change the declaration of the enum in one of the following ways:

// First option
    [Flags]
    public enum VariantProcessingFeatures : ulong
    {
        None = 0,
        ColorCompression = 1,
    }
// Second option
    [Flags]
    public enum VariantProcessingFeatures : ulong
    {
        None = 0,
        ColorCompression = 1,
        All = ColorCompression,
    }

I hope that helps,
Best regards,
Antonio

1 Like
5

Hi!

First off thank you for taking the time to elaborate this to me in such depth!

I do believe i’ll want to retain an All option in the members but your elaboration illuminated me that this is a rather more specific thing than i at first surmised.

I believe i’ll rather go with an option of defining my own method to correctly do that thing and simply suppress the warning but it’s certainly correct in giving one a warning, even if intentional it’ll atleast ensure someone intending to do what i plan to will perhaps notice this and take proper caution in their implementation.

Thank you once more and have a fantastic day!

1 Like
6

Hi @JJoensuu
Glad it was helpful!
What I think would help is a new rule that would capture this use of All and would be explicit about why this is an issue and how to fix it. I have created an issue in our backlog to capture the idea.
In your specific case, it can indeed make sense to keep the behavior if that better serves your specific use case of All, since you are aware of the potential downsides (that the new rule may help identify in the future).
Best regards,
Antonio

1 Like