Skip to content

Commit

Permalink
Fix CA2016 bugs - diagnostic triggered in edge cases (#3791)
Browse files Browse the repository at this point in the history
* Fixes CA2016

* Local static method should not get ct passed, address PR suggestions

* Diagnostic with no fix for static local method

* Add test for ocal method inside static local method passing a ct
  • Loading branch information
carlossanlop committed Jul 5, 2020
1 parent d89638c commit d752929
Show file tree
Hide file tree
Showing 3 changed files with 382 additions and 187 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public abstract class ForwardCancellationTokenToInvocationsAnalyzer : Diagnostic
isDataflowRule: false
);

internal const string ShouldFix = "ShouldFix";
internal const string ArgumentName = "ArgumentName";
internal const string ParameterName = "ParameterName";

Expand Down Expand Up @@ -88,6 +89,7 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
invocation,
containingMethod,
cancellationTokenType,
out int shouldFix,
out string? cancellationTokenArgumentName,
out string? invocationTokenParameterName))
{
Expand All @@ -98,6 +100,7 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
SyntaxNode? nodeToDiagnose = GetInvocationMethodNameNode(context.Operation.Syntax) ?? context.Operation.Syntax;
ImmutableDictionary<string, string?>.Builder properties = ImmutableDictionary.CreateBuilder<string, string?>(StringComparer.Ordinal);
properties.Add(ShouldFix, $"{shouldFix}");
properties.Add(ArgumentName, cancellationTokenArgumentName); // The new argument to pass to the invocation
properties.Add(ParameterName, invocationTokenParameterName); // If the passed argument should be named, then this will be non-null
Expand All @@ -115,9 +118,11 @@ private bool ShouldDiagnose(
IInvocationOperation invocation,
IMethodSymbol containingSymbol,
INamedTypeSymbol cancellationTokenType,
out int shouldFix,
[NotNullWhen(returnValue: true)] out string? ancestorTokenParameterName,
out string? invocationTokenParameterName)
{
shouldFix = 1;
ancestorTokenParameterName = null;
invocationTokenParameterName = null;

Expand Down Expand Up @@ -153,7 +158,9 @@ private bool ShouldDiagnose(
}

// Check if there is an ancestor method that has a ct that we can pass to the invocation
if (!TryGetClosestAncestorThatTakesAToken(invocation, containingSymbol, cancellationTokenType, out IMethodSymbol? ancestor, out ancestorTokenParameterName))
if (!TryGetClosestAncestorThatTakesAToken(
invocation, containingSymbol, cancellationTokenType,
out shouldFix, out IMethodSymbol? ancestor, out ancestorTokenParameterName))
{
return false;
}
Expand All @@ -175,9 +182,11 @@ private static bool TryGetClosestAncestorThatTakesAToken(
IInvocationOperation invocation,
IMethodSymbol containingSymbol,
INamedTypeSymbol cancellationTokenType,
out int shouldFix,
[NotNullWhen(returnValue: true)] out IMethodSymbol? ancestor,
[NotNullWhen(returnValue: true)] out string? cancellationTokenParameterName)
{
shouldFix = 1;
IOperation currentOperation = invocation.Parent;
while (currentOperation != null)
{
Expand All @@ -193,9 +202,26 @@ private static bool TryGetClosestAncestorThatTakesAToken(
}

// When the current ancestor does not contain a ct, will continue with the next ancestor
if (ancestor != null && TryGetTokenParamName(ancestor, cancellationTokenType, out cancellationTokenParameterName))
if (ancestor != null)
{
return true;
if (TryGetTokenParamName(ancestor, cancellationTokenType, out cancellationTokenParameterName))
{
return true;
}
// If no token param was found in the previous check, return false if the current operation is an anonymous function,
// we don't want to keep checking the superior ancestors because the ct may be unrelated
if (currentOperation.Kind == OperationKind.AnonymousFunction)
{
return false;
}

// If the current operation is a local static function, and is not passing a ct, but the parent is, then the
// ct cannot be passed to the inner invocations of the static local method, but we want to continue trying
// to find the ancestor method passing a ct so that we still trigger a diagnostic, we just won't offer a fix
if (currentOperation.Kind == OperationKind.LocalFunction && ancestor.IsStatic)
{
shouldFix = 0;
}
}

currentOperation = currentOperation.Parent;
Expand Down Expand Up @@ -321,6 +347,12 @@ static bool HasSameParametersPlusCancellationToken(ITypeSymbol cancellationToken
IMethodSymbol originalMethodWithAllParameters = (originalMethod.ReducedFrom ?? originalMethod).OriginalDefinition;
IMethodSymbol methodToCompareWithAllParameters = (methodToCompare.ReducedFrom ?? methodToCompare).OriginalDefinition;

// Ensure parameters only differ by one - the ct
if (originalMethodWithAllParameters.Parameters.Length != methodToCompareWithAllParameters.Parameters.Length - 1)
{
return false;
}

// Now compare the types of all parameters before the ct
// The largest i is the number of parameters in the method that has fewer parameters
for (int i = 0; i < originalMethodWithAllParameters.Parameters.Length; i++)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,16 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)

ImmutableDictionary<string, string>? properties = context.Diagnostics[0].Properties;

if (!properties.TryGetValue(ForwardCancellationTokenToInvocationsAnalyzer.ShouldFix, out string shouldFix) ||
string.IsNullOrEmpty(shouldFix) ||
shouldFix.Equals("0", StringComparison.InvariantCultureIgnoreCase))
{
return;
}

// The name that identifies the object that is to be passed
if (!properties.TryGetValue(ForwardCancellationTokenToInvocationsAnalyzer.ArgumentName, out string argumentName) || string.IsNullOrEmpty(argumentName))
if (!properties.TryGetValue(ForwardCancellationTokenToInvocationsAnalyzer.ArgumentName, out string argumentName) ||
string.IsNullOrEmpty(argumentName))
{
return;
}
Expand Down
Loading

0 comments on commit d752929

Please sign in to comment.