Skip to content

Commit

Permalink
Revert to not check assigned variable usage of RHS but add optional c…
Browse files Browse the repository at this point in the history 
…lang suppression, split rule and enhance assignment operator rule to not warn for more special cases on the RHS
  • Loading branch information
@bergmeister
bergmeister committed Feb 25, 2018
1 parent b467d10 commit 594414f
Show file tree
Hide file tree
Showing 11 changed files with 361 additions and 149 deletions.
54 changes: 54 additions & 0 deletions RuleDocumentation/PossibleIncorrectUsageOAssignmentOperator.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# PossibleIncorrectUsageOfAssignmentOperator

**Severity Level: Information**

## Description

In many programming languages, the equality operator is denoted as `==` or `=` in many programming languages, but `PowerShell` uses `-eq`. Therefore it can easily happen that the wrong operator is used unintentionally and this rule catches a few special cases where the likelihood of that is quite high.

The rule looks for usages of `==` and `=` operators inside if statements but it will not warn if any kind of command or expression is used at the right hand side as this is probably by design.

## Example

### Wrong

```` PowerShell
if ($a = $b)
{
...
}
````

```` PowerShell
if ($a == $b)
{
}
````

### Correct

```` PowerShell
if ($a -eq $b) # Compare $a with $b
{
...
}
````

```` PowerShell
if ($a = Get-Something) # Only execute action if command returns something and assign result to variable
{
Do-SomethingWith $a
}
````

## Implicit suppresion using Clang style

There are some rare cases where assignment of variable inside an if statement is by design. Instead of suppression the rule, one can also signal that assignment was intentional by wrapping the expression in extra parenthesis:

```` powershell
if (($shortVariableName = $SuperLongVariableName['SpecialItem']['AnotherItem']))
{
...
}
````
43 changes: 0 additions & 43 deletions RuleDocumentation/PossibleIncorrectUsageOfComparisonOperator.md
View file

This file was deleted.

29 changes: 29 additions & 0 deletions RuleDocumentation/PossibleIncorrectUsageOfRedirectionOperator.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# PossibleIncorrectUsageOfRedirectionOperator

**Severity Level: Information**

## Description

In many programming languages, the comparison operator for 'greater than' is `>` but `PowerShell` uses `-gt` for it and `-ge` (greater or equal) for `>=`. Therefore it can easily happen that the wrong operator is used unintentionally and this rule catches a few special cases where the likelihood of that is quite high.

The rule looks for usages of `>` or `>=` operators inside if statements because this is likely going to be unintentional usage.

## Example

### Wrong

```` PowerShell
if ($a > $b)
{
...
}
````

### Correct

```` PowerShell
if ($a -eq $b)
{
...
}
````
24 changes: 24 additions & 0 deletions Rules/ClangSuppresion.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Management.Automation.Language;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
{
/// <summary>
/// The idea behind clang suppresion style is to wrap a statement in extra parenthesis to implicitly suppress warnings of its content to signal that the offending operation was deliberate.
/// </summary>
internal static class ClangSuppresion
{
/// <summary>
/// The community requested an implicit suppression mechanism that follows the clang style where warnings are not issued if the expression is wrapped in extra parenthesis
/// See here for details: https://github.com/Microsoft/clang/blob/349091162fcf2211a2e55cf81db934978e1c4f0c/test/SemaCXX/warn-assignment-condition.cpp#L15-L18
/// </summary>
/// <param name="scriptExtent"></param>
/// <returns></returns>
internal static bool ScriptExtendIsWrappedInParenthesis(IScriptExtent scriptExtent)
{
return scriptExtent.Text.StartsWith("(") && scriptExtent.Text.EndsWith(")");
}
}
}
80 changes: 24 additions & 56 deletions Rules/PossibleIncorrectUsageOfAssignmentOperator.cs
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
//
// Copyright (c) Microsoft Corporation.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
using System;
Expand All @@ -18,7 +9,6 @@
#endif
using System.Management.Automation.Language;
using System.Globalization;
using System.Linq;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
Expand All @@ -27,9 +17,9 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
/// The origin of this rule is that people often forget that operators change when switching between different languages such as C# and PowerShell.
/// </summary>
#if !CORECLR
[Export(typeof(IScriptRule))]
[Export(typeof(IScriptRule))]
#endif
public class PossibleIncorrectUsageOfComparisonOperator : AstVisitor, IScriptRule
public class PossibleIncorrectUsageOfAssignmentOperator : AstVisitor, IScriptRule
{
/// <summary>
/// The idea is to get all AssignmentStatementAsts and then check if the parent is an IfStatementAst, which includes if, elseif and else statements.
Expand All @@ -43,67 +33,45 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
foreach (var clause in ifStatementAst.Clauses)
{
var ifStatementCondition = clause.Item1;
var assignmentStatementAst = ifStatementCondition.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst;
if (assignmentStatementAst != null)
var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst;
if (assignmentStatementAst != null && !ClangSuppresion.ScriptExtendIsWrappedInParenthesis(assignmentStatementAst.Extent))
{
// Check if someone used '==', which can easily happen when the person is used to coding a lot in C#.
// 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
if (assignmentStatementAst.Right.Extent.Text.StartsWith("="))
{
yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName);
yield return new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Warning, fileName);
}
else
{
var leftVariableExpressionAstOfAssignment = assignmentStatementAst.Left as VariableExpressionAst;
if (leftVariableExpressionAstOfAssignment != null)
{
var statementBlockOfIfStatenent = clause.Item2;
var variableExpressionAstsInStatementBlockOfIfStatement = statementBlockOfIfStatenent.FindAll(testAst =>
testAst is VariableExpressionAst, searchNestedScriptBlocks: true);
if (variableExpressionAstsInStatementBlockOfIfStatement == null) // no variable usages at all
{
yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName);
}
else
{
var variableOnLHSIsBeingUsed = variableExpressionAstsInStatementBlockOfIfStatement.Where(x => x is VariableExpressionAst)
.Any(x => ((VariableExpressionAst)x).VariablePath.UserPath.Equals(
leftVariableExpressionAstOfAssignment.VariablePath.UserPath, StringComparison.OrdinalIgnoreCase));
// If the RHS contains a CommandAst at some point, then we do not want to warn because this could be intentional in cases like 'if ($a = Get-ChildItem){ }'
var commandAst = assignmentStatementAst.Right.Find(testAst => testAst is CommandAst, searchNestedScriptBlocks: true) as CommandAst;
// If the RHS contains an InvokeMemberExpressionAst, then we also do not want to warn because this could e.g. be 'if ($f = [System.IO.Path]::GetTempFileName()){ }'
var invokeMemberExpressionAst = assignmentStatementAst.Right.Find(testAst => testAst is ExpressionAst, searchNestedScriptBlocks: true) as InvokeMemberExpressionAst;
// If the RHS contains a BinaryExpressionAst, then we also do not want to warn because this could e.g. be 'if ($a = $b -match $c){ }'
var binaryExpressionAst = assignmentStatementAst.Right.Find(testAst => testAst is BinaryExpressionAst, searchNestedScriptBlocks: true) as BinaryExpressionAst;

if (!variableOnLHSIsBeingUsed)
{
yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName);
}
}
if (commandAst == null && invokeMemberExpressionAst == null && binaryExpressionAst == null)
{
yield return new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfAssignmentOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Information, fileName);
}
}

}

var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst;
if (fileRedirectionAst != null)
{
yield return new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfComparisonOperatorFileRedirectionOperatorError, fileRedirectionAst.Extent,
GetName(), DiagnosticSeverity.Warning, fileName);
}
}
}
}

private DiagnosticRecord PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(IScriptExtent errorPosition, string fileName)
{
return new DiagnosticRecord(Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, errorPosition, GetName(), DiagnosticSeverity.Warning, fileName);
}

/// <summary>
/// GetName: Retrieves the name of this rule.
/// </summary>
/// <returns>The name of this rule</returns>
public string GetName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfComparisonOperatorName);
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.PossibleIncorrectUsageOfAssignmentOperatorName);
}

/// <summary>
Expand All @@ -112,7 +80,7 @@ public string GetName()
/// <returns>The common name of this rule</returns>
public string GetCommonName()
{
return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorCommonName);
return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorCommonName);
}

/// <summary>
Expand All @@ -121,7 +89,7 @@ public string GetCommonName()
/// <returns>The description of this rule</returns>
public string GetDescription()
{
return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfComparisonOperatorDescription);
return string.Format(CultureInfo.CurrentCulture, Strings.PossibleIncorrectUsageOfAssignmentOperatorDescription);
}

/// <summary>
Expand All @@ -138,7 +106,7 @@ public SourceType GetSourceType()
/// <returns></returns>
public RuleSeverity GetSeverity()
{
return RuleSeverity.Warning;
return RuleSeverity.Information;
}

/// <summary>
Expand Down
Loading

0 comments on commit 594414f

Please sign in to comment.