Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn pipeline inside if statement3 #2

Merged
merged 4 commits into from
Feb 25, 2018
Merged

Warn pipeline inside if statement3 #2

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d39e7cc
revert rule to not do check of variable usage but improve it to not w…
bergmeister Feb 25, 2018
c3fdef4
implement clang and more tweaks
bergmeister Feb 25, 2018
33a19ec
final tweaks and add docs
bergmeister Feb 25, 2018
00f520a
fix rule name in doc
bergmeister Feb 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
revert rule to not do check of variable usage but improve it to not w… 
…arn about certain operators on the RHS
  • Loading branch information
@bergmeister
bergmeister committed Feb 25, 2018
commit d39e7ccaa6879a2317ea6630c5a1ecee0e044db4
47 changes: 16 additions & 31 deletions Rules/PossibleIncorrectUsageOfAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#endif
using System.Management.Automation.Language;
using System.Globalization;
using System.Linq;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
Expand All @@ -43,42 +42,33 @@ 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;
var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst;
if (assignmentStatementAst != null)
{
// 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.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, 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.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Information, fileName);
}
}

}

var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst;
Expand All @@ -92,11 +82,6 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string 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>
Expand Down Expand Up @@ -138,7 +123,7 @@ public SourceType GetSourceType()
/// <returns></returns>
public RuleSeverity GetSeverity()
{
return RuleSeverity.Warning;
return RuleSeverity.Information;
}

/// <summary>
Expand Down
24 changes: 17 additions & 7 deletions Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,33 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" {
}

Context "When there are no violations" {
It "returns no violations when there is no equality operator" {
It "returns no violations when correct equality operator is used" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a -eq $b){ }' | Where-Object {$_.RuleName -eq $ruleName}
$warnings.Count | Should -Be 0
}

It "returns no violations when using assignment but the assigned variable on the LHS is used" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $b){ $a.DoSomething() }' | Where-Object {$_.RuleName -eq $ruleName}
It "returns no violations when using an InvokeMemberExpressionAst like a .net method on the RHS" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = [System.IO.Path]::GetTempFileName()){ }' | Where-Object {$_.RuleName -eq $ruleName}
$warnings.Count | Should -Be 0
}

It "returns no violations when there is an evaluation on the RHS but the assigned variable on the LHS is used" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = Get-ChildItem){ Get-Something $a }' | Where-Object {$_.RuleName -eq $ruleName}
It "returns no violations when there is an InvokeMemberExpressionAst on the RHS that looks like a variable" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $PSCmdlet.GetVariableValue($foo){ }' | Where-Object {$_.RuleName -eq $ruleName}
$warnings.Count | Should -Be 0
}

It "returns no violations when there is an evaluation on the RHS wrapped in an expression but the assigned variable on the LHS is used" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){ $b = $a }' | Where-Object {$_.RuleName -eq $ruleName}
It "returns no violations when using an expression like a Binaryexpressionastast on the RHS" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = $b -match $c){ }' | Where-Object {$_.RuleName -eq $ruleName}
$warnings.Count | Should -Be 0
}

It "returns no violations when there is a command on the RHS" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = Get-ChildItem){ }' | Where-Object {$_.RuleName -eq $ruleName}
$warnings.Count | Should -Be 0
}

It "returns no violations when there is a command on the RHS wrapped in an expression" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem)){ }' | Where-Object {$_.RuleName -eq $ruleName}
$warnings.Count | Should -Be 0
}
}
Expand Down