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

Misc improvements #270

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Misc improvements #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
acdebd2
Refactor and cleanup - step 1
Youssef1313 Jan 20, 2024
5acd0b4
Refactor and cleanup - step 2
Youssef1313 Jan 20, 2024
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
25 changes: 19 additions & 6 deletions src/ErrorProne.NET.Core/CompilationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,20 @@ public static class OperationExtensions
}
}

public sealed class TaskTypesInfo
{
public INamedTypeSymbol? TaskSymbol { get; }
public INamedTypeSymbol? TaskOfTSymbol { get; }
public INamedTypeSymbol? ValueTaskOfTSymbol { get; }

public TaskTypesInfo(Compilation compilation)
{
TaskSymbol = compilation.GetTypeByMetadataName(typeof(Task).FullName);
TaskOfTSymbol = compilation.GetTypeByMetadataName(typeof(Task<>).FullName);
ValueTaskOfTSymbol = compilation.GetTypeByMetadataName(typeof(ValueTask<>).FullName);
}
}

// Copied from internal ICompilationExtensions class from the roslyn codebase
public static class CompilationExtensions
{
Expand Down Expand Up @@ -124,30 +138,29 @@ public static (INamedTypeSymbol? taskType, INamedTypeSymbol? taskOfTType, INamed
return (taskType, taskOfTType, valueTaskOfTType);
}

public static bool IsTaskLike(this ITypeSymbol? returnType, Compilation compilation)
public static bool IsTaskLike(this ITypeSymbol? returnType, TaskTypesInfo info)
{
if (returnType == null)
{
return false;
}

var (taskType, taskOfTType, valueTaskOfTType) = GetTaskTypes(compilation);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path was repeatedly called, and GetTaskTypes would call compilation.GetTypeByMetadataName. For performance, it's recommended to do this call once, in compilation start (i.e, using RegisterCompilationStartAction).

So, instead of passing Compilation here, the symbols calculated in compilation start should be passed. Then, I needed to "group" all task symbols in TaskTypesInfo instead of passing multiple parameters here.

if (taskType == null || taskOfTType == null)
if (info.TaskSymbol == null || info.TaskOfTSymbol == null)
{
return false; // ?
}

if (returnType.Equals(taskType, SymbolEqualityComparer.Default))
if (returnType.Equals(info.TaskSymbol, SymbolEqualityComparer.Default))
{
return true;
}

if (returnType.OriginalDefinition.Equals(taskOfTType, SymbolEqualityComparer.Default))
if (returnType.OriginalDefinition.Equals(info.TaskOfTSymbol, SymbolEqualityComparer.Default))
{
return true;
}

if (returnType.OriginalDefinition.Equals(valueTaskOfTType, SymbolEqualityComparer.Default))
if (returnType.OriginalDefinition.Equals(info.ValueTaskOfTSymbol, SymbolEqualityComparer.Default))
{
return true;
}
Expand Down
9 changes: 4 additions & 5 deletions src/ErrorProne.NET.Core/DiagnosticAnalyzerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,10 @@ protected DiagnosticAnalyzerBase(params DiagnosticDescriptor[] diagnostics)
public sealed override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();

if (ReportDiagnosticsOnGeneratedCode)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
}
Comment on lines -37 to -40
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Analyze | ReportDiagnostics is already the default. So, currently ReportDiagnosticsOnGeneratedCode is broken. This wasn't observed because there are actually no analyzers that overrides this property to return false. So, it was always true.


var flags = ReportDiagnosticsOnGeneratedCode ? GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics : GeneratedCodeAnalysisFlags.None;

context.ConfigureGeneratedCodeAnalysis(flags);

InitializeCore(context);
}
Expand Down
32 changes: 0 additions & 32 deletions src/ErrorProne.NET.Core/NamedSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,37 +84,5 @@ private static string BuildQualifiedAssemblyName(string? nameSpace, string typeN

return $"{symbolType}, {new AssemblyName(assemblySymbol.Identity.GetDisplayName(true))}";
}

public static bool IsDerivedFromInterface(this INamedTypeSymbol namedType, Type type)
{
Contract.Requires(namedType != null);
Contract.Requires(type != null);

return Enumerable.Any(namedType.AllInterfaces, symbol => symbol.IsType(type));
}

public static bool IsExceptionType(this ISymbol? symbol, SemanticModel model)
{
if (!(symbol is INamedTypeSymbol namedSymbol))
{
return false;
}

var exceptionType = model.Compilation.GetTypeByFullName(typeof(Exception).FullName);

return TraverseTypeAndItsBaseTypes(namedSymbol).Any(x => x.Equals(exceptionType, SymbolEqualityComparer.Default));
}

public static bool IsArgumentExceptionType(this ISymbol? symbol, SemanticModel model)
{
if (!(symbol is INamedTypeSymbol namedSymbol))
{
return false;
}

var exceptionType = model.Compilation.GetTypeByFullName(typeof(ArgumentException).FullName);

return TraverseTypeAndItsBaseTypes(namedSymbol).Any(x => x.Equals(exceptionType, SymbolEqualityComparer.Default));
}
}
}
21 changes: 2 additions & 19 deletions src/ErrorProne.NET.Core/SymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,23 +135,6 @@ public static Location GetParameterLocation(this IParameterSymbol parameter)
return parameter.Locations[0];
}

/// <summary>
/// Returns true if a given <paramref name="method"/> is <see cref="Task.ConfigureAwait(bool)"/>.
/// </summary>
public static bool IsConfigureAwait(this IMethodSymbol method, Compilation compilation)
{
// Naive implementation
return method.Name == "ConfigureAwait" && method.ReceiverType.IsTaskLike(compilation);
}

/// <summary>
/// Returns true if a given <paramref name="method"/> is <see cref="Task.ContinueWith(System.Action{System.Threading.Tasks.Task,object},object)"/>.
/// </summary>
public static bool IsContinueWith(this IMethodSymbol method, Compilation compilation)
{
return method.Name == "ContinueWith" && method.ReceiverType.IsTaskLike(compilation) && method.ReturnType.IsTaskLike(compilation);
}

/// <summary>
/// Returns true if a given <paramref name="method"/> has iterator block inside of it.
/// </summary>
Expand All @@ -168,15 +151,15 @@ public static bool IsIteratorBlock(this IMethodSymbol method)
/// <summary>
/// Returns true if the given <paramref name="method"/> is async or return task-like type.
/// </summary>
public static bool IsAsyncOrTaskBased(this IMethodSymbol method, Compilation compilation)
public static bool IsAsyncOrTaskBased(this IMethodSymbol method, TaskTypesInfo info)
{
// Currently method detects only Task<T> or ValueTask<T>
if (method.IsAsync)
{
return true;
}

return method.ReturnType.IsTaskLike(compilation);
return method.ReturnType.IsTaskLike(info);
}

/// <summary>
Expand Down
9 changes: 0 additions & 9 deletions src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/ConfigureAwaitConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,4 @@ public enum ConfigureAwait
UseConfigureAwaitFalse,
DoNotUseConfigureAwait,
}

internal static class TempExtensions
{
public static bool IsConfigureAwait(this IMethodSymbol method, Compilation compilation)
{
// Naive implementation
return method.Name == "ConfigureAwait" && method.ReceiverType.IsTaskLike(compilation);
}
}
}
71 changes: 44 additions & 27 deletions src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/ConfigureAwaitRequiredAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
using System.Runtime.CompilerServices;
using System.Collections.Immutable;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using ErrorProne.NET.CoreAnalyzers;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using CompilationExtensions = ErrorProne.NET.Core.CompilationExtensions;

namespace ErrorProne.NET.AsyncAnalyzers
{
Expand All @@ -27,38 +26,56 @@ public ConfigureAwaitRequiredAnalyzer()
/// <inheritdoc />
protected override void InitializeCore(AnalysisContext context)
{
context.RegisterSyntaxNodeAction(AnalyzeAwaitExpression, SyntaxKind.AwaitExpression);
context.RegisterCompilationStartAction(context =>
{
var compilation = context.Compilation;

var configureAwaitConfig = ConfigureAwaitConfiguration.TryGetConfigureAwait(context.Compilation);
if (configureAwaitConfig != ConfigureAwait.UseConfigureAwaitFalse)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All callbacks are checking for this. So, we can actually check it once in compilation start, and not register any action at all when not needed.

{
return;
}

var taskType = compilation.GetTypeByMetadataName(typeof(Task).FullName);
if (taskType is null)
{
return;
}

var configureAwaitMethods = taskType.GetMembers("ConfigureAwait").OfType<IMethodSymbol>().ToImmutableArray();
if (configureAwaitMethods.IsEmpty)
{
return;
}

var yieldAwaitable = compilation.GetTypeByMetadataName(typeof(YieldAwaitable).FullName);

context.RegisterOperationAction(context => AnalyzeAwaitOperation(context, configureAwaitMethods, yieldAwaitable), OperationKind.Await);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched from syntax to operation.

At the end, GetOperation is called anyways.

});

}

private void AnalyzeAwaitExpression(SyntaxNodeAnalysisContext context)
private static void AnalyzeAwaitOperation(OperationAnalysisContext context, ImmutableArray<IMethodSymbol> configureAwaitMethods, INamedTypeSymbol? yieldAwaitable)
{
var invocation = (AwaitExpressionSyntax)context.Node;
var awaitOperation = (IAwaitOperation)context.Operation;

var configureAwaitConfig = ConfigureAwaitConfiguration.TryGetConfigureAwait(context.Compilation);
if (configureAwaitConfig == ConfigureAwait.UseConfigureAwaitFalse)
if (awaitOperation.Operation is IInvocationOperation configureAwaitOperation)
{
var operation = context.SemanticModel.GetOperation(invocation, context.CancellationToken);
if (operation is IAwaitOperation awaitOperation)
if (configureAwaitMethods.Contains(configureAwaitOperation.TargetMethod))
{
if (awaitOperation.Operation is IInvocationOperation configureAwaitOperation)
{
if (configureAwaitOperation.TargetMethod.IsConfigureAwait(context.Compilation))
{
return;
}

if (CompilationExtensions.IsClrType(configureAwaitOperation.Type, context.Compilation, typeof(YieldAwaitable)))
{
return;
}
}

var location = awaitOperation.Syntax.GetLocation();
return;
}

var diagnostic = Diagnostic.Create(Rule, location);
context.ReportDiagnostic(diagnostic);
if (SymbolEqualityComparer.Default.Equals(configureAwaitOperation.Type, yieldAwaitable))
{
return;
}
}

var location = awaitOperation.Syntax.GetLocation();

var diagnostic = Diagnostic.Create(Rule, location);
context.ReportDiagnostic(diagnostic);
}
}
}
80 changes: 50 additions & 30 deletions src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/RedundantConfigureAwaitFalseAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using System.Diagnostics.ContractsLight;
using System.Collections.Immutable;
using System.Diagnostics.ContractsLight;
using System.Threading.Tasks;
using ErrorProne.NET.CoreAnalyzers;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
Expand Down Expand Up @@ -27,45 +29,63 @@ public RedundantConfigureAwaitFalseAnalyzer()
/// <inheritdoc />
protected override void InitializeCore(AnalysisContext context)
{
context.RegisterSyntaxNodeAction(AnalyzeAwaitExpression, SyntaxKind.AwaitExpression);
context.RegisterCompilationStartAction(context =>
{
var compilation = context.Compilation;

var configureAwaitConfig = ConfigureAwaitConfiguration.TryGetConfigureAwait(context.Compilation);
if (configureAwaitConfig != ConfigureAwait.DoNotUseConfigureAwait)
{
return;
}

var taskType = compilation.GetTypeByMetadataName(typeof(Task).FullName);
if (taskType is null)
{
return;
}

var configureAwaitMethods = taskType.GetMembers("ConfigureAwait").OfType<IMethodSymbol>().ToImmutableArray();
if (configureAwaitMethods.IsEmpty)
{
return;
}

context.RegisterOperationAction(context => AnalyzeAwaitOperation(context, configureAwaitMethods), OperationKind.Await);
});

}

private void AnalyzeAwaitExpression(SyntaxNodeAnalysisContext context)
private static void AnalyzeAwaitOperation(OperationAnalysisContext context, ImmutableArray<IMethodSymbol> configureAwaitMethods)
{
var invocation = (AwaitExpressionSyntax)context.Node;
var awaitOperation = (IAwaitOperation)context.Operation;

var configureAwaitConfig = ConfigureAwaitConfiguration.TryGetConfigureAwait(context.Compilation);
if (configureAwaitConfig == ConfigureAwait.DoNotUseConfigureAwait)
if (awaitOperation.Operation is IInvocationOperation configureAwaitOperation &&
configureAwaitMethods.Contains(configureAwaitOperation.TargetMethod))
{
var operation = context.SemanticModel.GetOperation(invocation, context.CancellationToken);
if (operation is IAwaitOperation awaitOperation &&
awaitOperation.Operation is IInvocationOperation configureAwaitOperation &&
configureAwaitOperation.TargetMethod.IsConfigureAwait(context.Compilation))
if (configureAwaitOperation.Arguments.Length != 0 &&
configureAwaitOperation.Arguments[0].Value is ILiteralOperation literal &&
literal.ConstantValue.Value?.Equals(false) == true)
{
if (configureAwaitOperation.Arguments.Length != 0 &&
configureAwaitOperation.Arguments[0].Value is ILiteralOperation literal &&
literal.ConstantValue.Value?.Equals(false) == true)
{
var location = configureAwaitOperation.Syntax.GetLocation();
var location = configureAwaitOperation.Syntax.GetLocation();

// Need to find 'ConfigureAwait' node.
if (configureAwaitOperation.Syntax is InvocationExpressionSyntax i &&
i.Expression is
MemberAccessExpressionSyntax mae)
{
// This is a really weird way for getting a location for 'ConfigureAwait(false)' span!
// Need to find 'ConfigureAwait' node.
if (configureAwaitOperation.Syntax is InvocationExpressionSyntax i &&
i.Expression is
MemberAccessExpressionSyntax mae)
{
// This is a really weird way for getting a location for 'ConfigureAwait(false)' span!

var argsLocation = i.ArgumentList.GetLocation();
var nameLocation = mae.Name.GetLocation().SourceSpan;

Contract.Assert(argsLocation.SourceTree != null);
location = Location.Create(argsLocation.SourceTree,
TextSpan.FromBounds(nameLocation.Start, argsLocation.SourceSpan.End));
}
var argsLocation = i.ArgumentList.GetLocation();
var nameLocation = mae.Name.GetLocation().SourceSpan;

var diagnostic = Diagnostic.Create(Rule, location);
context.ReportDiagnostic(diagnostic);
Contract.Assert(argsLocation.SourceTree != null);
location = Location.Create(argsLocation.SourceTree,
TextSpan.FromBounds(nameLocation.Start, argsLocation.SourceSpan.End));
}

var diagnostic = Diagnostic.Create(Rule, location);
context.ReportDiagnostic(diagnostic);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/TaskInstanceToStringConversionAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ public TaskInstanceToStringConversionAnalyzer()
{
}

protected override bool TryCreateDiagnostic(Compilation compilation, ITypeSymbol type, Location location, [NotNullWhen(true)]out Diagnostic? diagnostic)
protected override bool TryCreateDiagnostic(TaskTypesInfo info, ITypeSymbol type, Location location, [NotNullWhen(true)]out Diagnostic? diagnostic)
{
diagnostic = null;

if (type.IsTaskLike(compilation))
if (type.IsTaskLike(info))
{
diagnostic = Diagnostic.Create(Rule, location);
}
Expand Down
Loading