Merge pull request #652 from PowerShell/kapilmb/FixShouldProcess

Fix ShouldProcess rule

Author: Kapil Borle

Rules/UseShouldProcessCorrectly.cs

@@ -37,11 +37,13 @@ public class UseShouldProcessCorrectly : IScriptRule
         private FunctionReferenceDigraph funcDigraph;
         private List<DiagnosticRecord> diagnosticRecords;
         private readonly Vertex shouldProcessVertex;
+        private readonly Vertex implicitShouldProcessVertex;
 
         public UseShouldProcessCorrectly()
         {
             diagnosticRecords = new List<DiagnosticRecord>();
             shouldProcessVertex = new Vertex("ShouldProcess", null);
+            implicitShouldProcessVertex = new Vertex("implicitShouldProcessVertex", null);
         }
 
         /// <summary>
@@ -150,10 +152,12 @@ private DiagnosticRecord GetViolation(Vertex v)
                 return null;
             }
 
-            bool callsShouldProcess = funcDigraph.IsConnected(v, shouldProcessVertex);
             if (DeclaresSupportsShouldProcess(fast))
             {
-                if (!callsShouldProcess)
+                bool callsShouldProcess = funcDigraph.IsConnected(v, shouldProcessVertex);
+                bool callsCommandWithShouldProcess = funcDigraph.IsConnected(v, implicitShouldProcessVertex);
+                if (!callsShouldProcess
+                    && !callsCommandWithShouldProcess)
                 {
                     return new DiagnosticRecord(
                         string.Format(
@@ -168,12 +172,12 @@ private DiagnosticRecord GetViolation(Vertex v)
             }
             else
             {
-                if (callsShouldProcess)
+                if (callsShouldProcessDirectly(v))
                 {
                     // check if upstream function declares SupportShouldProcess
                     // if so, this might just be a helper function
                     // do not flag this case
-                    if (UpstreamDeclaresShouldProcess(v))
+                    if (v.IsNestedFunctionDefinition && UpstreamDeclaresShouldProcess(v))
                     {
                         return null;
                     }
@@ -193,6 +197,11 @@ private DiagnosticRecord GetViolation(Vertex v)
             return null;
         }
 
+        private bool callsShouldProcessDirectly(Vertex vertex)
+        {
+            return funcDigraph.GetNeighbors(vertex).Contains(shouldProcessVertex);
+        }
+
         /// <summary>
         /// Checks if an upstream function declares SupportsShouldProcess
         /// </summary>
@@ -344,10 +353,10 @@ private void CheckForSupportShouldProcess()
 
             if (commandsWithSupportShouldProcess.Count > 0)
             {
-                funcDigraph.AddVertex(shouldProcessVertex);
+                funcDigraph.AddVertex(implicitShouldProcessVertex);
                 foreach(var v in commandsWithSupportShouldProcess)
                 {
-                    funcDigraph.AddEdge(v, shouldProcessVertex);
+                    funcDigraph.AddEdge(v, implicitShouldProcessVertex);
                 }
             }
         }
@@ -359,10 +368,23 @@ private void CheckForSupportShouldProcess()
     class Vertex
     {
         public string Name {get { return name;}}
-        public Ast Ast {get {return ast; }}
+        public Ast Ast
+        {
+            get
+            {
+                return ast;
+            }
+            set
+            {
+                ast = value;
+            }
+        }
+
+        public bool IsNestedFunctionDefinition {get {return isNestedFunctionDefinition;}}
 
         private string name;
         private Ast ast;
+        private bool isNestedFunctionDefinition;
 
         public Vertex()
         {
@@ -379,6 +401,12 @@ public Vertex (string name, Ast ast)
             this.ast = ast;
         }
 
+        public Vertex (String name, Ast ast, bool isNestedFunctionDefinition)
+            : this(name, ast)
+            {
+            this.isNestedFunctionDefinition = isNestedFunctionDefinition;
+        }
+
         /// <summary>
         /// Returns string representation of a Vertex instance
         /// </summary>
@@ -460,11 +488,30 @@ public FunctionReferenceDigraph()
         /// <summary>
         /// Add a vertex to the graph
         /// </summary>
-        public void AddVertex(Vertex name)
+        public void AddVertex(Vertex vertex)
         {
-            if (!digraph.ContainsVertex(name))
+            bool containsVertex = false;
+
+            // if the graph contains a vertex with name equal to that
+            // of the input vertex, then update the vertex's ast if the
+            // input vertex's ast is of FunctionDefinitionAst type
+            foreach(Vertex v in digraph.GetVertices())
+            {
+                if (v.Equals(vertex))
+                {
+                    containsVertex = true;
+                    if (vertex.Ast != null
+                        && vertex.Ast is FunctionDefinitionAst)
+                    {
+                        v.Ast = vertex.Ast;
+                    }
+                    break;
+                }
+            }
+
+            if (!containsVertex)
             {
-                digraph.AddVertex(name);
+                digraph.AddVertex(vertex);
             }
         }
 
@@ -491,7 +538,7 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast
                 return AstVisitAction.SkipChildren;
             }
 
-            var functionVertex = new Vertex (ast.Name, ast);
+            var functionVertex = new Vertex(ast.Name, ast, IsWithinFunctionDefinition());
             functionVisitStack.Push(functionVertex);
             AddVertex(functionVertex);
             ast.Body.Visit(this);
@@ -549,18 +596,15 @@ public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressio
             }
 
             // Suppose we find <Expression>.<Member>, we split it up and create
-            // and edge from <Expression>-><Member>. Even though <Expression> is not
+            // and edge only to <Member>. Even though <Expression> 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);
+                AddEdge(GetCurrentFunctionContext(), memberVertex);
             }
 
             return AstVisitAction.Continue;
@@ -597,5 +641,10 @@ public int GetOutDegree(Vertex v)
         {
             return digraph.GetOutDegree(v);
         }
+
+        public IEnumerable<Vertex> GetNeighbors(Vertex v)
+        {
+            return digraph.GetNeighbors(v);
+        }
     }
 }

Tests/Rules/UseShouldProcessCorrectly.tests.ps1

@@ -55,7 +55,7 @@ Foo
             $violations.Count | Should Be 0
         }
 
-        It "finds no violation if downstream function does not declare SupportsShouldProcess" {
+        It "finds violation if downstream function does not declare SupportsShouldProcess" {
               $scriptDef = @'
 function Foo
 {
@@ -80,10 +80,10 @@ function Bar
 Foo
 '@
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess
-            $violations.Count | Should Be 0
+            $violations.Count | Should Be 1
         }
 
-        It "finds no violation for 2 level downstream calls" {
+        It "finds violation for 2 level downstream calls" {
             $scriptDef = @'
 function Foo
 {
@@ -113,11 +113,11 @@ function Bar
 Foo
 '@
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess
-            $violations.Count | Should Be 0
+            $violations.Count | Should Be 1
         }
     }
 
-    Context "When downstream function is defined locally in a function scope" {
+    Context "When nested function definition calls ShouldProcess" {
         It "finds no violation" {
             $scriptDef = @'
 function Foo
@@ -142,8 +142,8 @@ function Foo
         }
     }
 
-    Context "When a builtin command with SupportsShouldProcess is called" {
-        It "finds no violation for a cmdlet" {
+    Context "When a builtin command that supports ShouldProcess is called" {
+        It "finds no violation when caller declares SupportsShouldProcess and callee is a cmdlet with ShouldProcess" {
             $scriptDef = @'
 function Remove-Foo {
 [CmdletBinding(SupportsShouldProcess)]
@@ -158,7 +158,21 @@ function Remove-Foo {
             $violations.Count | Should Be 0
         }
 
-        It "finds no violation for a function" {
+        It "finds no violation when caller does not declare SupportsShouldProcess and callee is a cmdlet with ShouldProcess" {
+            $scriptDef = @'
+function Remove-Foo {
+    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 when caller declares SupportsShouldProcess and callee is a function with ShouldProcess" {
             $scriptDef = @'
 function Install-Foo {
 [CmdletBinding(SupportsShouldProcess)]
@@ -167,6 +181,19 @@ function Install-Foo {
     )
     Install-Module $ModuleName
 }
+'@
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess
+            $violations.Count | Should Be 0
+        }
+
+        It "finds no violation when caller does not declare SupportsShouldProcess and callee is a function with ShouldProcess" {
+            $scriptDef = @'
+function Install-Foo {
+    Param(
+        [string] $ModuleName
+    )
+    Install-Module $ModuleName
+}
 '@
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess
             $violations.Count | Should Be 0
@@ -175,7 +202,7 @@ function Install-Foo {
        It "finds no violation for a function with self reference" {
             $scriptDef = @'
 function Install-ModuleWithDeps {
-[CmdletBinding(SupportsShouldProcess)]
+    [CmdletBinding(SupportsShouldProcess)]
     Param(
         [Parameter(ValueFromPipeline)]
         [string] $ModuleName