[Feature] Enum [Flags] should warn if values not powers of 2

[jzabroski]

The following examples do not follow the Remarks section in System.FlagsAttribute:

Define enumeration constants in powers of two, that is, 1, 2, 4, 8, and so on. This means the individual flags in combined enumeration constants do not overlap.

As a result of this invalid configuration, ToString() returns incorrect results because the Flags values use an unsupported configuration, so the behavior is undefined/non-supported.

    [Flags]
    enum TestEnum
    {
        A = 3, B = 4
    }
    [Flags]
    enum TestEnum2
    {
        A = 3, B = 4, C = 5
    }
    static void Main(string[] args)
    {
        Console.WriteLine(TestEnum.A|TestEnum.B);
        Console.WriteLine(TestEnum2.A|TestEnum2.B);
    }

[JohanLarsson]

This is starting to no longer be reflection territory but keep the issue.

[jzabroski]

I mean, I guess that a bunch of these could go into a new EnumAnalyzers project, but unless you develop a super-package of analyzers, it loses the utility and purpose.

[JohanLarsson]

We wrote a couple of analyzers for enums in Gu.Analyzers, maybe there is one checking this already, I don't remember.

[brian-reichle]

I thought there was something like this in the legacy FxCop, might have already been ported to FxCopAnalyzers. If you do create such a rule, it would be nice to have an exemption for mask values :)

[JohanLarsson]

Agreed it should only check literals.

[jzabroski]

We wrote a couple of analyzers for enums in Gu.Analyzers, maybe there is one checking this already, I don't remember.

I checked and do not see it. Here is what I see when I control+F search my web page in Chrome:

Code	Description
GU0060	Enum member value conflict.
GU0061	Enum member value out of range.

[jnm2]

I'm not even sure I agree with this analyzer. There's no rule that says flags may not be mutually exclusive. For example, KeyData or MethodAttributes. MethodAttributes.PrivateScope, Private, FamANDAssem, etc are mutually exclusive but are also flags that combine with the single-bit flags in the rest of the enum.

[jzabroski]

@jnm2 Rules can certainly have exceptions, but the point is to flag potentially bad code.

Frankly, I agree with Don Syme (F# creator): Discriminated union types are much better, and preferred, to Enumerations. The narrow use case is for representing physical bit positions. For KeyData, the documentation just proves why this rule makes sense:

Do not use the values in this enumeration for combined bitwise operations. The values in the enumeration are not mutually exclusive.
https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.keys?view=netframework-4.8#remarks

For MethodAttributes, arguably all mask fields should have ended in Mask suffix - so that it's clear it's a bitmask and not a direct bit position.

[jnm2]

@jzabroski

For KeyData, the documentation just proves why this rule makes sense:

Except that a core use case in real-world applications is Keys.Ctrl | Keys.A, etc, so it's hard to avoid ORing and masking those flags.

I'm not convinced it's a sign of potentially bad code. Why should everyone operate by the more narrow expectation?

[jzabroski]

In practice, you would write something like:

new KeyGesture(Key.C, (ModifierKeys.Control | ModifierKeys.Shift))

I guess someone (ahem) should open up a ticket to improve that documentation and explain best practices.

[jnm2]

@jzabroski I'm used to Windows Forms. KeyEventArgs has ORed keys and modifiers together since .NET Framework 1.0, I think.