Update S4070 to accept negative numbers (#8211)

SonarSource · Oct 18, 2023 · 35028aa · 35028aa
1 parent e2bcd79
commit 35028aa
Show file tree

Hide file tree

Showing 2 changed files with 105 additions and 28 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/DoNotMarkEnumsWithFlags.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/DoNotMarkEnumsWithFlags.cs
@@ -18,16 +18,17 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
+using System.Numerics;
+
 namespace SonarAnalyzer.Rules.CSharp
 {
     [DiagnosticAnalyzer(LanguageNames.CSharp)]
     public sealed class DoNotMarkEnumsWithFlags : SonarDiagnosticAnalyzer
     {
-        internal const string DiagnosticId = "S4070";
+        private const string DiagnosticId = "S4070";
         private const string MessageFormat = "Remove the 'FlagsAttribute' from this enum.";
 
-        private static readonly DiagnosticDescriptor Rule =
-            DescriptorFactory.Create(DiagnosticId, MessageFormat);
+        private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
 
         public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
 
@@ -52,7 +53,7 @@ protected override void Initialize(SonarAnalysisContext context) =>
                         .ToList();
 
                     var allValues = membersWithValues.Select(x => x.Value)
-                        .OfType<ulong>()
+                        .OfType<BigInteger>()
                         .Distinct()
                         .ToList();
 
@@ -67,35 +68,47 @@ protected override void Initialize(SonarAnalysisContext context) =>
                     }
                 }, SyntaxKind.EnumDeclaration);
 
-        // The idea of this method is to get rid of invalid values for flags such as negative values and decimals
-        private static ulong? GetEnumValueOrDefault(IFieldSymbol enumMember) =>
-            enumMember.HasConstantValue && ulong.TryParse(enumMember.ConstantValue.ToString(), out var longValue)
-                ? longValue
+        private static BigInteger? GetEnumValueOrDefault(IFieldSymbol enumMember) =>
+            enumMember.HasConstantValue
+                ? enumMember.ConstantValue switch // BigInteger is used here to support enums with base type of ulong. Direct cast is used to avoid string conversion.
+                  {
+                      byte x => (BigInteger)x,
+                      sbyte x => (BigInteger)x,
+                      short x => (BigInteger)x,
+                      ushort x => (BigInteger)x,
+                      int x => (BigInteger)x,
+                      uint x => (BigInteger)x,
+                      long x => (BigInteger)x,
+                      ulong x => (BigInteger)x,
+                      _ => null
+                  }
                 : null;
 
-        private static bool IsValidFlagValue(ulong? enumValue, List<ulong> allValues) =>
-            enumValue.HasValue && (IsZeroOrPowerOfTwo(enumValue.Value) || IsCombinationOfOtherValues(enumValue.Value, allValues));
+        private static bool IsValidFlagValue(BigInteger? enumValue, IEnumerable<BigInteger> allValues) =>
+            enumValue.HasValue
+            && (IsZeroOrPowerOfTwo(enumValue.Value)
+                || IsCombinationOfOtherValues(enumValue.Value, allValues));
 
-        // See https://stackoverflow.com/questions/600293/how-to-check-if-a-number-is-a-power-of-2
-        private static bool IsZeroOrPowerOfTwo(ulong value) =>
-            (value & (value - 1)) == 0;
+        private static bool IsZeroOrPowerOfTwo(BigInteger value) =>
+            value.IsZero
+            || BigInteger.Abs(value).IsPowerOfTwo;
 
-        private static bool IsCombinationOfOtherValues(ulong value, List<ulong> otherValues)
+        private static bool IsCombinationOfOtherValues(BigInteger value, IEnumerable<BigInteger> otherValues)
         {
-            var newValue = value;
-            foreach (var otherValue in otherValues.SkipWhile(v => value <= v))
+            var currentValue = value;
+            foreach (var otherValue in otherValues.SkipWhile(x => value <= x))
             {
-                if (otherValue <= newValue)
+                if (otherValue <= currentValue)
                 {
-                    newValue ^= otherValue;
-                    if (newValue == 0)
+                    currentValue ^= otherValue;
+                    if (currentValue == 0)
                     {
                         return true;
                     }
                 }
             }
 
-            return newValue == 0;
+            return currentValue == 0;
         }
     }
 }
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/DoNotMarkEnumsWithFlags.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/DoNotMarkEnumsWithFlags.cs
@@ -59,10 +59,15 @@ public enum Color4
     }
 
     [Flags]
-    public enum Color5 : int // Noncompliant
+    public enum NegativeValues
     {
-        Value = -3, // Secondary
-        Other = -4 // Secondary
+        Default = 0,
+        A = -2,
+        B = -4,
+        C = 1 << 31, // It overflows and becomes negative: https://github.com/SonarSource/sonar-dotnet/issues/7991
+        D = A | B,
+        E = 2,
+        F = D | E
     }
 
     [Flags]
@@ -102,12 +107,71 @@ enum EnumFoo4 // Noncompliant
         N3 = N1 | N2,
         N5 = 5 // Secondary
     }
+}
 
-    // Reproducer for https://github.com/SonarSource/sonar-dotnet/issues/7991
-    [FlagsAttribute]
-    public enum Repro_7991 // Noncompliant FP
+namespace DifferentBaseType
+{
+    [Flags]
+    public enum EByte : byte
     {
-        None = 0,
-        Red = 1 << 31 // Secondary
+        A = 1,
+        B = 2,
+        C = A | B
+    }
+
+    [Flags]
+    public enum ESByte : sbyte
+    {
+        A = -2,
+        B = 2,
+        C = A | B
+    }
+
+    [Flags]
+    public enum EShort : short
+    {
+        A = -2,
+        B = 2,
+        C = A | B
+    }
+
+    [Flags]
+    public enum EUShort : ushort
+    {
+        A = 1,
+        B = 2,
+        C = A | B
+    }
+
+    [Flags]
+    public enum EInt : int
+    {
+        A = -2,
+        B = 2,
+        C = A | B
+    }
+
+    [Flags]
+    public enum EUInt : uint
+    {
+        A = 1,
+        B = 2,
+        C = A | B
+    }
+
+    [Flags]
+    public enum ELong : long
+    {
+        A = -2,
+        B = 2,
+        C = A | B
+    }
+
+    [Flags]
+    public enum EULong : ulong
+    {
+        A = 1,
+        B = 2,
+        C = A | B
     }
 }