Skip to content

Commit

Permalink
Bunch of fixes for CA1859 (#6418)
Browse files Browse the repository at this point in the history 
* When using the ?? operator, the nullable annotation for the left-hand operand is now erased. This prevents the analyzer suggesting to use a replacement nullable type rather than its non-nullable variation.

* We no longer suggest to upgrade the type of a local/field/parameter if the symbol is being used to invoke a method that's is an explicit implementation of an interface method. If the user would upgrade the type, the call to that method would no longer work.

* Ensure that we never recommend upgrading the signature of a method that implements an interface method.

* Don't recommend a method to be upgraded if the method is an implementation of a partial method definition. This is because there might be different implementations of the method with conflicting behavior. Note that if you use #if constructs, the diagnostic may still make recommendations that would break your code since the analyzer only knows about the select #if block.

* Remove a field-specific optimization that was designed to speed up the analyzer since it actually broke analysis of fields, yielding bogus analysis results.

Co-authored-by: Martin Taillefer <mataille@microsoft.com>
  • Loading branch information
@geeknoid
geeknoid and Martin Taillefer authored Jan 9, 2023
1 parent adb9c1c commit 542756c
Show file tree
Hide file tree
Showing 3 changed files with 247 additions and 52 deletions.
49 changes: 27 additions & 22 deletions ...alyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseConcreteTypeAnalyzer.Collector.cs
View file
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
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

0 comments on commit 542756c

Please sign in to comment.