diff --git a/src/EditorFeatures/CSharpTest/InlineDeclaration/CSharpInlineDeclarationTests.cs b/src/EditorFeatures/CSharpTest/InlineDeclaration/CSharpInlineDeclarationTests.cs index 269a92e009b53d66e43b479ee1e5112e49500350..86750180aad8de4f292b4cc56707986783c7e32a 100644 --- a/src/EditorFeatures/CSharpTest/InlineDeclaration/CSharpInlineDeclarationTests.cs +++ b/src/EditorFeatures/CSharpTest/InlineDeclaration/CSharpInlineDeclarationTests.cs @@ -71,9 +71,11 @@ void M() public async Task InlineVariableWithConstructor1() { await TestInRegularAndScriptAsync( -@"class C +@"class C1 { - void M() + public C1(int v, out int i) {} + + void M(int v) { [|int|] i; if (new C1(v, out i)) @@ -81,9 +83,11 @@ void M() } } }", -@"class C +@"class C1 { - void M() + public C1(int v, out int i) {} + + void M(int v) { if (new C1(v, out int i)) { @@ -1825,7 +1829,7 @@ private static int[] GetValue(out string token) [WorkItem(17743, "https://github.com/dotnet/roslyn/issues/17743")] [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineDeclaration)] - public async Task TestInLocalFunction() + public async Task TestInLocalFunction1() { await TestMissingInRegularAndScriptAsync( @" @@ -1851,6 +1855,53 @@ void F() }"); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineDeclaration)] + public async Task TestInLocalFunction2() + { + await TestInRegularAndScriptAsync( +@" +using System; +using System.Collections.Generic; + +class Demo +{ + static void Main() + { + F(); + void F() + { + Action f = () => + { + Dictionary dict = null; + int [|x|] = 0; + dict.TryGetValue(0, out x); + Console.WriteLine(x); + }; + } + } +}", +@" +using System; +using System.Collections.Generic; + +class Demo +{ + static void Main() + { + F(); + void F() + { + Action f = () => + { + Dictionary dict = null; + dict.TryGetValue(0, out int x); + Console.WriteLine(x); + }; + } + } +}"); + } + [WorkItem(16676, "https://github.com/dotnet/roslyn/issues/16676")] [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineDeclaration)] public async Task TestMultipleDeclarationStatementsOnSameLine1() @@ -2114,5 +2165,92 @@ void Local() public static void Out(out T t) => t = default; }"); } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineDeclaration)] + public async Task TestDefiniteAssignment1() + { + await TestMissingInRegularAndScriptAsync( +@" +using System; + +class C +{ + static bool M(out bool i) => throw null; + + static void M(bool condition) + { + [|bool|] x = false; + if (condition || M(out x)) + { + Console.WriteLine(x); + } + } +}"); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineDeclaration)] + public async Task TestDefiniteAssignment2() + { + await TestMissingInRegularAndScriptAsync( +@" +using System; + +class C +{ + static bool M(out bool i) => throw null; + static bool Use(bool i) => throw null; + + static void M(bool condition) + { + [|bool|] x = false; + if (condition || M(out x)) + { + x = Use(x); + } + } +}"); + } + + [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsInlineDeclaration)] + [InlineData("c && M(out x)", "c && M(out bool x)")] + [InlineData("false || M(out x)", "false || M(out bool x)")] + [InlineData("M(out x) || M(out x)", "M(out bool x) || M(out x)")] + public async Task TestDefiniteAssignment3(string input, string output) + { + await TestInRegularAndScriptAsync( +$@" +using System; + +class C +{{ + static bool M(out bool i) => throw null; + static bool Use(bool i) => throw null; + + static void M(bool c) + {{ + [|bool|] x = false; + if ({input}) + {{ + Console.WriteLine(x); + }} + }} +}}", +$@" +using System; + +class C +{{ + static bool M(out bool i) => throw null; + static bool Use(bool i) => throw null; + + static void M(bool c) + {{ + if ({output}) + {{ + Console.WriteLine(x); + }} + }} +}}"); + } } } diff --git a/src/Features/CSharp/Portable/InlineDeclaration/CSharpInlineDeclarationDiagnosticAnalyzer.cs b/src/Features/CSharp/Portable/InlineDeclaration/CSharpInlineDeclarationDiagnosticAnalyzer.cs index 658050320c1e3240b64f2ccd3b87d7968597b864..eebf8c207175c830db19147a9f8c382c5a7c0227 100644 --- a/src/Features/CSharp/Portable/InlineDeclaration/CSharpInlineDeclarationDiagnosticAnalyzer.cs +++ b/src/Features/CSharp/Portable/InlineDeclaration/CSharpInlineDeclarationDiagnosticAnalyzer.cs @@ -220,9 +220,8 @@ private void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context, INamedTypeSymb // See if inlining this variable would make it so that some variables were no // longer definitely assigned. - if (WouldCauseDefiniteAssignmentErrors( - semanticModel, localDeclaration, localDeclarator, - enclosingBlockOfLocalStatement, outLocalSymbol, cancellationToken)) + if (WouldCauseDefiniteAssignmentErrors(semanticModel, localStatement, + enclosingBlockOfLocalStatement, outLocalSymbol)) { return; } @@ -248,12 +247,10 @@ private void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context, INamedTypeSymb } private bool WouldCauseDefiniteAssignmentErrors( - SemanticModel semanticModel, - VariableDeclarationSyntax localDeclaration, - VariableDeclaratorSyntax localDeclarator, + SemanticModel semanticModel, + LocalDeclarationStatementSyntax localStatement, BlockSyntax enclosingBlock, - ILocalSymbol outLocalSymbol, - CancellationToken cancellationToken) + ILocalSymbol outLocalSymbol) { // See if we have something like: // @@ -266,73 +263,10 @@ private void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context, INamedTypeSymb // In this case, inlining the 'i' would cause it to longer be definitely // assigned in the WriteLine invocation. - // Find all the current read-references to the local. - var query = from t in enclosingBlock.DescendantTokens() - where t.Kind() == SyntaxKind.IdentifierToken - where t.ValueText == outLocalSymbol.Name - let id = t.Parent as IdentifierNameSyntax - where id != null - where !id.IsOnlyWrittenTo() - let symbol = semanticModel.GetSymbolInfo(id).GetAnySymbol() - where outLocalSymbol.Equals(symbol) - select id; - - var references = query.ToImmutableArray(); - - var root = semanticModel.SyntaxTree.GetCompilationUnitRoot(cancellationToken); - - // Ensure we can track the references and the local variable as we make edits - // to the tree. - var rootWithTrackedNodes = root.TrackNodes( - references.Concat(ImmutableArray.Create(localDeclarator, localDeclaration, enclosingBlock))); - - // Now, take the local variable and remove it's initializer. Then go to all - // the locations where we read from it. If they're definitely assigned, then - // that means the out-var did it's work and assigned the variable across all - // paths. If it's not definitely assigned, then we can't inline this variable. - var currentLocalDeclarator = rootWithTrackedNodes.GetCurrentNode(localDeclarator); - var currentLocalDeclaration = rootWithTrackedNodes.GetCurrentNode(localDeclaration); - var updatedDeclaration = currentLocalDeclaration - .ReplaceNode(currentLocalDeclarator, currentLocalDeclarator.WithInitializer(null)); - - // If the declaration was a "var" declaration, then replace "var" with the actual - // type of the local. This way we don't get a "'var v' requires an initializer" which - // will suppress the message about definite assignment later. - if (updatedDeclaration.Type.IsVar) - { - updatedDeclaration = updatedDeclaration.WithType( - outLocalSymbol.Type.GenerateTypeSyntax()); - } - - var rootWithoutInitializer = rootWithTrackedNodes.ReplaceNode( - currentLocalDeclaration, updatedDeclaration); - - var rootWithoutInitializerTree = root.SyntaxTree.WithRootAndOptions( - rootWithoutInitializer, root.SyntaxTree.Options); - - // Fork the compilation so we can do this analysis. - var newCompilation = semanticModel.Compilation.ReplaceSyntaxTree( - root.SyntaxTree, rootWithoutInitializerTree); - var newSemanticModel = newCompilation.GetSemanticModel(rootWithoutInitializerTree); - - // NOTE: there is no current compiler API to determine if a variable is definitely - // assigned or not. So, for now, we just get diagnostics for this block and see if - // we get any definite assignment errors where we have a reference to the symbol. If - // so, then we don't offer the fix. - - rootWithoutInitializer = (CompilationUnitSyntax)rootWithoutInitializerTree.GetRoot(cancellationToken); - var currentBlock = rootWithoutInitializer.GetCurrentNode(enclosingBlock); - var diagnostics = newSemanticModel.GetDiagnostics(currentBlock.Span, cancellationToken); - - var diagnosticSpans = diagnostics.Where(d => d.Id == CS0165) - .Select(d => d.Location.SourceSpan) - .Distinct(); - - var newReferenceSpans = rootWithoutInitializer.GetCurrentNodes(references) - .Select(n => n.Span) - .Distinct(); - - return diagnosticSpans.Intersect(newReferenceSpans).Any(); + var dataFlow = semanticModel.AnalyzeDataFlow( + localStatement.GetNextStatement(), + enclosingBlock.Statements.Last()); + return dataFlow.DataFlowsIn.Contains(outLocalSymbol); } private SyntaxNode GetOutArgumentScope(SyntaxNode argumentExpression) @@ -351,15 +285,16 @@ private SyntaxNode GetOutArgumentScope(SyntaxNode argumentExpression) // * Using statements // * Fixed statements // * Try statements (specifically for exception filters) - if (current.Kind() == SyntaxKind.WhileStatement || - current.Kind() == SyntaxKind.DoStatement || - current.Kind() == SyntaxKind.ForStatement || - current.Kind() == SyntaxKind.ForEachStatement || - current.Kind() == SyntaxKind.UsingStatement || - current.Kind() == SyntaxKind.FixedStatement || - current.Kind() == SyntaxKind.TryStatement) + switch (current.Kind()) { - return current; + case SyntaxKind.WhileStatement: + case SyntaxKind.DoStatement: + case SyntaxKind.ForStatement: + case SyntaxKind.ForEachStatement: + case SyntaxKind.UsingStatement: + case SyntaxKind.FixedStatement: + case SyntaxKind.TryStatement: + return current; } if (current is StatementSyntax) @@ -417,10 +352,9 @@ private SyntaxNode GetOutArgumentScope(SyntaxNode argumentExpression) break; } - if (descendentNode.IsKind(SyntaxKind.IdentifierName)) + if (descendentNode.IsKind(SyntaxKind.IdentifierName, out IdentifierNameSyntax identifierName)) { // See if this looks like an accessor to the local variable syntactically. - var identifierName = (IdentifierNameSyntax)descendentNode; if (identifierName.Identifier.ValueText == variableName) { // Confirm that it is a access of the local.