Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update S4070 to accept negative numbers #8211

Merged
merged 2 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 33 additions & 20 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/DoNotMarkEnumsWithFlags.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -52,7 +53,7 @@ protected override void Initialize(SonarAnalysisContext context) =>
.ToList();

var allValues = membersWithValues.Select(x => x.Value)
.OfType<ulong>()
.OfType<BigInteger>()
.Distinct()
.ToList();

Expand All @@ -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) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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
}
}