Skip to content

Commit 048c7f3

Browse files
bergmeisterJamesWTruher
bergmeister
authored and
JamesWTruher
committed
Warn when an if statement contains an assignment (#859)
* first try at implementation rule. basically works but also catches case where the execution block of the if statement contains an assignment * remove unused variables and fix test to import pssa and fix some rule counter tests * Improve rule to only assert against clause in if statement. TODO: Make sure that the following case does not get flagged up (added a test case for it) 'if ( $files = get-childitem ) { }' * Improve rule to reduce false warnings and adds lots more tests. The only false positive that is left is 'if ($a = (Get-ChildItem)){}' where the RHS is wrapped in an exprssion * fix false positive when the command is wrapped in an expression * simplify and cleanup * Uncomment import of PSSA in test to be consistent with other tests
1 parent 3f08895 commit 048c7f3

7 files changed

+264
-39
lines changed

‎Engine/Generic/IScriptRule.cs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,7 @@
1010
// THE SOFTWARE.
1111
//
1212

13-
using System;
1413
using System.Collections.Generic;
15-
using System.Linq;
16-
using System.Text;
17-
using System.Threading.Tasks;
1814
using System.Management.Automation.Language;
1915

2016
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic

‎RuleDocumentation/PossibleIncorrectUsageOfAssignmentOperator.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# PossibleIncorrectUsageOfAssignmentOperator
2+
3+
**Severity Level: Information**
4+
5+
## Description
6+
7+
In many programming languages, the equality operator is denoted as `==` or `=`, but `PowerShell` uses `-eq`. Since assignment inside if statements are very rare, this rule wants to call out this case because it might have been unintentional.

‎Rules/PossibleIncorrectUsageOfAssignmentOperator.cs

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
//
2+
// Copyright (c) Microsoft Corporation.
3+
//
4+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
5+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
6+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
7+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
8+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
9+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
10+
// THE SOFTWARE.
11+
//
12+
13+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
14+
using System;
15+
using System.Collections.Generic;
16+
#if !CORECLR
17+
using System.ComponentModel.Composition;
18+
#endif
19+
using System.Management.Automation.Language;
20+
using System.Globalization;
21+
22+
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
23+
{
24+
/// <summary>
25+
/// PossibleIncorrectUsageOfAssignmentOperator: Warn if someone uses the '=' or '==' by accident in an if statement because in most cases that is not the intention.
26+
/// </summary>
27+
#if !CORECLR
28+
[Export(typeof(IScriptRule))]
29+
#endif
30+
public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule
31+
{
32+
/// <summary>
33+
/// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements.
34+
/// </summary>
35+
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
36+
{
37+
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
38+
39+
var ifStatementAsts = ast.FindAll(testAst => testAst is IfStatementAst, searchNestedScriptBlocks: true);
40+
foreach (IfStatementAst ifStatementAst in ifStatementAsts)
41+
{
42+
foreach (var clause in ifStatementAst.Clauses)
43+
{
44+
var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst;
45+
if (assignmentStatementAst != null)
46+
{
47+
// Check if someone used '==', which can easily happen when the person is used to coding a lot in C#.
48+
// In most cases, this will be a runtime error because PowerShell will look for a cmdlet name starting with '=', which is technically possible to define
49+
if (assignmentStatementAst.Right.Extent.Text.StartsWith("="))
50+
{
51+
yield return new DiagnosticRecord(
52+
Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent,
53+
GetName(), DiagnosticSeverity.Warning, fileName);
54+
}
55+
else
56+
{
57+
// If the right hand side contains a CommandAst at some point, then we do not want to warn
58+
// because this could be intentional in cases like 'if ($a = Get-ChildItem){ }'
59+
var commandAst = assignmentStatementAst.Right.Find(testAst => testAst is CommandAst, searchNestedScriptBlocks: true) as CommandAst;
60+
if (commandAst == null)
61+
{
62+
yield return new DiagnosticRecord(
63+
Strings.PossibleIncorrectUsageOfAssignmentOperatorError, assignmentStatementAst.Extent,
64+
GetName(), DiagnosticSeverity.Information, fileName);
65+
}
66+
}
67+
}
68+
}
69+
}
70+
}
71+
72+
/// <summary>
73+
/// GetName: Retrieves the name of this rule.
74+
/// </summary>
75+
/// <returns>The name of this rule</returns>
76+
public string GetName()
77+
{
78+
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfAssignmentOperatorName);
79+
}
80+
81+
/// <summary>
82+
/// GetCommonName: Retrieves the common name of this rule.
83+
/// </summary>
84+
/// <returns>The common name of this rule</returns>
85+
public string GetCommonName()
86+
{
87+
return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorCommonName);
88+
}
89+
90+
/// <summary>
91+
/// GetDescription: Retrieves the description of this rule.
92+
/// </summary>
93+
/// <returns>The description of this rule</returns>
94+
public string GetDescription()
95+
{
96+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingWriteHostDescription);
97+
}
98+
99+
/// <summary>
100+
/// GetSourceType: Retrieves the type of the rule: builtin, managed or module.
101+
/// </summary>
102+
public SourceType GetSourceType()
103+
{
104+
return SourceType.Builtin;
105+
}
106+
107+
/// <summary>
108+
/// GetSeverity: Retrieves the severity of the rule: error, warning of information.
109+
/// </summary>
110+
/// <returns></returns>
111+
public RuleSeverity GetSeverity()
112+
{
113+
return RuleSeverity.Information;
114+
}
115+
116+
/// <summary>
117+
/// GetSourceName: Retrieves the module/assembly name the rule is from.
118+
/// </summary>
119+
public string GetSourceName()
120+
{
121+
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
122+
}
123+
}
124+
}

‎Rules/Strings.Designer.cs

Lines changed: 31 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Rules/Strings.resx

Lines changed: 37 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
<?xml version="1.0" encoding="utf-8"?>
22
<root>
3-
<!--
4-
Microsoft ResX Schema
5-
3+
<!--
4+
Microsoft ResX Schema
5+
66
Version 2.0
7-
8-
The primary goals of this format is to allow a simple XML format
9-
that is mostly human readable. The generation and parsing of the
10-
various data types are done through the TypeConverter classes
7+
8+
The primary goals of this format is to allow a simple XML format
9+
that is mostly human readable. The generation and parsing of the
10+
various data types are done through the TypeConverter classes
1111
associated with the data types.
12-
12+
1313
Example:
14-
14+
1515
... ado.net/XML headers & schema ...
1616
<resheader name="resmimetype">text/microsoft-resx</resheader>
1717
<resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
2626
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
2727
<comment>This is a comment</comment>
2828
</data>
29-
30-
There are any number of "resheader" rows that contain simple
29+
30+
There are any number of "resheader" rows that contain simple
3131
name/value pairs.
32-
33-
Each data row contains a name, and value. The row also contains a
34-
type or mimetype. Type corresponds to a .NET class that support
35-
text/value conversion through the TypeConverter architecture.
36-
Classes that don't support this are serialized and stored with the
32+
33+
Each data row contains a name, and value. The row also contains a
34+
type or mimetype. Type corresponds to a .NET class that support
35+
text/value conversion through the TypeConverter architecture.
36+
Classes that don't support this are serialized and stored with the
3737
mimetype set.
38-
39-
The mimetype is used for serialized objects, and tells the
40-
ResXResourceReader how to depersist the object. This is currently not
38+
39+
The mimetype is used for serialized objects, and tells the
40+
ResXResourceReader how to depersist the object. This is currently not
4141
extensible. For a given mimetype the value must be set accordingly:
42-
43-
Note - application/x-microsoft.net.object.binary.base64 is the format
44-
that the ResXResourceWriter will generate, however the reader can
42+
43+
Note - application/x-microsoft.net.object.binary.base64 is the format
44+
that the ResXResourceWriter will generate, however the reader can
4545
read any of the formats listed below.
46-
46+
4747
mimetype: application/x-microsoft.net.object.binary.base64
48-
value : The object must be serialized with
48+
value : The object must be serialized with
4949
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
5050
: and then encoded with base64 encoding.
51-
51+
5252
mimetype: application/x-microsoft.net.object.soap.base64
53-
value : The object must be serialized with
53+
value : The object must be serialized with
5454
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
5555
: and then encoded with base64 encoding.
5656
5757
mimetype: application/x-microsoft.net.object.bytearray.base64
58-
value : The object must be serialized into a byte array
58+
value : The object must be serialized into a byte array
5959
: using a System.ComponentModel.TypeConverter
6060
: and then encoded with base64 encoding.
6161
-->
@@ -957,7 +957,6 @@
957957
<data name="UseConsistentWhitespaceErrorSeparatorSemi" xml:space="preserve">
958958
<value>Use space after a semicolon.</value>
959959
</data>
960-
961960
<data name="UseSupportsShouldProcessName" xml:space="preserve">
962961
<value>UseSupportsShouldProcess</value>
963962
</data>
@@ -982,4 +981,13 @@
982981
<data name="AlignAssignmentStatementError" xml:space="preserve">
983982
<value>Assignment statements are not aligned</value>
984983
</data>
985-
</root>
984+
<data name="PossibleIncorrectUsageOfAssignmentOperatorCommonName" xml:space="preserve">
985+
<value>'=' operator means assignment. Did you mean the equal operator '-eq'?</value>
986+
</data>
987+
<data name="PossibleIncorrectUsageOfAssignmentOperatorError" xml:space="preserve">
988+
<value>Did you really mean to make an assignment inside an if statement? If you rather meant to check for equality, use the '-eq' operator.</value>
989+
</data>
990+
<data name="PossibleIncorrectUsageOfAssignmentOperatorName" xml:space="preserve">
991+
<value>PossibleIncorrectUsageOfAssignmentOperator</value>
992+
</data>
993+
</root>

‎Tests/Engine/GetScriptAnalyzerRule.tests.ps1

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ Describe "Test Name parameters" {
6161

6262
It "get Rules with no parameters supplied" {
6363
$defaultRules = Get-ScriptAnalyzerRule
64-
$expectedNumRules = 52
64+
$expectedNumRules = 53
6565
if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4))
6666
{
6767
# for PSv3 PSAvoidGlobalAliases is not shipped because
@@ -159,7 +159,7 @@ Describe "TestSeverity" {
159159

160160
It "filters rules based on multiple severity inputs"{
161161
$rules = Get-ScriptAnalyzerRule -Severity Error,Information
162-
$rules.Count | Should be 14
162+
$rules.Count | Should be 15
163163
}
164164

165165
It "takes lower case inputs" {

‎Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
Import-Module PSScriptAnalyzer
2+
$ruleName = "PSPossibleIncorrectUsageOfAssignmentOperator"
3+
4+
Describe "PossibleIncorrectUsageOfAssignmentOperator" {
5+
Context "When there are violations" {
6+
It "assignment inside if statement causes warning" {
7+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=$b){}' | Where-Object {$_.RuleName -eq $ruleName}
8+
$warnings.Count | Should Be 1
9+
}
10+
11+
It "assignment inside if statement causes warning when when wrapped in command expression" {
12+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a=($b)){}' | Where-Object {$_.RuleName -eq $ruleName}
13+
$warnings.Count | Should Be 1
14+
}
15+
16+
It "assignment inside if statement causes warning when wrapped in expression" {
17+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a="$b"){}' | Where-Object {$_.RuleName -eq $ruleName}
18+
$warnings.Count | Should Be 1
19+
}
20+
21+
It "assignment inside elseif statement causes warning" {
22+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){}elseif($a = $b){}' | Where-Object {$_.RuleName -eq $ruleName}
23+
$warnings.Count | Should Be 1
24+
}
25+
26+
It "double equals inside if statement causes warning" {
27+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == $b){}' | Where-Object {$_.RuleName -eq $ruleName}
28+
$warnings.Count | Should Be 1
29+
}
30+
31+
It "double equals inside if statement causes warning when wrapping it in command expresion" {
32+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == ($b)){}' | Where-Object {$_.RuleName -eq $ruleName}
33+
$warnings.Count | Should Be 1
34+
}
35+
36+
It "double equals inside if statement causes warning when wrapped in expression" {
37+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a == "$b"){}' | Where-Object {$_.RuleName -eq $ruleName}
38+
$warnings.Count | Should Be 1
39+
}
40+
}
41+
42+
Context "When there are no violations" {
43+
It "returns no violations when there is no equality operator" {
44+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){$a=$b}' | Where-Object {$_.RuleName -eq $ruleName}
45+
$warnings.Count | Should Be 0
46+
}
47+
48+
It "returns no violations when there is an evaluation on the RHS" {
49+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = Get-ChildItem){}' | Where-Object {$_.RuleName -eq $ruleName}
50+
$warnings.Count | Should Be 0
51+
}
52+
53+
It "returns no violations when there is an evaluation on the RHS wrapped in an expression" {
54+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){}' | Where-Object {$_.RuleName -eq $ruleName}
55+
$warnings.Count | Should Be 0
56+
}
57+
58+
It "returns no violations when there is an evaluation on the RHS wrapped in an expression and also includes a variable" {
59+
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem $b)){}' | Where-Object {$_.RuleName -eq $ruleName}
60+
$warnings.Count | Should Be 0
61+
}
62+
}
63+
}

0 commit comments

Comments
 (0)