Skip to content

Commit 8673687

Browse files
mavasanimavasani
authored
Merge pull request #4639 from Evangelink/awaitable-awaiter
Do not report on awaitable-awaiter patterns
2 parents 7bd2c98 + 63c67a1 commit 8673687

File tree

7 files changed

+242
-22
lines changed

7 files changed

+242
-22
lines changed

‎src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriate.cs

+9-1
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,11 @@ public override void Initialize(AnalysisContext analysisContext)
5656

5757
var taskTypes = taskTypesBuilder.ToImmutable();
5858

59+
var inotifyCompletionType = context.Compilation.GetOrCreateTypeByMetadataName(
60+
WellKnownTypeNames.SystemRuntimeCompilerServicesINotifyCompletion);
61+
var icriticalNotifyCompletionType = context.Compilation.GetOrCreateTypeByMetadataName(
62+
WellKnownTypeNames.SystemRuntimeCompilerServicesICriticalNotifyCompletion);
63+
5964
context.RegisterOperationBlockStartAction(context =>
6065
{
6166
if (context.OwningSymbol is not IMethodSymbol methodSymbol ||
@@ -73,14 +78,17 @@ public override void Initialize(AnalysisContext analysisContext)
7378
// Ensure that the method is non-generic, non-virtual/override, has no overloads and doesn't have special names: 'GetHashCode' or 'GetEnumerator'.
7479
// Also avoid generating this diagnostic if the method body has any invocation expressions.
7580
// Also avoid implicit interface implementation (explicit are handled through the member accessibility)
81+
// Also avoid GetAwaiter and GetResult from awaitable-awaiter patterns.
7682
if (methodSymbol.IsGenericMethod ||
7783
methodSymbol.IsVirtual ||
7884
methodSymbol.IsOverride ||
7985
methodSymbol.Name == GetHashCodeName ||
8086
methodSymbol.Name == GetEnumeratorName ||
8187
methodSymbol.ContainingType.GetMembers(methodSymbol.Name).Length > 1 ||
8288
taskTypes.Contains(methodSymbol.ReturnType.OriginalDefinition) ||
83-
methodSymbol.IsImplementationOfAnyImplicitInterfaceMember())
89+
methodSymbol.IsImplementationOfAnyImplicitInterfaceMember() ||
90+
methodSymbol.IsGetAwaiterFromAwaitablePattern(inotifyCompletionType, icriticalNotifyCompletionType) ||
91+
methodSymbol.IsGetResultFromAwaiterPattern(inotifyCompletionType, icriticalNotifyCompletionType))
8492
{
8593
return;
8694
}

‎src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs

+47-21
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
using System.Collections.Generic;
44
using System.Collections.Immutable;
55
using System.Linq;
6-
using System.Threading;
76
using Analyzer.Utilities;
87
using Analyzer.Utilities.Extensions;
98
using Analyzer.Utilities.PooledObjects;
@@ -98,15 +97,7 @@ void OnOperationBlockStart(OperationBlockStartAnalysisContext blockStartContext)
9897
}
9998

10099
// Don't run any other check for this method if it isn't a valid analysis context
101-
if (!ShouldAnalyze(methodSymbol, wellKnownTypeProvider, skippedAttributes,
102-
blockStartContext.Options, isWebProject, blockStartContext.CancellationToken))
103-
{
104-
return;
105-
}
106-
107-
// Don't report methods which have a single throw statement
108-
// with NotImplementedException or NotSupportedException
109-
if (blockStartContext.IsMethodNotImplementedOrSupported())
100+
if (!ShouldAnalyze(methodSymbol, wellKnownTypeProvider, skippedAttributes, isWebProject, blockStartContext))
110101
{
111102
return;
112103
}
@@ -191,9 +182,10 @@ private static bool ShouldAnalyze(
191182
IMethodSymbol methodSymbol,
192183
WellKnownTypeProvider wellKnownTypeProvider,
193184
ImmutableArray<INamedTypeSymbol> skippedAttributes,
194-
AnalyzerOptions options,
195185
bool isWebProject,
196-
CancellationToken cancellationToken)
186+
#pragma warning disable RS1012 // Start action has no registered actions
187+
OperationBlockStartAnalysisContext blockStartContext)
188+
#pragma warning restore RS1012 // Start action has no registered actions
197189
{
198190
// Modifiers that we don't care about
199191
if (methodSymbol.IsStatic || methodSymbol.IsOverride || methodSymbol.IsVirtual ||
@@ -208,19 +200,53 @@ private static bool ShouldAnalyze(
208200
return false;
209201
}
210202

211-
// Do not analyze public APIs for web projects
212-
// See https://github.com/dotnet/roslyn-analyzers/issues/3835 for details.
213-
if (isWebProject && methodSymbol.IsExternallyVisible())
203+
// Don't report methods which have a single throw statement
204+
// with NotImplementedException or NotSupportedException
205+
if (blockStartContext.IsMethodNotImplementedOrSupported())
214206
{
215207
return false;
216208
}
217209

218-
// CA1000 says one shouldn't declare static members on generic types. So don't flag such cases.
219-
if (methodSymbol.ContainingType.IsGenericType && methodSymbol.IsExternallyVisible())
210+
if (methodSymbol.IsExternallyVisible())
211+
{
212+
// Do not analyze public APIs for web projects
213+
// See https://github.com/dotnet/roslyn-analyzers/issues/3835 for details.
214+
if (isWebProject)
215+
{
216+
return false;
217+
}
218+
219+
// CA1000 says one shouldn't declare static members on generic types. So don't flag such cases.
220+
if (methodSymbol.ContainingType.IsGenericType)
221+
{
222+
return false;
223+
}
224+
}
225+
226+
// We consider that auto-property have the intent to always be instance members so we want to workaround this issue.
227+
if (methodSymbol.IsAutoPropertyAccessor())
220228
{
221229
return false;
222230
}
223231

232+
// Awaitable-awaiter pattern members should not be marked as static.
233+
// There is no need to check for INotifyCompletion or ICriticalNotifyCompletion members as they are already excluded.
234+
if (wellKnownTypeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesINotifyCompletion, out var inotifyCompletionType)
235+
&& wellKnownTypeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemRuntimeCompilerServicesICriticalNotifyCompletion, out var icriticalNotifyCompletionType))
236+
{
237+
if (methodSymbol.IsGetAwaiterFromAwaitablePattern(inotifyCompletionType, icriticalNotifyCompletionType)
238+
|| methodSymbol.IsGetResultFromAwaiterPattern(inotifyCompletionType, icriticalNotifyCompletionType))
239+
{
240+
return false;
241+
}
242+
243+
if (methodSymbol.AssociatedSymbol is IPropertySymbol property
244+
&& property.IsIsCompletedFromAwaiterPattern(inotifyCompletionType, icriticalNotifyCompletionType))
245+
{
246+
return false;
247+
}
248+
}
249+
224250
var attributes = methodSymbol.GetAttributes();
225251
if (methodSymbol.AssociatedSymbol != null)
226252
{
@@ -248,14 +274,14 @@ private static bool ShouldAnalyze(
248274
return false;
249275
}
250276

251-
if (!options.MatchesConfiguredVisibility(Rule, methodSymbol, wellKnownTypeProvider.Compilation, cancellationToken,
252-
defaultRequiredVisibility: SymbolVisibilityGroup.All))
277+
var hasCorrectVisibility = blockStartContext.Options.MatchesConfiguredVisibility(Rule, methodSymbol, wellKnownTypeProvider.Compilation,
278+
blockStartContext.CancellationToken, defaultRequiredVisibility: SymbolVisibilityGroup.All);
279+
if (!hasCorrectVisibility)
253280
{
254281
return false;
255282
}
256283

257-
// We consider that auto-property have the intent to always be instance members so we want to workaround this issue.
258-
return !methodSymbol.IsAutoPropertyAccessor();
284+
return true;
259285
}
260286

261287
private static bool IsExplicitlyVisibleFromCom(IMethodSymbol methodSymbol, WellKnownTypeProvider wellKnownTypeProvider)

‎src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UsePropertiesWhereAppropriateTests.cs

+59
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,65 @@ End Class
478478
");
479479
}
480480

481+
[Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
482+
public async Task AwaiterPattern_INotifyCompletion_NoDiagnostic()
483+
{
484+
await VerifyCS.VerifyAnalyzerAsync(@"
485+
using System;
486+
using System.Runtime.CompilerServices;
487+
488+
public class DummyAwaiter : INotifyCompletion
489+
{
490+
public object GetResult() => null;
491+
492+
public bool IsCompleted => false;
493+
494+
public void OnCompleted(Action continuation) => throw null;
495+
}");
496+
}
497+
498+
[Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
499+
public async Task AwaiterPattern_ICriticalNotifyCompletion_NoDiagnostic()
500+
{
501+
await VerifyCS.VerifyAnalyzerAsync(@"
502+
using System;
503+
using System.Runtime.CompilerServices;
504+
505+
public class DummyAwaiter : ICriticalNotifyCompletion
506+
{
507+
public object GetResult() => null;
508+
509+
public bool IsCompleted => false;
510+
511+
public void OnCompleted(Action continuation) => throw null;
512+
public void UnsafeOnCompleted(Action continuation) => throw null;
513+
}");
514+
}
515+
516+
[Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
517+
public async Task AwaitablePattern_NoDiagnostic()
518+
{
519+
await VerifyCS.VerifyAnalyzerAsync(@"
520+
using System;
521+
using System.Runtime.CompilerServices;
522+
523+
public class DummyAwaitable
524+
{
525+
public DummyAwaiter GetAwaiter() => new DummyAwaiter();
526+
}
527+
528+
public class DummyAwaiter : INotifyCompletion
529+
{
530+
public void GetResult()
531+
{
532+
}
533+
534+
public bool IsCompleted => false;
535+
536+
public void OnCompleted(Action continuation) => throw null;
537+
}");
538+
}
539+
481540
private static DiagnosticResult GetCA1024CSharpResultAt(int line, int column, string methodName)
482541
#pragma warning disable RS0030 // Do not used banned APIs
483542
=> VerifyCS.Diagnostic()

‎src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStaticTests.cs

+63
Original file line numberDiff line numberDiff line change
@@ -1294,6 +1294,69 @@ public void M() {}
12941294
}.RunAsync();
12951295
}
12961296

1297+
[Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
1298+
public async Task AwaiterPattern_INotifyCompletion_NoDiagnostic()
1299+
{
1300+
await VerifyCS.VerifyAnalyzerAsync(@"
1301+
using System;
1302+
using System.Runtime.CompilerServices;
1303+
1304+
public class DummyAwaiter : INotifyCompletion
1305+
{
1306+
public void GetResult()
1307+
{
1308+
}
1309+
1310+
public bool IsCompleted => false;
1311+
1312+
public void OnCompleted(Action continuation) => throw null;
1313+
}");
1314+
}
1315+
1316+
[Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
1317+
public async Task AwaiterPattern_ICriticalNotifyCompletion_NoDiagnostic()
1318+
{
1319+
await VerifyCS.VerifyAnalyzerAsync(@"
1320+
using System;
1321+
using System.Runtime.CompilerServices;
1322+
1323+
public class DummyAwaiter : ICriticalNotifyCompletion
1324+
{
1325+
public void GetResult()
1326+
{
1327+
}
1328+
1329+
public bool IsCompleted => false;
1330+
1331+
public void OnCompleted(Action continuation) => throw null;
1332+
public void UnsafeOnCompleted(Action continuation) => throw null;
1333+
}");
1334+
}
1335+
1336+
[Fact, WorkItem(4623, "https://github.com/dotnet/roslyn-analyzers/issues/4623")]
1337+
public async Task AwaitablePattern_NoDiagnostic()
1338+
{
1339+
await VerifyCS.VerifyAnalyzerAsync(@"
1340+
using System;
1341+
using System.Runtime.CompilerServices;
1342+
1343+
public class DummyAwaitable
1344+
{
1345+
public DummyAwaiter GetAwaiter() => new DummyAwaiter();
1346+
}
1347+
1348+
public class DummyAwaiter : INotifyCompletion
1349+
{
1350+
public void GetResult()
1351+
{
1352+
}
1353+
1354+
public bool IsCompleted => false;
1355+
1356+
public void OnCompleted(Action continuation) => throw null;
1357+
}");
1358+
}
1359+
12971360
private DiagnosticResult GetCSharpResultAt(int line, int column, string symbolName)
12981361
#pragma warning disable RS0030 // Do not used banned APIs
12991362
=> VerifyCS.Diagnostic()

‎src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

+43
Original file line numberDiff line numberDiff line change
@@ -717,5 +717,48 @@ public static bool IsTopLevelStatementsEntryPointMethod([NotNullWhen(true)] this
717717
"<Main>$" => true,
718718
_ => false
719719
};
720+
721+
public static bool IsGetAwaiterFromAwaitablePattern([NotNullWhen(true)] this IMethodSymbol? method,
722+
[NotNullWhen(true)] INamedTypeSymbol? inotifyCompletionType,
723+
[NotNullWhen(true)] INamedTypeSymbol? icriticalNotifyCompletionType)
724+
{
725+
if (method is null
726+
|| !method.Name.Equals("GetAwaiter", StringComparison.Ordinal)
727+
|| method.Parameters.Length != 0)
728+
{
729+
return false;
730+
}
731+
732+
var returnType = method.ReturnType?.OriginalDefinition;
733+
if (returnType is null)
734+
{
735+
return false;
736+
}
737+
738+
return returnType.DerivesFrom(inotifyCompletionType) ||
739+
returnType.DerivesFrom(icriticalNotifyCompletionType);
740+
}
741+
742+
public static bool IsGetResultFromAwaiterPattern(
743+
[NotNullWhen(true)] this IMethodSymbol? method,
744+
[NotNullWhen(true)] INamedTypeSymbol? inotifyCompletionType,
745+
[NotNullWhen(true)] INamedTypeSymbol? icriticalNotifyCompletionType)
746+
{
747+
if (method is null
748+
|| !method.Name.Equals("GetResult", StringComparison.Ordinal)
749+
|| method.Parameters.Length != 0)
750+
{
751+
return false;
752+
}
753+
754+
var containingType = method.ContainingType?.OriginalDefinition;
755+
if (containingType is null)
756+
{
757+
return false;
758+
}
759+
760+
return containingType.DerivesFrom(inotifyCompletionType) ||
761+
containingType.DerivesFrom(icriticalNotifyCompletionType);
762+
}
720763
}
721764
}

‎src/Utilities/Compiler/Extensions/IPropertySymbolExtensions.cs

+19
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
22

3+
using System;
4+
using System.Diagnostics.CodeAnalysis;
35
using System.Linq;
46
using Microsoft.CodeAnalysis;
57

@@ -13,5 +15,22 @@ internal static class IPropertySymbolExtensions
1315
/// </summary>
1416
public static bool IsAutoProperty(this IPropertySymbol propertySymbol)
1517
=> propertySymbol.ContainingType.GetMembers().OfType<IFieldSymbol>().Any(f => f.IsImplicitlyDeclared && propertySymbol.Equals(f.AssociatedSymbol));
18+
19+
public static bool IsIsCompletedFromAwaiterPattern(
20+
[NotNullWhen(true)] this IPropertySymbol? property,
21+
[NotNullWhen(true)] INamedTypeSymbol? inotifyCompletionType,
22+
[NotNullWhen(true)] INamedTypeSymbol? icriticalNotifyCompletionType)
23+
{
24+
if (property is null
25+
|| !property.Name.Equals("IsCompleted", StringComparison.Ordinal)
26+
|| property.Type?.SpecialType != SpecialType.System_Boolean)
27+
{
28+
return false;
29+
}
30+
31+
var containingType = property.ContainingType?.OriginalDefinition;
32+
return containingType.DerivesFrom(inotifyCompletionType)
33+
|| containingType.DerivesFrom(icriticalNotifyCompletionType);
34+
}
1635
}
1736
}

‎src/Utilities/Compiler/WellKnownTypeNames.cs

+2
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,8 @@ internal static class WellKnownTypeNames
251251
public const string SystemRuntimeCompilerServicesCallerMemberNameAttribute = "System.Runtime.CompilerServices.CallerMemberNameAttribute";
252252
public const string SystemRuntimeCompilerServicesCompilerGeneratedAttribute = "System.Runtime.CompilerServices.CompilerGeneratedAttribute";
253253
public const string SystemRuntimeCompilerServicesConfiguredValueTaskAwaitable1 = "System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1";
254+
public const string SystemRuntimeCompilerServicesICriticalNotifyCompletion = "System.Runtime.CompilerServices.ICriticalNotifyCompletion";
255+
public const string SystemRuntimeCompilerServicesINotifyCompletion = "System.Runtime.CompilerServices.INotifyCompletion";
254256
public const string SystemRuntimeCompilerServicesInternalsVisibleToAttribute = "System.Runtime.CompilerServices.InternalsVisibleToAttribute";
255257
public const string SystemRuntimeCompilerServicesRestrictedInternalsVisibleToAttribute = "System.Runtime.CompilerServices.RestrictedInternalsVisibleToAttribute";
256258
public const string SystemRuntimeCompilerServicesTypeForwardedToAttribute = "System.Runtime.CompilerServices.TypeForwardedToAttribute";

0 commit comments

Comments
 (0)