Merge pull request #4986 from jmarolf/fix-ca2016-return-value-fix

Do Not consider overloads with different return types for CA2016

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Analyzer.cs

@@ -27,6 +27,7 @@ namespace Microsoft.NetCore.Analyzers.Runtime
  /// - The invocation method signature receives a ct and one is already being explicitly passed, or...
  /// - The invocation method does not have an overload with the exact same arguments that also receives a ct, or...
  /// - The invocation method only has overloads that receive more than one ct.
+ /// - The invocation method return types are not implicitly convertable to one another.
  /// </summary>
  public abstract class ForwardCancellationTokenToInvocationsAnalyzer : DiagnosticAnalyzer
  {
@@ -76,6 +77,10 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
  return;
  }
 
+ // We don't care if these symbols are not defined in our compilation. They are used to special case the Task<T> <-> ValueTask<T> logic
+ context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksTask1, out INamedTypeSymbol? genericTask);
+ context.Compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingTasksValueTask1, out INamedTypeSymbol? genericValueTask);
+
  context.RegisterOperationAction(context =>
  {
  IInvocationOperation invocation = (IInvocationOperation)context.Operation;
@@ -86,9 +91,12 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
  }
 
  if (!ShouldDiagnose(
+ context.Compilation,
  invocation,
  containingMethod,
  cancellationTokenType,
+ genericTask,
+ genericValueTask,
  out int shouldFix,
  out string? cancellationTokenArgumentName,
  out string? invocationTokenParameterName))
@@ -115,12 +123,13 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context)
 
  // Determines if an invocation should trigger a diagnostic for this rule or not.
  private bool ShouldDiagnose(
+ Compilation compilation,
  IInvocationOperation invocation,
  IMethodSymbol containingSymbol,
  INamedTypeSymbol cancellationTokenType,
- out int shouldFix,
- [NotNullWhen(returnValue: true)] out string? ancestorTokenParameterName,
- out string? invocationTokenParameterName)
+ INamedTypeSymbol? genericTask,
+ INamedTypeSymbol? genericValueTask,
+ out int shouldFix, [NotNullWhen(returnValue: true)] out string? ancestorTokenParameterName, out string? invocationTokenParameterName)
  {
  shouldFix = 1;
  ancestorTokenParameterName = null;
@@ -145,7 +154,7 @@ private bool ShouldDiagnose(
  }
  }
  // or an overload that takes a ct at the end
- else if (MethodHasCancellationTokenOverload(method, cancellationTokenType, out overload))
+ else if (MethodHasCancellationTokenOverload(compilation, method, cancellationTokenType, genericTask, genericValueTask, out overload))
  {
  if (ArgumentsImplicitOrNamed(cancellationTokenType, invocation.Arguments))
  {
@@ -321,20 +330,29 @@ lastParameter.Type is IArrayTypeSymbol arrayTypeSymbol &&
 
  // Check if there's a method overload with the same parameters as this one, in the same order, plus a ct at the end.
  private static bool MethodHasCancellationTokenOverload(
+ Compilation compilation,
  IMethodSymbol method,
  ITypeSymbol cancellationTokenType,
+ INamedTypeSymbol? genericTask,
+ INamedTypeSymbol? genericValueTask,
  [NotNullWhen(returnValue: true)] out IMethodSymbol? overload)
  {
  overload = method.ContainingType
  .GetMembers(method.Name)
  .OfType<IMethodSymbol>()
  .FirstOrDefault(methodToCompare =>
- HasSameParametersPlusCancellationToken(cancellationTokenType, method, methodToCompare));
+ HasSameParametersPlusCancellationToken(compilation, cancellationTokenType, genericTask, genericValueTask, method, methodToCompare));
 
  return overload != null;
 
  // Checks if the parameters of the two passed methods only differ in a ct.
- static bool HasSameParametersPlusCancellationToken(ITypeSymbol cancellationTokenType, IMethodSymbol originalMethod, IMethodSymbol methodToCompare)
+ static bool HasSameParametersPlusCancellationToken(
+ Compilation compilation,
+ ITypeSymbol cancellationTokenType,
+ INamedTypeSymbol? genericTask,
+ INamedTypeSymbol? genericValueTask,
+ IMethodSymbol originalMethod,
+ IMethodSymbol methodToCompare)
  {
  // Avoid comparing to itself, or when there are no parameters, or when the last parameter is not a ct
  if (originalMethod.Equals(methodToCompare) ||
@@ -365,7 +383,61 @@ static bool HasSameParametersPlusCancellationToken(ITypeSymbol cancellationToken
  }
  }
 
+ // Overload is valid if its return type is implicitly convertable
+ var toCompareReturnType = methodToCompareWithAllParameters.ReturnType;
+ var originalReturnType = originalMethodWithAllParameters.ReturnType;
+ if (!toCompareReturnType.IsAssignableTo(originalReturnType, compilation))
+ {
+ // Generic Task-like types are special since awaiting them essentially erases the task-like type.
+ // If both types are Task-like we will warn if their generic arguments are convertable to each other.
+ if (IsTaskLikeType(originalReturnType) && IsTaskLikeType(toCompareReturnType) &&
+ originalReturnType is INamedTypeSymbol originalNamedType &&
+ toCompareReturnType is INamedTypeSymbol toCompareNamedType &&
+ TypeArgumentsAreConvertable(originalNamedType, toCompareNamedType))
+ {
+ return true;
+ }
+
+ return false;
+ }
+
  return true;
+
+ bool IsTaskLikeType(ITypeSymbol typeSymbol)
+ {
+ if (genericTask is not null &&
+ typeSymbol.OriginalDefinition.Equals(genericTask))
+ {
+ return true;
+ }
+
+ if (genericValueTask is not null &&
+ typeSymbol.OriginalDefinition.Equals(genericValueTask))
+ {
+ return true;
+ }
+
+ return false;
+ }
+
+ bool TypeArgumentsAreConvertable(INamedTypeSymbol left, INamedTypeSymbol right)
+ {
+ if (left.Arity != 1 ||
+ right.Arity != 1 ||
+ left.Arity != right.Arity)
+ {
+ return false;
+ }
+
+ var leftTypeArgument = left.TypeArguments[0];
+ var rightTypeArgument = right.TypeArguments[0];
+ if (!leftTypeArgument.IsAssignableTo(rightTypeArgument, compilation))
+ {
+ return false;
+ }
+
+ return true;
+ }
  }
  }
  }