diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 1135bf120..ab3295cc4 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -3555,4 +3555,239 @@ public object VisitUsingExpression(UsingExpressionAst usingExpressionAst) return null; } } + + /// Class to represent a directed graph + public class Digraph + { + private List> graph; + private Dictionary vertexIndexMap; + + /// + /// Public constructor + /// + public Digraph() + { + graph = new List>(); + vertexIndexMap = new Dictionary(); + } + + /// + /// 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() + { + if (equalityComparer == null) + { + 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.ContainsKey(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); + var idx = GetIndex(vertex); + var idxVertexMap = vertexIndexMap.ToDictionary(x => x.Value, x => x.Key); + foreach (var neighbor in graph[idx]) + { + yield return idxVertexMap[neighbor]; + } + } + + /// + /// 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 GetOutDegree(T vertex) + { + 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) + { + ValidateNotNull(vertex); + if (GetIndex(vertex) != -1) + { + throw new ArgumentException( + String.Format( + Strings.DigraphVertexAlreadyExists, + vertex), + "vertex"); + } + + vertexIndexMap.Add(vertex, graph.Count); + 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); + ValidateVertexArgument(toVertex); + + var toIdx = GetIndex(toVertex); + var fromVertexList = graph[GetIndex(fromVertex)]; + if (fromVertexList.Contains(toIdx)) + { + throw new ArgumentException(String.Format( + Strings.DigraphEdgeAlreadyExists, + fromVertex.ToString(), + toVertex.ToString())); + } + else + { + fromVertexList.Add(toIdx); + } + } + + /// + /// 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; + if (fromIdx == toIdx) + { + return true; + } + + foreach(var vertexIdx in graph[fromIdx]) + { + if (!visited[vertexIdx]) + { + if(IsConnected(vertexIdx, toIdx, ref visited)) + { + return true; + } + } + } + + return false; + } + + /// + /// Throw an ArgumentNullException if vertex is null + /// + private void ValidateNotNull(T vertex) + { + if (vertex == null) + { + throw new ArgumentNullException("vertex"); + } + } + + /// + /// Throw an ArgumentOutOfRangeException if vertex is not present in the graph + /// + private void ValidateVertexPresence(T vertex) + { + if (GetIndex(vertex) == -1) + { + throw new ArgumentOutOfRangeException( + String.Format( + Strings.DigraphVertexDoesNotExists, + vertex.ToString()), + "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/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 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 d18646ae6..759a2ee4e 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -11,13 +11,16 @@ // 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 using System.ComponentModel.Composition; #endif using System.Globalization; +using System.Reflection; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -29,6 +32,18 @@ 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; + + public UseShouldProcessCorrectly() + { + diagnosticRecords = new List(); + shouldProcessVertex = new Vertex("ShouldProcess", null); + } + /// /// AnalyzeScript: Analyzes the ast to check that if the ShouldProcess attribute is present, the function calls ShouldProcess and vice versa. /// @@ -37,43 +52,21 @@ public class UseShouldProcessCorrectly : IScriptRule /// A List of diagnostic results of this rule 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)); - } - - 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 (ast == null) + { + throw new ArgumentNullException(Strings.NullAstErrorMessage); + } - 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); - } + diagnosticRecords.Clear(); + this.ast = ast; + this.fileName = fileName; + funcDigraph = new FunctionReferenceDigraph(); + ast.Visit(funcDigraph); + CheckForSupportShouldProcess(); + FindViolations(); + foreach (var dr in diagnosticRecords) + { + yield return dr; } } @@ -128,10 +121,481 @@ 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) + { + FunctionDefinitionAst fast = v.Ast as FunctionDefinitionAst; + if (fast == null) + { + return null; + } + + bool callsShouldProcess = funcDigraph.IsConnected(v, shouldProcessVertex); + 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; + } + + /// + /// 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) + { + foreach (var vertex in funcDigraph.GetVertices()) + { + if (v.Equals(vertex)) + { + continue; + } + + var fast = vertex.Ast as FunctionDefinitionAst; + if (fast == null) + { + continue; + } + + if (DeclaresSupportsShouldProcess(fast) + && funcDigraph.IsConnected(vertex, v)) + { + return true; + } + } + 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 + || ast.Body.ParamBlock.Attributes == null) + { + return false; + } + + var shouldProcessAttribute = Helper.Instance.GetShouldProcessAttributeAst(ast.Body.ParamBlock.Attributes); + if (shouldProcessAttribute == null) + { + return false; + } + + 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 + { + using (var ps = System.Management.Automation.PowerShell.Create()) + { + var cmdInfo = ps.AddCommand("Get-Command") + .AddArgument(cmdName) + .Invoke() + .FirstOrDefault(); + return cmdInfo; + } + } + catch (System.Management.Automation.CommandNotFoundException) + { + return null; + } + } + + /// + /// 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)) + { + return false; + } + + var cmdInfo = GetCommandInfo(cmdName); + if (cmdInfo == null) + { + return false; + } + + 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 cmdletBindingAttr = attr as CmdletBindingAttribute; + if (cmdletBindingAttr != null) + { + return cmdletBindingAttr.SupportsShouldProcess; + } + } + } + + return false; + } + + var attributes = cmdletInfo.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) + { + return cmdletAttribute.SupportsShouldProcess; + } + } + + return false; + } + + /// + /// Add a ShouldProcess edge from a command vertex if the command supports ShouldProcess + /// + 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); + } + } + } } -} + /// + /// Class to represent a vertex in a function call graph + /// + class Vertex + { + 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 + /// + public override string ToString() + { + return name; + } + /// + /// Compares two instances of Vertex class to check for equality + /// + public override bool Equals(Object other) + { + var otherVertex = other as Vertex; + if (otherVertex == null) + { + return false; + } + + if (name.Equals(otherVertex.name, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + return false; + } + + /// + /// Returns the Hash code of the given Vertex instance + /// + public override int GetHashCode() + { + return name.ToLower().GetHashCode(); + } + } + + /// + /// 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(); + functionVisitStack = new Stack(); + } + + /// + /// Add a vertex to the graph + /// + public void AddVertex(Vertex name) + { + if (!digraph.ContainsVertex(name)) + { + digraph.AddVertex(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)) + { + digraph.AddEdge(fromV, 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) + { + return AstVisitAction.SkipChildren; + } + + var functionVertex = new Vertex (ast.Name, ast); + functionVisitStack.Push(functionVertex); + AddVertex(functionVertex); + ast.Body.Visit(this); + functionVisitStack.Pop(); + 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(); + if (cmdName == null) + { + return AstVisitAction.Continue; + } + + var vertex = new Vertex (cmdName, 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) + { + 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; + } + + // 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 (expr, ast.Expression); + var memberVertex = new Vertex (memberExprAst.Value, memberExprAst); + AddVertex(exprVertex); + AddVertex(memberVertex); + AddEdge(exprVertex, memberVertex); + if (IsWithinFunctionDefinition()) + { + AddEdge(GetCurrentFunctionContext(), exprVertex); + } + + 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 + /// + public bool IsConnected(Vertex vertex, Vertex shouldVertex) + { + if (digraph.ContainsVertex(vertex) + && digraph.ContainsVertex(shouldVertex)) + { + return digraph.IsConnected(vertex, shouldVertex); + } + return false; + } + + /// + /// Get the number of edges out of the given vertex + /// + public int GetOutDegree(Vertex v) + { + return digraph.GetOutDegree(v); + } + } +} diff --git a/Tests/Engine/Helper.tests.ps1 b/Tests/Engine/Helper.tests.ps1 new file mode 100644 index 000000000..31fec12d3 --- /dev/null +++ b/Tests/Engine/Helper.tests.ps1 @@ -0,0 +1,31 @@ +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.AddEdge('v1', 'v2'); + $digraph.AddEdge('v1', 'v5'); + $digraph.AddEdge('v2', 'v4'); + + It "correctly adds the vertices" { + $digraph.NumVertices | Should Be 5 + } + + It "correctly adds the edges" { + $digraph.GetOutDegree('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('v1', 'v4') | Should Be $true + } + } +} \ No newline at end of file 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?"))) { } } diff --git a/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 b/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 index 908c96d24..fa571b929 100644 --- a/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 +++ b/Tests/Rules/UseShouldProcessCorrectly.tests.ps1 @@ -22,4 +22,203 @@ Describe "UseShouldProcessCorrectly" { $noViolations.Count | Should Be 0 } } + + Context "Where ShouldProcess is called by a downstream function" { + It "finds no violation for 1 level downstream call" { + $scriptDef = @' +function Foo +{ + [CmdletBinding(SupportsShouldProcess=$true)] + param() + + Bar +} + +function Bar +{ + [CmdletBinding(SupportsShouldProcess=$true)] + param() + + 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 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 +{ + [CmdletBinding(SupportsShouldProcess)] + param() + begin + { + function Bar + { + if ($PSCmdlet.ShouldProcess('','')) + { + + } + } + bar + } +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess + $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