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

Bunch of fixes for CA1859 #6418

Merged
merged 1 commit into from
Jan 9, 2023
Merged

Bunch of fixes for CA1859 #6418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 27 additions & 22 deletions ...alyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseConcreteTypeAnalyzer.Collector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Linq;
using System.Threading;
using Analyzer.Utilities.Extensions;
using Analyzer.Utilities.Lightup;
using Analyzer.Utilities.PooledObjects;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Operations;
Expand All @@ -17,9 +18,9 @@ private sealed class Collector
{
private static readonly ObjectPool<Collector> _pool = new(() => new Collector());

public ConcurrentDictionary<IFieldSymbol, bool> VirtualDispatchFields { get; } = new(SymbolEqualityComparer.Default);
public ConcurrentDictionary<ILocalSymbol, bool> VirtualDispatchLocals { get; } = new(SymbolEqualityComparer.Default);
public ConcurrentDictionary<IParameterSymbol, bool> VirtualDispatchParameters { get; } = new(SymbolEqualityComparer.Default);
public ConcurrentDictionary<IFieldSymbol, PooledConcurrentSet<IMethodSymbol>> VirtualDispatchFields { get; } = new(SymbolEqualityComparer.Default);
public ConcurrentDictionary<ILocalSymbol, PooledConcurrentSet<IMethodSymbol>> VirtualDispatchLocals { get; } = new(SymbolEqualityComparer.Default);
public ConcurrentDictionary<IParameterSymbol, PooledConcurrentSet<IMethodSymbol>> VirtualDispatchParameters { get; } = new(SymbolEqualityComparer.Default);
public ConcurrentDictionary<IMethodSymbol, bool> MethodsAssignedToDelegate { get; } = new(SymbolEqualityComparer.Default);

public ConcurrentDictionary<IFieldSymbol, PooledConcurrentSet<ITypeSymbol>> FieldAssignments { get; } = new(SymbolEqualityComparer.Default);
Expand All @@ -35,9 +36,9 @@ private Collector()

private void Reset()
{
VirtualDispatchFields.Clear();
VirtualDispatchLocals.Clear();
VirtualDispatchParameters.Clear();
DrainDictionary(VirtualDispatchFields);
DrainDictionary(VirtualDispatchLocals);
DrainDictionary(VirtualDispatchParameters);
MethodsAssignedToDelegate.Clear();

DrainDictionary(FieldAssignments);
Expand All @@ -47,7 +48,8 @@ private void Reset()

Void = null;

static void DrainDictionary<T>(ConcurrentDictionary<T, PooledConcurrentSet<ITypeSymbol>> d)
static void DrainDictionary<T, U>(ConcurrentDictionary<T, PooledConcurrentSet<U>> d)
where U : notnull
{
foreach (var kvp in d)
{
Expand Down Expand Up @@ -97,7 +99,7 @@ public void HandleInvocation(IInvocationOperation op)
var fieldRef = (IFieldReferenceOperation)instance;
if (CanUpgrade(fieldRef.Field))
{
VirtualDispatchFields[fieldRef.Field] = true;
RecordVirtualDispatch(fieldRef.Field, op.TargetMethod);
}

break;
Expand All @@ -106,14 +108,14 @@ public void HandleInvocation(IInvocationOperation op)
case OperationKind.ParameterReference:
{
var parameterRef = (IParameterReferenceOperation)instance;
VirtualDispatchParameters[parameterRef.Parameter] = true;
RecordVirtualDispatch(parameterRef.Parameter, op.TargetMethod);
break;
}

case OperationKind.LocalReference:
{
var localRef = (ILocalReferenceOperation)instance;
VirtualDispatchLocals[localRef.Local] = true;
RecordVirtualDispatch(localRef.Local, op.TargetMethod);
break;
}
}
Expand Down Expand Up @@ -306,7 +308,7 @@ private static bool CanUpgrade(IOperation target)
/// Trivial reject for methods that can't be upgraded in order to avoid wasted work.
/// </summary>
private static bool CanUpgrade(IMethodSymbol methodSym)
=> methodSym.DeclaredAccessibility == Accessibility.Private && methodSym.MethodKind == MethodKind.Ordinary;
=> methodSym.DeclaredAccessibility == Accessibility.Private && methodSym.MethodKind == MethodKind.Ordinary && !methodSym.IsImplementationOfAnyInterfaceMember();

/// <summary>
/// Trivial reject for fields that can't be upgraded in order to avoid wasted work.
Expand Down Expand Up @@ -361,7 +363,16 @@ private void GetValueTypes(List<ITypeSymbol> values, IOperation op)
case OperationKind.Coalesce:
{
var colOp = (ICoalesceOperation)op;

var oldCount = values.Count;
GetValueTypes(values, colOp.Value);

if (values.Count > oldCount)
{
// erase any potential nullable annotations of the left-hand value since when the value is null, it doesn't get used
values[^1] = values[^1].WithNullableAnnotation(Analyzer.Utilities.Lightup.NullableAnnotation.NotAnnotated);
}

GetValueTypes(values, colOp.WhenNull);
return;
}
Expand All @@ -373,19 +384,9 @@ private void GetValueTypes(List<ITypeSymbol> values, IOperation op)
case OperationKind.PropertyReference:
case OperationKind.MethodReference:
case OperationKind.LocalReference:
{
if (op.Type != null)
{
values.Add(op.Type!);
}

return;
}

case OperationKind.FieldReference:
{
var fieldRefOp = (IFieldReferenceOperation)op;
if (CanUpgrade(fieldRefOp.Field))
if (op.Type != null)
{
values.Add(op.Type!);
}
Expand Down Expand Up @@ -454,6 +455,10 @@ private void RecordAssignment(IOperation op, ITypeSymbol valueType)
}
}

private void RecordVirtualDispatch(IFieldSymbol field, IMethodSymbol target) => VirtualDispatchFields.GetOrAdd(field, _ => PooledConcurrentSet<IMethodSymbol>.GetInstance(SymbolEqualityComparer.Default)).Add(target);
private void RecordVirtualDispatch(ILocalSymbol local, IMethodSymbol target) => VirtualDispatchLocals.GetOrAdd(local, _ => PooledConcurrentSet<IMethodSymbol>.GetInstance(SymbolEqualityComparer.Default)).Add(target);
private void RecordVirtualDispatch(IParameterSymbol parameter, IMethodSymbol target) => VirtualDispatchParameters.GetOrAdd(parameter, _ => PooledConcurrentSet<IMethodSymbol>.GetInstance(SymbolEqualityComparer.Default)).Add(target);

private void RecordAssignment(IFieldSymbol field, ITypeSymbol valueType) => FieldAssignments.GetOrAdd(field, _ => PooledConcurrentSet<ITypeSymbol>.GetInstance(SymbolEqualityComparer.Default)).Add(valueType);
private void RecordAssignment(ILocalSymbol local, ITypeSymbol valueType) => LocalAssignments.GetOrAdd(local, _ => PooledConcurrentSet<ITypeSymbol>.GetInstance(SymbolEqualityComparer.Default)).Add(valueType);
private void RecordAssignment(IParameterSymbol parameter, ITypeSymbol valueType) => ParameterAssignments.GetOrAdd(parameter.OriginalDefinition, _ => PooledConcurrentSet<ITypeSymbol>.GetInstance(SymbolEqualityComparer.Default)).Add(valueType);
Expand Down
95 changes: 66 additions & 29 deletions src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseConcreteTypeAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ namespace Microsoft.NetCore.Analyzers.Performance
/// * The method must be private.
///
/// * The method must not have been assigned to a delegate.
///
/// * The method must not be the implementation of a partial method definition.
/// </remarks>
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
public sealed partial class UseConcreteTypeAnalyzer : DiagnosticAnalyzer
Expand Down Expand Up @@ -108,6 +110,7 @@ public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.EnableConcurrentExecution();

context.RegisterCompilationStartAction(context =>
{
var voidType = context.Compilation.GetSpecialType(SpecialType.System_Void);
Expand Down Expand Up @@ -141,33 +144,42 @@ public override void Initialize(AnalysisContext context)
private static void Report(SymbolAnalysisContext context, Collector coll)
{
// for all eligible private fields that are used as the receiver for a virtual call
foreach (var field in coll.VirtualDispatchFields.Keys)
foreach (var pair in coll.VirtualDispatchFields)
{
var field = pair.Key;
var methods = pair.Value;

if (coll.FieldAssignments.TryGetValue(field, out var assignments))
{
Report(field, field.Type, assignments, UseConcreteTypeForField);
Report(field, field.Type, assignments, methods, UseConcreteTypeForField);
}
}

// for all eligible local variables that are used as the receiver for a virtual call
foreach (var local in coll.VirtualDispatchLocals.Keys)
foreach (var pair in coll.VirtualDispatchLocals)
{
var local = pair.Key;
var methods = pair.Value;

if (coll.LocalAssignments.TryGetValue(local, out var assignments))
{
Report(local, local.Type, assignments, UseConcreteTypeForLocal);
Report(local, local.Type, assignments, methods, UseConcreteTypeForLocal);
}
}

// for all eligible parameters that are used as the receiver for a virtual call
foreach (var parameter in coll.VirtualDispatchParameters.Keys)
foreach (var pair in coll.VirtualDispatchParameters)
{
var parameter = pair.Key;
var methods = pair.Value;

if (coll.ParameterAssignments.TryGetValue(parameter, out var assignments))
{
if (parameter.ContainingSymbol is IMethodSymbol method)
{
if (CanUpgrade(method))
{
Report(parameter, parameter.Type, assignments, UseConcreteTypeForParameter);
Report(parameter, parameter.Type, assignments, methods, UseConcreteTypeForParameter);
}
}
}
Expand All @@ -182,49 +194,74 @@ private static void Report(SymbolAnalysisContext context, Collector coll)
// only report the method if it never assigned to a delegate
if (CanUpgrade(method))
{
Report(method, method.ReturnType, returns, UseConcreteTypeForMethodReturn);
Report(method, method.ReturnType, returns, null, UseConcreteTypeForMethodReturn);
}
}

void Report(ISymbol sym, ITypeSymbol fromType, PooledConcurrentSet<ITypeSymbol> assignments, DiagnosticDescriptor desc)
void Report(ISymbol sym, ITypeSymbol fromType, PooledConcurrentSet<ITypeSymbol> assignments, PooledConcurrentSet<IMethodSymbol>? targets, DiagnosticDescriptor desc)
{
// a set of the values assigned to the given symbol
using var types = PooledHashSet<ITypeSymbol>.GetInstance(assignments, SymbolEqualityComparer.Default);

// 'void' is the magic value we use to represent null assignment
var assignedNull = types.Remove(coll.Void!);

// We currently only handle the case where there is only a single consistent type of value assigned to the
// We currently only handle the case where there is a single consistent type of value assigned to the
// symbol. If there are multiple different types, we could try to find the common base for these, but it doesn't
// seem worth the complication.
if (types.Count == 1)
if (types.Count != 1)
{
var toType = types.Single();
if (assignedNull)
{
toType = toType.WithNullableAnnotation(Analyzer.Utilities.Lightup.NullableAnnotation.Annotated);
}
return;
}

if (!toType.DerivesFrom(fromType.OriginalDefinition))
{
// can readily replace fromType by toType
return;
}
var toType = types.Single();
if (assignedNull || fromType.NullableAnnotation() == Analyzer.Utilities.Lightup.NullableAnnotation.Annotated)
{
toType = toType.WithNullableAnnotation(Analyzer.Utilities.Lightup.NullableAnnotation.Annotated);
}

if (toType.TypeKind == TypeKind.Class
&& !SymbolEqualityComparer.Default.Equals(fromType, toType)
&& toType.SpecialType != SpecialType.System_Object
&& toType.SpecialType != SpecialType.System_Delegate)
if (!toType.DerivesFrom(fromType.OriginalDefinition))
{
// can readily replace fromType by toType
return;
}

// if any of the methods that are invoked on toType are explicit implementations of interface methods, then we don't want
// to recommend upgrading the type otherwise it would break those call sites
if (targets != null)
{
foreach (var t in targets)
{
var fromTypeName = GetTypeName(fromType);
var toTypeName = GetTypeName(toType);
var diagnostic = sym.CreateDiagnostic(desc, sym.Name, fromTypeName, toTypeName);
context.ReportDiagnostic(diagnostic);
foreach (var m in toType.GetMembers())
{
if (m.IsImplementationOfAnyExplicitInterfaceMember())
{
if (m.IsImplementationOfInterfaceMember(t))
{
return;
}
}
}
}
}

if (toType.TypeKind == TypeKind.Class
&& !SymbolEqualityComparer.Default.Equals(fromType, toType)
&& toType.SpecialType != SpecialType.System_Object
&& toType.SpecialType != SpecialType.System_Delegate)
{
var fromTypeName = GetTypeName(fromType);
var toTypeName = GetTypeName(toType);
var diagnostic = sym.CreateDiagnostic(desc, sym.Name, fromTypeName, toTypeName);
context.ReportDiagnostic(diagnostic);
}
}

bool CanUpgrade(IMethodSymbol methodSym) => !coll.MethodsAssignedToDelegate.ContainsKey(methodSym);
bool CanUpgrade(IMethodSymbol methodSym)
{
return !coll.MethodsAssignedToDelegate.ContainsKey(methodSym)
&& methodSym.PartialDefinitionPart == null;
}

static string GetTypeName(ITypeSymbol type) => type.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat);
}
Expand Down
Loading