diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Analyzer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Analyzer.cs index 51f85db38f..dcf2a13a3f 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Analyzer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Analyzer.cs @@ -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"; @@ -88,6 +89,7 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context) invocation, containingMethod, cancellationTokenType, + out int shouldFix, out string? cancellationTokenArgumentName, out string? invocationTokenParameterName)) { @@ -98,6 +100,7 @@ private void AnalyzeCompilationStart(CompilationStartAnalysisContext context) SyntaxNode? nodeToDiagnose = GetInvocationMethodNameNode(context.Operation.Syntax) ?? context.Operation.Syntax; ImmutableDictionary.Builder properties = ImmutableDictionary.CreateBuilder(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 @@ -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; @@ -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; } @@ -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) { @@ -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; @@ -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++) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Fixer.cs index 880c04c866..7ec3eb137c 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Fixer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Fixer.cs @@ -63,8 +63,16 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) ImmutableDictionary? 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; } diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs index fe54245c80..7b464d7d3a 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs @@ -3,7 +3,9 @@ using System; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Testing; +using Test.Utilities; using Xunit; using VerifyCS = Test.Utilities.CSharpCodeFixVerifier< Microsoft.NetCore.CSharp.Analyzers.Runtime.CSharpForwardCancellationTokenToInvocationsAnalyzer, @@ -357,6 +359,84 @@ public static void MyMethod(this MyClass mc, CancellationToken c) { } return CS8VerifyAnalyzerAsync(originalCode); } + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_NoDiagnostic_ParametersDifferMoreThanOne() + { + return CS8VerifyAnalyzerAsync(@" +using System; +using System.Threading; +class C +{ + void MyMethod(int i) {} + void MyMethod(int i, bool b) {} + void MyMethod(int i, bool b, CancellationToken c) {} + + public void M(CancellationToken ct) + { + MyMethod(1); + } +} + "); + } + + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_NoDiagnostic_LambdaAndExtensionMethod_NoTokenInLambda() + { + // Only for local methods will we look for the ct in the top-most ancestor + // For anonymous methods we will only look in the immediate ancestor + return VerifyCS.VerifyAnalyzerAsync(@" +using System; +using System.Threading; +public static class Extensions +{ + public static void Extension(this bool b, Action action) {} + public static void MyMethod(this int i, CancellationToken c = default) {} +} +class C +{ + public void M(CancellationToken ct) + { + bool b = false; + b.Extension((j) => + { + j.MyMethod(); + }); + } +} + "); + } + + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_NoDiagnostic_AnonymousDelegateAndExtensionMethod_NoTokenInAnonymousDelegate() + { + // Only for local methods will we look for the ct in the top-most ancestor + // For anonymous methods we will only look in the immediate ancestor + return VerifyCS.VerifyAnalyzerAsync(@" +using System; +using System.Threading; +public static class Extensions +{ + public delegate void MyDelegate(int i); + public static void Extension(this bool b, MyDelegate d) {} + public static void MyMethod(this int i, CancellationToken c = default) {} +} +class C +{ + public void M(CancellationToken ct) + { + bool b = false; + b.Extension((int j) => + { + j.MyMethod(); + }); + } +} + "); + } + #endregion #region Diagnostics with no fix = C# @@ -413,6 +493,62 @@ void M(CancellationToken ct) return CS8VerifyAnalyzerAsync(originalCode); } + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_AnalyzerOnlyDiagnostic_StaticLocalMethod() + { + // Local static functions are available in C# >= 8.0 + // The user should fix convert the static local method into a non-static local method, + // or pass `default` or `CancellationToken.None` manually + string originalCode = @" +using System; +using System.Threading; +class C +{ + public static void MyMethod(int i, CancellationToken c = default) {} + public void M(CancellationToken ct) + { + LocalStaticMethod(); + static void LocalStaticMethod() + { + [|MyMethod|](5); + } + } +} + "; + return CS8VerifyAnalyzerAsync(originalCode); + } + + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_AnalyzerOnlyDiagnostic_LocalMethod_InsideOf_StaticLocalMethod_TokenInTopParent() + { + // Local static functions are available in C# >= 8.0 + // The user should fix convert the static local method into a non-static local method, + // or pass `default` or `CancellationToken.None` manually + string originalCode = @" +using System; +using System.Threading; +class C +{ + public static void MyMethod(int i, CancellationToken c = default) {} + public void M(CancellationToken ct) + { + LocalStaticMethod(); + static void LocalStaticMethod() + { + LocalMethod(); + void LocalMethod() + { + [|MyMethod|](5); + } + } + } +} + "; + return CS8VerifyAnalyzerAsync(originalCode); + } + #endregion #region Diagnostics with fix = C# @@ -1881,61 +2017,6 @@ class O return CS8VerifyCodeFixAsync(originalCode, fixedCode); } - [Fact] - public Task CS_Diagnostic_LambdaAndExtensionMethod() - { - string originalCode = @" -using System; -using System.Threading; -public static class Extensions -{ - public static void Extension(this bool b, Action action) - { - } - public static void MyMethod(this int i, CancellationToken c = default) - { - } -} -class C -{ - public void M(CancellationToken ct) - { - bool b = false; - b.Extension((j) => - { - [|j.MyMethod|](); - }); - } -} - "; - // Notice the ct is passed with a name, because "this int i" is considered implicit - string fixedCode = @" -using System; -using System.Threading; -public static class Extensions -{ - public static void Extension(this bool b, Action action) - { - } - public static void MyMethod(this int i, CancellationToken c = default) - { - } -} -class C -{ - public void M(CancellationToken ct) - { - bool b = false; - b.Extension((j) => - { - j.MyMethod(c: ct); - }); - } -} - "; - return VerifyCS.VerifyCodeFixAsync(originalCode, fixedCode); - } - [Fact] public Task CS_Diagnostic_WithTrivia() { @@ -2002,71 +2083,152 @@ void MethodOverloadWithArguments(int x, CancellationToken c) {} return VerifyCS.VerifyCodeFixAsync(originalCode, fixedCode); } - [Theory] - [InlineData("CancellationToken c", "", "", "")] - [InlineData("", "CancellationToken c", "", "")] - [InlineData("", "", "", "c")] - public Task CS_Diagnostic_MultiNesting(string topMethodParam, string localMethodParam, string extensionTypeParam, string extensionArg) + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_Diagnostic_MultiNesting_TopMethod() { - string originalCode = $@" + string originalCode = @" using System; using System.Threading; using System.Threading.Tasks; -public static class Extensions -{{ - public static void Extension(this bool b, Action{extensionTypeParam} action) {{}} -}} class C -{{ +{ private readonly object lockingObject = new object(); - public void TopMethod({topMethodParam}) - {{ - void LocalMethod({localMethodParam}) - {{ + public void TopMethod(CancellationToken c) + { + void LocalMethod() + { bool b = false; - b.Extension(({extensionArg}) => - {{ - lock (lockingObject) - {{ - [|TokenMethod|](); - }} - }}); - }} - }} - void TokenMethod(CancellationToken ct = default) {{}} -}} + lock (lockingObject) + { + [|TokenMethod|](); + } + } + } + void TokenMethod(CancellationToken ct = default) {} +} "; - string fixedCode = $@" + string fixedCode = @" +using System; +using System.Threading; +using System.Threading.Tasks; +class C +{ + private readonly object lockingObject = new object(); + public void TopMethod(CancellationToken c) + { + void LocalMethod() + { + bool b = false; + lock (lockingObject) + { + TokenMethod(c); + } + } + } + void TokenMethod(CancellationToken ct = default) {} +} + "; + return VerifyCS.VerifyCodeFixAsync(originalCode, fixedCode); + } + + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_Diagnostic_MultiNesting_LocalMethod() + { + string originalCode = @" +using System; +using System.Threading; +using System.Threading.Tasks; +class C +{ + private readonly object lockingObject = new object(); + public void TopMethod() + { + void LocalMethod(CancellationToken c) + { + bool b = false; + lock (lockingObject) + { + [|TokenMethod|](); + } + } + } + void TokenMethod(CancellationToken ct = default) {} +} + "; + string fixedCode = @" using System; using System.Threading; using System.Threading.Tasks; -public static class Extensions -{{ - public static void Extension(this bool b, Action{extensionTypeParam} action) {{}} -}} class C -{{ +{ private readonly object lockingObject = new object(); - public void TopMethod({topMethodParam}) - {{ - void LocalMethod({localMethodParam}) - {{ + public void TopMethod() + { + void LocalMethod(CancellationToken c) + { bool b = false; - b.Extension(({extensionArg}) => - {{ - lock (lockingObject) - {{ - TokenMethod(c); - }} - }}); - }} - }} - void TokenMethod(CancellationToken ct = default) {{}} -}} + lock (lockingObject) + { + TokenMethod(c); + } + } + } + void TokenMethod(CancellationToken ct = default) {} +} "; return VerifyCS.VerifyCodeFixAsync(originalCode, fixedCode); } + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task CS_Diagnostic_LocalMethod_InsideOf_StaticLocalMethodPassingToken() + { + // Local static functions are available in C# >= 8.0 + string originalCode = @" +using System; +using System.Threading; +class C +{ + public static void MyMethod(int i, CancellationToken c = default) {} + public void M(CancellationToken c) + { + LocalStaticMethod(c); + static void LocalStaticMethod(CancellationToken ct) + { + LocalMethod(); + void LocalMethod() + { + [|MyMethod|](5); + } + } + } +} + "; + string fixedCode = @" +using System; +using System.Threading; +class C +{ + public static void MyMethod(int i, CancellationToken c = default) {} + public void M(CancellationToken c) + { + LocalStaticMethod(c); + static void LocalStaticMethod(CancellationToken ct) + { + LocalMethod(); + void LocalMethod() + { + MyMethod(5, ct); + } + } + } +} + "; + return CS8VerifyCodeFixAsync(originalCode, fixedCode); + } + #endregion #region No Diagnostic - VB @@ -2405,6 +2567,69 @@ End Module return VB16VerifyAnalyzerAsync(originalCode); } + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task VB_NoDiagnostic_LambdaAndExtensionMethod_NoTokenInLambda() + { + // Only for local methods will we look for the ct in the top-most ancestor + // For anonymous methods we will only look in the immediate ancestor + string originalCode = @" +Imports System +Imports System.Threading +Imports System.Runtime.CompilerServices + +Module Extensions + + Sub Extension(ByVal b As Boolean, ByVal action As Action(Of Integer)) + End Sub + + + Sub MyMethod(ByVal i As Integer, ByVal Optional c As CancellationToken = Nothing) + End Sub +End Module + +Class C + Public Sub M(ByVal ct As CancellationToken) + Dim b As Boolean = False + b.Extension(Sub(j) + j.MyMethod() + End Sub) + End Sub +End Class + "; + return VerifyVB.VerifyAnalyzerAsync(originalCode); + } + + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task VB_NoDiagnostic_AnonymousDelegateAndExtensionMethod_NoTokenInAnonymousDelegate() + { + // Only for local methods will we look for the ct in the top-most ancestor + // For anonymous methods we will only look in the immediate ancestor + return VerifyVB.VerifyAnalyzerAsync(@" +Imports System +Imports System.Threading +Imports System.Runtime.CompilerServices +Module Extensions + Public Delegate Sub MyDelegate(ByVal i As Integer) + + Sub Extension(ByVal b As Boolean, ByVal d As MyDelegate) + End Sub + + Sub MyMethod(ByVal i As Integer, ByVal Optional c As CancellationToken = Nothing) + End Sub +End Module +Class C + Public Sub M(ByVal ct As CancellationToken) + Dim b As Boolean = False + b.Extension(Sub(ByVal j As Integer) + j.MyMethod() + End Sub) + End Sub +End Class + "); + } + #endregion #region Diagnostics with no fix = VB @@ -3843,61 +4068,6 @@ End Structure return VB16VerifyCodeFixAsync(originalCode, fixedCode); } - [Fact] - public Task VB_Diagnostic_LambdaAndExtensionMethod() - { - // The ct parameter is not in the closest ancestor method (anonymous) - string originalCode = @" -Imports System -Imports System.Threading -Imports System.Runtime.CompilerServices - -Module Extensions - - Sub Extension(ByVal b As Boolean, ByVal action As Action(Of Integer)) - End Sub - - - Sub MyMethod(ByVal i As Integer, ByVal Optional c As CancellationToken = Nothing) - End Sub -End Module - -Class C - Public Sub M(ByVal ct As CancellationToken) - Dim b As Boolean = False - b.Extension(Sub(j) - j.[|MyMethod|]() - End Sub) - End Sub -End Class - "; - string fixedCode = @" -Imports System -Imports System.Threading -Imports System.Runtime.CompilerServices - -Module Extensions - - Sub Extension(ByVal b As Boolean, ByVal action As Action(Of Integer)) - End Sub - - - Sub MyMethod(ByVal i As Integer, ByVal Optional c As CancellationToken = Nothing) - End Sub -End Module - -Class C - Public Sub M(ByVal ct As CancellationToken) - Dim b As Boolean = False - b.Extension(Sub(j) - j.MyMethod(ct) - End Sub) - End Sub -End Class - "; - return VerifyVB.VerifyCodeFixAsync(originalCode, fixedCode); - } - [Fact] public Task VB_Diagnostic_WithTrivia() { @@ -3988,10 +4158,9 @@ End Class return VerifyVB.VerifyCodeFixAsync(originalCode, fixedCode); } - [Theory] - [InlineData("c As CancellationToken", "", "")] - [InlineData("", "(Of CancellationToken)", "c")] - public Task VB_Diagnostic_MultiNesting(string topMethodParam, string extensionTypeParam, string extensionArg) + [Fact] + [WorkItem(3786, "https://github.com/dotnet/roslyn-analyzers/issues/3786")] + public Task VB_Diagnostic_MultiNesting_TopMethod() { // Local methods do not exist in VB, it's the only difference with the CS mirror test string originalCode = $@" @@ -3999,20 +4168,13 @@ Imports System Imports System.Threading Imports System.Threading.Tasks Imports System.Runtime.CompilerServices -Module Extensions - - Sub Extension(ByVal b As Boolean, ByVal action As Action{extensionTypeParam}) - End Sub -End Module Class C Private ReadOnly lockingObject As Object = New Object() - Public Sub TopMethod({topMethodParam}) + Public Sub TopMethod(c As CancellationToken) Dim b As Boolean = False - b.Extension(Sub({extensionArg}) - SyncLock lockingObject - TokenMethod(c) - End SyncLock - End Sub) + SyncLock lockingObject + [|TokenMethod|]() + End SyncLock End Sub Private Sub TokenMethod(ByVal Optional ct As CancellationToken = Nothing) End Sub @@ -4023,20 +4185,13 @@ Imports System Imports System.Threading Imports System.Threading.Tasks Imports System.Runtime.CompilerServices -Module Extensions - - Sub Extension(ByVal b As Boolean, ByVal action As Action{extensionTypeParam}) - End Sub -End Module Class C Private ReadOnly lockingObject As Object = New Object() - Public Sub TopMethod({topMethodParam}) + Public Sub TopMethod(c As CancellationToken) Dim b As Boolean = False - b.Extension(Sub({extensionArg}) - SyncLock lockingObject - TokenMethod(c) - End SyncLock - End Sub) + SyncLock lockingObject + TokenMethod(c) + End SyncLock End Sub Private Sub TokenMethod(ByVal Optional ct As CancellationToken = Nothing) End Sub