Update S4070 to accept negative numbers #8211

costin-zaharia-sonarsource · 2023-10-17T12:49:29Z

if their absolute value is a power of two.

costin-zaharia-sonarsource · 2023-10-17T13:00:37Z

@antonioaversa, @sebastien-marichal what do you think?

From the runtime point of view, the negative values are also acceptable if their absolute value is a power of 2. From the usage point of view, the math can get confusing but that looks to me as a separate dedicated rule.

From the docs:

Use caution if you define a negative number as a flag enumerated constant because many flag positions might be set to 1, which might make your code confusing and encourage coding errors.

sebastien-marichal

LGTM!
I left one educational question.

sebastien-marichal · 2023-10-18T07:23:04Z

analyzers/src/SonarAnalyzer.CSharp/Rules/DoNotMarkEnumsWithFlags.cs

                : null;

-        private static bool IsValidFlagValue(ulong? enumValue, List<ulong> allValues) =>
+        private static bool IsValidFlagValue(BigInteger? enumValue, IEnumerable<BigInteger> allValues) =>


Educational: Why did you change the type from List to IEnumerable?
Isn't there a risk of multiple enumeration if we drop the ToList on allValues line 58?

I think that the method should not be aware of how it is used. If it does the iteration only once and doesn't use API specific to the collection it should use the most generic type. This in some scenarios will allow lazy evaluation, or no evaluation at all if the path is not reached.

antonioaversa

LGTM, we should be able to use BigInteger.Abs.

antonioaversa · 2023-10-18T08:30:18Z

analyzers/src/SonarAnalyzer.CSharp/Rules/DoNotMarkEnumsWithFlags.cs

+            || (value.Sign == -1
+                    ? BigInteger.Multiply(value, -1).IsPowerOfTwo
+                    : value.IsPowerOfTwo);


BigInteger.Abs(value).IsPowerOfTwo

Suggested change

|| (value.Sign == -1

? BigInteger.Multiply(value, -1).IsPowerOfTwo

: value.IsPowerOfTwo);

|| BigInteger.Abs(value).IsPowerOfTwo

antonioaversa · 2023-10-18T08:40:36Z

@costin-zaharia-sonarsource The safeguard and the _ => null seem difficult to cover.
I wonder whether we could just remove the safeguard, and return null whenever the type doesn't match one of the 8 types above.
I also wonder when enumMember.HasConstantValue is false and whether we can write an incomplete tuple to cover also tuple => tuple.Member.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax().GetLocation().

if their absolute value is a power of two.

costin-zaharia-sonarsource · 2023-10-18T09:27:31Z

@antonioaversa I removed the safeguard. Regarding the tuple, I think that's a scenario that could not actually happen, to have an enum declaration with a member that doesn't have a location. It looks like defensive coding.