Fix MSTEST0049 false positives inside expression trees (#7585)#7592
Fix MSTEST0049 false positives inside expression trees (#7585)#7592Evangelink wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the MSTEST0049 analyzer to avoid reporting diagnostics for invocations that occur inside expression trees (e.g., Moq Setup lambdas), where applying the code fix would be inappropriate.
Changes:
- Add expression-tree detection (
IsInsideExpressionTree) to suppress MSTEST0049 diagnostics when the invocation is within anExpression<T>lambda. - Extend
WellKnownTypeNameswithSystem.Linq.Expressions.LambdaExpressionfor symbol lookup. - Add unit tests covering suppression inside expression trees and ensuring normal lambdas still produce diagnostics/fixes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/UnitTests/MSTest.Analyzers.UnitTests/FlowTestContextCancellationTokenAnalyzerTests.cs | Adds new tests validating expression-tree suppression and non-expression lambda behavior. |
| src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs | Adds a well-known type name constant for LambdaExpression. |
| src/Analyzers/MSTest.Analyzers/FlowTestContextCancellationTokenAnalyzer.cs | Introduces expression-tree detection and skips MSTEST0049 reporting within expression trees. |
src/Analyzers/MSTest.Analyzers/FlowTestContextCancellationTokenAnalyzer.cs
Outdated
Show resolved
Hide resolved
Skip reporting MSTEST0049 diagnostics when the invocation is inside an expression tree (e.g. Moq Setup lambdas), where the code fix cannot be meaningfully applied. - Add IsInsideExpressionTree helper that walks up the operation tree to detect lambdas converted to Expression<T> - Add SystemLinqExpressionsLambdaExpression to WellKnownTypeNames - Add 3 tests covering expression tree suppression and regular lambda non-suppression
Evangelink
force-pushed
the
fix/mstest0049-expression-tree-false-positives
branch
from
3663f68 to
31a2317
Compare
| IOperation? current = operation.Parent; | ||
| while (current is not null) | ||
| { | ||
| if (current is IAnonymousFunctionOperation) |
There was a problem hiding this comment.
Looks like the implementation in SDK (moved from roslyn-analyzers) walks up to either LocalFunction or AnonymousFunction?
| if (current is IAnonymousFunctionOperation) | ||
| { | ||
| // Check if the parent converts this lambda to an Expression<T>. | ||
| if (current.Parent is IConversionOperation conversion && |
There was a problem hiding this comment.
Implementation referenced in my earlier comment doesn't check IConversionOperation, it just accesses Type right away.
| // Check if the parent converts this lambda to an Expression<T>. | ||
| if (current.Parent is IConversionOperation conversion && | ||
| conversion.Type is INamedTypeSymbol namedType && | ||
| namedType.Inherits(lambdaExpressionSymbol)) |
There was a problem hiding this comment.
No personal preference for me. But implementation from sdk uses Expression<T> instead of LambdaExpression, and checks for exact type instead of inheritance. I think today, Expression<T> is the only known type that inherits LambdaExpression.
| if (current is IAnonymousFunctionOperation) | ||
| { | ||
| // Check if the parent converts this lambda to an Expression<T>. | ||
| if (current.Parent is IConversionOperation conversion && | ||
| conversion.Type is INamedTypeSymbol namedType && | ||
| namedType.Inherits(lambdaExpressionSymbol)) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
We should probably stop walking up once we find anonymous function.
Youssef1313
left a comment
Youssef1313
left a comment
There was a problem hiding this comment.
Approving with few comments
Skip reporting MSTEST0049 diagnostics when the invocation is inside an expression tree (e.g. Moq Setup lambdas), where the code fix cannot be meaningfully applied.
Fixes #7585