Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Analyzer: Prefer Memory overloads for Stream async Read/Write methods #3497

Merged
merged 14 commits into from
Apr 29, 2020
Merged

Analyzer: Prefer Memory overloads for Stream async Read/Write methods #3497

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a66c256
Analyzer: Prefer Memory overloads for Stream async Read/Write methods
carlossanlop Apr 14, 2020
3395f4e
Address PR suggestions. Pending changing the text in the messages.
carlossanlop Apr 15, 2020
5db96f0
Enclose types in messages with single quotes. Add small explanation i…
carlossanlop Apr 15, 2020
435adca
Address additional suggestions.
carlossanlop Apr 15, 2020
679204a
Avoid diagnostic if invocation is not awaiting. Added unit tests.
carlossanlop Apr 16, 2020
69b2902
Bring back ConfigureAwait
carlossanlop Apr 23, 2020
0538844
Bump RuleID number.
carlossanlop Apr 23, 2020
cfe3802
Agnostic language diagnostic message
carlossanlop Apr 23, 2020
ee7e3ab
add missing nullcheck, add extra test for stream and buffer passed as…
carlossanlop Apr 24, 2020
93190ab
Minor correction to latest unit test
carlossanlop Apr 24, 2020
3369442
Add unit test that retrieves buffer from method invocation.
carlossanlop Apr 24, 2020
5aec81f
Address suggestions and register diagnostic for await operations.
carlossanlop Apr 28, 2020
4870bda
Do not use Children, use awaitOperation.Operation.
carlossanlop Apr 28, 2020
9a3bc64
buyaa-n 's suggested comment fix
carlossanlop Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 93 additions & 86 deletions ...NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/PreferStreamAsyncMemoryOverloads.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public sealed class PreferStreamAsyncMemoryOverloads : DiagnosticAnalyzer
MicrosoftNetCoreAnalyzersResources.ResourceManager,
typeof(MicrosoftNetCoreAnalyzersResources));

private static readonly LocalizableString s_localizableMessageRead = new LocalizableResourceString(
private static readonly LocalizableString s_localizableMessage = new LocalizableResourceString(
nameof(MicrosoftNetCoreAnalyzersResources.PreferStreamAsyncMemoryOverloadsMessage),
MicrosoftNetCoreAnalyzersResources.ResourceManager,
typeof(MicrosoftNetCoreAnalyzersResources));
Expand All @@ -52,11 +52,6 @@ public sealed class PreferStreamAsyncMemoryOverloads : DiagnosticAnalyzer
MicrosoftNetCoreAnalyzersResources.ResourceManager,
typeof(MicrosoftNetCoreAnalyzersResources));

private static readonly LocalizableString s_localizableMessageWrite = new LocalizableResourceString(
nameof(MicrosoftNetCoreAnalyzersResources.PreferStreamAsyncMemoryOverloadsMessage),
MicrosoftNetCoreAnalyzersResources.ResourceManager,
typeof(MicrosoftNetCoreAnalyzersResources));

private static readonly LocalizableString s_localizableDescriptionWrite = new LocalizableResourceString(
nameof(MicrosoftNetCoreAnalyzersResources.PreferStreamWriteAsyncMemoryOverloadsDescription),
MicrosoftNetCoreAnalyzersResources.ResourceManager,
Expand All @@ -65,7 +60,7 @@ public sealed class PreferStreamAsyncMemoryOverloads : DiagnosticAnalyzer
internal static DiagnosticDescriptor PreferStreamReadAsyncMemoryOverloadsRule = DiagnosticDescriptorHelper.Create(
RuleId,
s_localizableTitleRead,
s_localizableMessageRead,
s_localizableMessage,
DiagnosticCategory.Performance,
RuleLevel.IdeSuggestion,
s_localizableDescriptionRead,
Expand All @@ -75,7 +70,7 @@ public sealed class PreferStreamAsyncMemoryOverloads : DiagnosticAnalyzer
internal static DiagnosticDescriptor PreferStreamWriteAsyncMemoryOverloadsRule = DiagnosticDescriptorHelper.Create(
RuleId,
s_localizableTitleWrite,
s_localizableMessageWrite,
s_localizableMessage,
DiagnosticCategory.Performance,
RuleLevel.IdeSuggestion,
s_localizableDescriptionWrite,
Expand Down Expand Up @@ -110,20 +105,6 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
return;
}

// Retrieve the 3+ ReadAsync and WriteSync methods available in Stream
// If we find fewer than 3, the Memory based overloads are not supported in this .NET version
IEnumerable<IMethodSymbol> readAsyncMethodGroup = streamType.GetMembers("ReadAsync").OfType<IMethodSymbol>();
if (readAsyncMethodGroup.Count() < 3)
{
return;
}

IEnumerable<IMethodSymbol> writeAsyncMethodGroup = streamType.GetMembers("WriteAsync").OfType<IMethodSymbol>();
if (writeAsyncMethodGroup.Count() < 3)
{
return;
}

// Find the additional types for this analysis
if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingCancellationToken, out INamedTypeSymbol? cancellationTokenType))
{
Expand All @@ -142,33 +123,6 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
return;
}

// Retrieve the ConfigureAwait methods that could also be detected:
// - The undesired WriteAsync methods return a Task
// - The preferred WriteAsync method returns a ValueTask
// - The undesired ReadAsync methods return a Task<int>
// - The preferred ReadAsync method returns a ValueTask<int>
if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksTask, out INamedTypeSymbol? taskType))
{
return;
}

if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksGenericTask, out INamedTypeSymbol? genericTaskType))
{
return;
}

IMethodSymbol? configureAwaitMethod = taskType.GetMembers("ConfigureAwait").OfType<IMethodSymbol>().FirstOrDefault();
if (configureAwaitMethod == null)
{
return;
}

IMethodSymbol? genericConfigureAwaitMethod = genericTaskType.GetMembers("ConfigureAwait").OfType<IMethodSymbol>().FirstOrDefault();
if (genericConfigureAwaitMethod == null)
{
return;
}

// Create the arrays with the exact parameter order of the undesired methods
var undesiredParameters = new[]
{
Expand All @@ -185,6 +139,24 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
ParameterInfo.GetParameterInfo(cancellationTokenType)
};

// Create the arrays with the exact parameter order of the undesired methods
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
var preferredReadAsyncParameters = new[]
{
ParameterInfo.GetParameterInfo(readOnlyMemoryType), // ReadOnlyMemory<byte> buffer
ParameterInfo.GetParameterInfo(cancellationTokenType), // CancellationToken
};

var preferredWriteAsyncParameters = new[]
{
ParameterInfo.GetParameterInfo(memoryType), // ReadOnlyMemory<byte> buffer
ParameterInfo.GetParameterInfo(cancellationTokenType), // CancellationToken
};

// Retrieve the ReadAsync/WriteSync methods available in Stream
// If we don't find them all, the Memory based overloads are not supported in this .NET version
IEnumerable<IMethodSymbol> readAsyncMethodGroup = streamType.GetMembers("ReadAsync").OfType<IMethodSymbol>();
IEnumerable<IMethodSymbol> writeAsyncMethodGroup = streamType.GetMembers("WriteAsync").OfType<IMethodSymbol>();

// Retrieve the undesired methods
IMethodSymbol? undesiredReadAsyncMethod = readAsyncMethodGroup.GetFirstOrDefaultMemberWithParameterInfos(undesiredParameters);
if (undesiredReadAsyncMethod == null)
Expand All @@ -210,19 +182,6 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
return;
}

// Create the arrays with the exact parameter order of the undesired methods
var preferredReadAsyncParameters = new[]
{
ParameterInfo.GetParameterInfo(readOnlyMemoryType), // ReadOnlyMemory<byte> buffer
ParameterInfo.GetParameterInfo(cancellationTokenType), // CancellationToken
};

var preferredWriteAsyncParameters = new[]
{
ParameterInfo.GetParameterInfo(memoryType), // ReadOnlyMemory<byte> buffer
ParameterInfo.GetParameterInfo(cancellationTokenType), // CancellationToken
};

// Retrieve the preferred methods, which are used for constructing the rule message
IMethodSymbol? preferredReadAsyncMethod = readAsyncMethodGroup.FirstOrDefault(x =>
x.Parameters.Count() == 2 && x.Parameters[0].Type is INamedTypeSymbol type && type.ConstructedFrom.Equals(memoryType));
Expand All @@ -238,30 +197,38 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
return;
}

context.RegisterOperationAction(context =>
// Retrieve the ConfigureAwait methods that could also be detected:
// - The undesired WriteAsync methods return a Task
// - The preferred WriteAsync method returns a ValueTask
// - The undesired ReadAsync methods return a Task<int>
// - The preferred ReadAsync method returns a ValueTask<int>
if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksTask, out INamedTypeSymbol? taskType))
{
IInvocationOperation invocation = (IInvocationOperation)context.Operation;
return;
}

IOperation parentOperation = invocation.Parent;
if (parentOperation == null)
{
return;
}
// Only accept these two cases:
// - await {WriteAsync()|ReadAsync()}
// - await {WriteAsync()|ReadAsync()}.ConfigureAwait()
if (parentOperation.Kind == OperationKind.Await ||
(parentOperation.Parent != null && parentOperation.Parent.Kind == OperationKind.Await &&
parentOperation.Parent is IAwaitOperation awaitOperation && awaitOperation.Operation is IInvocationOperation grandparentInvocation &&
(grandparentInvocation.TargetMethod.OriginalDefinition.Equals(configureAwaitMethod) ||
grandparentInvocation.TargetMethod.OriginalDefinition.Equals(genericConfigureAwaitMethod))))
{
// Verify if the current method's type is or inherits from Stream
if (!IsDefinedBy(invocation.TargetMethod, streamType, out IMethodSymbol method))
{
return;
}
if (!context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksGenericTask, out INamedTypeSymbol? genericTaskType))
{
return;
}

IMethodSymbol? configureAwaitMethod = taskType.GetMembers("ConfigureAwait").OfType<IMethodSymbol>().FirstOrDefault();
if (configureAwaitMethod == null)
{
return;
}

IMethodSymbol? genericConfigureAwaitMethod = genericTaskType.GetMembers("ConfigureAwait").OfType<IMethodSymbol>().FirstOrDefault();
if (genericConfigureAwaitMethod == null)
{
return;
}

context.RegisterOperationAction(context =>
{
IAwaitOperation awaitOperation = (IAwaitOperation)context.Operation;
if (ShouldAnalyze(awaitOperation, configureAwaitMethod, genericConfigureAwaitMethod, streamType, out IMethodSymbol? method) && method != null)
{
DiagnosticDescriptor rule;
string ruleMessageMethod;
string ruleMessagePreferredMethod;
Expand All @@ -281,18 +248,58 @@ parentOperation.Parent is IAwaitOperation awaitOperation && awaitOperation.Opera
}
else
{
// Prevent use of unassigned variables error
return;
}

context.ReportDiagnostic(
invocation.CreateDiagnostic(rule, ruleMessageMethod, ruleMessagePreferredMethod));
context.ReportDiagnostic(awaitOperation.CreateDiagnostic(rule, ruleMessageMethod, ruleMessagePreferredMethod));
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
}
},
OperationKind.Await);
}

private static bool ShouldAnalyze(
IAwaitOperation awaitOperation,
IMethodSymbol configureAwaitMethod,
IMethodSymbol genericConfigureAwaitMethod,
INamedTypeSymbol streamType,
out IMethodSymbol? actualMethod)
{
actualMethod = null;

if (!awaitOperation.Children.Any())
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
{
return false;
}

if (!(awaitOperation.Children.FirstOrDefault() is IInvocationOperation invocation))
{
return false;
}

IMethodSymbol method = invocation.TargetMethod;

if (method.OriginalDefinition.Equals(configureAwaitMethod) ||
method.OriginalDefinition.Equals(genericConfigureAwaitMethod))
{
if (invocation.Instance is IInvocationOperation instanceInvocation)
{
method = instanceInvocation.TargetMethod;
}
else
{
return false;
}
}, OperationKind.Invocation);
}

// Verify if the current method's type is or inherits from Stream
return IsDefinedBy(method, streamType, out actualMethod);
}

carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
private static bool IsDefinedBy(IMethodSymbol method, INamedTypeSymbol baseType, out IMethodSymbol actualMethod)
{
actualMethod = method;

while (actualMethod.OverriddenMethod != null)
{
actualMethod = actualMethod.OverriddenMethod;
Expand Down
5 changes: 5 additions & 0 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,11 @@
<target state="new">All 'Stream' based classes provide a 'WriteAsync' overload that takes a 'ReadOnlyMemory&lt;Byte&gt;' as first argument. When possible, prefer the overload that takes a 'ReadOnlyMemory&lt;Byte&gt;', which is more efficient.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsMessage">
<source>Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</source>
<target state="new">Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsTitle">
<source>Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</source>
<target state="new">Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</target>
Expand Down
5 changes: 5 additions & 0 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.de.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,11 @@
<target state="new">All 'Stream' based classes provide a 'WriteAsync' overload that takes a 'ReadOnlyMemory&lt;Byte&gt;' as first argument. When possible, prefer the overload that takes a 'ReadOnlyMemory&lt;Byte&gt;', which is more efficient.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsMessage">
<source>Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</source>
<target state="new">Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsTitle">
<source>Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</source>
<target state="new">Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</target>
Expand Down
5 changes: 5 additions & 0 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.es.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,11 @@
<target state="new">All 'Stream' based classes provide a 'WriteAsync' overload that takes a 'ReadOnlyMemory&lt;Byte&gt;' as first argument. When possible, prefer the overload that takes a 'ReadOnlyMemory&lt;Byte&gt;', which is more efficient.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsMessage">
<source>Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</source>
<target state="new">Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsTitle">
<source>Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</source>
<target state="new">Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</target>
Expand Down
5 changes: 5 additions & 0 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,11 @@
<target state="new">All 'Stream' based classes provide a 'WriteAsync' overload that takes a 'ReadOnlyMemory&lt;Byte&gt;' as first argument. When possible, prefer the overload that takes a 'ReadOnlyMemory&lt;Byte&gt;', which is more efficient.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsMessage">
<source>Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</source>
<target state="new">Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsTitle">
<source>Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</source>
<target state="new">Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</target>
Expand Down
5 changes: 5 additions & 0 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.it.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,11 @@
<target state="new">All 'Stream' based classes provide a 'WriteAsync' overload that takes a 'ReadOnlyMemory&lt;Byte&gt;' as first argument. When possible, prefer the overload that takes a 'ReadOnlyMemory&lt;Byte&gt;', which is more efficient.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsMessage">
<source>Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</source>
<target state="new">Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsTitle">
<source>Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</source>
<target state="new">Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</target>
Expand Down
5 changes: 5 additions & 0 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ja.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,11 @@
<target state="new">All 'Stream' based classes provide a 'WriteAsync' overload that takes a 'ReadOnlyMemory&lt;Byte&gt;' as first argument. When possible, prefer the overload that takes a 'ReadOnlyMemory&lt;Byte&gt;', which is more efficient.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsMessage">
<source>Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</source>
<target state="new">Change the 'WriteAsync' method call to the overload that receives a 'ReadOnlyMemory&lt;Byte&gt;'.</target>
<note />
</trans-unit>
<trans-unit id="PreferStreamWriteAsyncMemoryOverloadsTitle">
<source>Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</source>
<target state="new">Prefer 'ReadOnlyMemory&lt;Byte&gt;' overload for 'WriteAsync'.</target>
Expand Down
Loading