Merge pull request #4639 from Evangelink/awaitable-awaiter

mavasani · web-flow · commit 86736870dfee · 2021-01-05T05:31:47.000-08:00
Do not report on awaitable-awaiter patterns
diff --git a/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriate.cs b/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriate.cs
@@ -56,6 +56,11 @@ public override void Initialize(AnalysisContext analysisContext)
 
                 var taskTypes = taskTypesBuilder.ToImmutable();
 
+                var inotifyCompletionType = context.Compilation.GetOrCreateTypeByMetadataName(
+                    WellKnownTypeNames.SystemRuntimeCompilerServicesINotifyCompletion);
+                var icriticalNotifyCompletionType = context.Compilation.GetOrCreateTypeByMetadataName(
+                    WellKnownTypeNames.SystemRuntimeCompilerServicesICriticalNotifyCompletion);
+
                 context.RegisterOperationBlockStartAction(context =>
                 {
                     if (context.OwningSymbol is not IMethodSymbol methodSymbol ||
@@ -73,14 +78,17 @@ public override void Initialize(AnalysisContext analysisContext)
                     // Ensure that the method is non-generic, non-virtual/override, has no overloads and doesn't have special names: 'GetHashCode' or 'GetEnumerator'.
                     // Also avoid generating this diagnostic if the method body has any invocation expressions.
                     // Also avoid implicit interface implementation (explicit are handled through the member accessibility)
+                    // Also avoid GetAwaiter and GetResult from awaitable-awaiter patterns.
                     if (methodSymbol.IsGenericMethod ||
                         methodSymbol.IsVirtual ||
                         methodSymbol.IsOverride ||
                         methodSymbol.Name == GetHashCodeName ||
                         methodSymbol.Name == GetEnumeratorName ||
                         methodSymbol.ContainingType.GetMembers(methodSymbol.Name).Length > 1 ||
                         taskTypes.Contains(methodSymbol.ReturnType.OriginalDefinition) ||
-                        methodSymbol.IsImplementationOfAnyImplicitInterfaceMember())
+                        methodSymbol.IsImplementationOfAnyImplicitInterfaceMember() ||
+                        methodSymbol.IsGetAwaiterFromAwaitablePattern(inotifyCompletionType, icriticalNotifyCompletionType) ||
+                        methodSymbol.IsGetResultFromAwaiterPattern(inotifyCompletionType, icriticalNotifyCompletionType))
                     {
                         return;
                     }
diff --git a/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs b/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs
@@ -3,7 +3,6 @@
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Linq;
-using System.Threading;
 using Analyzer.Utilities;
 using Analyzer.Utilities.Extensions;
 using Analyzer.Utilities.PooledObjects;
@@ -98,15 +97,7 @@ void OnOperationBlockStart(OperationBlockStartAnalysisContext blockStartContext)
                     }
 
                     // Don't run any other check for this method if it isn't a valid analysis context
-                    if (!ShouldAnalyze(methodSymbol, wellKnownTypeProvider, skippedAttributes,
-                            blockStartContext.Options, isWebProject, blockStartContext.CancellationToken))
-                    {
-                        return;
-                    }
-
-                    // Don't report methods which have a single throw statement
-                    // with NotImplementedException or NotSupportedException
-                    if (blockStartContext.IsMethodNotImplementedOrSupported())
+                    if (!ShouldAnalyze(methodSymbol, wellKnownTypeProvider, skippedAttributes, isWebProject, blockStartContext))
                     {
                         return;
                     }
@@ -191,9 +182,10 @@ private static bool ShouldAnalyze(
             IMethodSymbol methodSymbol,
             WellKnownTypeProvider wellKnownTypeProvider,
             ImmutableArray<INamedTypeSymbol> skippedAttributes,
-            AnalyzerOptions options,
             bool isWebProject,
-            CancellationToken cancellationToken)
+#pragma warning disable RS1012 // Start action has no registered actions
+            OperationBlockStartAnalysisContext blockStartContext)
+#pragma warning restore RS1012 // Start action has no registered actions
         {
             // Modifiers that we don't care about
             if (methodSymbol.IsStatic || methodSymbol.IsOverride || methodSymbol.IsVirtual ||
@@ -208,19 +200,53 @@ private static bool ShouldAnalyze(
                 return false;
             }
 
-            // Do not analyze public APIs for web projects
-            // See https://github.com/dotnet/roslyn-analyzers/issues/3835 for details.
-            if (isWebProject && methodSymbol.IsExternallyVisible())
+            // Don't report methods which have a single throw statement
+            // with NotImplementedException or NotSupportedException
+            if (blockStartContext.IsMethodNotImplementedOrSupported())
             {
                 return false;
             }
 
-            // CA1000 says one shouldn't declare static members on generic types. So don't flag such cases.
-            if (methodSymbol.ContainingType.IsGenericType && methodSymbol.IsExternallyVisible())
+            if (methodSymbol.IsExternallyVisible())
+            {
+                // Do not analyze public APIs for web projects
+                // See https://github.com/dotnet/roslyn-analyzers/issues/3835 for details.
+                if (isWebProject)
+                {
+                    return false;
+                }
+
+                // CA1000 says one shouldn't declare static members on generic types. So don't flag such cases.
+                if (methodSymbol.ContainingType.IsGenericType)
+                {
+                    return false;
+                }
+            }
+
+            // We consider that auto-property have the intent to always be instance members so we want to workaround this issue.
+            if (methodSymbol.IsAutoPropertyAccessor())
             {
                 return false;
             }
 
+            // Awaitable-awaiter pattern members should not be marked as static.
+            // There is no need to check for INotifyCompletion or ICriticalNotifyCompletion members as they are already excluded.
+            if (wellKnownTypeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesINotifyCompletion, out var inotifyCompletionType)
+                && wellKnownTypeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesICriticalNotifyCompletion, out var icriticalNotifyCompletionType))
+            {
+                if (methodSymbol.IsGetAwaiterFromAwaitablePattern(inotifyCompletionType, icriticalNotifyCompletionType)
+                    || methodSymbol.IsGetResultFromAwaiterPattern(inotifyCompletionType, icriticalNotifyCompletionType))
+                {
+                    return false;
+                }
+
+                if (methodSymbol.AssociatedSymbol is IPropertySymbol property
+                    && property.IsIsCompletedFromAwaiterPattern(inotifyCompletionType, icriticalNotifyCompletionType))
+                {
+                    return false;
+                }
+            }
+
             var attributes = methodSymbol.GetAttributes();
             if (methodSymbol.AssociatedSymbol != null)
             {
@@ -248,14 +274,14 @@ private static bool ShouldAnalyze(
                 return false;
             }
 
-            if (!options.MatchesConfiguredVisibility(Rule, methodSymbol, wellKnownTypeProvider.Compilation, cancellationToken,
-                    defaultRequiredVisibility: SymbolVisibilityGroup.All))
+            var hasCorrectVisibility = blockStartContext.Options.MatchesConfiguredVisibility(Rule, methodSymbol, wellKnownTypeProvider.Compilation,
+                blockStartContext.CancellationToken, defaultRequiredVisibility: SymbolVisibilityGroup.All);
+            if (!hasCorrectVisibility)
             {
                 return false;
             }
 
-            // We consider that auto-property have the intent to always be instance members so we want to workaround this issue.
-            return !methodSymbol.IsAutoPropertyAccessor();
+            return true;
         }
 
         private static bool IsExplicitlyVisibleFromCom(IMethodSymbol methodSymbol, WellKnownTypeProvider wellKnownTypeProvider)
diff --git a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriateTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriateTests.cs
@@ -478,6 +478,65 @@ End Class
 ");
         }
 
+        [Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
+        public async Task AwaiterPattern_INotifyCompletion_NoDiagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+using System;
+using System.Runtime.CompilerServices;
+
+public class DummyAwaiter : INotifyCompletion
+{
+    public object GetResult() => null;
+
+    public bool IsCompleted => false;
+
+    public void OnCompleted(Action continuation) => throw null;
+}");
+        }
+
+        [Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
+        public async Task AwaiterPattern_ICriticalNotifyCompletion_NoDiagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+using System;
+using System.Runtime.CompilerServices;
+
+public class DummyAwaiter : ICriticalNotifyCompletion
+{
+    public object GetResult() => null;
+
+    public bool IsCompleted => false;
+
+    public void OnCompleted(Action continuation) => throw null;
+    public void UnsafeOnCompleted(Action continuation) => throw null;
+}");
+        }
+
+        [Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
+        public async Task AwaitablePattern_NoDiagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+using System;
+using System.Runtime.CompilerServices;
+
+public class DummyAwaitable
+{
+    public DummyAwaiter GetAwaiter() => new DummyAwaiter();
+}
+
+public class DummyAwaiter : INotifyCompletion
+{
+    public void GetResult()
+    {
+    }
+
+    public bool IsCompleted => false;
+
+    public void OnCompleted(Action continuation) => throw null;
+}");
+        }
+
         private static DiagnosticResult GetCA1024CSharpResultAt(int line, int column, string methodName)
 #pragma warning disable RS0030 // Do not used banned APIs
             => VerifyCS.Diagnostic()
diff --git a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStaticTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStaticTests.cs
@@ -1294,6 +1294,69 @@ public void M() {}
             }.RunAsync();
         }
 
+        [Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
+        public async Task AwaiterPattern_INotifyCompletion_NoDiagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+using System;
+using System.Runtime.CompilerServices;
+
+public class DummyAwaiter : INotifyCompletion
+{
+    public void GetResult()
+    {
+    }
+
+    public bool IsCompleted => false;
+
+    public void OnCompleted(Action continuation) => throw null;
+}");
+        }
+
+        [Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
+        public async Task AwaiterPattern_ICriticalNotifyCompletion_NoDiagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+using System;
+using System.Runtime.CompilerServices;
+
+public class DummyAwaiter : ICriticalNotifyCompletion
+{
+    public void GetResult()
+    {
+    }
+
+    public bool IsCompleted => false;
+
+    public void OnCompleted(Action continuation) => throw null;
+    public void UnsafeOnCompleted(Action continuation) => throw null;
+}");
+        }
+
+        [Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
+        public async Task AwaitablePattern_NoDiagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+using System;
+using System.Runtime.CompilerServices;
+
+public class DummyAwaitable
+{
+    public DummyAwaiter GetAwaiter() => new DummyAwaiter();
+}
+
+public class DummyAwaiter : INotifyCompletion
+{
+    public void GetResult()
+    {
+    }
+
+    public bool IsCompleted => false;
+
+    public void OnCompleted(Action continuation) => throw null;
+}");
+        }
+
         private DiagnosticResult GetCSharpResultAt(int line, int column, string symbolName)
 #pragma warning disable RS0030 // Do not used banned APIs
             => VerifyCS.Diagnostic()
diff --git a/src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs b/src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs
@@ -717,5 +717,48 @@ public static bool IsTopLevelStatementsEntryPointMethod([NotNullWhen(true)] this
                    "<Main>$" => true,
                    _ => false
                };
+
+        public static bool IsGetAwaiterFromAwaitablePattern([NotNullWhen(true)] this IMethodSymbol? method,
+            [NotNullWhen(true)] INamedTypeSymbol? inotifyCompletionType,
+            [NotNullWhen(true)] INamedTypeSymbol? icriticalNotifyCompletionType)
+        {
+            if (method is null
+                || !method.Name.Equals("GetAwaiter", StringComparison.Ordinal)
+                || method.Parameters.Length != 0)
+            {
+                return false;
+            }
+
+            var returnType = method.ReturnType?.OriginalDefinition;
+            if (returnType is null)
+            {
+                return false;
+            }
+
+            return returnType.DerivesFrom(inotifyCompletionType) ||
+                returnType.DerivesFrom(icriticalNotifyCompletionType);
+        }
+
+        public static bool IsGetResultFromAwaiterPattern(
+            [NotNullWhen(true)] this IMethodSymbol? method,
+            [NotNullWhen(true)] INamedTypeSymbol? inotifyCompletionType,
+            [NotNullWhen(true)] INamedTypeSymbol? icriticalNotifyCompletionType)
+        {
+            if (method is null
+                || !method.Name.Equals("GetResult", StringComparison.Ordinal)
+                || method.Parameters.Length != 0)
+            {
+                return false;
+            }
+
+            var containingType = method.ContainingType?.OriginalDefinition;
+            if (containingType is null)
+            {
+                return false;
+            }
+
+            return containingType.DerivesFrom(inotifyCompletionType) ||
+                containingType.DerivesFrom(icriticalNotifyCompletionType);
+        }
     }
 }
diff --git a/src/Utilities/Compiler/Extensions/IPropertySymbolExtensions.cs b/src/Utilities/Compiler/Extensions/IPropertySymbolExtensions.cs
@@ -1,5 +1,7 @@
 ﻿// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+using System;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using Microsoft.CodeAnalysis;
 
@@ -13,5 +15,22 @@ internal static class IPropertySymbolExtensions
         /// </summary>
         public static bool IsAutoProperty(this IPropertySymbol propertySymbol)
             => propertySymbol.ContainingType.GetMembers().OfType<IFieldSymbol>().Any(f => f.IsImplicitlyDeclared && propertySymbol.Equals(f.AssociatedSymbol));
+
+        public static bool IsIsCompletedFromAwaiterPattern(
+            [NotNullWhen(true)] this IPropertySymbol? property,
+            [NotNullWhen(true)] INamedTypeSymbol? inotifyCompletionType,
+            [NotNullWhen(true)] INamedTypeSymbol? icriticalNotifyCompletionType)
+        {
+            if (property is null
+                || !property.Name.Equals("IsCompleted", StringComparison.Ordinal)
+                || property.Type?.SpecialType != SpecialType.System_Boolean)
+            {
+                return false;
+            }
+
+            var containingType = property.ContainingType?.OriginalDefinition;
+            return containingType.DerivesFrom(inotifyCompletionType)
+                || containingType.DerivesFrom(icriticalNotifyCompletionType);
+        }
     }
 }
diff --git a/src/Utilities/Compiler/WellKnownTypeNames.cs b/src/Utilities/Compiler/WellKnownTypeNames.cs
@@ -251,6 +251,8 @@ internal static class WellKnownTypeNames
         public const string SystemRuntimeCompilerServicesCallerMemberNameAttribute = "System.Runtime.CompilerServices.CallerMemberNameAttribute";
         public const string SystemRuntimeCompilerServicesCompilerGeneratedAttribute = "System.Runtime.CompilerServices.CompilerGeneratedAttribute";
         public const string SystemRuntimeCompilerServicesConfiguredValueTaskAwaitable1 = "System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1";
+        public const string SystemRuntimeCompilerServicesICriticalNotifyCompletion = "System.Runtime.CompilerServices.ICriticalNotifyCompletion";
+        public const string SystemRuntimeCompilerServicesINotifyCompletion = "System.Runtime.CompilerServices.INotifyCompletion";
         public const string SystemRuntimeCompilerServicesInternalsVisibleToAttribute = "System.Runtime.CompilerServices.InternalsVisibleToAttribute";
         public const string SystemRuntimeCompilerServicesRestrictedInternalsVisibleToAttribute = "System.Runtime.CompilerServices.RestrictedInternalsVisibleToAttribute";
         public const string SystemRuntimeCompilerServicesTypeForwardedToAttribute = "System.Runtime.CompilerServices.TypeForwardedToAttribute";
