Skip to content

Commit ae530a8

Browse files
author
Kapil Borle
committed
Refactor UseShouldProcessForStateChangingFunctions rule
* Removes the redundancies in the previous rule of the kind, "varAst is SomeAst" and then "varAst as SomeAst". * Removes searching for attributes in function definition asts. Function definition asts provide attributes as their properties. Hence, we can we use them directly instead of wasting cycles to search for attribute asts in a function definition ast.
1 parent f17b521 commit ae530a8

File tree

1 file changed

+100
-42
lines changed

1 file changed

+100
-42
lines changed

‎Rules/UseShouldProcessForStateChangingFunctions.cs

Lines changed: 100 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -51,55 +51,113 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
5151
{
5252
throw new ArgumentNullException(Strings.NullAstErrorMessage);
5353
}
54-
IEnumerable<Ast> funcDefAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true);
55-
foreach (FunctionDefinitionAst funcDefAst in funcDefAsts)
54+
IEnumerable<Ast> funcDefWithNoShouldProcessAttrAsts = ast.FindAll(IsStateChangingFunctionWithNoShouldProcessAttribute, true);
55+
foreach (FunctionDefinitionAst funcDefAst in funcDefWithNoShouldProcessAttrAsts)
5656
{
57-
string funcName = funcDefAst.Name;
58-
bool hasShouldProcessAttribute = false;
59-
var targetFuncName = this.stateChangingVerbs.Where(
60-
elem => funcName.StartsWith(
61-
elem,
62-
StringComparison.OrdinalIgnoreCase)).FirstOrDefault();
63-
if (targetFuncName != null)
57+
yield return new DiagnosticRecord(
58+
string.Format(CultureInfo.CurrentCulture, Strings.UseShouldProcessForStateChangingFunctionsError, funcDefAst.Name),
59+
funcDefAst.Extent,
60+
this.GetName(),
61+
DiagnosticSeverity.Warning,
62+
fileName);
63+
}
64+
65+
}
66+
/// <summary>
67+
/// Checks if the ast defines a state changing function
68+
/// </summary>
69+
/// <param name="ast"></param>
70+
/// <returns>Returns true or false</returns>
71+
private bool IsStateChangingFunctionWithNoShouldProcessAttribute(Ast ast)
72+
{
73+
var funcDefAst = ast as FunctionDefinitionAst;
74+
if (funcDefAst == null)
75+
{
76+
return false;
77+
}
78+
string funcName = funcDefAst.Name;
79+
var targetFuncName = this.stateChangingVerbs.Where(
80+
elem => funcName.StartsWith(
81+
elem,
82+
StringComparison.OrdinalIgnoreCase)).FirstOrDefault();
83+
if (targetFuncName == null
84+
|| funcDefAst.Body == null
85+
|| funcDefAst.Body.ParamBlock == null
86+
|| funcDefAst.Body.ParamBlock.Attributes == null
87+
|| !funcDefAst.Body.ParamBlock.Attributes.Any(
88+
attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute)))
89+
{
90+
return false;
91+
}
92+
return !HasShouldProcessTrue(funcDefAst.Body.ParamBlock.Attributes);
93+
}
94+
95+
/// <summary>
96+
/// Checks if an attribute has SupportShouldProcess set to $true
97+
/// </summary>
98+
/// <param name="attributeAsts"></param>
99+
/// <returns>Returns true or false</returns>
100+
private bool HasShouldProcessTrue(IEnumerable<AttributeAst> attributeAsts)
101+
{
102+
if (attributeAsts == null)
103+
{
104+
return false;
105+
}
106+
foreach (var attributeAst in attributeAsts)
107+
{
108+
if (attributeAst == null || attributeAst.NamedArguments == null)
109+
{
110+
continue;
111+
}
112+
foreach (var namedAttributeAst in attributeAst.NamedArguments)
64113
{
65-
IEnumerable<Ast> attributeAsts = funcDefAst.FindAll(testAst => testAst is NamedAttributeArgumentAst, true);
66-
if (funcDefAst.Body != null && funcDefAst.Body.ParamBlock != null
67-
&& funcDefAst.Body.ParamBlock.Attributes != null &&
68-
funcDefAst.Body.ParamBlock.Parameters != null)
114+
if (namedAttributeAst == null)
69115
{
70-
if (!funcDefAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute)))
71-
{
72-
continue;
73-
}
116+
continue;
117+
}
118+
if (!IsShouldProcessAttribute(namedAttributeAst))
119+
{
120+
continue;
121+
}
122+
return IsShouldProcessTrue(namedAttributeAst);
123+
}
124+
}
125+
// Cannot find any SupportShouldProcess attribute
126+
return false;
127+
}
74128

75-
foreach (NamedAttributeArgumentAst attributeAst in attributeAsts)
76-
{
77-
if (attributeAst.ArgumentName.Equals("SupportsShouldProcess", StringComparison.OrdinalIgnoreCase))
78-
{
79-
if (!attributeAst.ExpressionOmitted)
80-
{
81-
var varExpAst = attributeAst.Argument as VariableExpressionAst;
82-
if (varExpAst != null
83-
&& varExpAst.VariablePath.UserPath.Equals("true", StringComparison.OrdinalIgnoreCase))
84-
{
85-
hasShouldProcessAttribute = true;
86-
}
87-
}
88-
else
89-
{
90-
hasShouldProcessAttribute = true;
91-
}
92-
}
93-
}
129+
/// <summary>
130+
/// Checks if the named attribute is SupportShouldProcess
131+
/// </summary>
132+
/// <param name="namedAttributeArgumentAst"></param>
133+
/// <returns>Returns true or false</returns>
134+
private bool IsShouldProcessAttribute(NamedAttributeArgumentAst namedAttributeArgumentAst)
135+
{
136+
return namedAttributeArgumentAst != null
137+
&& namedAttributeArgumentAst.ArgumentName.Equals("SupportsShouldProcess", StringComparison.OrdinalIgnoreCase);
138+
}
94139

95-
if (!hasShouldProcessAttribute)
96-
{
97-
yield return
98-
new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.UseShouldProcessForStateChangingFunctionsError, funcName), funcDefAst.Extent, this.GetName(), DiagnosticSeverity.Warning, fileName);
99-
}
100-
}
140+
/// <summary>
141+
/// Checks if the SupportShouldProcess attribute is true
142+
/// </summary>
143+
/// <param name="namedAttributeArgumentAst"></param>
144+
/// <returns>Returns true or false</returns>
145+
private bool IsShouldProcessTrue(NamedAttributeArgumentAst namedAttributeArgumentAst)
146+
{
147+
if (namedAttributeArgumentAst.ExpressionOmitted)
148+
{
149+
return true;
150+
}
151+
else
152+
{
153+
var varExpAst = namedAttributeArgumentAst.Argument as VariableExpressionAst;
154+
if (varExpAst != null
155+
&& varExpAst.VariablePath.UserPath.Equals("true", StringComparison.OrdinalIgnoreCase))
156+
{
157+
return true;
101158
}
102159
}
160+
return false;
103161
}
104162

105163
/// <summary>

0 commit comments

Comments
 (0)