Fix MSTEST0017 false positive with user-defined conversion operators

[Copilot]

MSTEST0017 incorrectly flagged assertions where the actual argument used a user-defined conversion operator applied to a literal/constant (e.g., (Foo)"Hello"), because the analyzer walked through the conversion to the underlying constant and treated the whole expression as a constant value.

// Previously incorrectly flagged as swapped arguments
Assert.AreEqual(new Foo("Hello"), (Foo)"Hello");

Changes

• AssertionArgsShouldBePassedInCorrectOrderAnalyzer: IsConstant now uses WalkDownBuiltInConversion instead of WalkDownConversion, stopping at user-defined conversions rather than walking through them to the underlying operand.

• IOperationExtensions: Added WalkDownBuiltInConversion — like WalkDownConversion but stops when IConversionOperation.Conversion.IsUserDefined is true. Built-in/implicit conversions (e.g., (long)42) still walk through as before.

• Tests: Added UserDefinedConversionOperator_ShouldNotFlag covering the exact scenario from the bug report.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• x3yvsblobprodcus370.vsblob.vsassets.io
  • Triggering command: /home/REDACTED/work/testfx/testfx/artifacts/bin/MSTest.Analyzers.UnitTests/Release/net8.0/MSTest.Analyzers.UnitTests /home/REDACTED/work/testfx/testfx/artifacts/bin/MSTest.Analyzers.UnitTests/Release/net8.0/MSTest.Analyzers.UnitTests --diagnostic --diagnostic-output-directory /home/REDACTED/work/testfx/testfx/artifacts/log/Release --diagnostic-file-prefix MSTest.Analyzers.UnitTests_net8.0_Release_x64 --diagnostic-verbosity trace --crashdump --hangdump --hangdump-timeout 15m --report-azdo --coverage --coverage-settings /home/REDACTED/work/testfx/testfx/test/coverage.config --coverage-output MSTest.Analyzers.UnitTests_net8.0_Release_x64.coverage --filter AssertionArgsShouldBePassedInCorrectOrderAnalyzerTests (dns block)
  • Triggering command: /home/REDACTED/work/testfx/testfx/artifacts/bin/MSTest.Analyzers.UnitTests/Release/net8.0/MSTest.Analyzers.UnitTests /home/REDACTED/work/testfx/testfx/artifacts/bin/MSTest.Analyzers.UnitTests/Release/net8.0/MSTest.Analyzers.UnitTests --diagnostic --diagnostic-output-directory /home/REDACTED/work/testfx/testfx/artifacts/log/Release --diagnostic-file-prefix MSTest.Analyzers.UnitTests_net8.0_Release_x64 --diagnostic-verbosity trace --crashdump --hangdump --hangdump-timeout 15m --report-azdo --coverage --coverage-settings /home/REDACTED/work/testfx/testfx/test/coverage.config --coverage-output MSTest.Analyzers.UnitTests_net8.0_Release_x64.coverage --filter AssertionArgsShouldBePassedInCorrectOrderAnalyzerTests.UserDefinedConversionOperator_ShouldNotFlag (dns block)
  • Triggering command: /home/REDACTED/work/testfx/testfx/artifacts/bin/MSTest.Analyzers.UnitTests/Release/net8.0/MSTest.Analyzers.UnitTests /home/REDACTED/work/testfx/testfx/artifacts/bin/MSTest.Analyzers.UnitTests/Release/net8.0/MSTest.Analyzers.UnitTests --diagnostic --diagnostic-output-directory /home/REDACTED/work/testfx/testfx/artifacts/log/Release --diagnostic-file-prefix MSTest.Analyzers.UnitTests_net8.0_Release_x64 --diagnostic-verbosity trace --crashdump --hangdump --hangdump-timeout 15m --report-azdo --coverage --coverage-settings /home/REDACTED/work/testfx/testfx/test/coverage.config --coverage-output MSTest.Analyzers.UnitTests_net8.0_Release_x64.coverage --filter AssertionArgsShouldBePassedInCorrectOrderAnalyzerTests.WhenUsingLiterals (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[Evangelink]

Code Review — MSTEST0017 false positive with user-defined conversion operators

Summary

Good targeted fix. The root cause is correctly identified: WalkDownConversion walks through user-defined conversions to the underlying constant operand, causing IsConstant to treat (Foo)"Hello" as a constant "Hello". The fix introduces WalkDownBuiltInConversion that stops at user-defined conversions.

Correctness ✅

The fix is correct. The new WalkDownBuiltInConversion method properly stops when IConversionOperation.Conversion.IsUserDefined is true, which correctly handles:

• Built-in implicit conversions (e.g., int → long): walks through ✓
• Built-in explicit conversions (e.g., long → int): walks through ✓
• User-defined implicit conversions: stops ✓
• User-defined explicit conversions: stops ✓

A user-defined conversion is semantically a method call that can execute arbitrary logic, so its result should never be treated as a compile-time constant. This is the right distinction.

Scope ✅

The change is appropriately scoped — only IsConstant in AssertionArgsShouldBePassedInCorrectOrderAnalyzer is modified. Other analyzers use WalkDownConversion for different purposes (type extraction, pattern matching against null literals) where walking through user-defined conversions is either correct or a separate concern.

That said, PreferAssertFailOverAlwaysFalseConditionsAnalyzer and ReviewAlwaysTrueAssertConditionAnalyzer both use WalkDownConversion followed by ConstantValue.HasValue, which could in theory also have false positives with user-defined conversions (e.g., Assert.AreEqual((Foo)"Hello", (Foo)"Hello") being flagged as always-true). This is a separate bug if it exists, and not in scope for this PR, but worth tracking.

Issues

1. XML doc clarity (nit) — the doc comment on WalkDownBuiltInConversion uses double-negation that's hard to parse. See inline comment.

2. Test coverage gap — the test only covers explicit user-defined conversion ((Foo)"Hello"). An implicit user-defined conversion case and a chained conversion case (built-in wrapping user-defined, e.g. (object)(Foo)"Hello") would strengthen confidence.

3. No negative test — consider adding a test that ensures the analyzer still does flag swapped arguments when built-in casts are used, e.g. Assert.AreEqual(s, (long)42) should still be flagged. This proves WalkDownBuiltInConversion correctly walks through built-in conversions. (Existing tests may already cover this implicitly, but an explicit case would make the distinction clearer.)

Overall

The core fix is sound and well-motivated. The inline suggestions are minor polish.

[Copilot]

Pull request overview

Fixes an MSTEST0017 analyzer false-positive where user-defined conversion operators applied to constants/literals were being treated as constants during argument-order detection.

Changes:

• Update constant-detection logic to stop walking through user-defined conversions.
• Add WalkDownBuiltInConversion helper for unwrapping only built-in conversions.
• Add unit tests covering explicit/implicit user-defined conversions and ensuring built-in casts on literals still behave as before.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
test/UnitTests/MSTest.Analyzers.UnitTests/AssertionArgsShouldBePassedInCorrectOrderAnalyzerTests.cs	Adds regression coverage for user-defined conversion scenarios and ensures built-in literal casts are still flagged.
src/Analyzers/MSTest.Analyzers/RoslynAnalyzerHelpers/IOperationExtensions.cs	Introduces WalkDownBuiltInConversion to unwrap built-in conversions while stopping at user-defined ones.
src/Analyzers/MSTest.Analyzers/AssertionArgsShouldBePassedInCorrectOrderAnalyzer.cs	Switches IsConstant to use the new built-in-only conversion walker to avoid the false positive.

Comments suppressed due to low confidence (1)

test/UnitTests/MSTest.Analyzers.UnitTests/AssertionArgsShouldBePassedInCorrectOrderAnalyzerTests.cs:1072

• The _value field in this test snippet is assigned but never read, which will produce CS0414 in the compiled test input. Consider removing the field or using it (e.g., in an Equals override) so the snippet stays warning-free.

                private sealed class Foo
                {
                    private readonly string _value;
                    public Foo(string value) { _value = value; }