diff --git a/src/EditorFeatures/CSharpTest/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs b/src/EditorFeatures/CSharpTest/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs index 8e88b716d2e287a5589252b717251a7eaf594c00..1540d99375188c7be8f63ec8fe1fe4c0ec6e7abf 100644 --- a/src/EditorFeatures/CSharpTest/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs +++ b/src/EditorFeatures/CSharpTest/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs @@ -6227,6 +6227,134 @@ void M() T Invoke(Func a) { return a(); } } +}", options: PreferDiscard); + } + + [WorkItem(32946, "https://github.com/dotnet/roslyn/issues/32946")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)] + public async Task DelegateEscape_01() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + Action[] M() + { + var [|j|] = 0; + return new Action[1] { () => Console.WriteLine(j) }; + } +}", options: PreferDiscard); + } + + [WorkItem(32946, "https://github.com/dotnet/roslyn/issues/32946")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)] + public async Task DelegateEscape_02() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + Action[] M(Action[] actions) + { + var [|j|] = 0; + actions[0] = () => Console.WriteLine(j); + return actions; + } +}", options: PreferDiscard); + } + + [WorkItem(32946, "https://github.com/dotnet/roslyn/issues/32946")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)] + public async Task DelegateEscape_03() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + Action[,] M(Action[,] actions) + { + var [|j|] = 0; + actions[0, 0] = () => Console.WriteLine(j); + return actions; + } +}", options: PreferDiscard); + } + + [WorkItem(32946, "https://github.com/dotnet/roslyn/issues/32946")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)] + public async Task DelegateEscape_04() + { + await TestMissingInRegularAndScriptAsync( +@"using System; +using System.Collections.Generic; + +class C +{ + List M() + { + var [|j|] = 0; + return new List { () => Console.WriteLine(j) }; + } +}", options: PreferDiscard); + } + + [WorkItem(32946, "https://github.com/dotnet/roslyn/issues/32946")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)] + public async Task DelegateEscape_05() + { + await TestMissingInRegularAndScriptAsync( +@"using System; +using System.Collections.Generic; + +class C +{ + List M() + { + var [|j|] = 0; + var list = new List(); + list.Add(() => Console.WriteLine(j)); + return list; + } +}", options: PreferDiscard); + } + + [WorkItem(32924, "https://github.com/dotnet/roslyn/issues/32924")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)] + public async Task DelegateEscape_06() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + void M() + { + int [|j|] = 0; + Console.CancelKeyPress += (s, e) => e.Cancel = j != 0; + } +}", options: PreferDiscard); + } + + [WorkItem(32924, "https://github.com/dotnet/roslyn/issues/32924")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)] + public async Task DelegateEscape_07() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + void M() + { + int [|j|] = 0; + Console.CancelKeyPress += LocalFunctionHandler; + return; + + void LocalFunctionHandler(object s, ConsoleCancelEventArgs e) => e.Cancel = j != 0; + } }", options: PreferDiscard); } } diff --git a/src/Features/Core/Portable/RemoveUnusedParametersAndValues/AbstractRemoveUnusedParametersAndValuesDiagnosticAnalyzer.SymbolStartAnalyzer.BlockAnalyzer.cs b/src/Features/Core/Portable/RemoveUnusedParametersAndValues/AbstractRemoveUnusedParametersAndValuesDiagnosticAnalyzer.SymbolStartAnalyzer.BlockAnalyzer.cs index 6b93e9ff6a0fbea9533b58e2bd9425a138a2574d..629f227c30b5c7cca24e8d36f3d6c0e7b63f5910 100644 --- a/src/Features/Core/Portable/RemoveUnusedParametersAndValues/AbstractRemoveUnusedParametersAndValuesDiagnosticAnalyzer.SymbolStartAnalyzer.BlockAnalyzer.cs +++ b/src/Features/Core/Portable/RemoveUnusedParametersAndValues/AbstractRemoveUnusedParametersAndValuesDiagnosticAnalyzer.SymbolStartAnalyzer.BlockAnalyzer.cs @@ -30,30 +30,17 @@ private sealed partial class BlockAnalyzer private bool _hasDelegateCreationOrAnonymousFunction; /// - /// Indicates if the operation block has an with a delegate type argument. + /// Indicates if the operation block has an operation that leads to a delegate escaping the current block, + /// which would prevent us from performing accurate flow analysis of lambda/local function invocations + /// within this operation block. + /// Some examples: + /// 1. Delegate assigned to a field or property. + /// 2. Delegate passed as an argument to an invocation or object creation. + /// 3. Delegate added to an array or wrapped within a tuple. + /// 4. Delegate converted to a non-delegate type. /// We use this value in to determine whether to bail from analysis or not. /// - private bool _hasDelegateTypeArgument; - - /// - /// Indicates if a delegate instance escaped this operation block, via an assignment to a field or a property symbol. - /// that can be accessed outside this executable code block. - /// We use this value in to determine whether to bail from analysis or not. - /// - private bool _delegateAssignedToFieldOrProperty; - - /// - /// Indicates if a delegate was wrapped within an . - /// We use this value in to determine whether to bail from analysis or not. - /// - private bool _delegateWrappedInTuple; - - /// - /// Indicates if the operation block has an with a delegate type or an anonymous function - /// as it's source and a non-delegate type as it's target. - /// We use this value in to determine whether to bail from analysis or not. - /// - private bool _hasConversionFromDelegateTypeOrAnonymousFunctionToNonDelegateType; + private bool _hasDelegateEscape; /// /// Indicates if the operation block has an . @@ -103,11 +90,7 @@ public static void Analyze(OperationBlockStartAnalysisContext context, SymbolSta var blockAnalyzer = new BlockAnalyzer(symbolStartAnalyzer, options); context.RegisterOperationAction(blockAnalyzer.AnalyzeExpressionStatement, OperationKind.ExpressionStatement); context.RegisterOperationAction(blockAnalyzer.AnalyzeDelegateCreationOrAnonymousFunction, OperationKind.DelegateCreation, OperationKind.AnonymousFunction); - context.RegisterOperationAction(blockAnalyzer.AnalyzeArgument, OperationKind.Argument); - context.RegisterOperationAction(blockAnalyzer.AnalyzeConversion, OperationKind.Conversion); - context.RegisterOperationAction(blockAnalyzer.AnalyzeTuple, OperationKind.Tuple); - context.RegisterOperationAction(blockAnalyzer.AnalyzeFieldOrPropertyReference, OperationKind.FieldReference, OperationKind.PropertyReference); - context.RegisterOperationAction(blockAnalyzer.AnalyzeParameterReference, OperationKind.ParameterReference); + context.RegisterOperationAction(blockAnalyzer.AnalyzeLocalOrParameterReference, OperationKind.LocalReference, OperationKind.ParameterReference); context.RegisterOperationAction(_ => blockAnalyzer._hasInvalidOperation = true, OperationKind.Invalid); context.RegisterOperationBlockEndAction(blockAnalyzer.AnalyzeOperationBlockEnd); @@ -195,55 +178,110 @@ private void AnalyzeExpressionStatement(OperationAnalysisContext context) } private void AnalyzeDelegateCreationOrAnonymousFunction(OperationAnalysisContext operationAnalysisContext) - => _hasDelegateCreationOrAnonymousFunction = true; - - private void AnalyzeArgument(OperationAnalysisContext operationAnalysisContext) { - var argument = (IArgumentOperation)operationAnalysisContext.Operation; - if (!_hasDelegateTypeArgument && - argument.Value.Type.IsDelegateType()) + _hasDelegateCreationOrAnonymousFunction = true; + if (!_hasDelegateEscape) { - _hasDelegateTypeArgument = true; + _hasDelegateEscape = !IsHandledOperationTreeShape(operationAnalysisContext.Operation); } - } - private void AnalyzeConversion(OperationAnalysisContext operationAnalysisContext) - { - var conversion = (IConversionOperation)operationAnalysisContext.Operation; - if (!_hasConversionFromDelegateTypeOrAnonymousFunctionToNonDelegateType && - (conversion.Operand.Type.IsDelegateType() || conversion.Operand.Kind == OperationKind.AnonymousFunction) && - !conversion.Type.IsDelegateType()) + return; + + // Local functions. + bool IsHandledOperationTreeShape(IOperation operation) { - _hasConversionFromDelegateTypeOrAnonymousFunctionToNonDelegateType = true; + // We only handle certain operation tree shapes in flow analysis of delegate creations + // and consider those as non-delegate escaping operation trees. + // For the remaining unknown ones, we conservatively mark the operation to lead to + // delegate escape, and corresponding bail out from flow analysis in ShouldAnalyze method below. + + // 1. Delegate creation or anonymous function variable initializer is handled. + if (operation.Parent is IVariableInitializerOperation) + { + return true; + } + + // 2. Delegate creation or anonymous function assigned to a local or parameter are handled. + if (operation.Parent is ISimpleAssignmentOperation assignment && + (assignment.Target.Kind == OperationKind.LocalReference || + assignment.Target.Kind == OperationKind.ParameterReference)) + { + return true; + } + + // 3. For anonymous functions parented by delegate creation, we analyze the parent operation. + if (operation.Kind == OperationKind.AnonymousFunction && + operation.Parent is IDelegateCreationOperation) + { + return IsHandledOperationTreeShape(operation.Parent); + } + + // 4. Otherwise, conservatively consider this as an unhandled delegate escape. + return false; } } - private void AnalyzeTuple(OperationAnalysisContext operationAnalysisContext) + private void AnalyzeLocalOrParameterReference(OperationAnalysisContext operationAnalysisContext) { - var tupleOperation = (ITupleOperation)operationAnalysisContext.Operation; - if (!_delegateWrappedInTuple && - tupleOperation.Elements.Any(o => o.Type.IsDelegateType())) + if (operationAnalysisContext.Operation is IParameterReferenceOperation parameterReference) { - _delegateWrappedInTuple = true; + _referencedParameters.GetOrAdd(parameterReference.Parameter, true); } - } - private void AnalyzeFieldOrPropertyReference(OperationAnalysisContext operationAnalysisContextContext) - { - var fieldOrPropertyReference = operationAnalysisContextContext.Operation; - if (!_delegateAssignedToFieldOrProperty && - fieldOrPropertyReference.Type.IsDelegateType() && - fieldOrPropertyReference.Parent is ISimpleAssignmentOperation simpleAssignment && - simpleAssignment.Target.Equals(fieldOrPropertyReference)) + if (!_hasDelegateEscape) { - _delegateAssignedToFieldOrProperty = true; + _hasDelegateEscape = !IsHandledOperationTreeShape(operationAnalysisContext.Operation); } - } - private void AnalyzeParameterReference(OperationAnalysisContext operationAnalysisContextContext) - { - var parameter = ((IParameterReferenceOperation)operationAnalysisContextContext.Operation).Parameter; - _referencedParameters.GetOrAdd(parameter, true); + return; + + // Local functions + bool IsHandledOperationTreeShape(IOperation operation) + { + // We only handle certain operation tree shapes in flow analysis of delegates. + // and consider those as non-delegate escaping operation trees. + // For the remaining unknown ones, we conservatively mark the operation to lead to + // delegate escape, and corresponding bail out from flow analysis in ShouldAnalyze method below. + + // 1. We are only interested in parameters or locals of delegate type. + if (!operation.Type.IsDelegateType()) + { + return true; + } + + // 2. Delegate invocations are handled. + if (operation.Parent is IInvocationOperation) + { + return true; + } + + if (operation.Parent is ISimpleAssignmentOperation assignmentOperation) + { + // 3. Parameter/local as target of an assignment is handled. + if (assignmentOperation.Target == operation) + { + return true; + } + + // 4. Assignment from a parameter or local is only handled if being + // assigned to some parameter or local of delegate type. + if (assignmentOperation.Target.Type.IsDelegateType() && + (assignmentOperation.Target.Kind == OperationKind.LocalReference || + assignmentOperation.Target.Kind == OperationKind.ParameterReference)) + { + return true; + } + } + + // 5. Binary operations on parameter/local are fine. + if (operation.Parent is IBinaryOperation) + { + return true; + } + + // 6. Otherwise, conservatively consider this as an unhandled delegate escape. + return false; + } } /// @@ -276,31 +314,17 @@ private bool ShouldAnalyze(IOperation operationBlock, ISymbol owningSymbol) return true; } - // 2. Bail out if we have a delegate escape via an assigment to a field/property reference. - // This indicates the delegate targets (such as lambda/local functions) have been captured + // 2. Bail out if we have a delegate escape via operation tree shapes that we do not understand. + // This indicates the delegate targets (such as lambda/local functions) have escaped current method // and can be invoked from a separate method, and these invocations can read values written // to any local/parameter in the current method. We cannot reliably flag any write to a // local/parameter as unused for such cases. - if (_delegateAssignedToFieldOrProperty) - { - return false; - } - - // 3. Bail out if we have a conversion from a delegate or an anonymous function to a non-delegate type. - // We can analyze this correctly when we do points-to-analysis. - if (_hasConversionFromDelegateTypeOrAnonymousFunctionToNonDelegateType) + if (_hasDelegateEscape) { return false; } - // 4. Bail out if we pass a delegate type as an argument to a method. - // We can analyze this correctly when we do points-to-analysis. - if (_hasDelegateTypeArgument) - { - return false; - } - - // 5. Bail out for method returning delegates or ref/out parameters of delegate type. + // 3. Bail out for method returning delegates or ref/out parameters of delegate type. // We can analyze this correctly when we do points-to-analysis. if (owningSymbol is IMethodSymbol method && (method.ReturnType.IsDelegateType() || @@ -309,17 +333,7 @@ private bool ShouldAnalyze(IOperation operationBlock, ISymbol owningSymbol) return false; } - // 6. Bail out if we have a delegate wrapped in a tuple. - // This indicates the delegate targets (such as lambda/local functions) have been wrapped - // within a tuple and passed around and invoked, and these invocations can read values written - // to any local/parameter in the current method. We cannot reliably flag any write to a - // local/parameter as unused for such cases. - if (_delegateWrappedInTuple) - { - return false; - } - - // 7. Bail out on invalid operations, i.e. code with semantic errors. + // 4. Bail out on invalid operations, i.e. code with semantic errors. // We are likely to have false positives from flow analysis results // as we will not account for potential lambda/local function invocations. if (_hasInvalidOperation) @@ -327,7 +341,7 @@ private bool ShouldAnalyze(IOperation operationBlock, ISymbol owningSymbol) return false; } - // 7. Otherwise, we execute analysis by walking the reaching symbol write chain to attempt to + // 5. Otherwise, we execute analysis by walking the reaching symbol write chain to attempt to // find the target method being invoked. // This works for most common and simple cases where a local is assigned a lambda and invoked later. // If we are unable to find a target, we will conservatively mark all current symbol writes as read.