From ba127dc792c66462003a15218e2f4dd5a3412383 Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Thu, 31 Jan 2019 16:33:47 -0800 Subject: [PATCH] Make the analyzer conservative for handled delegate creation cases Prior implementation looked for specific unhandled operation tree shapes and was agressive, leading to false positives. New implementation handles only specific operation tree shapes and is much more conservative. Fixes #32946 Fixes #32924 --- .../RemoveUnusedValueAssignmentTests.cs | 128 ++++++++++++ ...lyzer.SymbolStartAnalyzer.BlockAnalyzer.cs | 196 ++++++++++-------- 2 files changed, 233 insertions(+), 91 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs b/src/EditorFeatures/CSharpTest/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs index 8e88b716d2e..1540d993751 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 6b93e9ff6a0..629f227c30b 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. -- GitLab