Skip to content

Do not report MA0192 when the HasFlag rewrite reorders an effectful operand - #1442

Merged
meziantou merged 1 commit into
mainfrom
feature/hasflag-conversion-reordering-790878
Sep 8, 2026
Merged

meziantou merged 1 commit into
mainfrom
feature/hasflag-conversion-reordering-790878

Conversation

@meziantou

Copy link
Copy Markdown
Owner

Problem

value.HasFlag(flag) evaluates the value operand before the flag operand, and drops the duplicated read of the flag. AreEquivalentOperands only proves that both flag reads target the same storage (parameter, local, field) — not that the value is stable across the evaluation of the other operand.

So MA0192 reported, and its fixer actually applied, an unsound rewrite:

enum E { A = 1, B = 2 }
class C
{
    E flag = E.A;
    E Change() { flag = E.B; return E.B; }

    bool M() => (flag & Change()) == flag;   // false: flag is read as A, then as B
}

becomes

bool M() => Change().HasFlag(flag);          // true: Change() runs before flag is read

Both versions compile, so the fix silently changes behaviour.

Fix

The pattern is now gated on UseHasFlagMethodCommon.CanRewriteToHasFlag, used by both the analyzer and the fixer. The rewrite is accepted only when one of these holds:

  • the flag is a compile-time constant — its reads are stable regardless of ordering;
  • the value operand is already evaluated before every read of the flag. That is only (value & flag) == flag / (value & flag) is …, where the & is the first-evaluated side of the comparison and the flag is the right & operand;
  • the value operand is side-effect free: constants, parameter / local / this / field references, and built-in conversions and unary/binary operators over those.

To support that, TryGetEnumFlagReference gained an isConstantFlag output and GetFromBitwiseAnd a comparedOperandEvaluatedFirst parameter, so comparison operand reversal (flag == (… & …)) is covered as well as bitwise operand reversal ((flag & value)). The three unsafe shapes — (flag & value) == flag, flag == (value & flag) and flag == (flag & value) — now require a pure value operand.

Constant-flag checks ((Change() & E.A) == E.A, the == 0 / != 0 forms, the is patterns) are unaffected, since reading a constant cannot observe a mutation.

Notes for the reviewer

  • Deliberate trade-off: a property getter as the value operand is treated as effectful, so comparand == (Value & comparand) is no longer reported. That is a false negative when the getter is pure, but proving getter purity is not feasible here and the shape is unusual. False negatives are the safe side for a rule whose fixer rewrites code.
  • IParenthesizedOperation is not part of the purity walk — the C# compiler does not produce it.
  • docs/Rules/MA0192.md documents the new evaluation-order requirement.

Tests

7 tests added to UseHasFlagMethodAnalyzerTests:

  • 4 no-diagnostic: the reported repro, comparison reversal, bitwise reversal, and a property getter as the value operand.
  • 3 still-reported: constant flag with an effectful value, the order-preserving shape with an effectful value, and a pure compound value operand (value | E.Flag2) in a reversed shape.

I verified the 4 negative tests genuinely cover the bug by temporarily stubbing CanRewriteToHasFlag to true: all 4 failed, then passed once restored.

Verification

  • dotnet build — clean.
  • MA0192 test class (49 tests) — passes on roslyn 4.8, 4.14, 5.0, 5.6 and 5.9.
  • Full roslyn5.9 suite — 4187/4187 pass.
  • dotnet run --project src/DocumentationGenerator — exits 0, no further markdown changes.

…perand

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.
@meziantou
meziantou merged commit 7fc1e76 into main Sep 8, 2026
13 checks passed
@meziantou
meziantou deleted the feature/hasflag-conversion-reordering-790878 branch September 8, 2026 17:34
This was referenced Sep 8, 2026
This was referenced Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant