From 001e85b936ce665a0da4f3c6ca587157241c2249 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 26 Sep 2016 13:48:55 -0700 Subject: [PATCH 01/19] Add class to create and analyze directed graph --- Engine/Helper.cs | 126 +++++++++++++++++++++++++++++++++ Tests/Engine/Helpers.tests.ps1 | 29 ++++++++ 2 files changed, 155 insertions(+) create mode 100644 Tests/Engine/Helpers.tests.ps1 diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 1135bf120..70c1191fc 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3555,4 +3555,130 @@ public object VisitUsingExpression(UsingExpressionAst usingExpressionAst) return null; } } + + /// Class to represent a directed graph + public class Digraph + { + public int NumVertices + { + get { return graph.Count; } + } + + private List> graph; + private Dictionary vertexIndexMap; + + + private int GetIndex(T vertex) + { + int idx; + return vertexIndexMap.TryGetValue(vertex, out idx) ? idx : -1; + } + + public IEnumerable GetNeighbors(T vertex) + { + ValidateVertexArgument(vertex); + var idx = GetIndex(vertex); + var idxVertexMap = vertexIndexMap.ToDictionary(x => x.Value, x => x.Key); + foreach (var neighbor in graph[idx]) + { + yield return idxVertexMap[neighbor]; + } + } + + public int GetNumNeighbors(T vertex) + { + ValidateVertexArgument(vertex); + return graph[GetIndex(vertex)].Count; + } + + public Digraph() + { + graph = new List> (); + vertexIndexMap = new Dictionary(); + } + + public void AddVertex(T vertex) + { + if (vertex == null) + { + throw new ArgumentNullException("vertex"); + } + + if (GetIndex(vertex) != -1) + { + throw new ArgumentException("Vertex already present! Cannot add it to the Digraph", "vertex"); + } + + vertexIndexMap.Add(vertex, graph.Count); + graph.Add(new List()); + } + + public void AddEdge(T fromVertex, T toVertex) + { + ValidateVertexArgument(fromVertex); + ValidateVertexArgument(toVertex); + + var toIdx = GetIndex(toVertex); + var fromVertexList = graph[GetIndex(fromVertex)]; + if (!fromVertexList.Contains(toIdx)) + { + fromVertexList.Add(toIdx); + } + } + + public bool IsConnected(T vertex1, T vertex2) + { + ValidateVertexArgument(vertex1); + ValidateVertexArgument(vertex2); + var visited = new bool[graph.Count]; + return IsConnected(GetIndex(vertex1), GetIndex(vertex2), ref visited); + } + + private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) + { + visited[fromIdx] = true; + if (fromIdx == toIdx) + { + return true; + } + + bool isConnected = false; + foreach(var vertexIdx in graph[fromIdx]) + { + if (!visited[vertexIdx]) + { + isConnected |= IsConnected(vertexIdx, toIdx, ref visited); + } + + if (isConnected) // no need to search further + { + break; + } + } + + return isConnected; + } + + private void ValidateNotNull(T vertex) + { + if (vertex == null) + { + throw new ArgumentNullException("vertex"); + } + } + + private void ValidateVertexPresence(T vertex) + { + if (GetIndex(vertex) == -1) + { + throw new ArgumentOutOfRangeException("vertex not present in the Digraph.", "vertex"); + } + } + + private void ValidateVertexArgument(T vertex) + { + ValidateNotNull(vertex); + ValidateVertexPresence(vertex); + } + } } diff --git a/Tests/Engine/Helpers.tests.ps1 b/Tests/Engine/Helpers.tests.ps1 new file mode 100644 index 000000000..c4a4f6dc5 --- /dev/null +++ b/Tests/Engine/Helpers.tests.ps1 @@ -0,0 +1,29 @@ +Import-Module PSScriptAnalyzer +$helperNamespace = 'Microsoft.Windows.PowerShell.ScriptAnalyzer'; + + +Describe "Test Directed Graph" { + Context "When a graph is created" { + $digraph = New-Object -TypeName 'Microsoft.Windows.PowerShell.ScriptAnalyzer.DiGraph[string]' + $digraph.AddVertex('v1'); + $digraph.AddVertex('v2'); + $digraph.AddVertex('v3'); + $digraph.AddVertex('v4'); + $digraph.AddEdge('v1', 'v2'); + $digraph.AddEdge('v2', 'v4'); + + It "correctly adds the vertices" { + $digraph.NumVertices | Should Be 4 + } + + It "correctly adds the edges" { + $digraph.GetNumNeighbors('v1') | Should Be 1 + $neighbors = $digraph.GetNeighbors('v1') + $neighbors[0] | Should Be 'v2' + } + + It "finds the connection" { + $digraph.IsConnected('v1', 'v4') | Should Be $true + } + } +} \ No newline at end of file From 6363d4c4a8cd5d2c4dde38bcfad7f5e1c92a0247 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 26 Sep 2016 13:55:41 -0700 Subject: [PATCH 02/19] Add a test case for directed graph implementation --- Tests/Engine/Helpers.tests.ps1 | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Tests/Engine/Helpers.tests.ps1 b/Tests/Engine/Helpers.tests.ps1 index c4a4f6dc5..f37fcf42e 100644 --- a/Tests/Engine/Helpers.tests.ps1 +++ b/Tests/Engine/Helpers.tests.ps1 @@ -1,6 +1,4 @@ Import-Module PSScriptAnalyzer -$helperNamespace = 'Microsoft.Windows.PowerShell.ScriptAnalyzer'; - Describe "Test Directed Graph" { Context "When a graph is created" { @@ -9,17 +7,21 @@ Describe "Test Directed Graph" { $digraph.AddVertex('v2'); $digraph.AddVertex('v3'); $digraph.AddVertex('v4'); + $digraph.AddVertex('v5'); + $digraph.AddEdge('v1', 'v2'); + $digraph.AddEdge('v1', 'v5'); $digraph.AddEdge('v2', 'v4'); It "correctly adds the vertices" { - $digraph.NumVertices | Should Be 4 + $digraph.NumVertices | Should Be 5 } It "correctly adds the edges" { - $digraph.GetNumNeighbors('v1') | Should Be 1 + $digraph.GetNumNeighbors('v1') | Should Be 2 $neighbors = $digraph.GetNeighbors('v1') - $neighbors[0] | Should Be 'v2' + $neighbors -contains 'v2' | Should Be $true + $neighbors -contains 'v5' | Should Be $true } It "finds the connection" { From af330077257a4b8678d1875828fa364f69801996 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 27 Sep 2016 11:23:34 -0700 Subject: [PATCH 03/19] Create a digraph base class --- Engine/Helper.cs | 91 ++++++++++------------------------ Tests/Engine/Helpers.tests.ps1 | 26 ++++------ 2 files changed, 38 insertions(+), 79 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 70c1191fc..1a9e5f1d8 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3557,81 +3557,58 @@ public object VisitUsingExpression(UsingExpressionAst usingExpressionAst) } /// Class to represent a directed graph - public class Digraph + public class Digraph { - public int NumVertices - { - get { return graph.Count; } - } - private List> graph; - private Dictionary vertexIndexMap; - - private int GetIndex(T vertex) + public Digraph() { - int idx; - return vertexIndexMap.TryGetValue(vertex, out idx) ? idx : -1; + graph = new List> (); } - public IEnumerable GetNeighbors(T vertex) + public int GetNumVertices() { - ValidateVertexArgument(vertex); - var idx = GetIndex(vertex); - var idxVertexMap = vertexIndexMap.ToDictionary(x => x.Value, x => x.Key); - foreach (var neighbor in graph[idx]) - { - yield return idxVertexMap[neighbor]; - } + return graph.Count; } - public int GetNumNeighbors(T vertex) + public int GetNumNeighbors(int vertex) { - ValidateVertexArgument(vertex); - return graph[GetIndex(vertex)].Count; + return GetNeighbors(vertex).Count(); } - public Digraph() + public IEnumerable GetNeighbors(int vertex) { - graph = new List> (); - vertexIndexMap = new Dictionary(); + ValidateVertex(vertex); + return graph[vertex]; } - public void AddVertex(T vertex) + public bool ContainsVertex(int vertex) { - if (vertex == null) - { - throw new ArgumentNullException("vertex"); - } - - if (GetIndex(vertex) != -1) - { - throw new ArgumentException("Vertex already present! Cannot add it to the Digraph", "vertex"); - } + return vertex >= 0 && vertex < graph.Count; + } - vertexIndexMap.Add(vertex, graph.Count); + public void AddVertex() + { graph.Add(new List()); } - public void AddEdge(T fromVertex, T toVertex) + public void AddEdge(int fromVertex, int toVertex) { - ValidateVertexArgument(fromVertex); - ValidateVertexArgument(toVertex); - - var toIdx = GetIndex(toVertex); - var fromVertexList = graph[GetIndex(fromVertex)]; - if (!fromVertexList.Contains(toIdx)) + ValidateVertex(fromVertex); + ValidateVertex(toVertex); + if (graph[fromVertex].Contains(toVertex)) { - fromVertexList.Add(toIdx); + throw new ArgumentException(String.Format("Edge from {0} to {1} already present", fromVertex, toVertex)); } + graph[fromVertex].Add(toVertex); } - public bool IsConnected(T vertex1, T vertex2) + public bool IsConnected(int vertex1, int vertex2) { - ValidateVertexArgument(vertex1); - ValidateVertexArgument(vertex2); + ValidateVertex(vertex1); + ValidateVertex(vertex2); var visited = new bool[graph.Count]; - return IsConnected(GetIndex(vertex1), GetIndex(vertex2), ref visited); + return IsConnected(vertex1, vertex2, ref visited); } private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) @@ -3659,26 +3636,12 @@ private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) return isConnected; } - private void ValidateNotNull(T vertex) + private void ValidateVertex(int vertex) { - if (vertex == null) - { - throw new ArgumentNullException("vertex"); - } - } - - private void ValidateVertexPresence(T vertex) - { - if (GetIndex(vertex) == -1) + if (!ContainsVertex(vertex)) { throw new ArgumentOutOfRangeException("vertex not present in the Digraph.", "vertex"); } } - - private void ValidateVertexArgument(T vertex) - { - ValidateNotNull(vertex); - ValidateVertexPresence(vertex); - } } } diff --git a/Tests/Engine/Helpers.tests.ps1 b/Tests/Engine/Helpers.tests.ps1 index f37fcf42e..a234cada3 100644 --- a/Tests/Engine/Helpers.tests.ps1 +++ b/Tests/Engine/Helpers.tests.ps1 @@ -2,30 +2,26 @@ Import-Module PSScriptAnalyzer Describe "Test Directed Graph" { Context "When a graph is created" { - $digraph = New-Object -TypeName 'Microsoft.Windows.PowerShell.ScriptAnalyzer.DiGraph[string]' - $digraph.AddVertex('v1'); - $digraph.AddVertex('v2'); - $digraph.AddVertex('v3'); - $digraph.AddVertex('v4'); - $digraph.AddVertex('v5'); + $digraph = New-Object -TypeName 'Microsoft.Windows.PowerShell.ScriptAnalyzer.DiGraph' + 0..4 | ForEach-Object {$digraph.AddVertex()} - $digraph.AddEdge('v1', 'v2'); - $digraph.AddEdge('v1', 'v5'); - $digraph.AddEdge('v2', 'v4'); + $digraph.AddEdge(0, 1); + $digraph.AddEdge(0, 4); + $digraph.AddEdge(1, 3); It "correctly adds the vertices" { - $digraph.NumVertices | Should Be 5 + $digraph.GetNumVertices() | Should Be 5 } It "correctly adds the edges" { - $digraph.GetNumNeighbors('v1') | Should Be 2 - $neighbors = $digraph.GetNeighbors('v1') - $neighbors -contains 'v2' | Should Be $true - $neighbors -contains 'v5' | Should Be $true + $digraph.GetNumNeighbors(0) | Should Be 2 + $neighbors = $digraph.GetNeighbors(0) + $neighbors -contains 1 | Should Be $true + $neighbors -contains 4 | Should Be $true } It "finds the connection" { - $digraph.IsConnected('v1', 'v4') | Should Be $true + $digraph.IsConnected(0, 3) | Should Be $true } } } \ No newline at end of file From 8d3d2b3795be6c2e5660276d85e65851c4883f79 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 27 Sep 2016 14:05:07 -0700 Subject: [PATCH 04/19] Make digraph base class immutable --- Engine/Helper.cs | 21 +++++++++++++++++++-- Tests/Engine/Helpers.tests.ps1 | 17 ++++++++++------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 1a9e5f1d8..ea54110b6 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3566,6 +3566,23 @@ public Digraph() graph = new List> (); } + public Digraph(int numVertices, List> edges) : this() + { + for (int v = 0; v < numVertices; v++) + { + AddVertex(); + } + + foreach (Tuple tuple in edges) + { + var fromV = tuple.Item1; + var toV = tuple.Item2; + ValidateVertex(fromV); + ValidateVertex(toV); + AddEdge(fromV, toV); + } + } + public int GetNumVertices() { return graph.Count; @@ -3587,12 +3604,12 @@ public bool ContainsVertex(int vertex) return vertex >= 0 && vertex < graph.Count; } - public void AddVertex() + protected void AddVertex() { graph.Add(new List()); } - public void AddEdge(int fromVertex, int toVertex) + protected void AddEdge(int fromVertex, int toVertex) { ValidateVertex(fromVertex); ValidateVertex(toVertex); diff --git a/Tests/Engine/Helpers.tests.ps1 b/Tests/Engine/Helpers.tests.ps1 index a234cada3..efbe2ac34 100644 --- a/Tests/Engine/Helpers.tests.ps1 +++ b/Tests/Engine/Helpers.tests.ps1 @@ -1,14 +1,17 @@ Import-Module PSScriptAnalyzer -Describe "Test Directed Graph" { - Context "When a graph is created" { - $digraph = New-Object -TypeName 'Microsoft.Windows.PowerShell.ScriptAnalyzer.DiGraph' - 0..4 | ForEach-Object {$digraph.AddVertex()} - $digraph.AddEdge(0, 1); - $digraph.AddEdge(0, 4); - $digraph.AddEdge(1, 3); +Function ConvertType($x) +{ + $z = [System.Collections.Generic.List[Tuple[int,int]]]::new() + $x | ForEach-Object {$z.Add([System.Tuple[int,int]]::new($_[0], $_[1]))} + return $z +} +Describe "Test Directed Graph" { + Context "When a graph is created" { + $edges = ConvertType (0,1),(0,4),(1,3) + $digraph = New-Object -TypeName 'Microsoft.Windows.PowerShell.ScriptAnalyzer.DiGraph' -ArgumentList 5,$edges It "correctly adds the vertices" { $digraph.GetNumVertices() | Should Be 5 } From 2faa4c3a14af0928f695fe233cfdb831b8ea1dcc Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 27 Sep 2016 16:14:16 -0700 Subject: [PATCH 05/19] Remove derived digraph class --- Engine/Helper.cs | 115 ++++++++++++++++++++--------- Rules/UseShouldProcessCorrectly.cs | 98 ++++++++++++++++++++++++ Tests/Engine/Helpers.tests.ps1 | 33 +++++---- 3 files changed, 194 insertions(+), 52 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index ea54110b6..83e06b996 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3557,75 +3557,104 @@ public object VisitUsingExpression(UsingExpressionAst usingExpressionAst) } /// Class to represent a directed graph - public class Digraph + /// Class to represent a directed graph + public class Digraph { + public int NumVertices + { + get { return graph.Count; } + } + private List> graph; + private Dictionary vertexIndexMap; - public Digraph() + + private int GetIndex(T vertex) { - graph = new List> (); + int idx; + return vertexIndexMap.TryGetValue(vertex, out idx) ? idx : -1; } - public Digraph(int numVertices, List> edges) : this() + public IEnumerable GetNeighbors(T vertex) { - for (int v = 0; v < numVertices; v++) + ValidateVertexArgument(vertex); + var idx = GetIndex(vertex); + var idxVertexMap = vertexIndexMap.ToDictionary(x => x.Value, x => x.Key); + foreach (var neighbor in graph[idx]) { - AddVertex(); + yield return idxVertexMap[neighbor]; } + } - foreach (Tuple tuple in edges) - { - var fromV = tuple.Item1; - var toV = tuple.Item2; - ValidateVertex(fromV); - ValidateVertex(toV); - AddEdge(fromV, toV); - } + public IEnumerable GetVertices() + { + return vertexIndexMap.Keys; } - public int GetNumVertices() + public bool ContainsVertex(T vertex) { - return graph.Count; + return vertexIndexMap.Keys.Contains(vertex); } - public int GetNumNeighbors(int vertex) + public int GetNumNeighbors(T vertex) { - return GetNeighbors(vertex).Count(); + ValidateVertexArgument(vertex); + return graph[GetIndex(vertex)].Count; } - public IEnumerable GetNeighbors(int vertex) + public Digraph() { - ValidateVertex(vertex); - return graph[vertex]; + graph = new List> (); + vertexIndexMap = new Dictionary(); } - public bool ContainsVertex(int vertex) + public Digraph(IEqualityComparer comparer) : this() { - return vertex >= 0 && vertex < graph.Count; + vertexIndexMap = new Dictionary (comparer); } - protected void AddVertex() + public void AddVertex(T vertex) { + if (vertex == null) + { + throw new ArgumentNullException("vertex"); + } + + if (GetIndex(vertex) != -1) + { + throw new ArgumentException("Vertex already present! Cannot add it to the Digraph", "vertex"); + } + + vertexIndexMap.Add(vertex, graph.Count); graph.Add(new List()); } - protected void AddEdge(int fromVertex, int toVertex) + public void AddEdge(T fromVertex, T toVertex) { - ValidateVertex(fromVertex); - ValidateVertex(toVertex); - if (graph[fromVertex].Contains(toVertex)) + ValidateVertexArgument(fromVertex); + ValidateVertexArgument(toVertex); + + var toIdx = GetIndex(toVertex); + var fromVertexList = graph[GetIndex(fromVertex)]; + if (fromVertexList.Contains(toIdx)) { - throw new ArgumentException(String.Format("Edge from {0} to {1} already present", fromVertex, toVertex)); + throw new ArgumentException(String.Format( + "Edge from {0} to {1} already present.", + fromVertex.ToString(), + toVertex.ToString())); + } + else + { + fromVertexList.Add(toIdx); } - graph[fromVertex].Add(toVertex); } - public bool IsConnected(int vertex1, int vertex2) + public bool IsConnected(T vertex1, T vertex2) { - ValidateVertex(vertex1); - ValidateVertex(vertex2); + ValidateVertexArgument(vertex1); + ValidateVertexArgument(vertex2); var visited = new bool[graph.Count]; - return IsConnected(vertex1, vertex2, ref visited); + return IsConnected(GetIndex(vertex1), GetIndex(vertex2), ref visited); } private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) @@ -3653,12 +3682,26 @@ private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) return isConnected; } - private void ValidateVertex(int vertex) + private void ValidateNotNull(T vertex) { - if (!ContainsVertex(vertex)) + if (vertex == null) + { + throw new ArgumentNullException("vertex"); + } + } + + private void ValidateVertexPresence(T vertex) + { + if (GetIndex(vertex) == -1) { throw new ArgumentOutOfRangeException("vertex not present in the Digraph.", "vertex"); } } + + private void ValidateVertexArgument(T vertex) + { + ValidateNotNull(vertex); + ValidateVertexPresence(vertex); + } } } diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index d18646ae6..967c42bc2 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -11,6 +11,7 @@ // using System; +using System.Linq; using System.Collections.Generic; using System.Management.Automation.Language; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; @@ -130,6 +131,103 @@ public string GetSourceName() } } + + + class FunctionReferenceDigraphCreator : AstVisitor + { + private Digraph digraph; + + private bool isWithinFunctionDefinition; + private string functionName; + + public Digraph GetDigraph() + { + return digraph; + } + public FunctionReferenceDigraphCreator() + { + digraph = new Digraph(StringComparer.OrdinalIgnoreCase); + isWithinFunctionDefinition = false; + } + + public override AstVisitAction VisitCommand(CommandAst ast) + { + if (ast == null) + { + return AstVisitAction.SkipChildren; + } + + var cmdName = ast.GetCommandName(); + AddVertex(cmdName); + if (isWithinFunctionDefinition) + { + AddEdge(functionName, cmdName); + } + return AstVisitAction.Continue; + } + + public void AddVertex(string name) + { + if (!digraph.ContainsVertex(name)) + { + digraph.AddVertex(name); + } + } + + public void AddEdge(string fromV, string toV) + { + if (!digraph.GetNeighbors(fromV).Contains(toV, StringComparer.OrdinalIgnoreCase)) + { + digraph.AddEdge(fromV, toV); + } + } + + public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast) + { + if (ast == null) + { + return AstVisitAction.SkipChildren; + } + + isWithinFunctionDefinition = true; + functionName = ast.Name; + AddVertex(ast.Name); + ast.Body.Visit(this); + isWithinFunctionDefinition = false; + return AstVisitAction.SkipChildren; + } + + public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst ast) + { + if (ast == null) + { + return AstVisitAction.SkipChildren; + } + + var expr = ast.Expression.Extent.Text; + var memberExprAst = ast.Member as StringConstantExpressionAst; + if (memberExprAst == null) + { + return AstVisitAction.Continue; + } + + var member = memberExprAst.Value; + if (string.IsNullOrWhiteSpace(member)) + { + return AstVisitAction.Continue; + } + + AddVertex(expr); + AddVertex(member); + AddEdge(expr, member); + if (isWithinFunctionDefinition) + { + AddEdge(functionName, expr); + } + + return AstVisitAction.Continue; + } + } } diff --git a/Tests/Engine/Helpers.tests.ps1 b/Tests/Engine/Helpers.tests.ps1 index efbe2ac34..f37fcf42e 100644 --- a/Tests/Engine/Helpers.tests.ps1 +++ b/Tests/Engine/Helpers.tests.ps1 @@ -1,30 +1,31 @@ Import-Module PSScriptAnalyzer - -Function ConvertType($x) -{ - $z = [System.Collections.Generic.List[Tuple[int,int]]]::new() - $x | ForEach-Object {$z.Add([System.Tuple[int,int]]::new($_[0], $_[1]))} - return $z -} - Describe "Test Directed Graph" { Context "When a graph is created" { - $edges = ConvertType (0,1),(0,4),(1,3) - $digraph = New-Object -TypeName 'Microsoft.Windows.PowerShell.ScriptAnalyzer.DiGraph' -ArgumentList 5,$edges + $digraph = New-Object -TypeName 'Microsoft.Windows.PowerShell.ScriptAnalyzer.DiGraph[string]' + $digraph.AddVertex('v1'); + $digraph.AddVertex('v2'); + $digraph.AddVertex('v3'); + $digraph.AddVertex('v4'); + $digraph.AddVertex('v5'); + + $digraph.AddEdge('v1', 'v2'); + $digraph.AddEdge('v1', 'v5'); + $digraph.AddEdge('v2', 'v4'); + It "correctly adds the vertices" { - $digraph.GetNumVertices() | Should Be 5 + $digraph.NumVertices | Should Be 5 } It "correctly adds the edges" { - $digraph.GetNumNeighbors(0) | Should Be 2 - $neighbors = $digraph.GetNeighbors(0) - $neighbors -contains 1 | Should Be $true - $neighbors -contains 4 | Should Be $true + $digraph.GetNumNeighbors('v1') | Should Be 2 + $neighbors = $digraph.GetNeighbors('v1') + $neighbors -contains 'v2' | Should Be $true + $neighbors -contains 'v5' | Should Be $true } It "finds the connection" { - $digraph.IsConnected(0, 3) | Should Be $true + $digraph.IsConnected('v1', 'v4') | Should Be $true } } } \ No newline at end of file From fc64631c2200b3da2966fbc4eabee895aa9c3aeb Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Tue, 27 Sep 2016 16:16:12 -0700 Subject: [PATCH 06/19] Change UseShouldProcess rule implementation The new implementation adds ability to check if a nested function calls shouldprocess/shouldcontinue when an upstream function declares SupportsShouldProcess attribute. --- Rules/UseShouldProcessCorrectly.cs | 332 +++++++++++++++--- .../Rules/UseShouldProcessCorrectly.tests.ps1 | 56 +++ 2 files changed, 333 insertions(+), 55 deletions(-) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 967c42bc2..128b514bf 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -30,6 +30,23 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public class UseShouldProcessCorrectly : IScriptRule { + private Ast ast; + private string fileName; + private FunctionReferenceDigraph funcDigraph; + + private List diagnosticRecords; + + private readonly Vertex shouldProcessVertex; + private readonly Vertex shouldContinueVertex; + + + public UseShouldProcessCorrectly() + { + diagnosticRecords = new List(); + shouldContinueVertex = new Vertex {name="ShouldContinue", ast=null}; + shouldProcessVertex = new Vertex {name="ShouldProcess", ast=null}; + } + /// /// AnalyzeScript: Analyzes the ast to check that if the ShouldProcess attribute is present, the function calls ShouldProcess and vice versa. /// @@ -40,40 +57,65 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - IEnumerable funcDefAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true); - IEnumerable attributeAsts; - IEnumerable memberAsts; - IScriptExtent extent; - string funcName; - string supportsShouldProcess = "SupportsShouldProcess"; - string trueString = "$true"; - bool hasShouldProcessAttribute; - bool callsShouldProcess; - - foreach (FunctionDefinitionAst funcDefAst in funcDefAsts) { - extent = funcDefAst.Extent; - funcName = funcDefAst.Name; - - hasShouldProcessAttribute = false; - callsShouldProcess = false; - - attributeAsts = funcDefAst.FindAll(testAst => testAst is NamedAttributeArgumentAst, true); - foreach (NamedAttributeArgumentAst attributeAst in attributeAsts) { - hasShouldProcessAttribute |= ((attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.Argument.Extent.Text.Equals(trueString, StringComparison.OrdinalIgnoreCase)) - // checks for the case if the user just use the attribute without setting it to true - || (attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.ExpressionOmitted)); - } + diagnosticRecords.Clear(); + this.ast = ast; + this.fileName = fileName; + funcDigraph = new FunctionReferenceDigraph(); + ast.Visit(funcDigraph); + FindViolations(); + foreach (var dr in diagnosticRecords) + { + yield return dr; + } - memberAsts = funcDefAst.FindAll(testAst => testAst is MemberExpressionAst, true); - foreach (MemberExpressionAst memberAst in memberAsts) { - callsShouldProcess |= memberAst.Member.Extent.Text.Equals("ShouldProcess", StringComparison.OrdinalIgnoreCase) || memberAst.Member.Extent.Text.Equals("ShouldContinue", StringComparison.OrdinalIgnoreCase); - } + // yield break; + + // IEnumerable funcDefAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true); + // IEnumerable attributeAsts; + // IEnumerable memberAsts; + // IScriptExtent extent; + // string funcName; + // string supportsShouldProcess = "SupportsShouldProcess"; + // string trueString = "$true"; + // bool hasShouldProcessAttribute; + // bool callsShouldProcess; + + // foreach (FunctionDefinitionAst funcDefAst in funcDefAsts) { + // extent = funcDefAst.Extent; + // funcName = funcDefAst.Name; + + // hasShouldProcessAttribute = false; + // callsShouldProcess = false; + + // attributeAsts = funcDefAst.FindAll(testAst => testAst is NamedAttributeArgumentAst, true); + // foreach (NamedAttributeArgumentAst attributeAst in attributeAsts) { + // hasShouldProcessAttribute |= ((attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.Argument.Extent.Text.Equals(trueString, StringComparison.OrdinalIgnoreCase)) + // // checks for the case if the user just use the attribute without setting it to true + // || (attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.ExpressionOmitted)); + // } + + // memberAsts = funcDefAst.FindAll(testAst => testAst is MemberExpressionAst, true); + // foreach (MemberExpressionAst memberAst in memberAsts) { + // callsShouldProcess |= memberAst.Member.Extent.Text.Equals("ShouldProcess", StringComparison.OrdinalIgnoreCase) || memberAst.Member.Extent.Text.Equals("ShouldContinue", StringComparison.OrdinalIgnoreCase); + // } + + // if (hasShouldProcessAttribute && !callsShouldProcess) { + // yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ShouldProcessErrorHasAttribute, funcName), extent, GetName(), DiagnosticSeverity.Warning, fileName); + // } + // else if (!hasShouldProcessAttribute && callsShouldProcess) { + // yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ShouldProcessErrorHasCmdlet, funcName), extent, GetName(), DiagnosticSeverity.Warning, fileName); + // } + // } + } - if (hasShouldProcessAttribute && !callsShouldProcess) { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ShouldProcessErrorHasAttribute, funcName), extent, GetName(), DiagnosticSeverity.Warning, fileName); - } - else if (!hasShouldProcessAttribute && callsShouldProcess) { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ShouldProcessErrorHasCmdlet, funcName), extent, GetName(), DiagnosticSeverity.Warning, fileName); + private void FindViolations() + { + foreach (var v in funcDigraph.GetVertices()) + { + var dr = GetViolation(v); + if (dr != null) + { + diagnosticRecords.Add(dr); } } } @@ -129,25 +171,187 @@ public string GetSourceName() { return string.Format(CultureInfo.CurrentCulture,Strings.SourceName); } + + private DiagnosticRecord GetViolation(Vertex v) + { + bool callsShouldProcess = funcDigraph.IsConnected(v, shouldContinueVertex) + || funcDigraph.IsConnected(v, shouldProcessVertex); + FunctionDefinitionAst fast = v.ast as FunctionDefinitionAst; + if (fast == null) + { + return null; + } + + if (DeclaresSupportsShouldProcess(fast)) + { + if (!callsShouldProcess) + { + return new DiagnosticRecord( + string.Format( + CultureInfo.CurrentCulture, + Strings.ShouldProcessErrorHasAttribute, + fast.Name), + ast.Extent, + GetName(), + DiagnosticSeverity.Warning, + ast.Extent.File); + } + } + else + { + if (callsShouldProcess) + { + // check if upstream function declares SupportShouldProcess\ + // if so, this might just be a helper function + // do not flag this case + if (UpstreamDeclaresShouldProcess(v)) + { + return null; + } + + return new DiagnosticRecord( + string.Format( + CultureInfo.CurrentCulture, + Strings.ShouldProcessErrorHasCmdlet, + fast.Name), + v.ast.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName); + } + } + + return null; + } + + private bool UpstreamDeclaresShouldProcess(Vertex v) + { + var equalityComparer = new VertexEqualityComparer(); + foreach (var vertex in funcDigraph.GetVertices()) + { + if (equalityComparer.Equals(vertex, v)) + { + continue; + } + + var fast = vertex.ast as FunctionDefinitionAst; + if (fast == null) + { + continue; + } + + if (DeclaresSupportsShouldProcess(fast) + && funcDigraph.IsConnected(vertex, v)) + { + return true; + } + } + return false; + } + + private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) + { + if (ast.Body.ParamBlock == null) + { + return false; + } + + foreach (var attr in ast.Body.ParamBlock.Attributes) + { + if (attr.NamedArguments == null) + { + continue; + } + + foreach (var namedArg in attr.NamedArguments) + { + if (namedArg.ArgumentName.Equals( + "SupportsShouldProcess", + StringComparison.OrdinalIgnoreCase)) + { + var argAst = namedArg.Argument as VariableExpressionAst; + if (argAst != null) + { + if (argAst.VariablePath.UserPath.Equals("true", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + else + { + return namedArg.ExpressionOmitted; + } + } + } + } + + return false; + } + } + + class Vertex + { + public string name; + public Ast ast; + public override string ToString() + { + return name; + } + } + + class VertexEqualityComparer : IEqualityComparer + { + public bool Equals(Vertex x, Vertex y) + { + if (x == null && y == null) + { + return true; + } + else if (x == null || y == null) + { + return false; + } + else if (x.name.Equals(y.name, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + else + { + return false; + } + } + + public int GetHashCode(Vertex obj) + { + return obj.name.GetHashCode(); + } } - class FunctionReferenceDigraphCreator : AstVisitor + class FunctionReferenceDigraph : AstVisitor { - private Digraph digraph; + private Digraph digraph; - private bool isWithinFunctionDefinition; - private string functionName; + private Stack functionVisitStack; + private bool IsWithinFunctionDefinition() + { + return functionVisitStack.Count > 0; + } - public Digraph GetDigraph() + private Vertex GetCurrentFunctionContext() + { + return functionVisitStack.Peek(); + } + + public Digraph GetDigraph() { return digraph; } - public FunctionReferenceDigraphCreator() + public FunctionReferenceDigraph() { - digraph = new Digraph(StringComparer.OrdinalIgnoreCase); - isWithinFunctionDefinition = false; + digraph = new Digraph(new VertexEqualityComparer()); + functionVisitStack = new Stack(); } public override AstVisitAction VisitCommand(CommandAst ast) @@ -158,15 +362,16 @@ public override AstVisitAction VisitCommand(CommandAst ast) } var cmdName = ast.GetCommandName(); - AddVertex(cmdName); - if (isWithinFunctionDefinition) + var vertex = new Vertex {name = cmdName, ast = ast}; + AddVertex(vertex); + if (IsWithinFunctionDefinition()) { - AddEdge(functionName, cmdName); + AddEdge(GetCurrentFunctionContext(), vertex); } return AstVisitAction.Continue; } - public void AddVertex(string name) + public void AddVertex(Vertex name) { if (!digraph.ContainsVertex(name)) { @@ -174,9 +379,9 @@ public void AddVertex(string name) } } - public void AddEdge(string fromV, string toV) + public void AddEdge(Vertex fromV, Vertex toV) { - if (!digraph.GetNeighbors(fromV).Contains(toV, StringComparer.OrdinalIgnoreCase)) + if (!digraph.GetNeighbors(fromV).Contains(toV, new VertexEqualityComparer())) { digraph.AddEdge(fromV, toV); } @@ -189,11 +394,11 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast return AstVisitAction.SkipChildren; } - isWithinFunctionDefinition = true; - functionName = ast.Name; - AddVertex(ast.Name); + var functionVertex = new Vertex {name=ast.Name, ast=ast}; + functionVisitStack.Push(functionVertex); + AddVertex(functionVertex); ast.Body.Visit(this); - isWithinFunctionDefinition = false; + functionVisitStack.Pop(); return AstVisitAction.SkipChildren; } @@ -217,16 +422,33 @@ public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressio return AstVisitAction.Continue; } - AddVertex(expr); - AddVertex(member); - AddEdge(expr, member); - if (isWithinFunctionDefinition) + var exprVertex = new Vertex {name=expr, ast=ast.Expression}; + var memberVertex = new Vertex {name=memberExprAst.Value, ast=memberExprAst}; + AddVertex(exprVertex); + AddVertex(memberVertex); + AddEdge(exprVertex, memberVertex); + if (IsWithinFunctionDefinition()) { - AddEdge(functionName, expr); + AddEdge(GetCurrentFunctionContext(), exprVertex); } return AstVisitAction.Continue; } + + public IEnumerable GetVertices() + { + return digraph.GetVertices(); + } + + internal bool IsConnected(Vertex vertex, Vertex shouldVertex) + { + if (digraph.ContainsVertex(vertex) + && digraph.ContainsVertex(shouldVertex)) + { + return digraph.IsConnected(vertex, shouldVertex); + } + return false; + } } } diff --git a/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 b/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 index 908c96d24..4b789e8d4 100644 --- a/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 +++ b/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 @@ -22,4 +22,60 @@ Describe "UseShouldProcessCorrectly" { $noViolations.Count | Should Be 0 } } + + Context "Where ShouldProcess is called in nested function" { + It "finds no violation" { + $scriptDef = @' +function Outer +{ + [CmdletBinding(SupportsShouldProcess=$true)] + param() + + Inner +} + +function Inner +{ + [CmdletBinding(SupportsShouldProcess=$true)] + param() + + if ($PSCmdlet.ShouldProcess("Inner")) + { + Write-Host "Process!" + } + else + { + Write-Host "Skipped!" + } +} + +Outer -WhatIf +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + + It "finds no violation" { + $scriptDef = @' +function Foo +{ + [CmdletBinding(SupportsShouldProcess)] + param() + begin + { + function helper + { + if ($PSCmdlet.ShouldProcess('','')) + { + + } + } + helper + } +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + } } \ No newline at end of file From 09b82d0e45dfee9dc70edab15ecb50dbfbf64015 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 28 Sep 2016 11:14:13 -0700 Subject: [PATCH 07/19] Remove commented code --- Rules/UseShouldProcessCorrectly.cs | 39 ------------------------------ 1 file changed, 39 deletions(-) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 128b514bf..cc1ba955a 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -67,45 +67,6 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { yield return dr; } - - // yield break; - - // IEnumerable funcDefAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true); - // IEnumerable attributeAsts; - // IEnumerable memberAsts; - // IScriptExtent extent; - // string funcName; - // string supportsShouldProcess = "SupportsShouldProcess"; - // string trueString = "$true"; - // bool hasShouldProcessAttribute; - // bool callsShouldProcess; - - // foreach (FunctionDefinitionAst funcDefAst in funcDefAsts) { - // extent = funcDefAst.Extent; - // funcName = funcDefAst.Name; - - // hasShouldProcessAttribute = false; - // callsShouldProcess = false; - - // attributeAsts = funcDefAst.FindAll(testAst => testAst is NamedAttributeArgumentAst, true); - // foreach (NamedAttributeArgumentAst attributeAst in attributeAsts) { - // hasShouldProcessAttribute |= ((attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.Argument.Extent.Text.Equals(trueString, StringComparison.OrdinalIgnoreCase)) - // // checks for the case if the user just use the attribute without setting it to true - // || (attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase) && attributeAst.ExpressionOmitted)); - // } - - // memberAsts = funcDefAst.FindAll(testAst => testAst is MemberExpressionAst, true); - // foreach (MemberExpressionAst memberAst in memberAsts) { - // callsShouldProcess |= memberAst.Member.Extent.Text.Equals("ShouldProcess", StringComparison.OrdinalIgnoreCase) || memberAst.Member.Extent.Text.Equals("ShouldContinue", StringComparison.OrdinalIgnoreCase); - // } - - // if (hasShouldProcessAttribute && !callsShouldProcess) { - // yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ShouldProcessErrorHasAttribute, funcName), extent, GetName(), DiagnosticSeverity.Warning, fileName); - // } - // else if (!hasShouldProcessAttribute && callsShouldProcess) { - // yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ShouldProcessErrorHasCmdlet, funcName), extent, GetName(), DiagnosticSeverity.Warning, fileName); - // } - // } } private void FindViolations() From d858e51989d5acdf4a98685a0388b980a818fbdf Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 28 Sep 2016 15:03:10 -0700 Subject: [PATCH 08/19] Add inline documentation --- Engine/Helper.cs | 140 +++++++++++++++++++++------ Rules/UseShouldProcessCorrectly.cs | 148 ++++++++++++++++++++++------- 2 files changed, 222 insertions(+), 66 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 83e06b996..53da8157d 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3556,70 +3556,107 @@ public object VisitUsingExpression(UsingExpressionAst usingExpressionAst) } } - /// Class to represent a directed graph /// Class to represent a directed graph public class Digraph { - public int NumVertices - { - get { return graph.Count; } - } - private List> graph; private Dictionary vertexIndexMap; - - private int GetIndex(T vertex) + /// + /// Public constructor + /// + public Digraph() { - int idx; - return vertexIndexMap.TryGetValue(vertex, out idx) ? idx : -1; + graph = new List>(); + vertexIndexMap = new Dictionary(); } - public IEnumerable GetNeighbors(T vertex) + /// + /// Construct digraph with an EqualityComparer used for comparing type T + /// + /// + public Digraph(IEqualityComparer equalityComparer) : this() { - ValidateVertexArgument(vertex); - var idx = GetIndex(vertex); - var idxVertexMap = vertexIndexMap.ToDictionary(x => x.Value, x => x.Key); - foreach (var neighbor in graph[idx]) + if (equalityComparer == null) { - yield return idxVertexMap[neighbor]; + throw new ArgumentNullException("equalityComparer"); } + + vertexIndexMap = new Dictionary(equalityComparer); } + /// + /// Return the number of vertices in the graph + /// + public int NumVertices + { + get { return graph.Count; } + } + + /// + /// Return an enumerator over the vertices in the graph + /// public IEnumerable GetVertices() { return vertexIndexMap.Keys; } + /// + /// Check if the given vertex is part of the graph. + /// + /// If the vertex is null, it will throw an ArgumentNullException. + /// If the vertex is non-null but not present in the graph, it will throw an ArgumentOutOfRangeException + /// + /// + /// True if the graph contains the vertex, otherwise false public bool ContainsVertex(T vertex) { return vertexIndexMap.Keys.Contains(vertex); } - public int GetNumNeighbors(T vertex) + /// + /// Get the neighbors of a given vertex + /// + /// If the vertex is null, it will throw an ArgumentNullException. + /// If the vertex is non-null but not present in the graph, it will throw an ArgumentOutOfRangeException + /// + /// + /// An enumerator over the neighbors of the vertex + public IEnumerable GetNeighbors(T vertex) { ValidateVertexArgument(vertex); - return graph[GetIndex(vertex)].Count; - } - - public Digraph() - { - graph = new List> (); - vertexIndexMap = new Dictionary(); + var idx = GetIndex(vertex); + var idxVertexMap = vertexIndexMap.ToDictionary(x => x.Value, x => x.Key); + foreach (var neighbor in graph[idx]) + { + yield return idxVertexMap[neighbor]; + } } - public Digraph(IEqualityComparer comparer) : this() + /// + /// Gets the number of neighbors of the given vertex + /// + /// If the vertex is null, it will throw an ArgumentNullException. + /// If the vertex is non-null but not present in the graph, it will throw an ArgumentOutOfRangeException + /// + /// + /// + public int GetNumNeighbors(T vertex) { - vertexIndexMap = new Dictionary (comparer); + ValidateVertexArgument(vertex); + return graph[GetIndex(vertex)].Count; } + /// + /// Add a vertex to the graph + /// + /// If the vertex is null, it will throw an ArgumentNullException. + /// If the vertex is non-null but already present in the graph, it will throw an ArgumentException + /// + /// public void AddVertex(T vertex) { - if (vertex == null) - { - throw new ArgumentNullException("vertex"); - } - + ValidateNotNull(vertex); if (GetIndex(vertex) != -1) { throw new ArgumentException("Vertex already present! Cannot add it to the Digraph", "vertex"); @@ -3629,6 +3666,14 @@ public void AddVertex(T vertex) graph.Add(new List()); } + /// + /// Add an edge from one vertex to another + /// + /// If any input vertex is null, it will throw an ArgumentNullException + /// If an edge is already present between the given vertices, it will throw an ArgumentException + /// + /// + /// public void AddEdge(T fromVertex, T toVertex) { ValidateVertexArgument(fromVertex); @@ -3649,14 +3694,28 @@ public void AddEdge(T fromVertex, T toVertex) } } + /// + /// Checks if a vertex is connected to another vertex within the graph + /// + /// + /// + /// public bool IsConnected(T vertex1, T vertex2) { ValidateVertexArgument(vertex1); ValidateVertexArgument(vertex2); + var visited = new bool[graph.Count]; return IsConnected(GetIndex(vertex1), GetIndex(vertex2), ref visited); } + /// + /// Check if two vertices are connected + /// + /// Origin vertex + /// Destination vertex + /// A boolean array indicating whether a vertex has been visited or not + /// True if the vertices are conneted, otherwise false private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) { visited[fromIdx] = true; @@ -3682,6 +3741,9 @@ private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) return isConnected; } + /// + /// Throw an ArgumentNullException if vertex is null + /// private void ValidateNotNull(T vertex) { if (vertex == null) @@ -3690,6 +3752,9 @@ private void ValidateNotNull(T vertex) } } + /// + /// Throw an ArgumentOutOfRangeException if vertex is not present in the graph + /// private void ValidateVertexPresence(T vertex) { if (GetIndex(vertex) == -1) @@ -3698,10 +3763,23 @@ private void ValidateVertexPresence(T vertex) } } + /// + /// Throw exception if vertex is null or not present in graph + /// private void ValidateVertexArgument(T vertex) { ValidateNotNull(vertex); ValidateVertexPresence(vertex); } + + /// + /// Get the index of the vertex in the graph array + /// + private int GetIndex(T vertex) + { + int idx; + return vertexIndexMap.TryGetValue(vertex, out idx) ? idx : -1; + } + } } diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index cc1ba955a..bb057b814 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -69,18 +69,6 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) } } - private void FindViolations() - { - foreach (var v in funcDigraph.GetVertices()) - { - var dr = GetViolation(v); - if (dr != null) - { - diagnosticRecords.Add(dr); - } - } - } - /// /// GetName: Retrieves the name of this rule. /// @@ -133,6 +121,26 @@ public string GetSourceName() return string.Format(CultureInfo.CurrentCulture,Strings.SourceName); } + /// + /// Find the violations in the current AST + /// + private void FindViolations() + { + foreach (var v in funcDigraph.GetVertices()) + { + var dr = GetViolation(v); + if (dr != null) + { + diagnosticRecords.Add(dr); + } + } + } + + /// + /// Get violation for a given function definition node + /// + /// Graph vertex, must be non-null + /// An instance of DiagnosticRecord if it find violation, otherwise null private DiagnosticRecord GetViolation(Vertex v) { bool callsShouldProcess = funcDigraph.IsConnected(v, shouldContinueVertex) @@ -185,6 +193,11 @@ private DiagnosticRecord GetViolation(Vertex v) return null; } + /// + /// Checks if an upstream function declares SupportsShouldProcess + /// + /// Graph vertex, must be non-null + /// true if an upstream function declares SupportsShouldProcess, otherwise false private bool UpstreamDeclaresShouldProcess(Vertex v) { var equalityComparer = new VertexEqualityComparer(); @@ -210,6 +223,11 @@ private bool UpstreamDeclaresShouldProcess(Vertex v) return false; } + /// + /// Checks if a function declares SupportShouldProcess attribute + /// + /// A non-null instance of FunctionDefinitionAst type + /// True if the given function declares SupportShouldProcess, otherwise null private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) { if (ast.Body.ParamBlock == null) @@ -250,18 +268,32 @@ private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) } } + /// + /// Class to represent a vertex in a function call graph + /// class Vertex { public string name; public Ast ast; + + /// + /// Returns string representation of a Vertex instance + /// public override string ToString() { return name; } } + /// + /// Implements IEqualityComparer interface for Vertex class + /// class VertexEqualityComparer : IEqualityComparer { + + /// + /// Compares two instances of Vertex class to check for equality + /// public bool Equals(Vertex x, Vertex y) { if (x == null && y == null) @@ -282,6 +314,9 @@ public bool Equals(Vertex x, Vertex y) } } + /// + /// Returns the Hash code of the given Vertex instance + /// public int GetHashCode(Vertex obj) { return obj.name.GetHashCode(); @@ -289,49 +324,51 @@ public int GetHashCode(Vertex obj) } - + /// + /// Class to encapsulate a function call graph and related actions + /// class FunctionReferenceDigraph : AstVisitor { private Digraph digraph; private Stack functionVisitStack; + + /// + /// Checks if the AST being visited is in an instance FunctionDefinitionAst type + /// private bool IsWithinFunctionDefinition() { return functionVisitStack.Count > 0; } + /// + /// Returns the function vertex whose children are being currently visited + /// private Vertex GetCurrentFunctionContext() { return functionVisitStack.Peek(); } + /// + /// Return the constructed digraph + /// public Digraph GetDigraph() { return digraph; } + + /// + /// Public constructor + /// public FunctionReferenceDigraph() { digraph = new Digraph(new VertexEqualityComparer()); functionVisitStack = new Stack(); } - public override AstVisitAction VisitCommand(CommandAst ast) - { - if (ast == null) - { - return AstVisitAction.SkipChildren; - } - - var cmdName = ast.GetCommandName(); - var vertex = new Vertex {name = cmdName, ast = ast}; - AddVertex(vertex); - if (IsWithinFunctionDefinition()) - { - AddEdge(GetCurrentFunctionContext(), vertex); - } - return AstVisitAction.Continue; - } - + /// + /// Add a vertex to the graph + /// public void AddVertex(Vertex name) { if (!digraph.ContainsVertex(name)) @@ -340,6 +377,11 @@ public void AddVertex(Vertex name) } } + /// + /// Add an edge from a vertex to another vertex + /// + /// start of the edge + /// end of the edge public void AddEdge(Vertex fromV, Vertex toV) { if (!digraph.GetNeighbors(fromV).Contains(toV, new VertexEqualityComparer())) @@ -348,6 +390,9 @@ public void AddEdge(Vertex fromV, Vertex toV) } } + /// + /// Add a function to the graph; create a function context; and visit the function body + /// public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast) { if (ast == null) @@ -355,7 +400,7 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast return AstVisitAction.SkipChildren; } - var functionVertex = new Vertex {name=ast.Name, ast=ast}; + var functionVertex = new Vertex { name = ast.Name, ast = ast }; functionVisitStack.Push(functionVertex); AddVertex(functionVertex); ast.Body.Visit(this); @@ -363,6 +408,29 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast return AstVisitAction.SkipChildren; } + /// + /// Add a command to the graph and if within a function definition, add an edge from the calling function to the command + /// + public override AstVisitAction VisitCommand(CommandAst ast) + { + if (ast == null) + { + return AstVisitAction.SkipChildren; + } + + var cmdName = ast.GetCommandName(); + var vertex = new Vertex { name = cmdName, ast = ast }; + AddVertex(vertex); + if (IsWithinFunctionDefinition()) + { + AddEdge(GetCurrentFunctionContext(), vertex); + } + return AstVisitAction.Continue; + } + + /// + /// Add a member to the graph and if within a function definition, add an edge from the function to the member + /// public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst ast) { if (ast == null) @@ -383,6 +451,11 @@ public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressio return AstVisitAction.Continue; } + // Suppose we find ., we split it up and create + // and edge from ->. Even though is not + // necessarily a function, we do it because we are mainly interested in + // finding connection between a function and ShouldProcess and this approach + // prevents any unnecessary complexity. var exprVertex = new Vertex {name=expr, ast=ast.Expression}; var memberVertex = new Vertex {name=memberExprAst.Value, ast=memberExprAst}; AddVertex(exprVertex); @@ -396,11 +469,20 @@ public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressio return AstVisitAction.Continue; } + /// + /// Return the vertices in the graph + /// public IEnumerable GetVertices() { return digraph.GetVertices(); } + /// + /// Check if two vertices are connected + /// + /// Origin vertxx + /// Destination vertex + /// internal bool IsConnected(Vertex vertex, Vertex shouldVertex) { if (digraph.ContainsVertex(vertex) @@ -412,7 +494,3 @@ internal bool IsConnected(Vertex vertex, Vertex shouldVertex) } } } - - - - From bdb3ef4fbb55ead4fa70197c0272b49d56836983 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 28 Sep 2016 15:04:46 -0700 Subject: [PATCH 09/19] Rename helper test file --- Tests/Engine/{Helpers.tests.ps1 => Helper.tests.ps1} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Tests/Engine/{Helpers.tests.ps1 => Helper.tests.ps1} (100%) diff --git a/Tests/Engine/Helpers.tests.ps1 b/Tests/Engine/Helper.tests.ps1 similarity index 100% rename from Tests/Engine/Helpers.tests.ps1 rename to Tests/Engine/Helper.tests.ps1 From 0d5b12f102b5bb7ba0dbd17786d5fd32ccfa42be Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 28 Sep 2016 15:48:19 -0700 Subject: [PATCH 10/19] Remove checking for ShouldContinue If a cmdlet declares SupportsShouldProcess attribute, it must call ShouldProcess but calling ShouldContinue is optional. --- .../UseShouldProcessCorrectly.md | 48 +++++-------------- Rules/UseShouldProcessCorrectly.cs | 6 +-- Tests/Rules/GoodCmdlet.ps1 | 34 ++++++------- 3 files changed, 30 insertions(+), 58 deletions(-) diff --git a/RuleDocumentation/UseShouldProcessCorrectly.md b/RuleDocumentation/UseShouldProcessCorrectly.md index 29fc398b7..e05dbc41c 100644 --- a/RuleDocumentation/UseShouldProcessCorrectly.md +++ b/RuleDocumentation/UseShouldProcessCorrectly.md @@ -2,17 +2,12 @@ **Severity Level: Warning** ##Description -Checks that if the `SupportsShouldProcess` is present, i.e, `[CmdletBinding(SupportsShouldProcess = $true)]`, and then tests if the function or cmdlet calls -ShouldProcess or ShouldContinue; i.e `$PSCmdlet.ShouldProcess` or `$PSCmdlet.ShouldContinue`. +If a cmdlet declares the `SupportsShouldProcess` attribute, then it should also call `ShouldProcess`. A violation is any function which either declares `SupportsShouldProcess` attribute but makes no calls to `ShouldProcess` or it calls `ShouldProcess` but does not does not declare `SupportsShouldProcess` -A violation is any function where `SupportsShouldProcess` that makes no calls to `ShouldProcess` or `ShouldContinue`. - -Scripts with one or the other but not both will generally run into an error or unexpected behavior. +For more information, please refer to `about_Functions_Advanced_Methods` and `about_Functions_CmdletBindingAttribute` ##How to Fix -To fix a violation of this rule, please call `ShouldProcess` method in advanced functions when `SupportsShouldProcess` argument is present. -Or please add `SupportsShouldProcess` argument when calling `ShouldProcess`. -You can get more details by running `Get-Help about_Functions_CmdletBindingAttribute` and `Get-Help about_Functions_Advanced_Methods` command in Windows PowerShell. +To fix a violation of this rule, please call `ShouldProcess` method when a cmdlet declares `SupportsShouldProcess` attribute. Or please add `SupportsShouldProcess` attribute argument when calling `ShouldProcess`. ##Example ###Wrong： @@ -26,17 +21,7 @@ You can get more details by running `Get-Help about_Functions_CmdletBindingAttri [Parameter(Mandatory=$true)] $Path ) - - Begin - { - } - Process - { - "String" | Out-File -FilePath $FilePath - } - End - { - } + "String" | Out-File -FilePath $FilePath } ``` @@ -52,22 +37,13 @@ You can get more details by running `Get-Help about_Functions_CmdletBindingAttri $Path ) - Begin - { - } - Process - { - if ($PSCmdlet.ShouldProcess("Target", "Operation")) - { - "String" | Out-File -FilePath $FilePath - } - } - End - { - if ($pscmdlet.ShouldContinue("Yes", "No")) - { - ... - } - } + if ($PSCmdlet.ShouldProcess("Target", "Operation")) + { + "String" | Out-File -FilePath $FilePath + } + else + { + Write-Host ('Write "String" to file {0}' -f $FilePath) + } } ``` diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index bb057b814..2e6e144ed 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -37,13 +37,10 @@ public class UseShouldProcessCorrectly : IScriptRule private List diagnosticRecords; private readonly Vertex shouldProcessVertex; - private readonly Vertex shouldContinueVertex; - public UseShouldProcessCorrectly() { diagnosticRecords = new List(); - shouldContinueVertex = new Vertex {name="ShouldContinue", ast=null}; shouldProcessVertex = new Vertex {name="ShouldProcess", ast=null}; } @@ -143,8 +140,7 @@ private void FindViolations() /// An instance of DiagnosticRecord if it find violation, otherwise null private DiagnosticRecord GetViolation(Vertex v) { - bool callsShouldProcess = funcDigraph.IsConnected(v, shouldContinueVertex) - || funcDigraph.IsConnected(v, shouldProcessVertex); + bool callsShouldProcess = funcDigraph.IsConnected(v, shouldProcessVertex); FunctionDefinitionAst fast = v.ast as FunctionDefinitionAst; if (fast == null) { diff --git a/Tests/Rules/GoodCmdlet.ps1 b/Tests/Rules/GoodCmdlet.ps1 index d4d6526da..c667cb5f1 100644 --- a/Tests/Rules/GoodCmdlet.ps1 +++ b/Tests/Rules/GoodCmdlet.ps1 @@ -22,8 +22,8 @@ #> function Get-File { - [CmdletBinding(DefaultParameterSetName='Parameter Set 1', - SupportsShouldProcess=$true, + [CmdletBinding(DefaultParameterSetName='Parameter Set 1', + SupportsShouldProcess=$true, PositionalBinding=$false, HelpUri = 'http://www.microsoft.com/', ConfirmImpact='Medium')] @@ -34,17 +34,17 @@ function Get-File Param ( # Param1 help description - [Parameter(Mandatory=$true, + [Parameter(Mandatory=$true, ValueFromPipeline=$true, - ValueFromPipelineByPropertyName=$true, - ValueFromRemainingArguments=$false, + ValueFromPipelineByPropertyName=$true, + ValueFromRemainingArguments=$false, Position=0, ParameterSetName='Parameter Set 1')] [ValidateNotNull()] [ValidateNotNullOrEmpty()] [ValidateCount(0,5)] [ValidateSet("sun", "moon", "earth")] - [Alias("p1")] + [Alias("p1")] $Param1, # Param2 help description @@ -131,8 +131,8 @@ function Get-File #> function Get-Folder { - [CmdletBinding(DefaultParameterSetName='Parameter Set 1', - SupportsShouldProcess, + [CmdletBinding(DefaultParameterSetName='Parameter Set 1', + SupportsShouldProcess, PositionalBinding=$false, HelpUri = 'http://www.microsoft.com/', ConfirmImpact='Medium')] @@ -143,17 +143,17 @@ function Get-Folder Param ( # Param1 help description - [Parameter(Mandatory=$true, + [Parameter(Mandatory=$true, ValueFromPipeline=$true, - ValueFromPipelineByPropertyName=$true, - ValueFromRemainingArguments=$false, + ValueFromPipelineByPropertyName=$true, + ValueFromRemainingArguments=$false, Position=0, ParameterSetName='Parameter Set 1')] [ValidateNotNull()] [ValidateNotNullOrEmpty()] [ValidateCount(0,5)] [ValidateSet("sun", "moon", "earth")] - [Alias("p1")] + [Alias("p1")] $Param1, # Param2 help description @@ -279,13 +279,13 @@ function Get-Reserved* <# .Synopsis - function that has a noun that is singular + function that has a noun that is singular .DESCRIPTION - + .EXAMPLE - + .EXAMPLE - + #> function Get-MyWidgetStatus { @@ -299,7 +299,7 @@ function Get-MyFood process { - if ($PSCmdlet.ShouldContinue("Are you sure?")) + if ($PSCmdlet.ShouldProcess(("Are you sure?"))) { } } From 1c711cff4b78304c34480638c920a539b296d467 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 28 Sep 2016 18:03:25 -0700 Subject: [PATCH 11/19] Change Vertex class implementation --- Engine/Helper.cs | 2 +- Rules/UseShouldProcessCorrectly.cs | 81 +++++++++++++++++------------- 2 files changed, 47 insertions(+), 36 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 53da8157d..e1dc537db 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3611,7 +3611,7 @@ public IEnumerable GetVertices() /// True if the graph contains the vertex, otherwise false public bool ContainsVertex(T vertex) { - return vertexIndexMap.Keys.Contains(vertex); + return vertexIndexMap.ContainsKey(vertex); } /// diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 2e6e144ed..26eb6ebdf 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -41,7 +41,7 @@ public class UseShouldProcessCorrectly : IScriptRule public UseShouldProcessCorrectly() { diagnosticRecords = new List(); - shouldProcessVertex = new Vertex {name="ShouldProcess", ast=null}; + shouldProcessVertex = new Vertex("ShouldProcess", null); } /// @@ -141,7 +141,7 @@ private void FindViolations() private DiagnosticRecord GetViolation(Vertex v) { bool callsShouldProcess = funcDigraph.IsConnected(v, shouldProcessVertex); - FunctionDefinitionAst fast = v.ast as FunctionDefinitionAst; + FunctionDefinitionAst fast = v.Ast as FunctionDefinitionAst; if (fast == null) { return null; @@ -179,7 +179,7 @@ private DiagnosticRecord GetViolation(Vertex v) CultureInfo.CurrentCulture, Strings.ShouldProcessErrorHasCmdlet, fast.Name), - v.ast.Extent, + v.Ast.Extent, GetName(), DiagnosticSeverity.Warning, fileName); @@ -196,15 +196,14 @@ private DiagnosticRecord GetViolation(Vertex v) /// true if an upstream function declares SupportsShouldProcess, otherwise false private bool UpstreamDeclaresShouldProcess(Vertex v) { - var equalityComparer = new VertexEqualityComparer(); foreach (var vertex in funcDigraph.GetVertices()) { - if (equalityComparer.Equals(vertex, v)) + if (v.Equals(vertex)) { continue; } - var fast = vertex.ast as FunctionDefinitionAst; + var fast = vertex.Ast as FunctionDefinitionAst; if (fast == null) { continue; @@ -269,8 +268,26 @@ private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) /// class Vertex { - public string name; - public Ast ast; + public string Name {get { return name;}} + public Ast Ast {get {return ast; }} + + private string name; + private Ast ast; + + public Vertex() + { + name = String.Empty; + } + + public Vertex (string name, Ast ast) + { + if (name == null) + { + throw new ArgumentNullException("name"); + } + this.name = name; + this.ast = ast; + } /// /// Returns string representation of a Vertex instance @@ -279,47 +296,35 @@ public override string ToString() { return name; } - } - - /// - /// Implements IEqualityComparer interface for Vertex class - /// - class VertexEqualityComparer : IEqualityComparer - { /// /// Compares two instances of Vertex class to check for equality /// - public bool Equals(Vertex x, Vertex y) + public override bool Equals(Object other) { - if (x == null && y == null) - { - return true; - } - else if (x == null || y == null) + var otherVertex = other as Vertex; + if (otherVertex == null) { return false; } - else if (x.name.Equals(y.name, StringComparison.OrdinalIgnoreCase)) + + if (name.Equals(otherVertex.name, StringComparison.OrdinalIgnoreCase)) { return true; } - else - { - return false; - } + + return false; } /// /// Returns the Hash code of the given Vertex instance /// - public int GetHashCode(Vertex obj) + public override int GetHashCode() { - return obj.name.GetHashCode(); + return name.GetHashCode(); } } - /// /// Class to encapsulate a function call graph and related actions /// @@ -358,7 +363,7 @@ public Digraph GetDigraph() /// public FunctionReferenceDigraph() { - digraph = new Digraph(new VertexEqualityComparer()); + digraph = new Digraph(); functionVisitStack = new Stack(); } @@ -380,7 +385,7 @@ public void AddVertex(Vertex name) /// end of the edge public void AddEdge(Vertex fromV, Vertex toV) { - if (!digraph.GetNeighbors(fromV).Contains(toV, new VertexEqualityComparer())) + if (!digraph.GetNeighbors(fromV).Contains(toV)) { digraph.AddEdge(fromV, toV); } @@ -396,7 +401,7 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast return AstVisitAction.SkipChildren; } - var functionVertex = new Vertex { name = ast.Name, ast = ast }; + var functionVertex = new Vertex (ast.Name, ast); functionVisitStack.Push(functionVertex); AddVertex(functionVertex); ast.Body.Visit(this); @@ -415,12 +420,18 @@ public override AstVisitAction VisitCommand(CommandAst ast) } var cmdName = ast.GetCommandName(); - var vertex = new Vertex { name = cmdName, ast = ast }; + if (cmdName == null) + { + return AstVisitAction.Continue; + } + + var vertex = new Vertex (cmdName, ast); AddVertex(vertex); if (IsWithinFunctionDefinition()) { AddEdge(GetCurrentFunctionContext(), vertex); } + return AstVisitAction.Continue; } @@ -452,8 +463,8 @@ public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressio // necessarily a function, we do it because we are mainly interested in // finding connection between a function and ShouldProcess and this approach // prevents any unnecessary complexity. - var exprVertex = new Vertex {name=expr, ast=ast.Expression}; - var memberVertex = new Vertex {name=memberExprAst.Value, ast=memberExprAst}; + var exprVertex = new Vertex (expr, ast.Expression); + var memberVertex = new Vertex (memberExprAst.Value, memberExprAst); AddVertex(exprVertex); AddVertex(memberVertex); AddEdge(exprVertex, memberVertex); From 66a942e0f3ee545fd884325b322c690e4f9c0e80 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 28 Sep 2016 19:11:14 -0700 Subject: [PATCH 12/19] Add exception messages to resource file --- Engine/Helper.cs | 14 +++++++--- Engine/Strings.resx | 63 ++++++++++++++++++++++++++------------------- 2 files changed, 47 insertions(+), 30 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index e1dc537db..dc4cfd635 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3659,7 +3659,11 @@ public void AddVertex(T vertex) ValidateNotNull(vertex); if (GetIndex(vertex) != -1) { - throw new ArgumentException("Vertex already present! Cannot add it to the Digraph", "vertex"); + throw new ArgumentException( + String.Format( + Strings.DigraphVertexAlreadyExists, + vertex), + "vertex"); } vertexIndexMap.Add(vertex, graph.Count); @@ -3684,7 +3688,7 @@ public void AddEdge(T fromVertex, T toVertex) if (fromVertexList.Contains(toIdx)) { throw new ArgumentException(String.Format( - "Edge from {0} to {1} already present.", + Strings.DigraphEdgeAlreadyExists, fromVertex.ToString(), toVertex.ToString())); } @@ -3759,7 +3763,11 @@ private void ValidateVertexPresence(T vertex) { if (GetIndex(vertex) == -1) { - throw new ArgumentOutOfRangeException("vertex not present in the Digraph.", "vertex"); + throw new ArgumentOutOfRangeException( + String.Format( + Strings.DigraphVertexDoesNotExists, + vertex.ToString()), + "vertex"); } } diff --git a/Engine/Strings.resx b/Engine/Strings.resx index e1e0de031..f71125269 100644 --- a/Engine/Strings.resx +++ b/Engine/Strings.resx @@ -1,17 +1,17 @@  - @@ -243,4 +243,13 @@ Value {0} for key {1} has the wrong data type. Value in the settings hashtable should be a string or an array of strings. + + Vertex {0} already exists! Cannot add it to the digraph. + + + Edge from {0} to {1} already exists. + + + Vertex {0} does not exist in the digraph. + \ No newline at end of file From a488d70eb378052303d9f86cd268896a0fffe08a Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Thu, 29 Sep 2016 18:00:37 -0700 Subject: [PATCH 13/19] Remove redundant code in digraph class --- Engine/Helper.cs | 18 +++++++++--------- Rules/UseShouldProcessCorrectly.cs | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index dc4cfd635..5dab44a70 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3572,7 +3572,10 @@ public Digraph() } /// - /// Construct digraph with an EqualityComparer used for comparing type T + /// Construct a directed graph that uses an EqualityComparer object for comparison with its vertices + /// + /// The class allows its client to use their choice of vertex type. To allow comparison for such a + /// vertex type, client can pass their own EqualityComparer object /// /// public Digraph(IEqualityComparer equalityComparer) : this() @@ -3728,21 +3731,18 @@ private bool IsConnected(int fromIdx, int toIdx, ref bool[] visited) return true; } - bool isConnected = false; foreach(var vertexIdx in graph[fromIdx]) { if (!visited[vertexIdx]) { - isConnected |= IsConnected(vertexIdx, toIdx, ref visited); - } - - if (isConnected) // no need to search further - { - break; + if(IsConnected(vertexIdx, toIdx, ref visited)) + { + return true; + } } } - return isConnected; + return false; } /// diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 26eb6ebdf..b3eae228a 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -140,13 +140,13 @@ private void FindViolations() /// An instance of DiagnosticRecord if it find violation, otherwise null private DiagnosticRecord GetViolation(Vertex v) { - bool callsShouldProcess = funcDigraph.IsConnected(v, shouldProcessVertex); FunctionDefinitionAst fast = v.Ast as FunctionDefinitionAst; if (fast == null) { return null; } + bool callsShouldProcess = funcDigraph.IsConnected(v, shouldProcessVertex); if (DeclaresSupportsShouldProcess(fast)) { if (!callsShouldProcess) From 841dba6ed29cfc2aef281a2a3a4a77c2bdfdc497 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Fri, 30 Sep 2016 15:25:13 -0700 Subject: [PATCH 14/19] Add method to check SupportShouldProcess for builtin cmdlet --- Rules/UseShouldProcessCorrectly.cs | 60 ++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index b3eae228a..08d2d1e21 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -19,6 +19,7 @@ using System.ComponentModel.Composition; #endif using System.Globalization; +using System.Reflection; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -261,6 +262,54 @@ private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) return false; } + + private bool DeclaresSupportsShouldProcess(string cmdName) + { + if (String.IsNullOrWhiteSpace(cmdName)) + { + return false; + } + + try + { + using (var ps = System.Management.Automation.PowerShell.Create()) + { + ps.AddCommand("Get-command").AddArgument("cmdName"); + var cmdInfo = ps.Invoke().FirstOrDefault(); + if (cmdInfo == null) + { + return false; + } + var attributes = cmdInfo.ImplementingType.GetTypeInfo().GetCustomAttributes( + typeof(System.Management.Automation.CmdletCommonMetadataAttribute), + true); + + foreach (var attr in attributes) + { + var cmdletAttribute = attr as System.Management.Automation.CmdletAttribute; + if (cmdletAttribute == null) + { + continue; + } + + if (cmdletAttribute.SupportsShouldProcess) + { + return true; + } + } + if (attributes.Count() == 0) + { + return false; + } + } + } + catch (System.Management.Automation.CommandNotFoundException e) + { + return false; + } + + return false; + } } /// @@ -425,6 +474,17 @@ public override AstVisitAction VisitCommand(CommandAst ast) return AstVisitAction.Continue; } + // if command is part of a binary module + // for now just check if (Get-Command ).DLL end with dll extension + // if so, check if it declares SupportsShouldProcess + // if so, then assume it also calls ShouldProcess + // because we do not have a way to analyze its definition + // to actually verify it is indeed calling ShouddProcess + + // if (IsPartOfBinaryModule(cmdName, out cmdInfo)) + // if (HasSupportShouldProcessAttribute(cmdInfo)) + // AddEdge(cmdName, shouldProcessVertex) + var vertex = new Vertex (cmdName, ast); AddVertex(vertex); if (IsWithinFunctionDefinition()) From 3c9a49cfc3c9859e1309af7562660c7a9c6a15da Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 3 Oct 2016 20:11:53 -0700 Subject: [PATCH 15/19] Change GetNumNeighbors to GetOutDegree --- Engine/Helper.cs | 2 +- Tests/Engine/Helper.tests.ps1 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 5dab44a70..ab3295cc4 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3644,7 +3644,7 @@ public IEnumerable GetNeighbors(T vertex) /// /// /// - public int GetNumNeighbors(T vertex) + public int GetOutDegree(T vertex) { ValidateVertexArgument(vertex); return graph[GetIndex(vertex)].Count; diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 index f37fcf42e..31fec12d3 100644 --- a/Tests/Engine/Helper.tests.ps1 +++ b/Tests/Engine/Helper.tests.ps1 @@ -18,7 +18,7 @@ Describe "Test Directed Graph" { } It "correctly adds the edges" { - $digraph.GetNumNeighbors('v1') | Should Be 2 + $digraph.GetOutDegree('v1') | Should Be 2 $neighbors = $digraph.GetNeighbors('v1') $neighbors -contains 'v2' | Should Be $true $neighbors -contains 'v5' | Should Be $true From 564b28dfbd9bfcbbc06a55d7196799e400731ace Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 3 Oct 2016 20:12:58 -0700 Subject: [PATCH 16/19] Check if a builtin cmdlet/function has SupportsShouldProcess --- Rules/UseShouldProcessCorrectly.cs | 154 ++++++++++++++++++----------- 1 file changed, 94 insertions(+), 60 deletions(-) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 08d2d1e21..87f1949bf 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -13,6 +13,7 @@ using System; using System.Linq; using System.Collections.Generic; +using System.Management.Automation; using System.Management.Automation.Language; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; #if !CORECLR @@ -34,9 +35,7 @@ public class UseShouldProcessCorrectly : IScriptRule private Ast ast; private string fileName; private FunctionReferenceDigraph funcDigraph; - private List diagnosticRecords; - private readonly Vertex shouldProcessVertex; public UseShouldProcessCorrectly() @@ -53,13 +52,17 @@ public UseShouldProcessCorrectly() /// A List of diagnostic results of this rule public IEnumerable AnalyzeScript(Ast ast, string fileName) { - if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + if (ast == null) + { + throw new ArgumentNullException(Strings.NullAstErrorMessage); + } diagnosticRecords.Clear(); this.ast = ast; this.fileName = fileName; funcDigraph = new FunctionReferenceDigraph(); ast.Visit(funcDigraph); + CheckForSupportShouldProcess(); FindViolations(); foreach (var dr in diagnosticRecords) { @@ -226,90 +229,117 @@ private bool UpstreamDeclaresShouldProcess(Vertex v) /// True if the given function declares SupportShouldProcess, otherwise null private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) { - if (ast.Body.ParamBlock == null) + if (ast.Body.ParamBlock == null + || ast.Body.ParamBlock.Attributes == null) { return false; } - foreach (var attr in ast.Body.ParamBlock.Attributes) + var shouldProcessAttribute = Helper.Instance.GetShouldProcessAttributeAst(ast.Body.ParamBlock.Attributes); + if (shouldProcessAttribute == null) { - if (attr.NamedArguments == null) - { - continue; - } + return false; + } + + return Helper.Instance.GetNamedArgumentAttributeValue(shouldProcessAttribute); + } - foreach (var namedArg in attr.NamedArguments) + private CommandInfo GetCommandInfo(string cmdName) + { + try + { + using (var ps = System.Management.Automation.PowerShell.Create()) { - if (namedArg.ArgumentName.Equals( - "SupportsShouldProcess", - StringComparison.OrdinalIgnoreCase)) - { - var argAst = namedArg.Argument as VariableExpressionAst; - if (argAst != null) - { - if (argAst.VariablePath.UserPath.Equals("true", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - } - else - { - return namedArg.ExpressionOmitted; - } - } + var cmdInfo = ps.AddCommand("Get-Command") + .AddArgument(cmdName) + .Invoke() + .FirstOrDefault(); + return cmdInfo; } } - - return false; + catch (System.Management.Automation.CommandNotFoundException) + { + return null; + } } - private bool DeclaresSupportsShouldProcess(string cmdName) + private bool SupportsShouldProcess(string cmdName) { if (String.IsNullOrWhiteSpace(cmdName)) { return false; } - try + var cmdInfo = GetCommandInfo(cmdName); + if (cmdInfo == null) { - using (var ps = System.Management.Automation.PowerShell.Create()) - { - ps.AddCommand("Get-command").AddArgument("cmdName"); - var cmdInfo = ps.Invoke().FirstOrDefault(); - if (cmdInfo == null) - { - return false; - } - var attributes = cmdInfo.ImplementingType.GetTypeInfo().GetCustomAttributes( - typeof(System.Management.Automation.CmdletCommonMetadataAttribute), - true); + return false; + } - foreach (var attr in attributes) + var cmdletInfo = cmdInfo as CmdletInfo; + if (cmdletInfo == null) + { + // check if it is of functioninfo type + var funcInfo = cmdInfo as FunctionInfo; + if (funcInfo != null + && funcInfo.CmdletBinding + && funcInfo.ScriptBlock != null + && funcInfo.ScriptBlock.Attributes != null) + { + foreach (var attr in funcInfo.ScriptBlock.Attributes) { - var cmdletAttribute = attr as System.Management.Automation.CmdletAttribute; - if (cmdletAttribute == null) - { - continue; - } - - if (cmdletAttribute.SupportsShouldProcess) + var cmdletBindingAttr = attr as CmdletBindingAttribute; + if (cmdletBindingAttr != null) { - return true; + return cmdletBindingAttr.SupportsShouldProcess; } } - if (attributes.Count() == 0) - { - return false; - } } + + return false; } - catch (System.Management.Automation.CommandNotFoundException e) + + var attributes = cmdletInfo.ImplementingType.GetTypeInfo().GetCustomAttributes( + typeof(System.Management.Automation.CmdletCommonMetadataAttribute), + true); + + foreach (var attr in attributes) { - return false; + var cmdletAttribute = attr as System.Management.Automation.CmdletAttribute; + if (cmdletAttribute != null) + { + return cmdletAttribute.SupportsShouldProcess; + } } return false; } + + private void CheckForSupportShouldProcess() + { + var commandsWithSupportShouldProcess = new List(); + + // for all the vertices without any neighbors check if they support shouldprocess + foreach (var v in funcDigraph.GetVertices()) + { + if (funcDigraph.GetOutDegree(v) == 0) + { + if (SupportsShouldProcess(v.Name)) + { + commandsWithSupportShouldProcess.Add(v); + } + } + } + + if (commandsWithSupportShouldProcess.Count > 0) + { + funcDigraph.AddVertex(shouldProcessVertex); + foreach(var v in commandsWithSupportShouldProcess) + { + funcDigraph.AddEdge(v, shouldProcessVertex); + } + } + } } /// @@ -370,7 +400,7 @@ public override bool Equals(Object other) /// public override int GetHashCode() { - return name.GetHashCode(); + return name.ToLower().GetHashCode(); } } @@ -484,7 +514,6 @@ public override AstVisitAction VisitCommand(CommandAst ast) // if (IsPartOfBinaryModule(cmdName, out cmdInfo)) // if (HasSupportShouldProcessAttribute(cmdInfo)) // AddEdge(cmdName, shouldProcessVertex) - var vertex = new Vertex (cmdName, ast); AddVertex(vertex); if (IsWithinFunctionDefinition()) @@ -550,7 +579,7 @@ public IEnumerable GetVertices() /// Origin vertxx /// Destination vertex /// - internal bool IsConnected(Vertex vertex, Vertex shouldVertex) + public bool IsConnected(Vertex vertex, Vertex shouldVertex) { if (digraph.ContainsVertex(vertex) && digraph.ContainsVertex(shouldVertex)) @@ -559,5 +588,10 @@ internal bool IsConnected(Vertex vertex, Vertex shouldVertex) } return false; } + + public int GetOutDegree(Vertex v) + { + return digraph.GetOutDegree(v); + } } } From d1f571ed8cbd28a30c6844ff16a845e895fcc659 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 3 Oct 2016 20:13:54 -0700 Subject: [PATCH 17/19] Add test to check PSShouldProcess for recursive calls --- .../Rules/UseShouldProcessCorrectly.tests.ps1 | 169 ++++++++++++++++-- 1 file changed, 156 insertions(+), 13 deletions(-) diff --git a/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 b/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 index 4b789e8d4..fa571b929 100644 --- a/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 +++ b/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 @@ -23,38 +23,101 @@ Describe "UseShouldProcessCorrectly" { } } - Context "Where ShouldProcess is called in nested function" { - It "finds no violation" { + Context "Where ShouldProcess is called by a downstream function" { + It "finds no violation for 1 level downstream call" { $scriptDef = @' -function Outer +function Foo { [CmdletBinding(SupportsShouldProcess=$true)] param() - Inner + Bar } -function Inner +function Bar { [CmdletBinding(SupportsShouldProcess=$true)] param() - if ($PSCmdlet.ShouldProcess("Inner")) + if ($PSCmdlet.ShouldProcess("")) { - Write-Host "Process!" + "Continue normally..." } else { - Write-Host "Skipped!" + "what would happen..." } } -Outer -WhatIf +Foo '@ - $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess - $violations.Count | Should Be 0 + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 } + It "finds no violation if downstream function does not declare SupportsShouldProcess" { + $scriptDef = @' +function Foo +{ + [CmdletBinding(SupportsShouldProcess=$true)] + param() + + Bar +} + +function Bar +{ + if ($PSCmdlet.ShouldProcess("")) + { + "Continue normally..." + } + else + { + "what would happen..." + } +} + +Foo +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + + It "finds no violation for 2 level downstream calls" { + $scriptDef = @' +function Foo +{ + [CmdletBinding(SupportsShouldProcess=$true)] + param() + + Baz +} + +function Baz +{ + Bar +} + +function Bar +{ + if ($PSCmdlet.ShouldProcess("")) + { + "Continue normally..." + } + else + { + "what would happen..." + } +} + +Foo +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + } + + Context "When downstream function is defined locally in a function scope" { It "finds no violation" { $scriptDef = @' function Foo @@ -63,14 +126,14 @@ function Foo param() begin { - function helper + function Bar { if ($PSCmdlet.ShouldProcess('','')) { } } - helper + bar } } '@ @@ -78,4 +141,84 @@ function Foo $violations.Count | Should Be 0 } } + + Context "When a builtin command with SupportsShouldProcess is called" { + It "finds no violation for a cmdlet" { + $scriptDef = @' +function Remove-Foo { +[CmdletBinding(SupportsShouldProcess)] + Param( + [string] $Path + ) + Write-Verbose "Removing $($path)" + Remove-Item -Path $Path +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + + It "finds no violation for a function" { + $scriptDef = @' +function Install-Foo { +[CmdletBinding(SupportsShouldProcess)] + Param( + [string] $ModuleName + ) + Install-Module $ModuleName +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + + It "finds no violation for a function with self reference" { + $scriptDef = @' +function Install-ModuleWithDeps { +[CmdletBinding(SupportsShouldProcess)] + Param( + [Parameter(ValueFromPipeline)] + [string] $ModuleName + ) + if ($PSCmdlet.ShouldProcess("Install module with dependencies")) + { + Get-Dependencies $ModuleName | Install-ModuleWithDeps + Install-ModuleCustom $ModuleName + } + else + { + Get-Dependencies $ModuleName | Install-ModuleWithDeps + Write-Host ("Would install module {0}" -f $ModuleName) + } +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + + It "finds no violation for a function with self reference and implicit call to ShouldProcess" { + $scriptDef = @' +function Install-ModuleWithDeps { +[CmdletBinding(SupportsShouldProcess)] + Param( + [Parameter(ValueFromPipeline)] + [string] $ModuleName + ) + $deps = Get-Dependencies $ModuleName + if ($deps -eq $null) + { + Install-Module $ModuleName + } + else + { + $deps | Install-ModuleWithDeps + } + Install-Module $ModuleName +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $violations.Count | Should Be 0 + } + + } } \ No newline at end of file From b807b9a8b870ad5942cfb214faf0f5f0aa0b642b Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 3 Oct 2016 20:22:19 -0700 Subject: [PATCH 18/19] Add inline documentation to UseShouldProcess rule --- Rules/UseShouldProcessCorrectly.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 87f1949bf..4c77d69da 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -244,6 +244,10 @@ private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) return Helper.Instance.GetNamedArgumentAttributeValue(shouldProcessAttribute); } + /// + /// Get a CommandInfo object of the given command name + /// + /// Returns null if command does not exists private CommandInfo GetCommandInfo(string cmdName) { try @@ -263,6 +267,10 @@ private CommandInfo GetCommandInfo(string cmdName) } } + /// + /// Checks if the given command supports ShouldProcess + /// + /// False if input is null. If the input command has declares SupportsShouldProcess attribute, returns true private bool SupportsShouldProcess(string cmdName) { if (String.IsNullOrWhiteSpace(cmdName)) @@ -315,6 +323,9 @@ private bool SupportsShouldProcess(string cmdName) return false; } + /// + /// Add a ShouldProcess edge from a command vertex if the command supports ShouldProcess + /// private void CheckForSupportShouldProcess() { var commandsWithSupportShouldProcess = new List(); @@ -589,6 +600,9 @@ public bool IsConnected(Vertex vertex, Vertex shouldVertex) return false; } + /// + /// Get the number of edges out of the given vertex + /// public int GetOutDegree(Vertex v) { return digraph.GetOutDegree(v); From 3b4900b8b17c50fb959bfa9a68d75c025c547397 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Mon, 3 Oct 2016 20:51:39 -0700 Subject: [PATCH 19/19] Clean up inline comments --- Rules/UseShouldProcessCorrectly.cs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index 4c77d69da..759a2ee4e 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -170,7 +170,7 @@ private DiagnosticRecord GetViolation(Vertex v) { if (callsShouldProcess) { - // check if upstream function declares SupportShouldProcess\ + // check if upstream function declares SupportShouldProcess // if so, this might just be a helper function // do not flag this case if (UpstreamDeclaresShouldProcess(v)) @@ -515,16 +515,6 @@ public override AstVisitAction VisitCommand(CommandAst ast) return AstVisitAction.Continue; } - // if command is part of a binary module - // for now just check if (Get-Command ).DLL end with dll extension - // if so, check if it declares SupportsShouldProcess - // if so, then assume it also calls ShouldProcess - // because we do not have a way to analyze its definition - // to actually verify it is indeed calling ShouddProcess - - // if (IsPartOfBinaryModule(cmdName, out cmdInfo)) - // if (HasSupportShouldProcessAttribute(cmdInfo)) - // AddEdge(cmdName, shouldProcessVertex) var vertex = new Vertex (cmdName, ast); AddVertex(vertex); if (IsWithinFunctionDefinition())