Skip to content

Commit

Permalink
Apply feedbacks
Browse files Browse the repository at this point in the history
  • Loading branch information
@buyaa-n
buyaa-n committed Apr 27, 2022
1 parent 3ddac4c commit 191c334
Show file tree
Hide file tree
Showing 17 changed files with 104 additions and 118 deletions.
4 changes: 2 additions & 2 deletions src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1559,10 +1559,10 @@
<value>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</value>
</data>
<data name="DoNotGuardDictionaryRemoveByContainsKeyTitle" xml:space="preserve">
<value>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</value>
<value>Unnecessary call to 'Dictionary.ContainsKey(key)'</value>
</data>
<data name="RemoveRedundantGuardCallCodeFixTitle" xml:space="preserve">
<value>Remove redundant guard call</value>
<value>Remove unnecessary call</value>
</data>
<data name="BufferBlockCopyLengthMessage" xml:space="preserve">
<value>'Buffer.BlockCopy' expects the number of bytes to be copied for the 'count' argument. Using 'Array.Length' may not match the number of bytes that needs to be copied.</value>
Expand Down
29 changes: 4 additions & 25 deletions .../Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.Fixer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using Analyzer.Utilities;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.NetCore.Analyzers.Performance
{
Expand All @@ -33,11 +32,10 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
}

var diagnostic = context.Diagnostics.FirstOrDefault();

if (!TryParseLocationInfo(diagnostic, DoNotGuardDictionaryRemoveByContainsKey.ConditionalOperation, out var conditionalOperationSpan) ||
!TryParseLocationInfo(diagnostic, DoNotGuardDictionaryRemoveByContainsKey.ChildStatementOperation, out var childStatementOperationSpan) ||
root.FindNode(conditionalOperationSpan) is not SyntaxNode conditionalSyntax ||
root.FindNode(childStatementOperationSpan) is not SyntaxNode childStatementSyntax)
var conditionalOperationSpan = diagnostic.AdditionalLocations[0];

This comment has been minimized.

Sign in to view

Copy link
@mavasani

mavasani Apr 28, 2022

Contributor

Should we have a defensive check to verify there are 2 additional locations on the diagnostic?

This comment has been minimized.

Sign in to view

Copy link
@buyaa-n

buyaa-n Apr 28, 2022

Author Contributor

For now it will always be 2, but can add a check to make sure it will not be changed in the future
var childLocation = diagnostic.AdditionalLocations[1];
if (root.FindNode(conditionalOperationSpan.SourceSpan) is not SyntaxNode conditionalSyntax ||
root.FindNode(childLocation.SourceSpan) is not SyntaxNode childStatementSyntax)
{
return;
}
Expand All @@ -57,25 +55,6 @@ protected abstract Document ReplaceConditionWithChild(Document document, SyntaxN
SyntaxNode conditionalOperationNode,
SyntaxNode childOperationNode);

private static bool TryParseLocationInfo(Diagnostic diagnostic, string propertyKey, out TextSpan span)
{
span = default;

if (!diagnostic.Properties.TryGetValue(propertyKey, out var locationInfo))
return false;

var parts = locationInfo.Split(DoNotGuardDictionaryRemoveByContainsKey.AdditionalDocumentLocationInfoSeparatorArray, StringSplitOptions.None);
if (parts.Length != 2 ||
!int.TryParse(parts[0], out var spanStart) ||
!int.TryParse(parts[1], out var spanLength))
{
return false;
}

span = new TextSpan(spanStart, spanLength);
return true;
}

private class DoNotGuardDictionaryRemoveByContainsKeyCodeAction : DocumentChangeAction
{
public DoNotGuardDictionaryRemoveByContainsKeyCodeAction(Func<CancellationToken, Task<Document>> action)
Expand Down
83 changes: 45 additions & 38 deletions ...s/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardDictionaryRemoveByContainsKey.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ public sealed class DoNotGuardDictionaryRemoveByContainsKey : DiagnosticAnalyzer
public static readonly string[] AdditionalDocumentLocationInfoSeparatorArray = new[] { AdditionalDocumentLocationInfoSeparator };
public const string ConditionalOperation = nameof(ConditionalOperation);
public const string ChildStatementOperation = nameof(ChildStatementOperation);
private const string Remove = nameof(Remove);
private const string ContainsKey = nameof(ContainsKey);

internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create(
RuleId,
Expand All @@ -33,8 +35,7 @@ public sealed class DoNotGuardDictionaryRemoveByContainsKey : DiagnosticAnalyzer
RuleLevel.IdeSuggestion,
CreateLocalizableResourceString(nameof(DoNotGuardDictionaryRemoveByContainsKeyDescription)),
isPortedFxCopRule: false,
isDataflowRule: false,
additionalCustomTags: WellKnownDiagnosticTags.Unnecessary);
isDataflowRule: false);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);

Expand All @@ -47,14 +48,14 @@ public override void Initialize(AnalysisContext context)

private static void OnCompilationStart(CompilationStartAnalysisContext context)
{
if (!TryGetDictionaryTypeAndContainsKeyMethod(context.Compilation, out var dictionaryType, out var containsKeyMethod))
if (!TryGetDictionaryTypeAndMethods(context.Compilation, out var containsKey, out var remove1Param, out var remove2Param))
{
return;
}

context.RegisterOperationAction(context => AnalyzeOperation(context, dictionaryType, containsKeyMethod), OperationKind.Conditional);
context.RegisterOperationAction(context => AnalyzeOperation(context, containsKey, remove1Param, remove2Param), OperationKind.Conditional);

static void AnalyzeOperation(OperationAnalysisContext context, INamedTypeSymbol dictionaryType, IMethodSymbol containsKeyMethod)
static void AnalyzeOperation(OperationAnalysisContext context, IMethodSymbol containsKeyMethod, IMethodSymbol remove1Param, IMethodSymbol remove2Param)
{
var conditionalOperation = (IConditionalOperation)context.Operation;

Expand All @@ -69,6 +70,10 @@ static void AnalyzeOperation(OperationAnalysisContext context, INamedTypeSymbol
if (unaryOperation.Operand is IInvocationOperation operand)
invocationOperation = operand;
break;
case IParenthesizedOperation parenthesizedOperation:
if (parenthesizedOperation.Operand is IInvocationOperation invocation)
invocationOperation = invocation;
break;
default:
return;
}
Expand All @@ -80,18 +85,16 @@ static void AnalyzeOperation(OperationAnalysisContext context, INamedTypeSymbol

if (conditionalOperation.WhenTrue.Children.Any())
{
var properties = ImmutableDictionary.CreateBuilder<string, string?>();
properties[ConditionalOperation] = CreateLocationInfo(conditionalOperation.Syntax);

var additionalLocation = ImmutableArray.CreateBuilder<Location>(1);

This comment has been minimized.

Sign in to view

Copy link
@mavasani

mavasani Apr 28, 2022

Contributor

Better to use the ArrayBuilder here, which is pooled.
additionalLocation.Add(conditionalOperation.Syntax.GetLocation());
switch (conditionalOperation.WhenTrue.Children.First())
{
case IInvocationOperation childInvocationOperation:
if (childInvocationOperation.TargetMethod.Name == "Remove" &&
childInvocationOperation.TargetMethod.OriginalDefinition.ContainingType.Equals(dictionaryType, SymbolEqualityComparer.Default))
if (childInvocationOperation.TargetMethod.OriginalDefinition.Equals(remove1Param, SymbolEqualityComparer.Default) ||
childInvocationOperation.TargetMethod.OriginalDefinition.Equals(remove2Param, SymbolEqualityComparer.Default))
{
properties[ChildStatementOperation] = CreateLocationInfo(childInvocationOperation.Syntax.Parent);

context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, properties.ToImmutable()));
additionalLocation.Add(childInvocationOperation.Syntax.Parent.GetLocation());
context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, additionalLocations: additionalLocation.ToImmutable(), null));
}

break;
Expand All @@ -102,14 +105,13 @@ static void AnalyzeOperation(OperationAnalysisContext context, INamedTypeSymbol
*/

var nestedInvocationOperation = childStatementOperation.Children.OfType<IInvocationOperation>()
.FirstOrDefault(op => op.TargetMethod.Name == "Remove" &&
op.TargetMethod.OriginalDefinition.ContainingType.Equals(dictionaryType, SymbolEqualityComparer.Default));
.FirstOrDefault(op => op.TargetMethod.OriginalDefinition.Equals(remove1Param, SymbolEqualityComparer.Default) ||
op.TargetMethod.OriginalDefinition.Equals(remove2Param, SymbolEqualityComparer.Default));

if (nestedInvocationOperation != null)
{
properties[ChildStatementOperation] = CreateLocationInfo(nestedInvocationOperation.Syntax.Parent);

context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, properties.ToImmutable()));
additionalLocation.Add(nestedInvocationOperation.Syntax.Parent.GetLocation());
context.ReportDiagnostic(invocationOperation.CreateDiagnostic(Rule, additionalLocations: additionalLocation.ToImmutable(), null));
}

break;
Expand All @@ -118,38 +120,43 @@ static void AnalyzeOperation(OperationAnalysisContext context, INamedTypeSymbol
}
}
}
static bool TryGetDictionaryTypeAndContainsKeyMethod(Compilation compilation, [NotNullWhen(true)] out INamedTypeSymbol? dictionaryType, [NotNullWhen(true)] out IMethodSymbol? containsKeyMethod)
static bool TryGetDictionaryTypeAndMethods(Compilation compilation, [NotNullWhen(true)] out IMethodSymbol? containsKey,
[NotNullWhen(true)] out IMethodSymbol? remove1Param, [NotNullWhen(true)] out IMethodSymbol? remove2Param)
{
containsKeyMethod = null;
containsKey = null;
remove1Param = null;
remove2Param = null;

if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericDictionary2, out dictionaryType))
if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsGenericDictionary2, out var dictionary))
{
return false;
}

foreach (var m in dictionaryType.GetMembers("ContainsKey").OfType<IMethodSymbol>())
foreach (var m in dictionary.GetMembers().OfType<IMethodSymbol>())
{
if (m.ReturnType.SpecialType == SpecialType.System_Boolean &&
m.Parameters.Length == 1 &&
m.Parameters[0].Name == "key")
if (m.ReturnType.SpecialType == SpecialType.System_Boolean)
{
containsKeyMethod = m;
return true;
switch (m.Parameters.Length)
{
case 1:
switch (m.Name)
{
case ContainsKey: containsKey = m; break;
case Remove: remove1Param = m; break;
}
break;
case 2:
if (m.Name == Remove)
{
remove2Param = m;
}
break;
}
}
}

return false;
return containsKey != null && remove1Param != null && remove2Param != null;
}
}

private static string CreateLocationInfo(SyntaxNode syntax)
{
// see DiagnosticDescriptorCreationAnalyzer

var location = syntax.GetLocation();
var span = location.SourceSpan;

return $"{span.Start}{AdditionalDocumentLocationInfoSeparator}{span.Length}";
}
}
}
8 changes: 4 additions & 4 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@
<note />
</trans-unit>
<trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
<source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
<target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
<source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
<target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
<note />
</trans-unit>
<trans-unit id="DoNotHardCodeCertificate">
Expand Down Expand Up @@ -2158,8 +2158,8 @@
<note />
</trans-unit>
<trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
<source>Remove redundant guard call</source>
<target state="new">Remove redundant guard call</target>
<source>Remove unnecessary call</source>
<target state="new">Remove unnecessary call</target>
<note />
</trans-unit>
<trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
Expand Down
8 changes: 4 additions & 4 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.de.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@
<note />
</trans-unit>
<trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
<source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
<target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
<source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
<target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
<note />
</trans-unit>
<trans-unit id="DoNotHardCodeCertificate">
Expand Down Expand Up @@ -2158,8 +2158,8 @@
<note />
</trans-unit>
<trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
<source>Remove redundant guard call</source>
<target state="new">Remove redundant guard call</target>
<source>Remove unnecessary call</source>
<target state="new">Remove unnecessary call</target>
<note />
</trans-unit>
<trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
Expand Down
8 changes: 4 additions & 4 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.es.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@
<note />
</trans-unit>
<trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
<source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
<target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
<source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
<target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
<note />
</trans-unit>
<trans-unit id="DoNotHardCodeCertificate">
Expand Down Expand Up @@ -2158,8 +2158,8 @@
<note />
</trans-unit>
<trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
<source>Remove redundant guard call</source>
<target state="new">Remove redundant guard call</target>
<source>Remove unnecessary call</source>
<target state="new">Remove unnecessary call</target>
<note />
</trans-unit>
<trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
Expand Down
8 changes: 4 additions & 4 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.fr.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@
<note />
</trans-unit>
<trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
<source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
<target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
<source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
<target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
<note />
</trans-unit>
<trans-unit id="DoNotHardCodeCertificate">
Expand Down Expand Up @@ -2158,8 +2158,8 @@
<note />
</trans-unit>
<trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
<source>Remove redundant guard call</source>
<target state="new">Remove redundant guard call</target>
<source>Remove unnecessary call</source>
<target state="new">Remove unnecessary call</target>
<note />
</trans-unit>
<trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
Expand Down
8 changes: 4 additions & 4 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.it.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@
<note />
</trans-unit>
<trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
<source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
<target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
<source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
<target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
<note />
</trans-unit>
<trans-unit id="DoNotHardCodeCertificate">
Expand Down Expand Up @@ -2158,8 +2158,8 @@
<note />
</trans-unit>
<trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
<source>Remove redundant guard call</source>
<target state="new">Remove redundant guard call</target>
<source>Remove unnecessary call</source>
<target state="new">Remove unnecessary call</target>
<note />
</trans-unit>
<trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
Expand Down
8 changes: 4 additions & 4 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ja.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@
<note />
</trans-unit>
<trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
<source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
<target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
<source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
<target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
<note />
</trans-unit>
<trans-unit id="DoNotHardCodeCertificate">
Expand Down Expand Up @@ -2158,8 +2158,8 @@
<note />
</trans-unit>
<trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
<source>Remove redundant guard call</source>
<target state="new">Remove redundant guard call</target>
<source>Remove unnecessary call</source>
<target state="new">Remove unnecessary call</target>
<note />
</trans-unit>
<trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
Expand Down
8 changes: 4 additions & 4 deletions ...tAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ko.xlf
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,8 @@
<note />
</trans-unit>
<trans-unit id="DoNotGuardDictionaryRemoveByContainsKeyTitle">
<source>Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</source>
<target state="new">Do not guard 'Dictionary.Remove(key)' with 'Dictionary.ContainsKey(key)'</target>
<source>Unnecessary call to 'Dictionary.ContainsKey(key)'</source>
<target state="new">Unnecessary call to 'Dictionary.ContainsKey(key)'</target>
<note />
</trans-unit>
<trans-unit id="DoNotHardCodeCertificate">
Expand Down Expand Up @@ -2158,8 +2158,8 @@
<note />
</trans-unit>
<trans-unit id="RemoveRedundantGuardCallCodeFixTitle">
<source>Remove redundant guard call</source>
<target state="new">Remove redundant guard call</target>
<source>Remove unnecessary call</source>
<target state="new">Remove unnecessary call</target>
<note />
</trans-unit>
<trans-unit id="ReplaceStringLiteralWithCharLiteralCodeActionTitle">
Expand Down
Loading

0 comments on commit 191c334

Please sign in to comment.