From 728bf0522418779be7e8ab570d6fc0f5446ce644 Mon Sep 17 00:00:00 2001 From: Mansheng Yang Date: Fri, 15 Jun 2012 21:26:39 +0800 Subject: [PATCH] [CodeIssue] Added AccessToModifiedClosureIssue and AccessToDisposedClosureIssue --- .../ICSharpCode.NRefactory.CSharp.csproj | 4 + .../AccessToClosureIssue.cs | 329 ++++++++ .../AccessToDisposedClosureIssue.cs | 94 +++ .../AccessToModifiedClosureIssue.cs | 113 +++ .../LocalVariableNamePicker.cs | 70 ++ .../AccessToDisposedClosureTests.cs | 106 +++ .../AccessToModifiedClosureTests.cs | 753 ++++++++++++++++++ .../ICSharpCode.NRefactory.Tests.csproj | 2 + 8 files changed, 1471 insertions(+) create mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs create mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToDisposedClosureIssue.cs create mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToModifiedClosureIssue.cs create mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/LocalVariableNamePicker.cs create mode 100644 ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/AccessToDisposedClosureTests.cs create mode 100644 ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/AccessToModifiedClosureTests.cs diff --git a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj index 6064d95b7e..a99f28494d 100644 --- a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj +++ b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj @@ -256,6 +256,10 @@ + + + + diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs new file mode 100644 index 0000000000..e19ddf0ff8 --- /dev/null +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToClosureIssue.cs @@ -0,0 +1,329 @@ +// +// AccessToClosureIssue.cs +// +// Author: +// Mansheng Yang +// +// Copyright (c) 2012 Mansheng Yang +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +using System; +using System.Collections.Generic; +using System.Linq; +using ICSharpCode.NRefactory.CSharp.Analysis; +using ICSharpCode.NRefactory.CSharp.Resolver; +using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; + +namespace ICSharpCode.NRefactory.CSharp.Refactoring +{ + public abstract class AccessToClosureIssue : ICodeIssueProvider + { + static FindReferences refFinder = new FindReferences (); + static ControlFlowGraphBuilder cfgBuilder = new ControlFlowGraphBuilder (); + + public string Title + { get; private set; } + + protected AccessToClosureIssue (string title) + { + Title = title; + } + + public IEnumerable GetIssues (BaseRefactoringContext context) + { + var unit = context.RootNode as CompilationUnit; + if (unit == null) + return Enumerable.Empty (); + return new GatherVisitor (context, unit, this).GetIssues (); + } + + protected virtual bool IsTargetVariable (IVariable variable) + { + return true; + } + + protected abstract NodeKind GetNodeKind (AstNode node); + + protected virtual bool CanReachModification (ControlFlowNode node, Statement start, + IDictionary> modifications) + { + return node.NextStatement != null && node.NextStatement != start && + modifications.ContainsKey (node.NextStatement); + } + + protected abstract IEnumerable GetFixes (BaseRefactoringContext context, Node env, + string variableName, AstType variableType); + + #region GatherVisitor + + class GatherVisitor : GatherVisitorBase + { + CompilationUnit unit; + string title; + AccessToClosureIssue issueProvider; + + public GatherVisitor (BaseRefactoringContext context, CompilationUnit unit, + AccessToClosureIssue issueProvider) + : base (context) + { + this.title = context.TranslateString (issueProvider.Title); + this.unit = unit; + this.issueProvider = issueProvider; + } + + public override void VisitVariableInitializer (VariableInitializer variableInitializer) + { + var variableDecl = variableInitializer.Parent as VariableDeclarationStatement; + if (variableDecl == null) + return; + + CheckVariable (((LocalResolveResult)ctx.Resolve (variableInitializer)).Variable, + variableDecl.Type, + variableDecl.GetParent ()); + base.VisitVariableInitializer (variableInitializer); + } + + public override void VisitForeachStatement (ForeachStatement foreachStatement) + { + CheckVariable (((LocalResolveResult)ctx.Resolve (foreachStatement.VariableNameToken)).Variable, + foreachStatement.VariableType, + foreachStatement); + base.VisitForeachStatement (foreachStatement); + } + + public override void VisitParameterDeclaration (ParameterDeclaration parameterDeclaration) + { + var parent = parameterDeclaration.Parent; + Statement body = null; + if (parent is MethodDeclaration) { + body = ((MethodDeclaration)parent).Body; + } else if (parent is AnonymousMethodExpression) { + body = ((AnonymousMethodExpression)parent).Body; + } else if (parent is LambdaExpression) { + body = ((LambdaExpression)parent).Body as Statement; + } + if (body != null) + CheckVariable (((LocalResolveResult)ctx.Resolve (parameterDeclaration)).Variable, + parameterDeclaration.Type, body); + base.VisitParameterDeclaration (parameterDeclaration); + } + + void FindLocalReferences (IVariable variable, FoundReferenceCallback callback) + { + refFinder.FindLocalReferences (variable, ctx.ParsedFile, unit, ctx.Compilation, callback, + ctx.CancellationToken); + } + + void CheckVariable (IVariable variable, AstType type, Statement env) + { + if (!issueProvider.IsTargetVariable (variable)) + return; + + var root = new Environment (env, env); + var envLookup = new Dictionary (); + envLookup [env] = root; + + FindLocalReferences (variable, (astNode, resolveResult) => + AddNode (envLookup, new Node (astNode, issueProvider.GetNodeKind (astNode)))); + + root.SortChildren (); + CollectIssues (root, variable.Name, type); + } + + void CollectIssues (Environment env, string variableName, AstType variableType) + { + IList cfg = null; + IDictionary> modifications = null; + + if (env.Body != null) { + cfg = cfgBuilder.BuildControlFlowGraph (env.Body); + modifications = new Dictionary> (); + foreach (var node in env.Children) { + if (node.Kind == NodeKind.Modification || node.Kind == NodeKind.ReferenceAndModification) { + IList nodes; + if (!modifications.TryGetValue (node.ContainingStatement, out nodes)) + modifications [node.ContainingStatement] = nodes = new List (); + nodes.Add (node); + } + } + } + + foreach (var child in env.GetChildEnvironments ()) { + if (!child.IssueCollected && cfg != null && + CanReachModification (cfg, child, modifications)) + CollectAllIssues (child, variableName, variableType); + + CollectIssues (child, variableName, variableType); + } + } + + void CollectAllIssues (Environment env, string variableName, AstType variableType) + { + var fixes = issueProvider.GetFixes (ctx, env, variableName, variableType).ToArray (); + env.IssueCollected = true; + + foreach (var child in env.Children) { + if (child is Environment) { + CollectAllIssues ((Environment)child, variableName, variableType); + } else { + if (child.Kind != NodeKind.Modification) + AddIssue (child.AstNode, title, fixes); + // stop marking references after the variable is modified in current environment + if (child.Kind != NodeKind.Reference) + break; + } + } + } + + void AddNode (IDictionary envLookup, Node node) + { + var astParent = node.AstNode.Parent; + var path = new List (); + while (astParent != null) { + Environment parent; + if (envLookup.TryGetValue (astParent, out parent)) { + parent.Children.Add (node); + return; + } + + if (astParent is LambdaExpression) { + parent = new Environment (astParent, ((LambdaExpression)astParent).Body as Statement); + } else if (astParent is AnonymousMethodExpression) { + parent = new Environment (astParent, ((AnonymousMethodExpression)astParent).Body); + } + + path.Add (astParent); + if (parent != null) { + foreach (var astNode in path) + envLookup [astNode] = parent; + path.Clear (); + + parent.Children.Add (node); + node = parent; + } + astParent = astParent.Parent; + } + } + + bool CanReachModification (IEnumerable cfg, Environment env, + IDictionary> modifications) + { + if (modifications.Count == 0) + return false; + + var start = env.ContainingStatement; + if (modifications.ContainsKey (start) && + modifications [start].Any (v => v.AstNode.StartLocation > env.AstNode.EndLocation)) + return true; + + var stack = new Stack (cfg.Where (node => node.NextStatement == start)); + var visitedNodes = new HashSet (stack); + while (stack.Count > 0) { + var node = stack.Pop (); + if (issueProvider.CanReachModification (node, start, modifications)) + return true; + foreach (var edge in node.Outgoing) { + if (visitedNodes.Add (edge.To)) + stack.Push (edge.To); + } + } + return false; + } + + } + + #endregion + + #region Node + + protected enum NodeKind + { + Reference, + Modification, + ReferenceAndModification, + Environment, + } + + protected class Node + { + public AstNode AstNode + { get; private set; } + + public NodeKind Kind + { get; private set; } + + public Statement ContainingStatement + { get; private set; } + + public Node (AstNode astNode, NodeKind kind) + { + AstNode = astNode; + Kind = kind; + ContainingStatement = astNode.GetParent (); + } + + public virtual IEnumerable GetAllReferences () + { + yield return this; + } + } + + protected class Environment : Node + { + public Statement Body + { get; private set; } + + public bool IssueCollected + { get; set; } + + public List Children + { get; private set; } + + public Environment (AstNode astNode, Statement body) + : base (astNode, NodeKind.Environment) + { + Body = body; + Children = new List (); + } + + public override IEnumerable GetAllReferences () + { + return Children.SelectMany (child => child.GetAllReferences ()); + } + + public IEnumerable GetChildEnvironments () + { + return from child in Children + where child is Environment + select (Environment)child; + } + + public void SortChildren () + { + Children.Sort ((x, y) => x.AstNode.StartLocation.CompareTo(y.AstNode.StartLocation)); + foreach (var env in GetChildEnvironments ()) + env.SortChildren (); + } + } + + #endregion + } +} diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToDisposedClosureIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToDisposedClosureIssue.cs new file mode 100644 index 0000000000..c1e557dafc --- /dev/null +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToDisposedClosureIssue.cs @@ -0,0 +1,94 @@ +// +// AccessToDisposedClosureIssue.cs +// +// Author: +// Mansheng Yang +// +// Copyright (c) 2012 Mansheng Yang +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +using System; +using System.Collections.Generic; +using System.Linq; +using ICSharpCode.NRefactory.CSharp.Analysis; +using ICSharpCode.NRefactory.TypeSystem; + +namespace ICSharpCode.NRefactory.CSharp.Refactoring +{ + + [IssueDescription ("Access to disposed closure variable", + Description = "Access to closure variable from anonymous method when the variable is" + + " disposed externally", + Category = IssueCategories.CodeQualityIssues, + Severity = Severity.Warning, + IssueMarker = IssueMarker.Underline)] + public class AccessToDisposedClosureIssue : AccessToClosureIssue + { + public AccessToDisposedClosureIssue () + : base ("Access to disposed closure") + { + } + + protected override bool IsTargetVariable (IVariable variable) + { + return variable.Type.GetAllBaseTypeDefinitions ().Any (t => t.KnownTypeCode == KnownTypeCode.IDisposable); + } + + protected override NodeKind GetNodeKind (AstNode node) + { + if (node.Parent is UsingStatement) + return NodeKind.ReferenceAndModification; + + if (node.Parent is VariableDeclarationStatement && node.Parent.Parent is UsingStatement) + return NodeKind.Modification; + + var memberRef = node.Parent as MemberReferenceExpression; + if (memberRef != null && memberRef.Parent is InvocationExpression && memberRef.MemberName == "Dispose") + return NodeKind.ReferenceAndModification; + + return NodeKind.Reference; + } + + protected override bool CanReachModification (ControlFlowNode node, Statement start, + IDictionary> modifications) + { + if (base.CanReachModification (node, start, modifications)) + return true; + + if (node.NextStatement != start) { + var usingStatement = node.PreviousStatement as UsingStatement; + if (usingStatement != null) { + if (modifications.ContainsKey(usingStatement)) + return true; + if (usingStatement.ResourceAcquisition is Statement && + modifications.ContainsKey ((Statement)usingStatement.ResourceAcquisition)) + return true; + } + } + return false; + } + + protected override IEnumerable GetFixes (BaseRefactoringContext context, Node env, + string variableName, AstType variableType) + { + yield break; + } + } +} \ No newline at end of file diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToModifiedClosureIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToModifiedClosureIssue.cs new file mode 100644 index 0000000000..a75f5e1a92 --- /dev/null +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/AccessToClosureIssues/AccessToModifiedClosureIssue.cs @@ -0,0 +1,113 @@ +// +// AccessToModifiedClosureIssue.cs +// +// Author: +// Mansheng Yang +// +// Copyright (c) 2012 Mansheng Yang +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +using System; +using System.Collections.Generic; +using System.Linq; + +namespace ICSharpCode.NRefactory.CSharp.Refactoring +{ + [IssueDescription ("Access to modified closure variable", + Description = "Access to closure variable from anonymous method when the variable is modified " + + "externally", + Category = IssueCategories.CodeQualityIssues, + Severity = Severity.Warning, + IssueMarker = IssueMarker.Underline)] + public class AccessToModifiedClosureIssue : AccessToClosureIssue + { + public AccessToModifiedClosureIssue () + : base ("Access to modified closure") + { + } + + protected override NodeKind GetNodeKind (AstNode node) + { + var assignment = node.GetParent (); + if (assignment != null && assignment.Left == node) { + if (assignment.Operator == AssignmentOperatorType.Assign) + return NodeKind.Modification; + return NodeKind.ReferenceAndModification; + } + var unaryExpr = node.GetParent (); + if (unaryExpr != null && unaryExpr.Expression == node && + (unaryExpr.Operator == UnaryOperatorType.Increment || + unaryExpr.Operator == UnaryOperatorType.PostIncrement || + unaryExpr.Operator == UnaryOperatorType.Decrement || + unaryExpr.Operator == UnaryOperatorType.PostDecrement)) { + return NodeKind.ReferenceAndModification; + } + if (node.Parent is ForeachStatement) + return NodeKind.Modification; + + return NodeKind.Reference; + } + + protected override IEnumerable GetFixes (BaseRefactoringContext context, Node env, + string variableName, AstType variableType) + { + var containingStatement = env.ContainingStatement; + + // we don't give a fix for these cases since the general fix may not work + // lambda in while/do-while/for condition + if (containingStatement is WhileStatement || containingStatement is DoWhileStatement || + containingStatement is ForStatement) + yield break; + // lambda in for initializer/iterator + if (containingStatement.Parent is ForStatement && + ((ForStatement)containingStatement.Parent).EmbeddedStatement != containingStatement) + yield break; + + Action