Review [FLAGS] analyzers

[Vannevelj]

I noticed some of the Roslyn solution's enums trigger the error-level FLAGS warnings. Investigate closer.

[Vannevelj]

Severity Code Description Project File Line Suppression State Error VSD0004 [Flags] enum BestSymbolLocation its values are not explicit powers of 2 CSharpCodeAnalysis D:\Github\Roslyn\src\Compilers\CSharp\Portable\Binder\Binder_Symbols.cs 1578 Active Error VSD0004 [Flags] enum WorkspaceBackgroundWork its values are not explicit powers of 2 ServicesVisualStudio D:\Github\Roslyn\src\VisualStudio\Core\Def\Implementation\ProjectSystem\WorkspaceBackgroundWork.cs 18 Active Error VSD0039 [Flags] enum UnaryOperatorKind its values are not explicit powers of 2 and does not fit in a int enum type. CSharpCodeAnalysis D:\Github\Roslyn\src\Compilers\CSharp\Portable\Binder\Semantics\Operators\OperatorKind.cs 13 Active Error VSD0039 [Flags] enum PInvokeAttributes its values are not explicit powers of 2 and does not fit in a ushort enum type. CodeAnalysis D:\Github\Roslyn\src\Compilers\Core\Portable\PEWriter\Miscellaneous.cs 221 Active Error VSD0039 [Flags] enum NodeFlags its values are not explicit powers of 2 and does not fit in a byte enum type. CodeAnalysis D:\Github\Roslyn\src\Compilers\Core\Portable\Syntax\GreenNode.cs 195 Active Error VSD0004 [Flags] enum MemberFlags its values are not explicit powers of 2 CodeAnalysis D:\Github\Roslyn\src\Compilers\Core\Portable\MemberDescriptor.cs 12 Active Error VSD0004 [Flags] enum LexerMode its values are not explicit powers of 2 CSharpCodeAnalysis D:\Github\Roslyn\src\Compilers\CSharp\Portable\Parser\Lexer.cs 14 Active Error VSD0039 [Flags] enum CompletionPart its values are not explicit powers of 2 and does not fit in a int enum type. CSharpCodeAnalysis D:\Github\Roslyn\src\Compilers\CSharp\Portable\Symbols\CompletionPart.cs 20 Active Error VSD0004 [Flags] enum CallingConvention its values are not explicit powers of 2 CodeAnalysis D:\Github\Roslyn\src\Compilers\Core\Portable\PEWriter\Members.cs 20 Active Error VSD0004 [Flags] enum BinderFlags its values are not explicit powers of 2 CSharpCodeAnalysis D:\Github\Roslyn\src\Compilers\CSharp\Portable\Binder\BinderFlags.cs 11 Active Error VSD0039 [Flags] enum BinaryOperatorKind its values are not explicit powers of 2 and does not fit in a int enum type. CSharpCodeAnalysis D:\Github\Roslyn\src\Compilers\CSharp\Portable\Binder\Semantics\Operators\OperatorKind.cs 262 Active

[Hosch250]

Looking through this set, there is a bug here, but there are also bugs in Roslyn. Our bug is that we flag any values without explicit values as non-flag values if there are any with explicit values:

[Flags]
internal enum WorkspaceBackgroundWork
{
    None,
    Parse = 1,
    Compile = 2,
    ParseAndCompile = Parse | Compile
}

While that enum above is valid, we flag it. We do not flag this:

[Flags]
enum Flags
{
    A, B, C
}

Here is another example showing that we cannot just ignore 0 values to fix this:

[Flags]
public enum DkmEvaluationResultFlags
{
    None,
    SideEffect,
    Expandable,
    Boolean = 4,
    /// ...
}

[Hosch250]

This enum does not have a bug, but should be written more clearly with the | operator:

    [Flags]
    private enum ExpressionType
    {
        Invalid = 0x0,
        ValidExpression = 0x1,

        // Note: ValidTerm implies ValidExpression.
        ValidTerm = 0x3
    }

[Hosch250]

This is a tricky one, but we handle it correctly. It turns out that SignatureCallingConvention.VarArgs == 5 and SignatureCallingConvention.ThisCall == 3

[Flags]
internal enum CallingConvention
{
    // ...        

    /// <summary>
    /// The convention for calling managed methods that accept extra arguments.
    /// </summary>
    ExtraArguments = SignatureCallingConvention.VarArgs,

    // ...

    /// <summary>
    /// C++ member unmanaged method (non-vararg) calling convention. The callee cleans the stack and the this pointer is pushed on the stack last.
    /// </summary>
    ThisCall = SignatureCallingConvention.ThisCall,

    // ...
}

[Hosch250]

These are flagged because 2 ^ {number of enum values} is greater than the the largest value the enum can hold. However, none of the actual values is too large because they overlap. This should probably be moved into its own diagnostic because being too large is not related to not being a flag value and can be applied to any enum.

[Flags]
internal enum CompletionPart
{
    // For all symbols
    None = 0,
    Attributes = 1 << 0,

    // For method symbols
    ReturnTypeAttributes = 1 << 1,

    // For symbols with parameters: method and property symbols.
    Parameters = 1 << 2,

    // For symbols with type: method, property and field symbols
    Type = 1 << 3,

    // For named type symbols
    StartBaseType = 1 << 4,
    FinishBaseType = 1 << 5,
    StartInterfaces = 1 << 6,
    FinishInterfaces = 1 << 7,
    EnumUnderlyingType = 1 << 8,
    TypeArguments = 1 << 9,
    TypeParameters = 1 << 10,
    Members = 1 << 11,
    TypeMembers = 1 << 12,
    SynthesizedExplicitImplementations = 1 << 13,
    StartMemberChecks = 1 << 14,
    FinishMemberChecks = 1 << 15,
    MembersCompleted = 1 << 16, // this should be the last (highest-value) part

    All = (1 << 17) - 1,

    // This is the work we can do if ForceComplete is scoped to a particular SyntaxTree.
    NamedTypeSymbolWithLocationAll = Attributes | StartBaseType | FinishBaseType | StartInterfaces | FinishInterfaces | EnumUnderlyingType |
        TypeArguments | TypeParameters | Members | TypeMembers | SynthesizedExplicitImplementations | StartMemberChecks | FinishMemberChecks,

    NamedTypeSymbolAll = NamedTypeSymbolWithLocationAll | MembersCompleted,

    // For Usings
    StartValidatingImports = 1 << 4,
    FinishValidatingImports = 1 << 5,
    ImportsAll = StartValidatingImports | FinishValidatingImports,

    // For namespace symbols
    NameToMembersMap = 1 << 11,
    NamespaceSymbolAll = NameToMembersMap | MembersCompleted,

    // For field symbols
    FixedSize = 1 << 11,
    ConstantValue = 1 << 12,
    FieldSymbolAll = Attributes | Type | FixedSize | ConstantValue,

    // For method symbols
    StartAsyncMethodChecks = 1 << 11,
    FinishAsyncMethodChecks = 1 << 12,
    StartMethodChecks = 1 << 13,
    FinishMethodChecks = 1 << 14,
    MethodSymbolAll = Attributes | ReturnTypeAttributes | Parameters | Type | TypeParameters | StartMethodChecks | FinishMethodChecks | StartAsyncMethodChecks | FinishAsyncMethodChecks,

    // For type parameter symbols
    TypeParameterConstraints = 1 << 11,
    TypeParameterSymbolAll = Attributes | TypeParameterConstraints,

    // For property symbols
    PropertySymbolAll = Attributes | Parameters | Type,

    // For alias symbols
    AliasTarget = 1 << 4,

    // For assembly symbols
    StartAttributeChecks = 1 << 4,
    FinishAttributeChecks = 1 << 5,
    Module = 1 << 6,
    StartValidatingAddedModules = 1 << 8,
    FinishValidatingAddedModules = 1 << 9,
    AssemblySymbolAll = Attributes | StartAttributeChecks | FinishAttributeChecks | Module | StartValidatingAddedModules | FinishValidatingAddedModules,

    // For module symbol
    StartValidatingReferencedAssemblies = 1 << 4,
    FinishValidatingReferencedAssemblies = 1 << 5,
}

---

    [Flags]
    internal enum NodeFlags : byte
    {
        None = 0,
        ContainsDiagnostics = 1 << 0,
        ContainsStructuredTrivia = 1 << 1,
        ContainsDirectives = 1 << 2,
        ContainsSkippedText = 1 << 3,
        ContainsAnnotations = 1 << 4,
        IsNotMissing = 1 << 5,

        FactoryContextIsInAsync = 1 << 6,
        FactoryContextIsInQuery = 1 << 7,
        FactoryContextIsInIterator = FactoryContextIsInQuery,  // VB does not use "InQuery", but uses "InIterator" instead

        InheritMask = ContainsDiagnostics | ContainsStructuredTrivia | ContainsDirectives | ContainsSkippedText | ContainsAnnotations | IsNotMissing,
    }

[Hosch250]

I'm not sure why these are [Flags] enums:

[Flags]
internal enum UnaryOperatorKind
{
    // NOTE: these types should line up with the elements in BinaryOperatorKind

    TypeMask = 0x000000FF,

    SByte = 0x00000011,
    Byte = 0x00000012,
    Short = 0x00000013,
    UShort = 0x00000014,
    Int = 0x00000015,
    UInt = 0x00000016,
    Long = 0x00000017,
    ULong = 0x00000018,
    Char = 0x00000019,
    Float = 0x0000001A,
    Double = 0x0000001B,
    Decimal = 0x0000001C,
    Bool = 0x0000001D,
    _Object = 0x0000001E, // reserved for binary op
    _String = 0x0000001F, // reserved for binary op
    // ...
}

---

[Flags]
private enum BestSymbolLocation
{
    None,
    FromSourceModule,
    FromAddedModule,
    FromReferencedAssembly,
    FromCorLibrary,
}

[Flags]
internal enum BinaryOperatorKind
{
    // NOTE: these types should line up with the elements in UnaryOperatorKind

    TypeMask = UnaryOperatorKind.TypeMask,

    Int = UnaryOperatorKind.Int,
    UInt = UnaryOperatorKind.UInt,
    Long = UnaryOperatorKind.Long,
    ULong = UnaryOperatorKind.ULong,
    Char = UnaryOperatorKind.Char, //not used
    Float = UnaryOperatorKind.Float,
    Double = UnaryOperatorKind.Double,
    Decimal = UnaryOperatorKind.Decimal,
    Bool = UnaryOperatorKind.Bool,
    Object = UnaryOperatorKind._Object,
    String = UnaryOperatorKind._String,
    StringAndObject = UnaryOperatorKind._StringAndObject,
    ObjectAndString = UnaryOperatorKind._ObjectAndString,
    /// ...
}

---

[Flags]
internal enum LexerMode
{
    // ...
    MaskXmlDocCommentLocation = 0xF0000,
    MaskXmlDocCommentStyle = 0x300000,

    // ...
}

[Hosch250]

Notice that the analyzer message has particularly bad grammar as well.

[Pilchie]

Tagging @jaredpar, as all of these seem to be in the compilers.

[jaredpar]

@Hosch250

I'm not sure why these are [Flags] enums:

Look at the bottom of many of those enums. That is where the flags aspect comes into play.

CC @gafter for comments