Fix VSTHRD002 completion analysis and extensibility

docfx/analyzers/VSTHRD002.md

@@ -40,6 +40,14 @@ void DoSomething()
 }
 ```
 
+Accessing a task's result is not reported when the analyzer can prove the task has completed.
+Recognized proofs include awaiting the task (directly or through `Task.WhenAll`), guarding the
+access with a completion property such as `IsCompletedSuccessfully`, and awaiting the task in
+the negative branch of such a guard.
+
+VSTHRD002 can also report project-specific synchronous blocking methods configured in
+`vs-threading.SyncBlockingMethods.txt`. See [Analyzer Configuration](configuration.md#additional-synchronous-blocking-methods-for-vsthrd002).
+
 Refer to [Asynchronous and multithreaded programming within VS using the JoinableTaskFactory][1] for more information.
 
 [1]: https://devblogs.microsoft.com/premier-developer/asynchronous-and-multithreaded-programming-within-vs-using-the-joinabletaskfactory/

docfx/analyzers/configuration.md

@@ -105,6 +105,19 @@ excluded from VSTHRD103 analysis by specifying them in a configuration file.
 
 **Generic sample:** ``[Microsoft.EntityFrameworkCore.DbSet`1]::Add``
 
+## Additional synchronous blocking methods for VSTHRD002
+
+Projects that wrap synchronous waits in their own APIs can configure those methods to be
+reported by VSTHRD002. Instance, static, and extension methods are supported. Because the
+analyzer cannot infer an asynchronous equivalent for a configured method, it does not offer
+the "use await instead" code fix for these diagnostics.
+
+**Filename:** `vs-threading.SyncBlockingMethods.txt`
+
+**Line format:** `[Namespace.TypeName]::MethodName`
+
+**Sample:** `[Contoso.Threading.TaskExtensions]::WaitSynchronously`
+
 ## Types that require the Async suffix
 
 VSTHRD200 requires methods returning `Task`, `ValueTask`, and other async-focused types

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

@@ -208,10 +208,9 @@ public static MemberAccessExpressionSyntax MemberAccess(IReadOnlyList<string> qu
     /// </summary>
     public static bool IsWithinNameOf([NotNullWhen(true)] SyntaxNode? syntaxNode)
     {
-        InvocationExpressionSyntax? invocation = syntaxNode?.FirstAncestorOrSelf<InvocationExpressionSyntax>();
-        return invocation is object
-            && (invocation.Expression as IdentifierNameSyntax)?.Identifier.Text == "nameof"
-            && invocation.ArgumentList.Arguments.Count == 1;
+        return syntaxNode?.AncestorsAndSelf().OfType<InvocationExpressionSyntax>().Any(
+            invocation => (invocation.Expression as IdentifierNameSyntax)?.Identifier.Text == "nameof"
+                && invocation.ArgumentList.Arguments.Count == 1) is true;
     }
 
     public override Location? GetLocationOfBaseTypeName(INamedTypeSymbol symbol, INamedTypeSymbol baseType, Compilation compilation, CancellationToken cancellationToken)

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

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Linq;
+using System.Text.RegularExpressions;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis;
@@ -61,7 +62,15 @@ public override void Initialize(AnalysisContext context)
         context.RegisterCompilationStartAction(compilationContext =>
         {
             INamedTypeSymbol? taskSymbol = compilationContext.Compilation.GetTypeByMetadataName(Types.Task.FullName);
-            if (taskSymbol is object)
+            ImmutableArray<CommonInterest.QualifiedMember> configuredSyncBlockingMethods = CommonInterest.ReadMethods(
+                compilationContext.Options,
+                new Regex(@"^vs-threading\.SyncBlockingMethods(\..*)?.txt$", RegexOptions.IgnoreCase | RegexOptions.Singleline),
+                compilationContext.CancellationToken).ToImmutableArray();
+            ImmutableArray<CommonInterest.QualifiedMember> methodsExcludedFromVSTHRD103 = CommonInterest.ReadMethods(
+                compilationContext.Options,
+                CommonInterest.FileNamePatternForSyncMethodsToExcludeFromVSTHRD103,
+                compilationContext.CancellationToken).ToImmutableArray();
+            if (taskSymbol is object || !configuredSyncBlockingMethods.IsEmpty)
             {
                 compilationContext.RegisterCodeBlockStartAction<SyntaxKind>(codeBlockContext =>
                 {
@@ -70,8 +79,13 @@ public override void Initialize(AnalysisContext context)
                     if (propertySymbol is object || methodSymbol is object)
                     {
                         bool analyzeWholeCodeBlock = propertySymbol is object || !methodSymbol!.HasAsyncCompatibleReturnType();
-                        codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeInvocation(c, taskSymbol, analyzeWholeCodeBlock)), SyntaxKind.InvocationExpression);
-                        codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeMemberAccess(c, taskSymbol, analyzeWholeCodeBlock)), SyntaxKind.SimpleMemberAccessExpression);
+                        codeBlockContext.RegisterSyntaxNodeAction(
+                            Utils.DebuggableWrapper(c => AnalyzeInvocation(c, configuredSyncBlockingMethods, methodsExcludedFromVSTHRD103, analyzeWholeCodeBlock, taskSymbol is object)),
+                            SyntaxKind.InvocationExpression);
+                        if (taskSymbol is object)
+                        {
+                            codeBlockContext.RegisterSyntaxNodeAction(Utils.DebuggableWrapper(c => AnalyzeMemberAccess(c, analyzeWholeCodeBlock)), SyntaxKind.SimpleMemberAccessExpression);
+                        }
                     }
                 });
             }
@@ -98,80 +112,138 @@ private static bool ShouldAnalyze(SyntaxNodeAnalysisContext context, bool analyz
             && !containingMethod.HasAsyncCompatibleReturnType();
     }
 
-    private static ParameterSyntax? GetFirstParameter(AnonymousFunctionExpressionSyntax? anonymousFunctionSyntax)
+    private static void InspectMemberAccess(
+        SyntaxNodeAnalysisContext context,
+        MemberAccessExpressionSyntax? memberAccessSyntax,
+        IEnumerable<CommonInterest.SyncBlockingMethod> problematicMethods)
     {
-        switch (anonymousFunctionSyntax)
+        if (memberAccessSyntax is null)
         {
-            case SimpleLambdaExpressionSyntax lambda:
-                return lambda.Parameter;
-            case ParenthesizedLambdaExpressionSyntax lambda:
-                return lambda.ParameterList.Parameters.FirstOrDefault();
-            case AnonymousMethodExpressionSyntax anonymousMethod:
-                return anonymousMethod.ParameterList?.Parameters.FirstOrDefault();
+            return;
         }
 
-        return null;
+        CSharpCommonInterest.InspectMemberAccess(context, memberAccessSyntax, Descriptor, problematicMethods);
     }
 
-    private static void InspectMemberAccess(
+    private static void AnalyzeInvocation(
         SyntaxNodeAnalysisContext context,
-        MemberAccessExpressionSyntax? memberAccessSyntax,
-        IEnumerable<CommonInterest.SyncBlockingMethod> problematicMethods,
-        INamedTypeSymbol taskSymbol)
+        ImmutableArray<CommonInterest.QualifiedMember> configuredSyncBlockingMethods,
+        ImmutableArray<CommonInterest.QualifiedMember> methodsExcludedFromVSTHRD103,
+        bool analyzeWholeCodeBlock,
+        bool analyzeBuiltInBlockingMethods)
     {
-        if (memberAccessSyntax is null)
+        var invocationExpressionSyntax = (InvocationExpressionSyntax)context.Node;
+        if (analyzeBuiltInBlockingMethods && ShouldAnalyze(context, analyzeWholeCodeBlock))
+        {
+            if (invocationExpressionSyntax.Expression is MemberAccessExpressionSyntax memberAccess)
+            {
+                InspectMemberAccess(context, memberAccess, CommonInterest.ProblematicSyncBlockingMethods);
+            }
+            else if (invocationExpressionSyntax.Expression is MemberBindingExpressionSyntax memberBinding
+                && invocationExpressionSyntax.FirstAncestorOrSelf<ConditionalAccessExpressionSyntax>() is { } conditionalAccess)
+            {
+                CSharpCommonInterest.InspectMemberBinding(
+                    context,
+                    memberBinding,
+                    conditionalAccess.Expression,
+                    conditionalAccess,
+                    Descriptor,
+                    CommonInterest.ProblematicSyncBlockingMethods);
+            }
+        }
+
+        if (configuredSyncBlockingMethods.IsEmpty
+            || context.SemanticModel.GetSymbolInfo(invocationExpressionSyntax, context.CancellationToken).Symbol is not IMethodSymbol invokedMethod)
+        {
+            return;
+        }
+
+        IMethodSymbol methodDefinition = invokedMethod.ReducedFrom ?? invokedMethod;
+        bool isConfiguredSyncBlockingMethod = configuredSyncBlockingMethods.Any(
+            method => method.IsMatch(invokedMethod) || method.IsMatch(methodDefinition));
+        if (!isConfiguredSyncBlockingMethod)
         {
             return;
         }
 
-        // Are we in the context of an anonymous function that is passed directly in as an argument to another method?
-        AnonymousFunctionExpressionSyntax? anonymousFunctionSyntax = context.Node.FirstAncestorOrSelf<AnonymousFunctionExpressionSyntax>();
-        var anonFuncAsArgument = anonymousFunctionSyntax?.Parent as ArgumentSyntax;
-        var invocationPassingExpression = anonFuncAsArgument?.Parent?.Parent as InvocationExpressionSyntax;
-        var invokedMemberAccess = invocationPassingExpression?.Expression as MemberAccessExpressionSyntax;
-        if (invokedMemberAccess?.Name is object)
+        bool isBuiltInSyncBlockingMethod = CommonInterest.ProblematicSyncBlockingMethods.Any(
+            method => method.Method.IsMatch(invokedMethod) || method.Method.IsMatch(methodDefinition));
+        bool coveredByVSTHRD103 = !methodsExcludedFromVSTHRD103.Contains(invokedMethod)
+            && !methodsExcludedFromVSTHRD103.Contains(methodDefinition)
+            && !invokedMethod.Name.EndsWith(VSTHRD200UseAsyncNamingConventionAnalyzer.MandatoryAsyncSuffix, StringComparison.CurrentCulture)
+            && !invokedMethod.HasAsyncCompatibleReturnType()
+            && IsInTaskReturningMethodOrDelegate(context)
+            && HasAsyncAlternative(context, invocationExpressionSyntax, invokedMethod);
+        if (!isBuiltInSyncBlockingMethod
+            && !coveredByVSTHRD103)
         {
-            // Does the anonymous function appear as the first argument to Task.ContinueWith?
-            var invokedMemberSymbol = context.SemanticModel.GetSymbolInfo(invokedMemberAccess.Name, context.CancellationToken).Symbol as IMethodSymbol;
-            if (invokedMemberSymbol?.Name == nameof(Task.ContinueWith) &&
-                Utils.IsEqualToOrDerivedFrom(invokedMemberSymbol?.ContainingType, taskSymbol) &&
-                invocationPassingExpression?.ArgumentList?.Arguments.FirstOrDefault() == anonFuncAsArgument)
+            SimpleNameSyntax? methodName = invocationExpressionSyntax.Expression switch
             {
-                // Does the member access being analyzed belong to the Task that just completed?
-                ParameterSyntax? firstParameter = GetFirstParameter(anonymousFunctionSyntax);
-                if (firstParameter is object)
-                {
-                    // Are we accessing a member of the completed task?
-                    ISymbol? invokedObjectSymbol = context.SemanticModel.GetSymbolInfo(memberAccessSyntax.Expression, context.CancellationToken).Symbol;
-                    IParameterSymbol? completedTask = context.SemanticModel.GetDeclaredSymbol(firstParameter);
-                    if (EqualityComparer<ISymbol?>.Default.Equals(invokedObjectSymbol, completedTask))
-                    {
-                        // Skip analysis since Task.Result (et. al) of a completed Task is fair game.
-                        return;
-                    }
-                }
+                MemberAccessExpressionSyntax memberAccess => memberAccess.Name,
+                MemberBindingExpressionSyntax memberBinding => memberBinding.Name,
+                SimpleNameSyntax simpleName => simpleName,
+                _ => null,
+            };
+
+            if (methodName is object
+                && !CSharpCommonInterest.ShouldIgnoreContext(context)
+                && !CSharpUtils.IsWithinNameOf(invocationExpressionSyntax))
+            {
+                ImmutableDictionary<string, string?> properties = ImmutableDictionary<string, string?>.Empty.Add("SuppressAwaitCodeFix", null);
+                context.ReportDiagnostic(Diagnostic.Create(Descriptor, methodName.GetLocation(), properties));
             }
         }
-
-        CSharpCommonInterest.InspectMemberAccess(context, memberAccessSyntax, Descriptor, problematicMethods);
     }
 
-    private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, INamedTypeSymbol taskSymbol, bool analyzeWholeCodeBlock)
+    private static bool HasAsyncAlternative(
+        SyntaxNodeAnalysisContext context,
+        InvocationExpressionSyntax invocation,
+        IMethodSymbol invokedMethod)
     {
-        if (!ShouldAnalyze(context, analyzeWholeCodeBlock))
+        string asyncMethodName = invokedMethod.Name + VSTHRD200UseAsyncNamingConventionAnalyzer.MandatoryAsyncSuffix;
+        INamespaceOrTypeSymbol lookupContainer = invokedMethod.ContainingType;
+        if (invokedMethod.ReducedFrom is object)
         {
-            return;
+            ExpressionSyntax? receiver = invocation.Expression is MemberAccessExpressionSyntax memberAccess
+                ? memberAccess.Expression
+                : invocation.FirstAncestorOrSelf<ConditionalAccessExpressionSyntax>()?.Expression;
+            if (receiver is null
+                || context.SemanticModel.GetTypeInfo(receiver, context.CancellationToken).Type is not INamespaceOrTypeSymbol receiverType)
+            {
+                return false;
+            }
+
+            lookupContainer = receiverType;
         }
 
-        var invocationExpressionSyntax = (InvocationExpressionSyntax)context.Node;
-        InspectMemberAccess(
-            context,
-            invocationExpressionSyntax.Expression as MemberAccessExpressionSyntax,
-            CommonInterest.ProblematicSyncBlockingMethods,
-            taskSymbol);
+        string? declaringMethodName = invocation.FirstAncestorOrSelf<MethodDeclarationSyntax>()?.Identifier.Text;
+        return context.SemanticModel.LookupSymbols(
+                invocation.Expression.SpanStart,
+                lookupContainer,
+                asyncMethodName,
+                includeReducedExtensionMethods: true)
+            .OfType<IMethodSymbol>()
+            .Any(candidate => !candidate.IsObsolete()
+                && candidate.Name != declaringMethodName
+                && candidate.HasAsyncCompatibleReturnType()
+                && CSharpCommonInterest.IsApplicableAsyncAlternative(context, invocation, candidate));
+    }
+
+    private static bool IsInTaskReturningMethodOrDelegate(SyntaxNodeAnalysisContext context)
+    {
+        SyntaxNode? containingFunction = context.Node.Ancestors().FirstOrDefault(
+            node => node is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax or MethodDeclarationSyntax);
+        IMethodSymbol? containingMethod = containingFunction switch
+        {
+            AnonymousFunctionExpressionSyntax anonymousFunction => context.SemanticModel.GetSymbolInfo(anonymousFunction, context.CancellationToken).Symbol as IMethodSymbol,
+            LocalFunctionStatementSyntax localFunction => context.SemanticModel.GetDeclaredSymbol(localFunction, context.CancellationToken),
+            MethodDeclarationSyntax method => context.SemanticModel.GetDeclaredSymbol(method, context.CancellationToken),
+            _ => null,
+        };
+        return containingMethod?.HasAsyncCompatibleReturnType() is true;
     }
 
-    private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, INamedTypeSymbol taskSymbol, bool analyzeWholeCodeBlock)
+    private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, bool analyzeWholeCodeBlock)
     {
         if (!ShouldAnalyze(context, analyzeWholeCodeBlock))
         {
@@ -182,7 +254,6 @@ private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context, IName
         InspectMemberAccess(
             context,
             memberAccessSyntax,
-            CommonInterest.SyncBlockingProperties,
-            taskSymbol);
+            CommonInterest.SyncBlockingProperties);
     }
 }