From 106dd99c674020ae11ff207ad68870fcb036cef8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9rald=20Barr=C3=A9?= Date: Tue, 8 Sep 2026 13:02:35 -0400 Subject: [PATCH] Do not report MA0192 when the HasFlag rewrite reorders an effectful operand value.HasFlag(flag) evaluates the value operand before the flag operand and drops the duplicated read of the flag. AreEquivalentOperands only proves that both reads target the same storage, not that the value is stable across the evaluation of the other operand, so `(flag & Change()) == flag` was rewritten to `Change().HasFlag(flag)` even when Change() assigns a new value to flag, which changes the result of the check. The pattern is now gated on CanRewriteToHasFlag, which accepts the rewrite only when the flag is a compile-time constant, when the value operand is already evaluated before every read of the flag, or when the value operand is side-effect free. Both the comparison operand reversal and the bitwise operand reversal are covered. --- docs/Rules/MA0192.md | 2 + .../Rules/UseHasFlagMethodFixer.cs | 20 +- .../Rules/UseHasFlagMethodAnalyzer.cs | 24 +- .../Rules/UseHasFlagMethodCommon.cs | 35 +++ .../Rules/UseHasFlagMethodAnalyzerTests.cs | 244 ++++++++++++++++++ 5 files changed, 309 insertions(+), 16 deletions(-) diff --git a/docs/Rules/MA0192.md b/docs/Rules/MA0192.md index b82caf34f..cbbc2b1f1 100644 --- a/docs/Rules/MA0192.md +++ b/docs/Rules/MA0192.md @@ -23,6 +23,8 @@ The compared flag doesn't have to be a constant. `(value & flags) == flags` is r Expressions that may return a different value on each evaluation, such as properties, method calls, or `volatile` fields, are not reported. +When the flag is not a constant, the rewrite must not change the order in which the operands are evaluated. `value.HasFlag(flags)` reads `value` before `flags` and drops the duplicated read of `flags`, so the check is only reported when `value` is already evaluated before every read of `flags`, or when evaluating `value` cannot change `flags`. For instance, `(flags & Compute()) == flags` is not reported, as `Compute()` could assign a new value to `flags`. + For comparisons against `0`, the enum member used in the bitwise `&` must be a single-bit value (for example `1`, `2`, `4`, `8`, ...). Combined values are ignored. Zero-valued enum members are not reported by this rule. They are covered by [MA0201](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0201.md). diff --git a/src/Meziantou.Analyzer.CodeFixers/Rules/UseHasFlagMethodFixer.cs b/src/Meziantou.Analyzer.CodeFixers/Rules/UseHasFlagMethodFixer.cs index 31a9f1dfa..b262348ac 100644 --- a/src/Meziantou.Analyzer.CodeFixers/Rules/UseHasFlagMethodFixer.cs +++ b/src/Meziantou.Analyzer.CodeFixers/Rules/UseHasFlagMethodFixer.cs @@ -96,7 +96,7 @@ private static bool TryGetHasFlagPattern(IOperation operation, [NotNullWhen(true { if (TryGetComparedOperand(patternOperation, out var comparedOperand, out var negate)) { - pattern = GetFromBitwiseAnd(andOperation, comparedOperand, operationExpression, negate); + pattern = GetFromBitwiseAnd(andOperation, comparedOperand, operationExpression, negate, comparedOperandEvaluatedFirst: false); return pattern is not null; } } @@ -137,14 +137,14 @@ private static bool TryGetComparedOperand(IPatternOperation patternOperation, [N if (leftOperand is IBinaryOperation { OperatorKind: BinaryOperatorKind.And } leftBitwiseAnd) { - var pattern = GetFromBitwiseAnd(leftBitwiseAnd, rightOperand, operationExpression, negate); + var pattern = GetFromBitwiseAnd(leftBitwiseAnd, rightOperand, operationExpression, negate, comparedOperandEvaluatedFirst: false); if (pattern is not null) return pattern; } if (rightOperand is IBinaryOperation { OperatorKind: BinaryOperatorKind.And } rightBitwiseAnd) { - var pattern = GetFromBitwiseAnd(rightBitwiseAnd, leftOperand, operationExpression, negate); + var pattern = GetFromBitwiseAnd(rightBitwiseAnd, leftOperand, operationExpression, negate, comparedOperandEvaluatedFirst: true); if (pattern is not null) return pattern; } @@ -152,22 +152,24 @@ private static bool TryGetComparedOperand(IPatternOperation patternOperation, [N return null; } - private static HasFlagPattern? GetFromBitwiseAnd(IBinaryOperation bitwiseAndOperation, IOperation comparedOperand, ExpressionSyntax operationExpression, bool negate) + private static HasFlagPattern? GetFromBitwiseAnd(IBinaryOperation bitwiseAndOperation, IOperation comparedOperand, ExpressionSyntax operationExpression, bool negate, bool comparedOperandEvaluatedFirst) { var leftOperand = bitwiseAndOperation.LeftOperand.UnwrapImplicitConversions(); var rightOperand = bitwiseAndOperation.RightOperand.UnwrapImplicitConversions(); comparedOperand = comparedOperand.UnwrapImplicitConversions(); - if (TryGetEnumFlagReference(rightOperand, comparedOperand, out var flagOperation, out var comparedWithZero) && + if (TryGetEnumFlagReference(rightOperand, comparedOperand, out var flagOperation, out var comparedWithZero, out var isConstantFlag) && IsValidPattern(leftOperand, flagOperation) && + UseHasFlagMethodCommon.CanRewriteToHasFlag(leftOperand, isConstantFlag, preservesEvaluationOrder: !comparedOperandEvaluatedFirst) && leftOperand.Syntax is ExpressionSyntax enumValueExpression && flagOperation.Syntax is ExpressionSyntax flagExpression) { return new(operationExpression, enumValueExpression, flagExpression, comparedWithZero ? !negate : negate); } - if (TryGetEnumFlagReference(leftOperand, comparedOperand, out flagOperation, out comparedWithZero) && + if (TryGetEnumFlagReference(leftOperand, comparedOperand, out flagOperation, out comparedWithZero, out isConstantFlag) && IsValidPattern(rightOperand, flagOperation) && + UseHasFlagMethodCommon.CanRewriteToHasFlag(rightOperand, isConstantFlag, preservesEvaluationOrder: false) && rightOperand.Syntax is ExpressionSyntax enumValueExpression2 && flagOperation.Syntax is ExpressionSyntax flagExpression2) { @@ -177,7 +179,7 @@ rightOperand.Syntax is ExpressionSyntax enumValueExpression2 && return null; } - private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation comparedOperand, [NotNullWhen(true)] out IOperation? flagOperation, out bool comparedWithZero) + private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation comparedOperand, [NotNullWhen(true)] out IOperation? flagOperation, out bool comparedWithZero, out bool isConstantFlag) { potentialFlag = potentialFlag.UnwrapImplicitConversions(); comparedOperand = comparedOperand.UnwrapImplicitConversions(); @@ -192,6 +194,7 @@ private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation { flagOperation = secondFieldReference; comparedWithZero = false; + isConstantFlag = true; return true; } @@ -199,6 +202,7 @@ private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation { flagOperation = firstFieldReference; comparedWithZero = true; + isConstantFlag = true; return true; } } @@ -207,11 +211,13 @@ private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation { flagOperation = comparedOperand; comparedWithZero = false; + isConstantFlag = comparedOperand.ConstantValue.HasValue; return true; } flagOperation = null; comparedWithZero = false; + isConstantFlag = false; return false; } diff --git a/src/Meziantou.Analyzer/Rules/UseHasFlagMethodAnalyzer.cs b/src/Meziantou.Analyzer/Rules/UseHasFlagMethodAnalyzer.cs index 650c3f329..6c486af19 100644 --- a/src/Meziantou.Analyzer/Rules/UseHasFlagMethodAnalyzer.cs +++ b/src/Meziantou.Analyzer/Rules/UseHasFlagMethodAnalyzer.cs @@ -252,7 +252,7 @@ private static bool TryGetHasFlagPattern(IOperation operation, [NotNullWhen(true { if (TryGetComparedOperand(patternOperation, out var comparedOperand, out _)) { - pattern = GetFromBitwiseAnd(andOperation, comparedOperand); + pattern = GetFromBitwiseAnd(andOperation, comparedOperand, comparedOperandEvaluatedFirst: false); return pattern is not null; } } @@ -268,14 +268,14 @@ private static bool TryGetHasFlagPattern(IOperation operation, [NotNullWhen(true if (leftOperand is IBinaryOperation { OperatorKind: BinaryOperatorKind.And } leftBitwiseAnd) { - var pattern = GetFromBitwiseAnd(leftBitwiseAnd, rightOperand); + var pattern = GetFromBitwiseAnd(leftBitwiseAnd, rightOperand, comparedOperandEvaluatedFirst: false); if (pattern is not null) return pattern; } if (rightOperand is IBinaryOperation { OperatorKind: BinaryOperatorKind.And } rightBitwiseAnd) { - var pattern = GetFromBitwiseAnd(rightBitwiseAnd, leftOperand); + var pattern = GetFromBitwiseAnd(rightBitwiseAnd, leftOperand, comparedOperandEvaluatedFirst: true); if (pattern is not null) return pattern; } @@ -283,20 +283,22 @@ private static bool TryGetHasFlagPattern(IOperation operation, [NotNullWhen(true return null; } - private static HasFlagPattern? GetFromBitwiseAnd(IBinaryOperation bitwiseAndOperation, IOperation comparedOperand) + private static HasFlagPattern? GetFromBitwiseAnd(IBinaryOperation bitwiseAndOperation, IOperation comparedOperand, bool comparedOperandEvaluatedFirst) { var leftOperand = bitwiseAndOperation.LeftOperand.UnwrapImplicitConversions(); var rightOperand = bitwiseAndOperation.RightOperand.UnwrapImplicitConversions(); comparedOperand = comparedOperand.UnwrapImplicitConversions(); - if (TryGetEnumFlagReference(rightOperand, comparedOperand, out var flagOperation) && - IsValidPattern(leftOperand, flagOperation)) + if (TryGetEnumFlagReference(rightOperand, comparedOperand, out var flagOperation, out var isConstantFlag) && + IsValidPattern(leftOperand, flagOperation) && + UseHasFlagMethodCommon.CanRewriteToHasFlag(leftOperand, isConstantFlag, preservesEvaluationOrder: !comparedOperandEvaluatedFirst)) { return new(leftOperand, flagOperation); } - if (TryGetEnumFlagReference(leftOperand, comparedOperand, out flagOperation) && - IsValidPattern(rightOperand, flagOperation)) + if (TryGetEnumFlagReference(leftOperand, comparedOperand, out flagOperation, out isConstantFlag) && + IsValidPattern(rightOperand, flagOperation) && + UseHasFlagMethodCommon.CanRewriteToHasFlag(rightOperand, isConstantFlag, preservesEvaluationOrder: false)) { return new(rightOperand, flagOperation); } @@ -304,7 +306,7 @@ private static bool TryGetHasFlagPattern(IOperation operation, [NotNullWhen(true return null; } - private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation comparedOperand, [NotNullWhen(true)] out IOperation? flagOperation) + private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation comparedOperand, [NotNullWhen(true)] out IOperation? flagOperation, out bool isConstantFlag) { potentialFlag = potentialFlag.UnwrapImplicitConversions(); comparedOperand = comparedOperand.UnwrapImplicitConversions(); @@ -319,12 +321,14 @@ private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation !NumericHelpers.IsZero(firstFieldReference.Field.ConstantValue)) { flagOperation = secondFieldReference; + isConstantFlag = true; return true; } if (comparedOperand.IsConstantZero() && NumericHelpers.IsSingleBitSet(firstFieldReference.Field.ConstantValue)) { flagOperation = firstFieldReference; + isConstantFlag = true; return true; } } @@ -332,10 +336,12 @@ private static bool TryGetEnumFlagReference(IOperation potentialFlag, IOperation if (!potentialFlag.IsConstantZero() && UseHasFlagMethodCommon.AreEquivalentOperands(potentialFlag, comparedOperand)) { flagOperation = comparedOperand; + isConstantFlag = comparedOperand.ConstantValue.HasValue; return true; } flagOperation = null; + isConstantFlag = false; return false; } diff --git a/src/Meziantou.Analyzer/Rules/UseHasFlagMethodCommon.cs b/src/Meziantou.Analyzer/Rules/UseHasFlagMethodCommon.cs index 8d5507ad3..684995eae 100644 --- a/src/Meziantou.Analyzer/Rules/UseHasFlagMethodCommon.cs +++ b/src/Meziantou.Analyzer/Rules/UseHasFlagMethodCommon.cs @@ -23,4 +23,39 @@ public static bool AreEquivalentOperands(IOperation? left, IOperation? right) _ => false, }; } + + /// + /// Determines if replacing the flag check by value.HasFlag(flag) preserves the semantics of the check. + /// The rewrite evaluates the value operand before the flag operand and drops the duplicated read of the flag, + /// so it is only valid when evaluating the value operand cannot change the value of the flag. + /// + /// The operand the HasFlag method is called on. + /// Whether the flag is a compile-time constant, in which case its reads are stable. + /// Whether the value operand is already evaluated before every read of the flag. + public static bool CanRewriteToHasFlag(IOperation enumValueOperation, bool isConstantFlag, bool preservesEvaluationOrder) + { + return isConstantFlag || preservesEvaluationOrder || IsSideEffectFree(enumValueOperation); + } + + /// + /// Determines if evaluating the operation cannot have any observable side effect, so that it can be reordered with the read of the flag. + /// + private static bool IsSideEffectFree(IOperation? operation) + { + if (operation is null) + return false; + + if (operation.ConstantValue.HasValue) + return true; + + return operation switch + { + IParameterReferenceOperation or ILocalReferenceOperation or IInstanceReferenceOperation or ILiteralOperation => true, + IFieldReferenceOperation fieldReference => fieldReference.Field.IsStatic || IsSideEffectFree(fieldReference.Instance), + IConversionOperation conversion => conversion.OperatorMethod is null && IsSideEffectFree(conversion.Operand), + IUnaryOperation unary => unary.OperatorMethod is null && IsSideEffectFree(unary.Operand), + IBinaryOperation binary => binary.OperatorMethod is null && IsSideEffectFree(binary.LeftOperand) && IsSideEffectFree(binary.RightOperand), + _ => false, + }; + } } diff --git a/tests/Meziantou.Analyzer.Test/Rules/UseHasFlagMethodAnalyzerTests.cs b/tests/Meziantou.Analyzer.Test/Rules/UseHasFlagMethodAnalyzerTests.cs index 95baa33e0..bdb85b57d 100644 --- a/tests/Meziantou.Analyzer.Test/Rules/UseHasFlagMethodAnalyzerTests.cs +++ b/tests/Meziantou.Analyzer.Test/Rules/UseHasFlagMethodAnalyzerTests.cs @@ -963,6 +963,250 @@ class Sample return test.RunAsync(); } + [Fact] + public Task SideEffectBeforeFlagRead_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + private MyEnum _comparand = MyEnum.Flag1; + + MyEnum Change() + { + _comparand = MyEnum.Flag2; + return MyEnum.Flag2; + } + + bool M() => (_comparand & Change()) == _comparand; + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task SideEffectAfterComparedOperandRead_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + private MyEnum _comparand = MyEnum.Flag1; + + MyEnum Change() + { + _comparand = MyEnum.Flag2; + return MyEnum.Flag2; + } + + bool M() => _comparand == (Change() & _comparand); + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task SideEffectBeforeFlagRead_ReversedAndOperands_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + private MyEnum _comparand = MyEnum.Flag1; + + MyEnum Change() + { + _comparand = MyEnum.Flag2; + return MyEnum.Flag2; + } + + bool M() => _comparand == (_comparand & Change()); + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task SideEffectWithPropertyFlag_NoDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + private MyEnum _comparand = MyEnum.Flag1; + + MyEnum Value => _comparand; + + bool M(MyEnum comparand) => comparand == (Value & comparand); + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task SideEffectBeforeConstantFlagRead_ReportDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + MyEnum Change() => MyEnum.Flag2; + + bool M() => {|MA0192:MyEnum.Flag1 == (Change() & MyEnum.Flag1)|}; + } + """; + test.FixedCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + MyEnum Change() => MyEnum.Flag2; + + bool M() => Change().HasFlag(MyEnum.Flag1); + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task SideEffectAfterFlagRead_ReportDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + private MyEnum _comparand = MyEnum.Flag1; + + MyEnum Change() + { + _comparand = MyEnum.Flag2; + return MyEnum.Flag2; + } + + bool M() => {|MA0192:(Change() & _comparand) == _comparand|}; + } + """; + test.FixedCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + private MyEnum _comparand = MyEnum.Flag1; + + MyEnum Change() + { + _comparand = MyEnum.Flag2; + return MyEnum.Flag2; + } + + bool M() => Change().HasFlag(_comparand); + } + """; + + return test.RunAsync(); + } + + [Fact] + public Task SideEffectFreeValue_ReversedAndOperands_ReportDiagnostic() + { + var test = CreateTest(); + test.TestCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + bool M(MyEnum value, MyEnum comparand) => {|MA0192:comparand == (comparand & (value | MyEnum.Flag2))|}; + } + """; + test.FixedCode = """ + [System.Flags] + enum MyEnum + { + None = 0, + Flag1 = 1, + Flag2 = 2, + } + + class Sample + { + bool M(MyEnum value, MyEnum comparand) => (value | MyEnum.Flag2).HasFlag(comparand); + } + """; + + return test.RunAsync(); + } + [Theory] [InlineData("sbyte")] [InlineData("byte")]