Skip to content

Commit fcb411a

Browse files
AArnottCopilot
andauthored
Fix VSTHRD002 completion analysis and extensibility (#1648)
* Fix VSTHRD002 completion analysis and extensibility Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address VSTHRD002 review feedback Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix code fix analyzer style violations Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Handle parenthesized awaiters in continuations Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden VSTHRD002 completion proofs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Track VSTHRD002 ref aliases Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Refine VSTHRD002 control flow proofs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Complete VSTHRD002 alias analysis Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden completion branches and awaiter fixes Recognize nested conditional branches that definitely await the same task, and suppress invalid await code fixes for unrelated static GetAwaiter factories. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Make VSTHRD002 alias analysis flow-aware Separate definite ref aliases used for completion proofs from potential aliases used for reassignment invalidation, and include closure writes from accessors and top-level statements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden deferred VSTHRD002 analysis Include continuation-scope aliases in deferred-write checks, validate awaiter chains semantically, preserve closure ordering, recognize WhenAll arrays, and avoid configured-method duplicates with VSTHRD103. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Respect VSTHRD103 exclusions in VSTHRD002 Keep configured blockers diagnosed when VSTHRD103 is excluded, and harden completion proofs for guarded conditions, nested closures, nested guards, conditional awaits, and custom ConfigureAwait methods. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden VSTHRD002 review edge cases Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Restrict await fixes to convertible methods Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Align completion proofs across analyzers Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Handle completed waits and forbidden awaits Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Avoid await fixes for ref-like signatures Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden VSTHRD002 completion analysis Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Guard await fixes against delegate references Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Reject by-ref parameter completion proofs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden VSTHRD002 flow and code fixes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Validate async alternative applicability Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden VSTHRD002 contract conversion Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Cover additional VSTHRD002 flow cases Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden VSTHRD002 flow analysis Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden VSTHRD002 caller conversion Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2 parents b1a9eb8 + 2e2d14e commit fcb411a

11 files changed

Lines changed: 4518 additions & 330 deletions

File tree

docfx/analyzers/VSTHRD002.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,14 @@ void DoSomething()
4040
}
4141
```
4242

43+
Accessing a task's result is not reported when the analyzer can prove the task has completed.
44+
Recognized proofs include awaiting the task (directly or through `Task.WhenAll`), guarding the
45+
access with a completion property such as `IsCompletedSuccessfully`, and awaiting the task in
46+
the negative branch of such a guard.
47+
48+
VSTHRD002 can also report project-specific synchronous blocking methods configured in
49+
`vs-threading.SyncBlockingMethods.txt`. See [Analyzer Configuration](configuration.md#additional-synchronous-blocking-methods-for-vsthrd002).
50+
4351
Refer to [Asynchronous and multithreaded programming within VS using the JoinableTaskFactory][1] for more information.
4452

4553
[1]: https://devblogs.microsoft.com/premier-developer/asynchronous-and-multithreaded-programming-within-vs-using-the-joinabletaskfactory/

docfx/analyzers/configuration.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,19 @@ excluded from VSTHRD103 analysis by specifying them in a configuration file.
105105

106106
**Generic sample:** ``[Microsoft.EntityFrameworkCore.DbSet`1]::Add``
107107

108+
## Additional synchronous blocking methods for VSTHRD002
109+
110+
Projects that wrap synchronous waits in their own APIs can configure those methods to be
111+
reported by VSTHRD002. Instance, static, and extension methods are supported. Because the
112+
analyzer cannot infer an asynchronous equivalent for a configured method, it does not offer
113+
the "use await instead" code fix for these diagnostics.
114+
115+
**Filename:** `vs-threading.SyncBlockingMethods.txt`
116+
117+
**Line format:** `[Namespace.TypeName]::MethodName`
118+
119+
**Sample:** `[Contoso.Threading.TaskExtensions]::WaitSynchronously`
120+
108121
## Types that require the Async suffix
109122

110123
VSTHRD200 requires methods returning `Task`, `ValueTask`, and other async-focused types

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/CSharpCommonInterest.cs

Lines changed: 1312 additions & 98 deletions
Large diffs are not rendered by default.

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/CSharpUtils.cs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -208,10 +208,9 @@ public static MemberAccessExpressionSyntax MemberAccess(IReadOnlyList<string> qu
208208
/// </summary>
209209
public static bool IsWithinNameOf([NotNullWhen(true)] SyntaxNode? syntaxNode)
210210
{
211-
InvocationExpressionSyntax? invocation = syntaxNode?.FirstAncestorOrSelf<InvocationExpressionSyntax>();
212-
return invocation is object
213-
&& (invocation.Expression as IdentifierNameSyntax)?.Identifier.Text == "nameof"
214-
&& invocation.ArgumentList.Arguments.Count == 1;
211+
return syntaxNode?.AncestorsAndSelf().OfType<InvocationExpressionSyntax>().Any(
212+
invocation => (invocation.Expression as IdentifierNameSyntax)?.Identifier.Text == "nameof"
213+
&& invocation.ArgumentList.Arguments.Count == 1) is true;
215214
}
216215

217216
public override Location? GetLocationOfBaseTypeName(INamedTypeSymbol symbol, INamedTypeSymbol baseType, Compilation compilation, CancellationToken cancellationToken)

src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/VSTHRD002UseJtfRunAnalyzer.cs

Lines changed: 126 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.Collections.Immutable;
77
using System.Linq;
8+
using System.Text.RegularExpressions;
89
using System.Threading;
910
using System.Threading.Tasks;
1011
using Microsoft.CodeAnalysis;
@@ -61,7 +62,15 @@ public override void Initialize(AnalysisContext context)
6162
context.RegisterCompilationStartAction(compilationContext =>
6263
{
6364
INamedTypeSymbol? taskSymbol = compilationContext.Compilation.GetTypeByMetadataName(Types.Task.FullName);
64-
if (taskSymbol is object)
65+
ImmutableArray<CommonInterest.QualifiedMember> configuredSyncBlockingMethods = CommonInterest.ReadMethods(
66+
compilationContext.Options,
67+
new Regex(@"^vs-threading\.SyncBlockingMethods(\..*)?.txt$", RegexOptions.IgnoreCase | RegexOptions.Singleline),
68+
compilationContext.CancellationToken).ToImmutableArray();
69+
ImmutableArray<CommonInterest.QualifiedMember> methodsExcludedFromVSTHRD103 = CommonInterest.ReadMethods(
70+
compilationContext.Options,
71+
CommonInterest.FileNamePatternForSyncMethodsToExcludeFromVSTHRD103,
72+
compilationContext.CancellationToken).ToImmutableArray();
73+
if (taskSymbol is object || !configuredSyncBlockingMethods.IsEmpty)
6574
{
6675
compilationContext.RegisterCodeBlockStartAction<SyntaxKind>(codeBlockContext =>
6776
{
@@ -70,8 +79,13 @@ public override void Initialize(AnalysisContext context)
7079
if (propertySymbol is object || methodSymbol is object)
7180
{
7281
bool analyzeWholeCodeBlock = propertySymbol is object || !methodSymbol!.HasAsyncCompatibleReturnType();
73-
codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeInvocation(c, taskSymbol, analyzeWholeCodeBlock)), SyntaxKind.InvocationExpression);
74-
codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeMemberAccess(c, taskSymbol, analyzeWholeCodeBlock)), SyntaxKind.SimpleMemberAccessExpression);
82+
codeBlockContext.RegisterSyntaxNodeAction(
83+
Utils.DebuggableWrapper(c => AnalyzeInvocation(c, configuredSyncBlockingMethods, methodsExcludedFromVSTHRD103, analyzeWholeCodeBlock, taskSymbol is object)),
84+
SyntaxKind.InvocationExpression);
85+
if (taskSymbol is object)
86+
{
87+
codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeMemberAccess(c, analyzeWholeCodeBlock)), SyntaxKind.SimpleMemberAccessExpression);
88+
}
7589
}
7690
});
7791
}
@@ -98,80 +112,138 @@ private static bool ShouldAnalyze(SyntaxNodeAnalysisContext context, bool analyz
98112
&& !containingMethod.HasAsyncCompatibleReturnType();
99113
}
100114

101-
private static ParameterSyntax? GetFirstParameter(AnonymousFunctionExpressionSyntax? anonymousFunctionSyntax)
115+
private static void InspectMemberAccess(
116+
SyntaxNodeAnalysisContext context,
117+
MemberAccessExpressionSyntax? memberAccessSyntax,
118+
IEnumerable<CommonInterest.SyncBlockingMethod> problematicMethods)
102119
{
103-
switch (anonymousFunctionSyntax)
120+
if (memberAccessSyntax is null)
104121
{
105-
case SimpleLambdaExpressionSyntax lambda:
106-
return lambda.Parameter;
107-
case ParenthesizedLambdaExpressionSyntax lambda:
108-
return lambda.ParameterList.Parameters.FirstOrDefault();
109-
case AnonymousMethodExpressionSyntax anonymousMethod:
110-
return anonymousMethod.ParameterList?.Parameters.FirstOrDefault();
122+
return;
111123
}
112124

113-
return null;
125+
CSharpCommonInterest.InspectMemberAccess(context, memberAccessSyntax, Descriptor, problematicMethods);
114126
}
115127

116-
private static void InspectMemberAccess(
128+
private static void AnalyzeInvocation(
117129
SyntaxNodeAnalysisContext context,
118-
MemberAccessExpressionSyntax? memberAccessSyntax,
119-
IEnumerable<CommonInterest.SyncBlockingMethod> problematicMethods,
120-
INamedTypeSymbol taskSymbol)
130+
ImmutableArray<CommonInterest.QualifiedMember> configuredSyncBlockingMethods,
131+
ImmutableArray<CommonInterest.QualifiedMember> methodsExcludedFromVSTHRD103,
132+
bool analyzeWholeCodeBlock,
133+
bool analyzeBuiltInBlockingMethods)
121134
{
122-
if (memberAccessSyntax is null)
135+
var invocationExpressionSyntax = (InvocationExpressionSyntax)context.Node;
136+
if (analyzeBuiltInBlockingMethods && ShouldAnalyze(context, analyzeWholeCodeBlock))
137+
{
138+
if (invocationExpressionSyntax.Expression is MemberAccessExpressionSyntax memberAccess)
139+
{
140+
InspectMemberAccess(context, memberAccess, CommonInterest.ProblematicSyncBlockingMethods);
141+
}
142+
else if (invocationExpressionSyntax.Expression is MemberBindingExpressionSyntax memberBinding
143+
&& invocationExpressionSyntax.FirstAncestorOrSelf<ConditionalAccessExpressionSyntax>() is { } conditionalAccess)
144+
{
145+
CSharpCommonInterest.InspectMemberBinding(
146+
context,
147+
memberBinding,
148+
conditionalAccess.Expression,
149+
conditionalAccess,
150+
Descriptor,
151+
CommonInterest.ProblematicSyncBlockingMethods);
152+
}
153+
}
154+
155+
if (configuredSyncBlockingMethods.IsEmpty
156+
|| context.SemanticModel.GetSymbolInfo(invocationExpressionSyntax, context.CancellationToken).Symbol is not IMethodSymbol invokedMethod)
157+
{
158+
return;
159+
}
160+
161+
IMethodSymbol methodDefinition = invokedMethod.ReducedFrom ?? invokedMethod;
162+
bool isConfiguredSyncBlockingMethod = configuredSyncBlockingMethods.Any(
163+
method => method.IsMatch(invokedMethod) || method.IsMatch(methodDefinition));
164+
if (!isConfiguredSyncBlockingMethod)
123165
{
124166
return;
125167
}
126168

127-
// Are we in the context of an anonymous function that is passed directly in as an argument to another method?
128-
AnonymousFunctionExpressionSyntax? anonymousFunctionSyntax = context.Node.FirstAncestorOrSelf<AnonymousFunctionExpressionSyntax>();
129-
var anonFuncAsArgument = anonymousFunctionSyntax?.Parent as ArgumentSyntax;
130-
var invocationPassingExpression = anonFuncAsArgument?.Parent?.Parent as InvocationExpressionSyntax;
131-
var invokedMemberAccess = invocationPassingExpression?.Expression as MemberAccessExpressionSyntax;
132-
if (invokedMemberAccess?.Name is object)
169+
bool isBuiltInSyncBlockingMethod = CommonInterest.ProblematicSyncBlockingMethods.Any(
170+
method => method.Method.IsMatch(invokedMethod) || method.Method.IsMatch(methodDefinition));
171+
bool coveredByVSTHRD103 = !methodsExcludedFromVSTHRD103.Contains(invokedMethod)
172+
&& !methodsExcludedFromVSTHRD103.Contains(methodDefinition)
173+
&& !invokedMethod.Name.EndsWith(VSTHRD200UseAsyncNamingConventionAnalyzer.MandatoryAsyncSuffix, StringComparison.CurrentCulture)
174+
&& !invokedMethod.HasAsyncCompatibleReturnType()
175+
&& IsInTaskReturningMethodOrDelegate(context)
176+
&& HasAsyncAlternative(context, invocationExpressionSyntax, invokedMethod);
177+
if (!isBuiltInSyncBlockingMethod
178+
&& !coveredByVSTHRD103)
133179
{
134-
// Does the anonymous function appear as the first argument to Task.ContinueWith?
135-
var invokedMemberSymbol = context.SemanticModel.GetSymbolInfo(invokedMemberAccess.Name, context.CancellationToken).Symbol as IMethodSymbol;
136-
if (invokedMemberSymbol?.Name == nameof(Task.ContinueWith) &&
137-
Utils.IsEqualToOrDerivedFrom(invokedMemberSymbol?.ContainingType, taskSymbol) &&
138-
invocationPassingExpression?.ArgumentList?.Arguments.FirstOrDefault() == anonFuncAsArgument)
180+
SimpleNameSyntax? methodName = invocationExpressionSyntax.Expression switch
139181
{
140-
// Does the member access being analyzed belong to the Task that just completed?
141-
ParameterSyntax? firstParameter = GetFirstParameter(anonymousFunctionSyntax);
142-
if (firstParameter is object)
143-
{
144-
// Are we accessing a member of the completed task?
145-
ISymbol? invokedObjectSymbol = context.SemanticModel.GetSymbolInfo(memberAccessSyntax.Expression, context.CancellationToken).Symbol;
146-
IParameterSymbol? completedTask = context.SemanticModel.GetDeclaredSymbol(firstParameter);
147-
if (EqualityComparer<ISymbol?>.Default.Equals(invokedObjectSymbol, completedTask))
148-
{
149-
// Skip analysis since Task.Result (et. al) of a completed Task is fair game.
150-
return;
151-
}
152-
}
182+
MemberAccessExpressionSyntax memberAccess => memberAccess.Name,
183+
MemberBindingExpressionSyntax memberBinding => memberBinding.Name,
184+
SimpleNameSyntax simpleName => simpleName,
185+
_ => null,
186+
};
187+
188+
if (methodName is object
189+
&& !CSharpCommonInterest.ShouldIgnoreContext(context)
190+
&& !CSharpUtils.IsWithinNameOf(invocationExpressionSyntax))
191+
{
192+
ImmutableDictionary<string, string?> properties = ImmutableDictionary<string, string?>.Empty.Add("SuppressAwaitCodeFix", null);
193+
context.ReportDiagnostic(Diagnostic.Create(Descriptor, methodName.GetLocation(), properties));
153194
}
154195
}
155-
156-
CSharpCommonInterest.InspectMemberAccess(context, memberAccessSyntax, Descriptor, problematicMethods);
157196
}
158197

159-
private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, INamedTypeSymbol taskSymbol, bool analyzeWholeCodeBlock)
198+
private static bool HasAsyncAlternative(
199+
SyntaxNodeAnalysisContext context,
200+
InvocationExpressionSyntax invocation,
201+
IMethodSymbol invokedMethod)
160202
{
161-
if (!ShouldAnalyze(context, analyzeWholeCodeBlock))
203+
string asyncMethodName = invokedMethod.Name + VSTHRD200UseAsyncNamingConventionAnalyzer.MandatoryAsyncSuffix;
204+
INamespaceOrTypeSymbol lookupContainer = invokedMethod.ContainingType;
205+
if (invokedMethod.ReducedFrom is object)
162206
{
163-
return;
207+
ExpressionSyntax? receiver = invocation.Expression is MemberAccessExpressionSyntax memberAccess
208+
? memberAccess.Expression
209+
: invocation.FirstAncestorOrSelf<ConditionalAccessExpressionSyntax>()?.Expression;
210+
if (receiver is null
211+
|| context.SemanticModel.GetTypeInfo(receiver, context.CancellationToken).Type is not INamespaceOrTypeSymbol receiverType)
212+
{
213+
return false;
214+
}
215+
216+
lookupContainer = receiverType;
164217
}
165218

166-
var invocationExpressionSyntax = (InvocationExpressionSyntax)context.Node;
167-
InspectMemberAccess(
168-
context,
169-
invocationExpressionSyntax.Expression as MemberAccessExpressionSyntax,
170-
CommonInterest.ProblematicSyncBlockingMethods,
171-
taskSymbol);
219+
string? declaringMethodName = invocation.FirstAncestorOrSelf<MethodDeclarationSyntax>()?.Identifier.Text;
220+
return context.SemanticModel.LookupSymbols(
221+
invocation.Expression.SpanStart,
222+
lookupContainer,
223+
asyncMethodName,
224+
includeReducedExtensionMethods: true)
225+
.OfType<IMethodSymbol>()
226+
.Any(candidate => !candidate.IsObsolete()
227+
&& candidate.Name != declaringMethodName
228+
&& candidate.HasAsyncCompatibleReturnType()
229+
&& CSharpCommonInterest.IsApplicableAsyncAlternative(context, invocation, candidate));
230+
}
231+
232+
private static bool IsInTaskReturningMethodOrDelegate(SyntaxNodeAnalysisContext context)
233+
{
234+
SyntaxNode? containingFunction = context.Node.Ancestors().FirstOrDefault(
235+
node => node is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax or MethodDeclarationSyntax);
236+
IMethodSymbol? containingMethod = containingFunction switch
237+
{
238+
AnonymousFunctionExpressionSyntax anonymousFunction => context.SemanticModel.GetSymbolInfo(anonymousFunction, context.CancellationToken).Symbol as IMethodSymbol,
239+
LocalFunctionStatementSyntax localFunction => context.SemanticModel.GetDeclaredSymbol(localFunction, context.CancellationToken),
240+
MethodDeclarationSyntax method => context.SemanticModel.GetDeclaredSymbol(method, context.CancellationToken),
241+
_ => null,
242+
};
243+
return containingMethod?.HasAsyncCompatibleReturnType() is true;
172244
}
173245

174-
private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, INamedTypeSymbol taskSymbol, bool analyzeWholeCodeBlock)
246+
private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, bool analyzeWholeCodeBlock)
175247
{
176248
if (!ShouldAnalyze(context, analyzeWholeCodeBlock))
177249
{
@@ -182,7 +254,6 @@ private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, IName
182254
InspectMemberAccess(
183255
context,
184256
memberAccessSyntax,
185-
CommonInterest.SyncBlockingProperties,
186-
taskSymbol);
257+
CommonInterest.SyncBlockingProperties);
187258
}
188259
}

0 commit comments

Comments
 (0)