Update UseShouldProcessForStateChangingFunctions rule implemetation

This commit makes the following changes.

* Extent includes only the function name instead of the entire function definition
* Use GetReflectionTypeAttribute instead of GetReflectionType to compare with the type of CmdletBindingAttribute
* Iterate through the attributes only once
* Instead of checking for $true for SupportShouldProcess attribute, check for any constant that can be converted to true

Author: Kapil Borle

Rules/UseShouldProcessForStateChangingFunctions.cs

@@ -56,7 +56,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
             {
                 yield return new DiagnosticRecord(
                     string.Format(CultureInfo.CurrentCulture, Strings.UseShouldProcessForStateChangingFunctionsError, funcDefAst.Name), 
-                    funcDefAst.Extent, 
+                    Helper.Instance.GetScriptExtentForFunctionName(funcDefAst),                    
                     this.GetName(), 
                     DiagnosticSeverity.Warning, 
                     fileName);
@@ -80,12 +80,9 @@ private bool IsStateChangingFunctionWithNoShouldProcessAttribute(Ast ast)
                 elem => funcName.StartsWith(
                     elem,
                     StringComparison.OrdinalIgnoreCase)).FirstOrDefault();
-            if (targetFuncName == null
-                || funcDefAst.Body == null
+            if (targetFuncName == null                
                 || funcDefAst.Body.ParamBlock == null
-                || funcDefAst.Body.ParamBlock.Attributes == null
-                || !funcDefAst.Body.ParamBlock.Attributes.Any(
-                    attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute)))
+                || funcDefAst.Body.ParamBlock.Attributes == null)
             {
                 return false;
             }
@@ -99,44 +96,32 @@ private bool IsStateChangingFunctionWithNoShouldProcessAttribute(Ast ast)
         /// <returns>Returns true or false</returns>
         private bool HasShouldProcessTrue(IEnumerable<AttributeAst> attributeAsts)
         {
-            if (attributeAsts == null)
-            {
-                return false;
-            }
             foreach (var attributeAst in attributeAsts)
             {                
                 if (attributeAst == null || attributeAst.NamedArguments == null)
                 {
                     continue;
                 }
-                foreach (var namedAttributeAst in attributeAst.NamedArguments)
+                if (attributeAst.TypeName.GetReflectionAttributeType() 
+                    == typeof(CmdletBindingAttribute))
                 {
-                    if (namedAttributeAst == null)
-                    {
-                        continue;
-                    }
-                    if (!IsShouldProcessAttribute(namedAttributeAst))
+                    foreach (var namedAttributeAst in attributeAst.NamedArguments)
                     {
-                        continue;
+                        if (namedAttributeAst == null
+                            || !namedAttributeAst.ArgumentName.Equals(
+                                "SupportsShouldProcess", 
+                                StringComparison.OrdinalIgnoreCase))
+                        {
+                            continue;
+                        }
+                        return IsShouldProcessTrue(namedAttributeAst);                        
                     }
-                    return IsShouldProcessTrue(namedAttributeAst);                    
                 }
             }
             // Cannot find any SupportShouldProcess attribute   
             return false;
         }
 
-        /// <summary>
-        /// Checks if the named attribute is SupportShouldProcess
-        /// </summary>
-        /// <param name="namedAttributeArgumentAst"></param>
-        /// <returns>Returns true or false</returns>
-        private bool IsShouldProcessAttribute(NamedAttributeArgumentAst namedAttributeArgumentAst)
-        {
-            return namedAttributeArgumentAst != null
-                && namedAttributeArgumentAst.ArgumentName.Equals("SupportsShouldProcess", StringComparison.OrdinalIgnoreCase);
-        }
-
         /// <summary>
         /// Checks if the SupportShouldProcess attribute is true
         /// </summary>
@@ -151,11 +136,25 @@ private bool IsShouldProcessTrue(NamedAttributeArgumentAst namedAttributeArgumen
             else
             {
                 var varExpAst = namedAttributeArgumentAst.Argument as VariableExpressionAst;
-                if (varExpAst != null
-                    && varExpAst.VariablePath.UserPath.Equals("true", StringComparison.OrdinalIgnoreCase))
+                if (varExpAst == null)
                 {
-                    return true;
+                    var constExpAst = namedAttributeArgumentAst.Argument as ConstantExpressionAst;
+                    if (constExpAst == null)
+                    {
+                        return false;
+                    }
+                    bool constExpVal;
+                    if (LanguagePrimitives.TryConvertTo<bool>(constExpAst.Value, out constExpVal))
+                    {
+                        return constExpVal;
+                    }                    
                 }
+                else if (varExpAst.VariablePath.UserPath.Equals(
+                    bool.TrueString, 
+                    StringComparison.OrdinalIgnoreCase))
+                {
+                    return true;
+                }                
             }
             return false;
         }

Tests/Rules/UseShouldProcessForStateChangingFunctions.ps1

@@ -4,6 +4,18 @@
 
 } 
 
+function Stop-MyObject{ 
+    [CmdletBinding(SupportsShouldProcess = 0)]
+    param([string]$c, [int]$d) 
+
+}
+
+function New-MyObject{ 
+    [CmdletBinding(SupportsShouldProcess = "")]
+    param([string]$c, [int]$d) 
+
+}
+
 function Set-MyObject{ 
     [CmdletBinding()]
     param([string]$c, [int]$d) 

Tests/Rules/UseShouldProcessForStateChangingFunctions.tests.ps1

@@ -7,14 +7,18 @@ $noViolations = Invoke-ScriptAnalyzer $directory\UseShouldProcessForStateChangin
 
 Describe "It checks UseShouldProcess is enabled when there are state changing verbs in the function names" {
     Context "When there are violations" {
-    	$numViolations = 3
+    	$numViolations = 5
         It ("has {0} violations where ShouldProcess is not supported" -f $numViolations) {
             $violations.Count | Should Be $numViolations
         }
 
         It "has the correct description message" {
             $violations[0].Message | Should Match $violationMessage
         }
+
+	It "has the correct extent" {
+	   $violations[0].Extent.Text | Should Be "Set-MyObject"
+	}
     }
 
     Context "When there are no violations" {

Tests/Rules/UseShouldProcessForStateChangingFunctionsNoViolations.ps1

@@ -4,6 +4,18 @@
     param ([string]$c)
 }
 
+function Stop-MyObject{ 
+    [CmdletBinding(SupportsShouldProcess = 1)]
+    param([string]$c, [int]$d) 
+
+}
+
+function New-MyObject{ 
+    [CmdletBinding(SupportsShouldProcess = "a")]
+    param([string]$c, [int]$d) 
+
+}
+
 function Test-GetMyObject{ 
 
     param([string]$c, [int]$d)