Skip to content

Report MA0179 when the constant is on the left of the length comparison - #1512

Merged
meziantou merged 1 commit into
mainfrom
feature/ma0179-length-comparison-branch-79af79
Sep 12, 2026
Merged

meziantou merged 1 commit into
mainfrom
feature/ma0179-length-comparison-branch-79af79

Conversation

@meziantou

Copy link
Copy Markdown
Owner

What

UseAttributeIsDefinedAnalyzer (MA0179) only detected GetCustomAttributes().Length / .Count() existence checks when the length was the left operand. The reversed form was silently missed:

member.GetCustomAttributes(typeof(ObsoleteAttribute), false).Length > 0  // MA0179 reported
0 < member.GetCustomAttributes(typeof(ObsoleteAttribute), false).Length  // no diagnostic

Why

IsValidLengthComparisonPattern takes a lengthIsOnLeft flag and carefully implements both orientations. But both call sites hard-coded lengthIsOnLeft: true and only ever passed (operation.LeftOperand, operation.RightOperand) in that order, so the entire else block was unreachable dead code.

IsGetCustomAttributeComparison (the == null variant) already handles both orientations, which makes the asymmetry an oversight rather than a deliberate scope decision — so this fixes it rather than deleting the branch.

Changes

  • AnalyzeBinary now tries both operand orders for the Length and Count() comparisons, passing the orientation through to IsValidLengthComparisonPattern instead of hard-coding true.
  • Renamed the left/right parameters of the two helpers to lengthOperand/countOperand and otherOperand, since they no longer correspond to syntactic sides.
  • Documented the reversed form in docs/Rules/MA0179.md.

Note for reviewers

The code fixer needed no change. ShouldNegateLengthComparison already implemented both orientations and already searched both operands — it was dead in exactly the same way as the analyzer's else block, and simply becomes live now. That is why the diff touches only the analyzer.

Tests

15 new cases in UseAttributeIsDefinedAnalyzerTests, each asserting the diagnostic and the fixed code, so the negation direction is verified rather than just detection:

  • 6 reversed Length forms added to the existing theory: 0 == len → !IsDefined, 0 != len → IsDefined, 0 < len → IsDefined, 1 <= len → IsDefined, 1 > len → !IsDefined, 0 >= len → !IsDefined
  • 6 equivalent reversed Count() forms
  • 3 negative cases confirming ambiguous reversed comparisons stay unreported (0 > len, 1 == len, 2 <= len)

Verification

  • dotnet test --max-parallel-test-modules 2 across all five Roslyn versions: 22751 passed, 0 failed.
  • Confirmed the new tests actually ran by checking the test names in the .trx output rather than relying on the pass count.
  • dotnet run --project src/DocumentationGenerator exits 0 with no further markdown changes.

IsValidLengthComparisonPattern implements both orientations of the
comparison, but the two call sites hard-coded lengthIsOnLeft: true and
only ever passed (LeftOperand, RightOperand), so its else block was
unreachable and the reversed form was never reported:

    member.GetCustomAttributes(typeof(ObsoleteAttribute), false).Length > 0 // reported
    0 < member.GetCustomAttributes(typeof(ObsoleteAttribute), false).Length // not reported

The null comparison already handles both orientations, so the asymmetry
was an oversight. Try both operand orders and pass the flag through.

The code fixer needed no change: ShouldNegateLengthComparison already
implemented both orientations and searched both operands, and was dead
in the same way.
@meziantou
meziantou merged commit 1a4c353 into main Sep 12, 2026
13 checks passed
@meziantou
meziantou deleted the feature/ma0179-length-comparison-branch-79af79 branch September 12, 2026 20:18
This was referenced Sep 12, 2026
This was referenced Oct 1, 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