From 9bea83559a4ed3de6af6c31b96eb7be84db8fed7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Wed, 30 Dec 2020 20:02:57 +0100 Subject: [PATCH 1/3] Small refactoring --- .../QualityGuidelines/MarkMembersAsStatic.cs | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs b/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs index dce8f94b73..bdfc001b8b 100644 --- a/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs +++ b/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs @@ -98,15 +98,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 +183,10 @@ private static bool ShouldAnalyze( IMethodSymbol methodSymbol, WellKnownTypeProvider wellKnownTypeProvider, ImmutableArray 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,15 +201,31 @@ 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; } @@ -248,14 +257,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) From bc278dab920da9deac3dc7450c0f64a87309b9f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Wed, 30 Dec 2020 21:33:23 +0100 Subject: [PATCH 2/3] CA1822: Do not report on awaitable-awaiter --- .../QualityGuidelines/MarkMembersAsStatic.cs | 19 +++++- .../MarkMembersAsStaticTests.cs | 63 +++++++++++++++++++ .../Extensions/IMethodSymbolExtensions.cs | 43 +++++++++++++ .../Extensions/IPropertySymbolExtensions.cs | 19 ++++++ src/Utilities/Compiler/WellKnownTypeNames.cs | 2 + 5 files changed, 145 insertions(+), 1 deletion(-) diff --git a/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs b/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs index bdfc001b8b..576576ae20 100644 --- 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; @@ -230,6 +229,24 @@ private static bool ShouldAnalyze( 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) { diff --git a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStaticTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStaticTests.cs index 309aad35fa..e19a999938 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStaticTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStaticTests.cs @@ -1286,6 +1286,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) => VerifyCS.Diagnostic() .WithLocation(line, column) diff --git a/src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs b/src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs index abecc85421..320f1f03eb 100644 --- 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 "
$" => 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 index 04bf0ac17e..c933917294 100644 --- 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 /// public static bool IsAutoProperty(this IPropertySymbol propertySymbol) => propertySymbol.ContainingType.GetMembers().OfType().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 index 55b620b352..2a45fad59c 100644 --- 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"; From 63c67a10f498bfa1ce825d53349a0a34b70c14a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Wed, 30 Dec 2020 22:38:25 +0100 Subject: [PATCH 3/3] CA1024: Do not report on awaitable-awaiter --- .../UsePropertiesWhereAppropriate.cs | 10 +++- .../UsePropertiesWhereAppropriateTests.cs | 59 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriate.cs b/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriate.cs index 6670161544..1668001648 100644 --- 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,6 +78,7 @@ 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 || @@ -80,7 +86,9 @@ public override void Initialize(AnalysisContext analysisContext) 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/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriateTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriateTests.cs index a123583892..1850cfec48 100644 --- 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) => VerifyCS.Diagnostic() .WithLocation(line, column)