New Rule S6966: Awaitable method should be used#9179
New Rule S6966: Awaitable method should be used#9179martin-strecker-sonarsource merged 11 commits intomasterfrom
Conversation
|
Performance investigation based on https://docs.google.com/spreadsheets/d/1PcQ8NIEmelvtnyTmQ24wKk8T6LGkp7NZ_LDYiosWBT4/edit?usp=sharing SimplCommerceTPs: S6966: 96 instances Overall contribution: The costs are mostly distributed between speculative binding (64%) and IsAwaitableNonDynamic (22%) |
Northwind TradersTPs: S6966: 3 instances Performance results do not reflect what Peach measured. 80ms total contribution (less than 0.01% of overall runtime): |
Aspnet BoilerplateTPs: S6966: 129 instances Overall (0.03%) In the rule, the main driver are the same as above (speculative binding and IsAwaitableNonDynamic) |
|
Performance gain by b42423c SimplCommerce: The IsAwaitableNonDynamic perf issue is gone. |
|
Performance improvement by 8047ff0 SimpleCommerce |
|
Results after the two improvements:
|
martin-strecker-sonarsource
force-pushed
the
revert-9178-revert-9101-Martin/S6966
branch
from
e2d5008 to
8047ff0
Compare
zsolt-kolbay-sonarsource
left a comment
zsolt-kolbay-sonarsource
left a comment
There was a problem hiding this comment.
Some polishing comments
| var wellKnownExtensionMethodContainer = new WellKnownExtensionMethodContainer(); | ||
| var queryable = compilation.GetTypeByMetadataName(KnownType.System_Linq_Queryable); | ||
| var enumerable = compilation.GetTypeByMetadataName(KnownType.System_Linq_Enumerable); | ||
| if (queryable is not null && enumerable is not null) |
There was a problem hiding this comment.
This check seems unnecessary. These types are available in all supported versions of .NET.
There was a problem hiding this comment.
Better safe than sorry. GetTypeByMetadataName may return null.
| InvocationExpressionSyntax invocationExpression, SemanticModel semanticModel, ISymbol containingSymbol, CancellationToken cancel) | ||
| { | ||
| var awaitableRoot = GetAwaitableRootOfInvocation(invocationExpression); | ||
| if (awaitableRoot is { Parent: AwaitExpressionSyntax }) |
There was a problem hiding this comment.
The two if statements can be merged, they return the same value.
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
|
|
||
| public class C |
There was a problem hiding this comment.
Another test case to consider: when the async method has the same parameter + a CancellationToken.
void Foo(int arg) { }
Task FooAsync(int arg, CancellationToken token) => Task.CompletedTask;
...
async Task Bar()
{
Foo(); // Noncompliant
}| { | ||
| return ImmutableArray<ISymbol>.Empty; // Not in an async scope | ||
| } | ||
| if (semanticModel.GetSymbolInfo(invocationExpression, cancel).Symbol is IMethodSymbol { MethodKind: not MethodKind.DelegateInvoke } methodSymbol |
There was a problem hiding this comment.
Can this be cached with a ConcurrentDictionary<IMethodSymbol, ImmutableArray<ISymbol>> to avoid further calls if the method was checked by the analyzer before?
There was a problem hiding this comment.
As discussed offline, this cache would bring little benefit based on the current performance measures.
…)" This reverts commit 3adb12e.
martin-strecker-sonarsource
force-pushed
the
revert-9178-revert-9101-Martin/S6966
branch
from
a18bbba to
ec32e36
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
martin-strecker-sonarsource
deleted the
revert-9178-revert-9101-Martin/S6966
branch
|
Peach validations FPs
Statistics:
|
2 participants
Fixes #9096
Reverts #9178 which reverted #9101
FPs:
See also #9101 (comment) for FPs
The last FP is non-fixable.