Skip to content

Add analyzer for ignoring string method return values (MSTEST0055)#6482

Merged
Youssef1313 merged 7 commits intomainfrom
copilot/fix-6213
Sep 2, 2025
Merged

Add analyzer for ignoring string method return values (MSTEST0055)#6482
Youssef1313 merged 7 commits intomainfrom
copilot/fix-6213

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Sep 1, 2025

  • Add analyzer for ignoring string method return values (MSTEST0055)
  • Address review feedback: Use GetSpecialType instead of TryGetOrCreateTypeByMetadataName for System.String

Changes Made

Initial Implementation:

  • Added IgnoreStringMethodReturnValueAnalyzer that warns when return values of string.Contains(), string.StartsWith(), or string.EndsWith() are ignored
  • Added diagnostic ID MSTEST0055 with appropriate localization resources
  • Added comprehensive test coverage for various scenarios

Code Review Updates:

  • Replaced context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemString, out INamedTypeSymbol? stringSymbol) with context.Compilation.GetSpecialType(SpecialType.System_String)
  • Removed unnecessary null checking since GetSpecialType always returns a valid symbol for core types like System.String
  • Removed unused using MSTest.Analyzers.Helpers; import
  • This follows the established pattern in the codebase and is more efficient

Fixes #6213


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Copilot AI changed the title [WIP] Add analyzer for ignoring Contains, StartsWith, EndsWith returns Add analyzer for ignoring string method return values (MSTEST0055) Sep 1, 2025
Copilot AI requested a review from Youssef1313 September 1, 2025 12:57
…etadataName

Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
@drieseng
Copy link
Copy Markdown

drieseng commented Sep 1, 2025

@Youssef1313: I must be missing something here, but what does this have to do with MSTest?

There's already an IDE0058 Roslyn rule that will report a diagnostic if the return value of a method is not "consumed".
Anyway, this MSTEST0055 rule is not specific to MSTest at all.

@Youssef1313
Copy link
Copy Markdown
Member

@drieseng We have seen this pattern come up few times where people intended to do "assertion" but ended up just calling string method and ignoring the result. This rule is different from any existing IDExxx or CAxxxx in that it's:

  1. warning and enabled by default
  2. very specific to cases that are more relevant for testing (common mistake we saw) which is very unlikely to have false positives.

@Youssef1313 Youssef1313 marked this pull request as ready for review September 1, 2025 15:31
Copy link
Copy Markdown
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the sake of completeness I would have added a few tests with Assert.IsTrue, Assert.IsFalse but that's optional.

@Youssef1313
Copy link
Copy Markdown
Member

IsTrue/IsFalse are irrelevant for this analyzer (analyzer doesn't handle anything related to them).

@Youssef1313 Youssef1313 merged commit dbe6640 into main Sep 2, 2025
10 checks passed
@Youssef1313 Youssef1313 deleted the copilot/fix-6213 branch September 2, 2025 07:27
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.

Add analyzer for ignoring Contains, StartsWith, EndsWith returns

4 participants