Roslyn-analyzers: CA2248 has incorrect and undocumented diagnostic

[AArnott]

Analyzer

Diagnostic ID: CA2248: Provide correct enum argument to Enum.HasFlag

Analyzer source

NuGet Package: Microsoft.CodeAnalysis.FxCopAnalyzers

Version: v3.3.1 (Latest)

Describe the bug

The CA2248 diagnostic incorrectly fires on uses of HasFlag that appears on enum types that are missing a [Flags] attribute, claiming that "this call will always return 'false'" when in fact it may return true just like any other enum, as attested to by this C# Interactive Window output:

> enum A { B = 1
. }
> A a = A.B;
> a.HasFlag(A.B)
true
> 

Steps To Reproduce

Given this code:

            if (((__VSFINDOPTIONS)grfOptions).HasFlag(__VSFINDOPTIONS.FR_Find) ||
                ((__VSFINDOPTIONS)grfOptions).HasFlag(__VSFINDOPTIONS.FR_Replace))

Where the enum is defined as:

public enum __VSFINDOPTIONS
{
FR_Find = 0x100000,
FR_Replace = 0x400000
}

Expected behavior

No warning. In all cases, whether the enum is attributed with a FlagsAttribute or not, or even if its members are defined with flags-friendly values, HasFlag will indeed return true if the given value is set as if it _were_ a flags enum.

Actual behavior

The analyzer incorrectly claims:

This call will always returns 'false' because the enum type '__VSFINDOPTIONS' is not marked with 'FlagsAttribute'

It does _not_ return false, so the message is dangerously misleading as someone might remove the code that is guarded by the condition that allegedly will never evaluate to true.

[AArnott]

What's more, the docs for this analyzer doesn't even talk about this message. It only talks about passing in a different enum value that doesn't match the type that is receiving the call.

[mavasani]

Tagging @Evangelink

[Youssef1313]

Does this suggest to completely remove the NotFlagsRule?

https://github.com/dotnet/roslyn-analyzers/blob/25f1e288fb3c6ebcb6adb3543a5efd6d41ad6b95/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/ProvideCorrectArgumentToEnumHasFlag.cs#L32

Related to https://github.com/dotnet/roslyn-analyzers/issues/3528 and #3652

[AArnott]

@Youssef1313, perhaps yes. If it stays, at least the message should be changed to "you might be treating this enum like a flags enum inappropriately" instead of falsely claiming that a false will always be the result.
Even better would be to not display the message if the enum values appear to be bit flags (e.g. incrementing in multiples of 2). Of course the enums that are intended as flags should be attributed as such, but in the cases I'm seeing, the enums are in interop assemblies so we can't change them and maybe IDL doesn't provide a way to tag as flags.

[Evangelink]

@AArnott Sorry about that! I actually faced an error in a client codebase that I interpreted as HasFlag was always returning false for non-flag enums hence this part of the analyzer. In this case, the enum wasn't designed as a flag (power of 2) so it's different from your case.

I have been giving some thoughts about what could be a good experience and I see 2 options:

1. The option you describe where if the enum is flag-like we don't report at all + an update to the message for the other cases. It might be worth (if that doesn't already exist) to have an extra rule (different ID) that would recommend marking the enum with [Flags] when it's a flag-like enum.

2. Update the message + add the user-option excluded_symbol_names where you would be able to exclude special enums like __VSFINDOPTIONS.

We could also think about some kind of hybrid approach where we update the message and we introduce a new user-option like ignore_flag_like_enum set to true by default. Not sure who would want to disable it.

[Evangelink]

@mavasani feel free to assign me on this ticket.

[AArnott]

We should keep in mind as well that some enums are flags, but also define values that serve as masks, so not every enum member will be a power of two higher than the previous one.

What if we just remove the new diagnostic? Even in the intended case for the diagnostic of using HasFlag on a non-flags enum, it seems the risk isn't that false will be returned, but rather that true will be returned falsely. Consider this case:

enum Options {
  First = 1, 
  Second,
  Third
}

Options o = Options.Third;
if (o.HasFlag(Options.First)) {
  // This will be hit!
}

So there _is_ room for a bug here and a diagnostic would be useful. But the bug isn't that HasFlag will return false, but rather that it will return true in an unexpected case as is the above one.

[Evangelink]

Sorry for the delay, I forgot about this ticket.

I would like to suggest that I reuse the checks from the other rule that checks an enum claiming to be FlagsAttribute is correctly defined in case of a call to HasFlag on an enum without the FlagsAttribute and if it is too complex then I simply get rid of this second part.

I will try to push a PR today/tomorrow and will ping you for review if that's ok?

[Evangelink]

I am still quite late but I had time to actually try a couple of implementations and sadly none is working as I would expect. My main is problem is when the enum is declared in another assembly (no syntax reference) because from the symbol point of view I can access a value but in the event of a non power of two value, I cannot tell whether this value is being declared directly or as a reference to other fields enum.

Given all of that we can:

1. Drop the second part of the rule
2. Add a user-option to allow flagging these cases knowing that we might have some to a lot of false-positives

Based upon all the discussions I think we need to drop the second part and wait for smarter analyzers (from past experience roslyn team was pretty reluctant to expose more info on the symbols).

[mavasani]

Based upon all the discussions I think we need to drop the second part and wait for smarter analyzers (from past experience roslyn team was pretty reluctant to expose more info on the symbols).

Agreed, that sounds reasonable to me.