diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs index 925a2c65bb..7e95ab7f3a 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs @@ -26,6 +26,8 @@ using System.Collections.Generic; using System.Linq; using System; +using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; namespace ICSharpCode.NRefactory.CSharp.Refactoring { @@ -75,41 +77,154 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring // Start at the parent node. Presumably this is a BlockStatement var rootNode = variableDeclarationStatement.Parent; var variableInitializer = variableDeclarationStatement.Variables.First(); - var identifiers = from node in rootNode.Descendants - let identifier = node as IdentifierExpression - where identifier != null && identifier.Identifier == variableInitializer.Name - select node; + var identifiers = GetIdentifiers(rootNode.Descendants, variableInitializer.Name).ToList(); - if (identifiers.Count() == 0) + if (identifiers.Count == 0) // variable is not used return; - AstNode lowestCommonAncestor = GetLowestCommonAncestor(rootNode, identifiers); - var path = GetPath(rootNode, lowestCommonAncestor); - - var firstLoopStatement = (from node in path - where moveTargetBlacklist.Contains(node.GetType()) - select node).FirstOrDefault(); - IList possibleDestinationsPath; - if (firstLoopStatement == null) { - possibleDestinationsPath = path; - } else { - possibleDestinationsPath = GetPath(rootNode, firstLoopStatement); + AstNode deepestCommonAncestor = GetDeepestCommonAncestor(rootNode, identifiers); + var path = GetPath(rootNode, deepestCommonAncestor); + + // Restrict path to only those where the initializer has not changed + var firstInitializerChange = GetFirstInitializerChange(variableDeclarationStatement, path, variableInitializer.Initializer); + if (firstInitializerChange != null) { + path = GetPath(rootNode, firstInitializerChange); } - var mostNestedEmbeddedStatement = (from node in possibleDestinationsPath - let statement = node as Statement - let blockStatement = statement as BlockStatement - where statement != null && IsNestedScope(statement) - select statement).LastOrDefault(); - if (mostNestedEmbeddedStatement != null && mostNestedEmbeddedStatement != rootNode) { + // Restict to locations outside of blacklisted node types + var firstBlackListedNode = (from node in path + where moveTargetBlacklist.Contains(node.GetType()) + select node).FirstOrDefault(); + if (firstBlackListedNode != null) { + path = GetPath(rootNode, firstBlackListedNode); + } + + // Get the most nested possible target for the move + Statement mostNestedFollowingStatement = null; + for (int i = path.Count - 1; i >= 0; i--) { + var statement = path[i] as Statement; + if (statement != null && (IsScopeContainer(statement.Parent) || IsScopeContainer(statement))) { + mostNestedFollowingStatement = statement; + break; + } + } + + if (mostNestedFollowingStatement != null && mostNestedFollowingStatement != rootNode && mostNestedFollowingStatement.Parent != rootNode) { AddIssue(variableDeclarationStatement, context.TranslateString("Variable could be moved to a nested scope"), - GetActions(variableDeclarationStatement, mostNestedEmbeddedStatement)); + GetActions(variableDeclarationStatement, mostNestedFollowingStatement)); + } + } + + static IEnumerable GetIdentifiers(IEnumerable candidates, string name = null) + { + return + from node in candidates + let identifier = node as IdentifierExpression + where identifier != null && (name == null || identifier.Identifier == name) + select identifier; + } + + AstNode GetFirstInitializerChange(AstNode variableDeclarationStatement, IList path, Expression initializer) + { + var identifiers = GetIdentifiers(initializer.DescendantsAndSelf).ToList(); + var mayChangeInitializer = GetChecker (initializer, identifiers); + AstNode lastChange = null; + for (int i = path.Count - 1; i >= 0; i--) { + for (AstNode node = path[i].PrevSibling; node != null && node != variableDeclarationStatement; node = node.PrevSibling) { + // Special case for IfElseStatement: The AST nesting does not match the scope nesting, so + // don't handle branches here: The correct one has already been checked anyway. + // This also works to our advantage: No special checking is needed for the condition since + // it is a the same level in the tree as the false branch + if (node.Role == IfElseStatement.TrueRole || node.Role == IfElseStatement.FalseRole) + continue; + foreach (var expression in node.DescendantsAndSelf.Where(n => n is Expression).Cast()) { + if (mayChangeInitializer(expression)) { + lastChange = expression; + } + } + } + } + return lastChange; + } + + Func GetChecker(Expression expression, IList identifiers) + { + // TODO: This only works for simple cases. + IList members; + IList locals; + var identifierResolveResults = identifiers.Select(identifier => context.Resolve(identifier)).ToList(); + SplitResolveResults(identifierResolveResults, out members, out locals); + + if (expression is InvocationExpression || expression is ObjectCreateExpression) { + return node => { + if (node is InvocationExpression || expression is ObjectCreateExpression) + // We don't know what these might do, so assume it will change the initializer + return true; + var binaryOperator = node as BinaryOperatorExpression; + if (binaryOperator != null) { + var resolveResult = context.Resolve(binaryOperator) as OperatorResolveResult; + // Built-in operators are ok, user defined ones not so much + return resolveResult.UserDefinedOperatorMethod != null; + } + return IsConflictingAssignment(node, identifiers, members, locals); + }; + } else if (expression is IdentifierExpression) { + var initializerDependsOnMembers = identifierResolveResults.Any(result => result is MemberResolveResult); + var initializerDependsOnReferenceType = identifierResolveResults.Any(result => result.Type.IsReferenceType == true); + return node => { + if ((node is InvocationExpression || node is ObjectCreateExpression) && + (initializerDependsOnMembers || initializerDependsOnReferenceType)) + // Anything can happen... + return true; + var binaryOperator = node as BinaryOperatorExpression; + if (binaryOperator != null) { + var resolveResult = context.Resolve(binaryOperator) as OperatorResolveResult; + return resolveResult.UserDefinedOperatorMethod != null; + } + return IsConflictingAssignment(node, identifiers, members, locals); + }; + } + + return node => false; + } + + bool IsConflictingAssignment (Expression node, IList identifiers, IList members, IList locals) + { + var assignmentExpression = node as AssignmentExpression; + if (assignmentExpression != null) { + IList targetMembers; + IList targetLocals; + var identifierResolveResults = identifiers.Select(identifier => context.Resolve(identifier)).ToList(); + SplitResolveResults(identifierResolveResults, out targetMembers, out targetLocals); + + return members.Any(member => targetMembers.Contains(member)) || + locals.Any(local => targetLocals.Contains(local)); } + return false; } - bool IsNestedScope(AstNode node) + static void SplitResolveResults(List identifierResolveResults, out IList members, out IList locals) { + members = new List(); + locals = new List(); + foreach (var resolveResult in identifierResolveResults) { + var memberResolveResult = resolveResult as MemberResolveResult; + if (memberResolveResult != null) { + members.Add(memberResolveResult.Member); + } + var localResolveResult = resolveResult as LocalResolveResult; + if (localResolveResult != null) { + locals.Add(localResolveResult.Variable); + } + } + } + + bool IsScopeContainer(AstNode node) + { + if (node == null) + return false; + var blockStatement = node as BlockStatement; if (blockStatement != null) return true; @@ -127,27 +242,26 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring return false; } - IEnumerable GetActions(VariableDeclarationStatement declaration, Statement targetScope) + IEnumerable GetActions(Statement oldStatement, Statement followingStatement) { yield return new CodeAction(context.TranslateString("Move to nested scope"), script => { - var blockStatement = targetScope as BlockStatement; - if (blockStatement == null) { + if (!(followingStatement.Parent is BlockStatement)) { var newBlockStatement = new BlockStatement { Statements = { - declaration.Clone(), - targetScope.Clone() + oldStatement.Clone(), + followingStatement.Clone() } }; - script.Replace(targetScope, newBlockStatement); - script.FormatText(targetScope.Parent); + script.Replace(followingStatement, newBlockStatement); + script.FormatText(followingStatement.Parent); } else { - script.InsertBefore(blockStatement.Statements.First(), declaration.Clone()); + script.InsertBefore(followingStatement, oldStatement.Clone()); } - script.Remove(declaration); + script.Remove(oldStatement); }); } - AstNode GetLowestCommonAncestor(AstNode assumedRoot, IEnumerable leaves) + AstNode GetDeepestCommonAncestor(AstNode assumedRoot, IEnumerable leaves) { var previousPath = GetPath(assumedRoot, leaves.First()); int lowestIndex = previousPath.Count - 1; diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs index 201665e507..fc117fb96b 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs @@ -94,8 +94,8 @@ class A if (true) { val2 = 2; } else { - int val = 2; val2 = 3; + int val = 2; System.Console.WriteLine(val); } } @@ -249,6 +249,49 @@ class A Assert.AreEqual(0, issues.Count); } + [Test] + public void IfElseIf() + { + var input = @" +using System; +class A +{ + int GetValue () { return 0; } + + void F (bool condition) + { + int val = GetValue (); + if (condition) { + int val2 = GetValue (); + Console.WriteLine(val2); + } else if (!condition) { + Console.WriteLine(val); + } + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new VariableDeclaredInWideScopeIssue(), input, out context); + Assert.AreEqual(1, issues.Count); + + CheckFix(context, issues [0], @" +using System; +class A +{ + int GetValue () { return 0; } + + void F (bool condition) + { + if (condition) { + int val2 = GetValue (); + Console.WriteLine(val2); + } else if (!condition) { + int val = GetValue (); + Console.WriteLine(val); + } + } +}"); + } + [Test] public void DoesNotSuggestMovingIntoTryCatch() { @@ -340,6 +383,88 @@ class A System.Console.WriteLine (val); } } +}"); + } + + [Test] + public void DoesNotSuggestMovingPastChangeOfInitializingExpression1 () + { + var input = @" +using System; +class A +{ + void F () + { + int i = 1; + int j = i; + i = 2; + if (true) + Console.WriteLine(j); + Console.WriteLine(i); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new VariableDeclaredInWideScopeIssue(), input, out context); + Assert.AreEqual(0, issues.Count); + } + + [Test] + public void DoesNotSuggestMovingPastChangeOfInitializingExpression2 () + { + var input = @" +using System; +class A +{ + int GetValue () { return 0; } + int SetValue (int i) { } + + void F () + { + int j = GetValue(); + SetValue(2); + if (true) + Console.WriteLine(j); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new VariableDeclaredInWideScopeIssue(), input, out context); + Assert.AreEqual(0, issues.Count); + } + + [Test] + public void SuggestsMovingMethodCallInitializer () + { + var input = @" +using System; +class A +{ + int GetValue () { return 0; } + + void F () + { + int j = GetValue (); + if (true) { + Console.WriteLine(j); + } + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new VariableDeclaredInWideScopeIssue(), input, out context); + Assert.AreEqual(1, issues.Count); + + CheckFix(context, issues[0], @" +using System; +class A +{ + int GetValue () { return 0; } + + void F () + { + if (true) { + int j = GetValue (); + Console.WriteLine(j); + } + } }"); } }