Skip to content

Do not merge MA0148/MA0149 comparisons when the operand can change between evaluations - #1466

Merged
meziantou merged 1 commit into
mainfrom
feature/ma0148-repeated-calls-5204a0
Sep 12, 2026
Merged

meziantou merged 1 commit into
mainfrom
feature/ma0148-repeated-calls-5204a0

Conversation

@meziantou

Copy link
Copy Markdown
Owner

Problem

The MA0148/MA0149 code fix merges adjacent comparisons into a single pattern when their operands are syntactically equivalent. Syntax equivalence does not guarantee the operand returns the same value or has no side effect:

static int count;
static int Next() => ++count;
public static object Run() => Next() == 0 || Next() == 2;

was fixed as Next() is 0 or 2. The original calls Next() twice (returns true), the fixed code calls it once (returns false). The same issue applied to != / && merged into is not (… or …).

Fix

The comparisons are merged only when the operand is stable, i.e. evaluating it again has no side effect and returns the same value:

  • constants, locals, parameters, this
  • non-volatile fields, static or on a stable instance
  • built-in conversions (no user-defined operator) of a stable operand

Other operands (method calls, properties, indexers, …) are still converted to patterns, but kept as separate comparisons: Next() is 0 || Next() is 2, Next() is not 0 && Next() is not 2.

Notes for reviewers

  • Behavior change: x.Length == 0 || x.Length == 1 is no longer merged into x.Length is 0 or 1, as a property getter can execute any code.
  • Non-volatile fields are still merged, consistent with UseHasFlagMethodCommon.AreEquivalentOperands.
  • docs/Rules/MA0148.md and MA0149.md document when the fix merges comparisons.

Tests

Added tests for method calls (==/|| and !=/&&), a property, and a volatile field (not merged), and a static field (merged). The 4 "do not merge" tests fail without the fix. The UsePatternMatchingForEqualityComparisonsAnalyzer* tests (37) pass on Roslyn 4.8, 4.14, 5.0, 5.6 and 5.9.

The code fix merged adjacent comparisons into a single pattern as soon as
their operands were syntactically equivalent. `Next() == 0 || Next() == 2`
became `Next() is 0 or 2`, which calls `Next()` once instead of twice and
changes the result.

The comparisons are now merged only when the operand is a constant, a
local, a parameter, `this`, or a non-volatile field of such a value
(optionally through a built-in conversion). Other operands, such as method
calls, properties and indexers, are converted to separate patterns.
@meziantou
meziantou merged commit 78ac410 into main Sep 12, 2026
13 checks passed
@meziantou
meziantou deleted the feature/ma0148-repeated-calls-5204a0 branch September 12, 2026 01:52
This was referenced Sep 12, 2026
This was referenced Sep 17, 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