Skip to content

Commit cddf24f

Browse files
author
Kapil Borle
committed
Merge pull request #503 from PowerShell/AddRemoveToUseShouldProcess
Add remove verb to UseShouldProcessForStateChangingFunctions rule
2 parents 14eaf70 + 54dc762 commit cddf24f

5 files changed

+220
-59
lines changed

‎Engine/Helper.cs

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1329,6 +1329,134 @@ public static string[] ProcessCustomRulePaths(string[] rulePaths, SessionState s
13291329

13301330
}
13311331

1332+
/// <summary>
1333+
/// Check if the function name starts with one of potentailly state changing verbs
1334+
/// </summary>
1335+
/// <param name="functionName"></param>
1336+
/// <returns>true if the function name starts with a state changing verb, otherwise false</returns>
1337+
public bool IsStateChangingFunctionName(string functionName)
1338+
{
1339+
if (functionName == null)
1340+
{
1341+
throw new ArgumentNullException("functionName");
1342+
}
1343+
// Array of verbs that can potentially change the state of a system
1344+
string[] stateChangingVerbs =
1345+
{
1346+
"Restart-",
1347+
"Stop-",
1348+
"New-",
1349+
"Set-",
1350+
"Update-",
1351+
"Reset-",
1352+
"Remove-"
1353+
};
1354+
foreach (var verb in stateChangingVerbs)
1355+
{
1356+
if (functionName.StartsWith(verb, StringComparison.OrdinalIgnoreCase))
1357+
{
1358+
return true;
1359+
}
1360+
}
1361+
return false;
1362+
}
1363+
1364+
/// <summary>
1365+
/// Get the SupportShouldProcess attribute ast
1366+
/// </summary>
1367+
/// <param name="attributeAsts"></param>
1368+
/// <returns>Returns SupportShouldProcess attribute ast if it exists, otherwise returns null</returns>
1369+
public NamedAttributeArgumentAst GetShouldProcessAttributeAst(IEnumerable<AttributeAst> attributeAsts)
1370+
{
1371+
if (attributeAsts == null)
1372+
{
1373+
throw new ArgumentNullException("attributeAsts");
1374+
}
1375+
var cmdletBindingAttributeAst = this.GetCmdletBindingAttributeAst(attributeAsts);
1376+
if (cmdletBindingAttributeAst == null
1377+
|| cmdletBindingAttributeAst.NamedArguments == null)
1378+
{
1379+
return null;
1380+
}
1381+
foreach (var namedAttributeAst in cmdletBindingAttributeAst.NamedArguments)
1382+
{
1383+
if (namedAttributeAst != null
1384+
&& namedAttributeAst.ArgumentName.Equals(
1385+
"SupportsShouldProcess",
1386+
StringComparison.OrdinalIgnoreCase))
1387+
{
1388+
return namedAttributeAst;
1389+
}
1390+
}
1391+
return null;
1392+
}
1393+
1394+
/// <summary>
1395+
/// Get the CmdletBinding attribute ast
1396+
/// </summary>
1397+
/// <param name="attributeAsts"></param>
1398+
/// <returns>Returns CmdletBinding attribute ast if it exists, otherwise returns null</returns>
1399+
public AttributeAst GetCmdletBindingAttributeAst(IEnumerable<AttributeAst> attributeAsts)
1400+
{
1401+
if (attributeAsts == null)
1402+
{
1403+
throw new ArgumentNullException("attributeAsts");
1404+
}
1405+
foreach (var attributeAst in attributeAsts)
1406+
{
1407+
if (attributeAst == null || attributeAst.NamedArguments == null)
1408+
{
1409+
continue;
1410+
}
1411+
if (attributeAst.TypeName.GetReflectionAttributeType()
1412+
== typeof(CmdletBindingAttribute))
1413+
{
1414+
return attributeAst;
1415+
}
1416+
}
1417+
return null;
1418+
}
1419+
1420+
/// <summary>
1421+
/// Get the boolean value of the named attribute argument
1422+
/// </summary>
1423+
/// <param name="namedAttributeArgumentAst"></param>
1424+
/// <returns>Boolean value of the named attribute argument</returns>
1425+
public bool GetNamedArgumentAttributeValue(NamedAttributeArgumentAst namedAttributeArgumentAst)
1426+
{
1427+
if (namedAttributeArgumentAst == null)
1428+
{
1429+
throw new ArgumentNullException("namedAttributeArgumentAst");
1430+
}
1431+
if (namedAttributeArgumentAst.ExpressionOmitted)
1432+
{
1433+
return true;
1434+
}
1435+
else
1436+
{
1437+
var varExpAst = namedAttributeArgumentAst.Argument as VariableExpressionAst;
1438+
if (varExpAst == null)
1439+
{
1440+
var constExpAst = namedAttributeArgumentAst.Argument as ConstantExpressionAst;
1441+
if (constExpAst == null)
1442+
{
1443+
return false;
1444+
}
1445+
bool constExpVal;
1446+
if (LanguagePrimitives.TryConvertTo<bool>(constExpAst.Value, out constExpVal))
1447+
{
1448+
return constExpVal;
1449+
}
1450+
}
1451+
else
1452+
{
1453+
return varExpAst.VariablePath.UserPath.Equals(
1454+
bool.TrueString,
1455+
StringComparison.OrdinalIgnoreCase);
1456+
}
1457+
}
1458+
return false;
1459+
}
13321460

13331461
#endregion
13341462
}

‎Rules/UseShouldProcessForStateChangingFunctions.cs

Lines changed: 55 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
//
2-
// Copyright (c) Microsoft Corporation.
1+
// Copyright (c) Microsoft Corporation.
32
//
43
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
54
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
@@ -8,19 +7,19 @@
87
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
98
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
109
// THE SOFTWARE.
11-
//
1210

1311
using System;
1412
using System.Collections.Generic;
15-
using System.Linq;
13+
using System.ComponentModel.Composition;
1614
using System.Management.Automation;
1715
using System.Management.Automation.Language;
18-
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
19-
using System.ComponentModel.Composition;
2016
using System.Globalization;
17+
using System.Linq;
18+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
2119

2220
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
23-
{/// <summary>
21+
{
22+
/// <summary>
2423
/// UseShouldProcessForStateChangingFunctions: Analyzes the ast to check if ShouldProcess is included in Advanced functions if the Verb of the function could change system state.
2524
/// </summary>
2625
[Export(typeof(IScriptRule))]
@@ -34,53 +33,53 @@ public class UseShouldProcessForStateChangingFunctions : IScriptRule
3433
/// <returns>A List of diagnostic results of this rule</returns>
3534
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
3635
{
37-
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
38-
39-
IEnumerable<Ast> funcDefAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true);
40-
string supportsShouldProcess = "SupportsShouldProcess";
41-
string trueString = "$true";
42-
foreach (FunctionDefinitionAst funcDefAst in funcDefAsts)
36+
if (ast == null)
4337
{
44-
string funcName = funcDefAst.Name;
45-
bool hasShouldProcessAttribute = false;
46-
47-
if (funcName.StartsWith("Restart-", StringComparison.OrdinalIgnoreCase) ||
48-
funcName.StartsWith("Stop-", StringComparison.OrdinalIgnoreCase)||
49-
funcName.StartsWith("New-", StringComparison.OrdinalIgnoreCase) ||
50-
funcName.StartsWith("Set-", StringComparison.OrdinalIgnoreCase) ||
51-
funcName.StartsWith("Update-", StringComparison.OrdinalIgnoreCase) ||
52-
funcName.StartsWith("Reset-", StringComparison.OrdinalIgnoreCase))
53-
{
54-
IEnumerable<Ast> attributeAsts = funcDefAst.FindAll(testAst => testAst is NamedAttributeArgumentAst, true);
55-
if (funcDefAst.Body != null && funcDefAst.Body.ParamBlock != null
56-
&& funcDefAst.Body.ParamBlock.Attributes != null &&
57-
funcDefAst.Body.ParamBlock.Parameters != null)
58-
{
59-
if (!funcDefAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof (CmdletBindingAttribute)))
60-
{
61-
continue;
62-
}
63-
64-
foreach (NamedAttributeArgumentAst attributeAst in attributeAsts)
65-
{
66-
if (attributeAst.ArgumentName.Equals(supportsShouldProcess, StringComparison.OrdinalIgnoreCase))
67-
{
68-
if((attributeAst.Argument.Extent.Text.Equals(trueString, StringComparison.OrdinalIgnoreCase)) && !attributeAst.ExpressionOmitted ||
69-
attributeAst.ExpressionOmitted)
70-
{
71-
hasShouldProcessAttribute = true;
72-
}
73-
}
74-
}
38+
throw new ArgumentNullException(Strings.NullAstErrorMessage);
39+
}
40+
IEnumerable<Ast> funcDefWithNoShouldProcessAttrAsts = ast.FindAll(IsStateChangingFunctionWithNoShouldProcessAttribute, true);
41+
foreach (FunctionDefinitionAst funcDefAst in funcDefWithNoShouldProcessAttrAsts)
42+
{
43+
yield return new DiagnosticRecord(
44+
string.Format(CultureInfo.CurrentCulture, Strings.UseShouldProcessForStateChangingFunctionsError, funcDefAst.Name),
45+
Helper.Instance.GetScriptExtentForFunctionName(funcDefAst),
46+
this.GetName(),
47+
DiagnosticSeverity.Warning,
48+
fileName);
49+
}
50+
51+
}
52+
/// <summary>
53+
/// Checks if the ast defines a state changing function
54+
/// </summary>
55+
/// <param name="ast"></param>
56+
/// <returns>Returns true or false</returns>
57+
private bool IsStateChangingFunctionWithNoShouldProcessAttribute(Ast ast)
58+
{
59+
var funcDefAst = ast as FunctionDefinitionAst;
60+
if (funcDefAst == null)
61+
{
62+
return false;
63+
}
64+
return Helper.Instance.IsStateChangingFunctionName(funcDefAst.Name)
65+
&& (funcDefAst.Body.ParamBlock == null
66+
|| funcDefAst.Body.ParamBlock.Attributes == null
67+
|| !HasShouldProcessTrue(funcDefAst.Body.ParamBlock.Attributes));
68+
}
7569

76-
if (!hasShouldProcessAttribute)
77-
{
78-
yield return
79-
new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture,Strings.UseShouldProcessForStateChangingFunctionsError, funcName),funcDefAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName);
80-
}
81-
}
82-
}
70+
/// <summary>
71+
/// Checks if an attribute has SupportShouldProcess set to $true
72+
/// </summary>
73+
/// <param name="attributeAsts"></param>
74+
/// <returns>Returns true or false</returns>
75+
private bool HasShouldProcessTrue(IEnumerable<AttributeAst> attributeAsts)
76+
{
77+
var shouldProcessAttributeAst = Helper.Instance.GetShouldProcessAttributeAst(attributeAsts);
78+
if (shouldProcessAttributeAst == null)
79+
{
80+
return false;
8381
}
82+
return Helper.Instance.GetNamedArgumentAttributeValue(shouldProcessAttributeAst);
8483
}
8584

8685
/// <summary>
@@ -89,7 +88,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
8988
/// <returns>The name of this rule</returns>
9089
public string GetName()
9190
{
92-
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseShouldProcessForStateChangingFunctionsName);
91+
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, this.GetSourceName(), Strings.UseShouldProcessForStateChangingFunctionsName);
9392
}
9493

9594
/// <summary>
@@ -111,8 +110,9 @@ public string GetDescription()
111110
}
112111

113112
/// <summary>
114-
/// GetSourceType: Retrieves the type of the rule: builtin, managed or module.
113+
/// GetSourceType: Retrieves the type of the rule: built-in, managed or module.
115114
/// </summary>
115+
/// <returns>Source type {PS, PSDSC}</returns>
116116
public SourceType GetSourceType()
117117
{
118118
return SourceType.Builtin;
@@ -121,20 +121,19 @@ public SourceType GetSourceType()
121121
/// <summary>
122122
/// GetSeverity: Retrieves the severity of the rule: error, warning of information.
123123
/// </summary>
124-
/// <returns></returns>
124+
/// <returns>Rule severity {Information, Warning, Error}</returns>
125125
public RuleSeverity GetSeverity()
126126
{
127127
return RuleSeverity.Warning;
128128
}
129129

130-
131130
/// <summary>
132131
/// GetSourceName: Retrieves the module/assembly name the rule is from.
133132
/// </summary>
133+
/// <returns>Source name</returns>
134134
public string GetSourceName()
135135
{
136136
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
137137
}
138138
}
139-
140139
}

‎Tests/Rules/UseShouldProcessForStateChangingFunctions.ps1

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,25 @@
44

55
}
66

7+
function Stop-MyObject{
8+
[CmdletBinding(SupportsShouldProcess = 0)]
9+
param([string]$c, [int]$d)
10+
11+
}
12+
13+
function New-MyObject{
14+
[CmdletBinding(SupportsShouldProcess = "")]
15+
param([string]$c, [int]$d)
16+
17+
}
18+
719
function Set-MyObject{
820
[CmdletBinding()]
921
param([string]$c, [int]$d)
1022

11-
}
23+
}
24+
25+
function Remove-MyObject{
26+
[CmdletBinding()]
27+
param([string]$c, [int]$d)
28+
}

‎Tests/Rules/UseShouldProcessForStateChangingFunctions.tests.ps1

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,18 @@ $noViolations = Invoke-ScriptAnalyzer $directory\UseShouldProcessForStateChangin
77

88
Describe "It checks UseShouldProcess is enabled when there are state changing verbs in the function names" {
99
Context "When there are violations" {
10-
It "has 2 violations where ShouldProcess is not supported" {
11-
$violations.Count | Should Be 2
10+
$numViolations = 5
11+
It ("has {0} violations where ShouldProcess is not supported" -f $numViolations) {
12+
$violations.Count | Should Be $numViolations
1213
}
1314

1415
It "has the correct description message" {
1516
$violations[0].Message | Should Match $violationMessage
1617
}
18+
19+
It "has the correct extent" {
20+
$violations[0].Extent.Text | Should Be "Set-MyObject"
21+
}
1722
}
1823

1924
Context "When there are no violations" {

‎Tests/Rules/UseShouldProcessForStateChangingFunctionsNoViolations.ps1

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,18 @@
44
param ([string]$c)
55
}
66

7+
function Stop-MyObject{
8+
[CmdletBinding(SupportsShouldProcess = 1)]
9+
param([string]$c, [int]$d)
10+
11+
}
12+
13+
function New-MyObject{
14+
[CmdletBinding(SupportsShouldProcess = "a")]
15+
param([string]$c, [int]$d)
16+
17+
}
18+
719
function Test-GetMyObject{
820

921
param([string]$c, [int]$d)

0 commit comments

Comments
 (0)