Fixes microsoft#637. VSTHRD114 now ignores C# methods returning nulla…

…ble tasks. The implementation uses syntax nodes to detect the nullable return type. Converted VSTHRD114 to an abstract base class in order to use LanguageUtils.
bluetarpmedia · Jun 15, 2021 · eda99c2 · eda99c2
1 parent 94e1337
commit eda99c2
Show file tree

Hide file tree

Showing 9 changed files with 123 additions and 23 deletions.
diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/CSharpUtils.cs b/src/Microsoft.VisualStudio.Threading.Analyzers.CSharp/CSharpUtils.cs
@@ -203,6 +203,29 @@ internal override SyntaxNode IsolateMethodName(IObjectCreationOperation objectCr
             return objectCreation.Syntax;
         }
 
+        internal override bool MethodReturnsNullableReferenceType(IMethodSymbol methodSymbol)
+        {
+            SyntaxReference? syntaxReference = methodSymbol.DeclaringSyntaxReferences.FirstOrDefault();
+            if (syntaxReference == null)
+            {
+                return false;
+            }
+
+            SyntaxNode syntaxNode = syntaxReference.GetSyntax();
+            TypeSyntax? returnType = null;
+
+            if (syntaxNode is MethodDeclarationSyntax methodDeclSyntax)
+            {
+                returnType = methodDeclSyntax.ReturnType;
+            }
+            else if (syntaxNode is LocalFunctionStatementSyntax localFunc)
+            {
+                returnType = localFunc.ReturnType;
+            }
+
+            return returnType != null && returnType.IsKind(SyntaxKind.NullableType);
+        }
+
         internal readonly struct ContainingFunctionData
         {
             internal ContainingFunctionData(CSharpSyntaxNode function, bool isAsync, ParameterListSyntax parameterList, CSharpSyntaxNode blockOrExpression)

diff --git a/....VisualStudio.Threading.Analyzers.CSharp/CSharpVSTHRD114AvoidReturningNullTaskAnalyzer.cs b/....VisualStudio.Threading.Analyzers.CSharp/CSharpVSTHRD114AvoidReturningNullTaskAnalyzer.cs
@@ -0,0 +1,14 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.Threading.Analyzers
+{
+    using Microsoft.CodeAnalysis;
+    using Microsoft.CodeAnalysis.Diagnostics;
+
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class CSharpVSTHRD114AvoidReturningNullTaskAnalyzer : AbstractVSTHRD114AvoidReturningNullTaskAnalyzer
+    {
+        private protected override LanguageUtils LanguageUtils => CSharpUtils.Instance;
+    }
+}
diff --git a/...soft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD114AvoidReturningNullTaskCodeFix.cs b/...soft.VisualStudio.Threading.Analyzers.CodeFixes/VSTHRD114AvoidReturningNullTaskCodeFix.cs
@@ -16,7 +16,7 @@ namespace Microsoft.VisualStudio.Threading.Analyzers
     public class VSTHRD114AvoidReturningNullTaskCodeFix : CodeFixProvider
     {
         private static readonly ImmutableArray<string> ReusableFixableDiagnosticIds = ImmutableArray.Create(
-            VSTHRD114AvoidReturningNullTaskAnalyzer.Id);
+            AbstractVSTHRD114AvoidReturningNullTaskAnalyzer.Id);
 
         /// <inheritdoc />
         public override ImmutableArray<string> FixableDiagnosticIds => ReusableFixableDiagnosticIds;

diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers.VisualBasic/VisualBasicUtils.cs b/src/Microsoft.VisualStudio.Threading.Analyzers.VisualBasic/VisualBasicUtils.cs
@@ -87,5 +87,11 @@ internal override SyntaxNode IsolateMethodName(IObjectCreationOperation objectCr
         {
             return objectCreation.Syntax;
         }
+
+        internal override bool MethodReturnsNullableReferenceType(IMethodSymbol methodSymbol)
+        {
+            // VB.NET doesn't support nullable reference types
+            return false;
+        }
     }
 }
diff --git a/...dio.Threading.Analyzers.VisualBasic/VisualBasicVSTHRD114AvoidReturningNullTaskAnalyzer.cs b/...dio.Threading.Analyzers.VisualBasic/VisualBasicVSTHRD114AvoidReturningNullTaskAnalyzer.cs
@@ -0,0 +1,14 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.Threading.Analyzers
+{
+    using Microsoft.CodeAnalysis;
+    using Microsoft.CodeAnalysis.Diagnostics;
+
+    [DiagnosticAnalyzer(LanguageNames.VisualBasic)]
+    public sealed class VisualBasicVSTHRD114AvoidReturningNullTaskAnalyzer : AbstractVSTHRD114AvoidReturningNullTaskAnalyzer
+    {
+        private protected override LanguageUtils LanguageUtils => VisualBasicUtils.Instance;
+    }
+}
diff --git a/...STHRD114AvoidReturningNullTaskAnalyzer.cs → ...STHRD114AvoidReturningNullTaskAnalyzer.cs b/...STHRD114AvoidReturningNullTaskAnalyzer.cs → ...STHRD114AvoidReturningNullTaskAnalyzer.cs
@@ -13,8 +13,7 @@ namespace Microsoft.VisualStudio.Threading.Analyzers
     /// Finds await expressions on <see cref="Task"/> that do not use <see cref="Task.ConfigureAwait(bool)"/>.
     /// Also works on <see cref="ValueTask"/>.
     /// </summary>
-    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
-    public class VSTHRD114AvoidReturningNullTaskAnalyzer : DiagnosticAnalyzer
+    public abstract class AbstractVSTHRD114AvoidReturningNullTaskAnalyzer : DiagnosticAnalyzer
     {
         public const string Id = "VSTHRD114";
 
@@ -30,28 +29,15 @@ public class VSTHRD114AvoidReturningNullTaskAnalyzer : DiagnosticAnalyzer
         /// <inheritdoc />
         public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Descriptor);
 
+        private protected abstract LanguageUtils LanguageUtils { get; }
+
         /// <inheritdoc />
         public override void Initialize(AnalysisContext context)
         {
             context.EnableConcurrentExecution();
             context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze);
 
-            context.RegisterOperationAction(Utils.DebuggableWrapper(context => AnalyzerReturnOperation(context)), OperationKind.Return);
-        }
-
-        private static void AnalyzerReturnOperation(OperationAnalysisContext context)
-        {
-            var returnOperation = (IReturnOperation)context.Operation;
-
-            if (returnOperation.ReturnedValue is { ConstantValue: { HasValue: true, Value: null } } && // could be null for implicit returns
-                returnOperation.ReturnedValue.Syntax is { } returnedValueSyntax &&
-                Utils.GetContainingFunctionBlock(returnOperation) is { } block &&
-                FindOwningSymbol(block, context.ContainingSymbol) is { } method &&
-                !method.IsAsync &&
-                Utils.IsTask(method.ReturnType))
-            {
-                context.ReportDiagnostic(Diagnostic.Create(Descriptor, returnedValueSyntax.GetLocation()));
-            }
+            context.RegisterOperationAction(Utils.DebuggableWrapper(context => this.AnalyzerReturnOperation(context)), OperationKind.Return);
         }
 
         private static IMethodSymbol? FindOwningSymbol(IBlockOperation block, ISymbol containingSymbol)
@@ -68,5 +54,21 @@ private static void AnalyzerReturnOperation(OperationAnalysisContext context)
                 _ => null,
             };
         }
+
+        private void AnalyzerReturnOperation(OperationAnalysisContext context)
+        {
+            var returnOperation = (IReturnOperation)context.Operation;
+
+            if (returnOperation.ReturnedValue is { ConstantValue: { HasValue: true, Value: null } } && // could be null for implicit returns
+                returnOperation.ReturnedValue.Syntax is { } returnedValueSyntax &&
+                Utils.GetContainingFunctionBlock(returnOperation) is { } block &&
+                FindOwningSymbol(block, context.ContainingSymbol) is { } method &&
+                !method.IsAsync &&
+                Utils.IsTask(method.ReturnType) &&
+                !this.LanguageUtils.MethodReturnsNullableReferenceType(method))
+            {
+                context.ReportDiagnostic(Diagnostic.Create(Descriptor, returnedValueSyntax.GetLocation()));
+            }
+        }
     }
 }
diff --git a/src/Microsoft.VisualStudio.Threading.Analyzers/LanguageUtils.cs b/src/Microsoft.VisualStudio.Threading.Analyzers/LanguageUtils.cs
@@ -14,5 +14,7 @@ internal abstract class LanguageUtils
         internal abstract SyntaxNode IsolateMethodName(IInvocationOperation invocation);
 
         internal abstract SyntaxNode IsolateMethodName(IObjectCreationOperation objectCreation);
+
+        internal abstract bool MethodReturnsNullableReferenceType(IMethodSymbol method);
     }
 }
diff --git a/...ft.VisualStudio.Threading.Analyzers.Tests/VSTHRD114AvoidReturningNullTaskAnalyzerTests.cs b/...ft.VisualStudio.Threading.Analyzers.Tests/VSTHRD114AvoidReturningNullTaskAnalyzerTests.cs
@@ -5,8 +5,8 @@ namespace Microsoft.VisualStudio.Threading.Analyzers.Tests
 {
     using System.Threading.Tasks;
     using Xunit;
-    using VerifyCS = CSharpCodeFixVerifier<VSTHRD114AvoidReturningNullTaskAnalyzer, CodeAnalysis.Testing.EmptyCodeFixProvider>;
-    using VerifyVB = VisualBasicCodeFixVerifier<VSTHRD114AvoidReturningNullTaskAnalyzer, CodeAnalysis.Testing.EmptyCodeFixProvider>;
+    using VerifyCS = CSharpCodeFixVerifier<CSharpVSTHRD114AvoidReturningNullTaskAnalyzer, CodeAnalysis.Testing.EmptyCodeFixProvider>;
+    using VerifyVB = VisualBasicCodeFixVerifier<VisualBasicVSTHRD114AvoidReturningNullTaskAnalyzer, CodeAnalysis.Testing.EmptyCodeFixProvider>;
 
     public class VSTHRD114AvoidReturningNullTaskAnalyzerTests
     {
@@ -280,6 +280,45 @@ async Task<object> GetTaskObj()
         }
     }
 }
+";
+            await new VerifyCS.Test
+            {
+                TestCode = csharpTest,
+            }.RunAsync();
+        }
+
+        [Fact]
+        public async Task ReturnNullableTask_NoDiagnostic()
+        {
+            var csharpTest = @"
+using System;
+using System.Threading.Tasks;
+
+class Test
+{
+    public Task? GetTask()
+    {
+        return null;
+    }
+
+    public Task<int>? GetTaskOfInt()
+    {
+        return null;
+    }
+
+    public Task<object?>? GetTaskOfNullableObject()
+    {
+        return null;
+    }
+
+    public void LocalFunc()
+    {
+        Task<object>? GetTaskObj()
+        {
+            return null;
+        }
+    }
+}
 ";
             await new VerifyCS.Test
             {

diff --git a/...oft.VisualStudio.Threading.Analyzers.Tests/VSTHRD114AvoidReturningNullTaskCodeFixTests.cs b/...oft.VisualStudio.Threading.Analyzers.Tests/VSTHRD114AvoidReturningNullTaskCodeFixTests.cs
@@ -5,8 +5,8 @@ namespace Microsoft.VisualStudio.Threading.Analyzers.Tests
 {
     using System.Threading.Tasks;
     using Xunit;
-    using VerifyCS = CSharpCodeFixVerifier<VSTHRD114AvoidReturningNullTaskAnalyzer, VSTHRD114AvoidReturningNullTaskCodeFix>;
-    using VerifyVB = VisualBasicCodeFixVerifier<VSTHRD114AvoidReturningNullTaskAnalyzer, VSTHRD114AvoidReturningNullTaskCodeFix>;
+    using VerifyCS = CSharpCodeFixVerifier<CSharpVSTHRD114AvoidReturningNullTaskAnalyzer, VSTHRD114AvoidReturningNullTaskCodeFix>;
+    using VerifyVB = VisualBasicCodeFixVerifier<VisualBasicVSTHRD114AvoidReturningNullTaskAnalyzer, VSTHRD114AvoidReturningNullTaskCodeFix>;
 
     public class VSTHRD114AvoidReturningNullTaskCodeFixTests
     {