Fix PSShouldProcess rule violation extents (#668)

* Fix PSShouldProcess rule violation extents

For the case when only the SupportsShouldProcess attribute is present but the call ShouldProcess is absent the violation extent covers only the attribute.

For the case when only the ShouldProcess call is present but not the SupportsShouldProcess attribute, the violation extent covers only the ShouldProcess call.

* Add tests to check PSShouldProcess violation extent

Author: Kapil Borle

Rules/UseShouldProcessCorrectly.cs

@@ -107,6 +107,14 @@ public SourceType GetSourceType()
             return SourceType.Builtin;
         }
 
+        /// <summary>
+        /// GetSourceName: Retrieves the module/assembly name the rule is from.
+        /// </summary>
+        public string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+
         /// <summary>
         /// GetSeverity: Retrieves the severity of the rule: error, warning of information.
         /// </summary>
@@ -116,12 +124,9 @@ public RuleSeverity GetSeverity()
             return RuleSeverity.Warning;
         }
 
-        /// <summary>
-        /// GetSourceName: Retrieves the module/assembly name the rule is from.
-        /// </summary>
-        public string GetSourceName()
+        private DiagnosticSeverity GetDianosticSeverity()
         {
-            return string.Format(CultureInfo.CurrentCulture,Strings.SourceName);
+            return DiagnosticSeverity.Warning;
         }
 
         /// <summary>
@@ -164,9 +169,9 @@ private DiagnosticRecord GetViolation(Vertex v)
                             CultureInfo.CurrentCulture,
                             Strings.ShouldProcessErrorHasAttribute,
                             fast.Name),
-                        ast.Extent,
+                        Helper.Instance.GetShouldProcessAttributeAst(fast.Body.ParamBlock.Attributes).Extent,
                         GetName(),
-                        DiagnosticSeverity.Warning,
+                        GetDianosticSeverity(),
                         ast.Extent.File);
                 }
             }
@@ -187,16 +192,55 @@ private DiagnosticRecord GetViolation(Vertex v)
                              CultureInfo.CurrentCulture,
                              Strings.ShouldProcessErrorHasCmdlet,
                              fast.Name),
-                            v.Ast.Extent,
+                            GetShouldProcessCallExtent(fast),
                             GetName(),
-                            DiagnosticSeverity.Warning,
+                            GetDianosticSeverity(),
                             fileName);
                 }
             }
 
             return null;
         }
 
+        /// <summary>
+        /// Gets the extent of ShouldProcess call
+        /// </summary>
+        private static IScriptExtent GetShouldProcessCallExtent(FunctionDefinitionAst functionDefinitionAst)
+        {
+            var invokeMemberExpressionAstFound = functionDefinitionAst.Find(IsShouldProcessCall, true);
+            if (invokeMemberExpressionAstFound == null)
+            {
+                return functionDefinitionAst.Extent;
+            }
+
+            return (invokeMemberExpressionAstFound as InvokeMemberExpressionAst).Member.Extent;
+        }
+
+        /// <summary>
+        /// Returns true if ast if of the form $PSCmdlet.PSShouldProcess()
+        /// </summary>
+        private static bool IsShouldProcessCall(Ast ast)
+        {
+            var invokeMemberExpressionAst = ast as InvokeMemberExpressionAst;
+            if (invokeMemberExpressionAst == null)
+            {
+                return false;
+            }
+
+            var memberExprAst = invokeMemberExpressionAst.Member as StringConstantExpressionAst;
+            if (memberExprAst == null)
+            {
+                return false;
+            }
+
+            if ("ShouldProcess".Equals(memberExprAst.Value, StringComparison.OrdinalIgnoreCase))
+            {
+                return true;
+            }
+
+            return false;
+        }
+
         private bool callsShouldProcessDirectly(Vertex vertex)
         {
             return funcDigraph.GetNeighbors(vertex).Contains(shouldProcessVertex);

Tests/Rules/UseShouldProcessCorrectly.tests.ps1

@@ -246,6 +246,31 @@ function Install-ModuleWithDeps {
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess
             $violations.Count | Should Be 0
         }
+    }
+
+    Context "Violation Extent" {
+        It "should mark only the SupportsShouldProcess attribute" {
+            $scriptDef = @'
+function Foo
+{
+   [CmdletBinding(SupportsShouldProcess)]
+   param()
+}
+'@
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess
+            $violations[0].Extent.Text | Should Be 'SupportsShouldProcess'
+        }
 
+        It "should mark only the ShouldProcess call" {
+             $scriptDef = @'
+function Foo
+{
+   param()
+   if ($PSCmdlet.ShouldProcess('','')) { Write-Output "Should Process" }
+}
+'@
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDef -IncludeRule PSShouldProcess
+            $violations[0].Extent.Text | Should Be 'ShouldProcess'
+        }
     }
 }