-
Notifications
You must be signed in to change notification settings - Fork 290
Analyzer implementation to enhance assertions #6720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1ea58a1
1ea6cac
2570d48
fc96809
d79d506
ff88554
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -125,6 +125,19 @@ private enum CountCheckStatus | |||||||||||||||||
| HasCount, | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private enum LinqPredicateCheckStatus | ||||||||||||||||||
| { | ||||||||||||||||||
| Unknown, | ||||||||||||||||||
| Any, | ||||||||||||||||||
| Count, | ||||||||||||||||||
| WhereAny, | ||||||||||||||||||
| WhereCount, | ||||||||||||||||||
| Single, | ||||||||||||||||||
| SingleOrDefault, | ||||||||||||||||||
| WhereSingle, | ||||||||||||||||||
| WhereSingleOrDefault, | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| internal const string ProperAssertMethodNameKey = nameof(ProperAssertMethodNameKey); | ||||||||||||||||||
|
|
||||||||||||||||||
| /// <summary> | ||||||||||||||||||
|
|
@@ -268,6 +281,56 @@ private static void AnalyzeInvocationOperation(OperationAnalysisContext context, | |||||||||||||||||
| case "AreNotEqual": | ||||||||||||||||||
| AnalyzeAreEqualOrAreNotEqualInvocation(context, firstArgument, isAreEqualInvocation: false, objectTypeSymbol); | ||||||||||||||||||
| break; | ||||||||||||||||||
| case "IsNull": | ||||||||||||||||||
| AnalyzeIsNullOrIsNotNullInvocation(context, firstArgument, isNullCheck: true); | ||||||||||||||||||
| break; | ||||||||||||||||||
|
|
||||||||||||||||||
| case "IsNotNull": | ||||||||||||||||||
| AnalyzeIsNullOrIsNotNullInvocation(context, firstArgument, isNullCheck: false); | ||||||||||||||||||
| break; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private static void AnalyzeIsNullOrIsNotNullInvocation(OperationAnalysisContext context, IOperation argument, bool isNullCheck) | ||||||||||||||||||
| { | ||||||||||||||||||
| RoslynDebug.Assert(context.Operation is IInvocationOperation, "Expected IInvocationOperation."); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Check for Single/SingleOrDefault patterns | ||||||||||||||||||
| LinqPredicateCheckStatus linqStatus = RecognizeLinqPredicateCheck( | ||||||||||||||||||
| argument, | ||||||||||||||||||
| out SyntaxNode? linqCollectionExpr, | ||||||||||||||||||
| out SyntaxNode? predicateExpr, | ||||||||||||||||||
| out _); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (linqStatus is LinqPredicateCheckStatus.Single or | ||||||||||||||||||
| LinqPredicateCheckStatus.SingleOrDefault or | ||||||||||||||||||
| LinqPredicateCheckStatus.WhereSingle or | ||||||||||||||||||
| LinqPredicateCheckStatus.WhereSingleOrDefault && | ||||||||||||||||||
|
Comment on lines
+305
to
+308
|
||||||||||||||||||
| if (linqStatus is LinqPredicateCheckStatus.Single or | |
| LinqPredicateCheckStatus.SingleOrDefault or | |
| LinqPredicateCheckStatus.WhereSingle or | |
| LinqPredicateCheckStatus.WhereSingleOrDefault && | |
| if ((linqStatus is LinqPredicateCheckStatus.Single or | |
| LinqPredicateCheckStatus.SingleOrDefault or | |
| LinqPredicateCheckStatus.WhereSingle or | |
| LinqPredicateCheckStatus.WhereSingleOrDefault) && |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The analyzer implementation has significant code duplication in the RecognizeLinqPredicateCheck method. The patterns for checking Where().Single(), Where().SingleOrDefault(), Where().Any(), and Where().Count() follow nearly identical logic with only method names differing. Consider extracting this repetitive logic into a helper method to improve maintainability.
Example refactoring approach:
private static bool TryMatchWherePattern(
IInvocationOperation invocation,
string methodName,
out SyntaxNode? collectionExpression,
out SyntaxNode? predicateExpression)
{
if (invocation.TargetMethod.Name == methodName &&
invocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" &&
invocation.Arguments.Length == 1 &&
invocation.Arguments[0].Value is IInvocationOperation whereInvocation &&
whereInvocation.TargetMethod.Name == "Where" &&
whereInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" &&
whereInvocation.Arguments.Length == 2)
{
collectionExpression = whereInvocation.Arguments[0].Value.Syntax;
predicateExpression = whereInvocation.Arguments[1].Value.Syntax;
return true;
}
collectionExpression = null;
predicateExpression = null;
return false;
}
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Count pattern checking (lines 860-906) doesn't validate that the Count method is actually from LINQ (System.Linq.Enumerable) or has a predicate. This could lead to false positives with other Count methods (e.g., string.Count from LINQ, or custom Count methods on collections).
The check should verify:
- That it's the LINQ
Countextension method with a predicate - That it's from
System.Linq.Enumerable
Example:
if (countInvocation.TargetMethod.Name == "Count" &&
countInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable")This is especially important since RecognizeLinqPredicateCheck already properly checks for this, so this code should either reuse that method or apply similar validation.
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -84,7 +84,8 @@ private static void ValidateOutputIsNotMixed(IEnumerable<TestResult> testResults | |||||||||||
| Assert.Contains(methodName, message.Text); | ||||||||||||
| Assert.Contains("TestInitialize", message.Text); | ||||||||||||
| Assert.Contains("TestCleanup", message.Text); | ||||||||||||
| Assert.IsFalse(shouldNotContain.Any(message.Text.Contains)); | ||||||||||||
| // Assert.IsFalse(shouldNotContain.Any(message.Text.Contains)); | ||||||||||||
| Assert.DoesNotContain(message.Text.Contains, shouldNotContain); | ||||||||||||
|
||||||||||||
| Assert.DoesNotContain(message.Text.Contains, shouldNotContain); | |
| foreach (string item in shouldNotContain) | |
| { | |
| Assert.DoesNotContain(item, message.Text); | |
| } |
Uh oh!
There was an error while loading. Please reload this page.