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

Enhance check for assignment by rather checking if assigned variable is used in assignment block #1

Merged
merged 10 commits into from
Feb 17, 2018
Merged

Enhance check for assignment by rather checking if assigned variable is used in assignment block #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a6285b4
prototype of detecting variable usage on LHS to avoid false positives
bergmeister Feb 13, 2018
2ea4546
simplify and reduce nesting
bergmeister Feb 13, 2018
3d33169
fix regression by continue statements
bergmeister Feb 13, 2018
f9fd379
fix last regression
bergmeister Feb 13, 2018
4f7dbe6
add simple test case for new improvemeent
bergmeister Feb 13, 2018
135be11
finish it off and adapt tests
bergmeister Feb 17, 2018
d8666b0
update docs
bergmeister Feb 17, 2018
bd72cc3
minor style update
bergmeister Feb 17, 2018
0269ac0
fix typo
bergmeister Feb 17, 2018
edd37c0
fix test due to warning level change
bergmeister Feb 17, 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
Prev Previous commit
Next Next commit
finish it off and adapt tests
  • Loading branch information
@bergmeister
bergmeister committed Feb 17, 2018
commit 135be1158672a5b2c9a6af99538d1f8b2e99a320
28 changes: 11 additions & 17 deletions Rules/PossibleIncorrectUsageOfAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
// 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 new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Warning, fileName);
yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName);
}
else
{
Expand All @@ -62,11 +60,9 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
var statementBlockOfIfStatenent = clause.Item2;
var variableExpressionAstsInStatementBlockOfIfStatement = statementBlockOfIfStatenent.FindAll(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: true);
if (variableExpressionAstsInStatementBlockOfIfStatement == null) // no variable uages
if (variableExpressionAstsInStatementBlockOfIfStatement == null) // no variable uages at all
{
yield return new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Information, fileName);
yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName);
}
else
{
Expand All @@ -75,21 +71,14 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)

if (!variableOnLHSIsBeingUsed)
{
// If the right hand side 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 (commandAst == null)
{
yield return new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Information, fileName);
}
yield return PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError(assignmentStatementAst.ErrorPosition, fileName);
}
}
}
}

}

var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst;
if (fileRedirectionAst != null)
{
Expand All @@ -101,6 +90,11 @@ 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 @@ -142,7 +136,7 @@ public SourceType GetSourceType()
/// <returns></returns>
public RuleSeverity GetSeverity()
{
return RuleSeverity.Information;
return RuleSeverity.Warning;
}

/// <summary>
Expand Down
17 changes: 6 additions & 11 deletions Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,27 +61,22 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" {

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

It "returns no violations when the variable assigned on the LHS is used" {
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}
$warnings.Count | Should Be 0
}

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

It "returns no violations when there is an evaluation 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
}

It "returns no violations when there is an evaluation on the RHS wrapped in an expression and also includes a variable" {
$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a = (Get-ChildItem $b)){}' | Where-Object {$_.RuleName -eq $ruleName}
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}
$warnings.Count | Should Be 0
}
}
Expand Down