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 5 commits
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
41 changes: 32 additions & 9 deletions Rules/PossibleIncorrectUsageOfAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#endif
using System.Management.Automation.Language;
using System.Globalization;
using System.Linq;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
Expand All @@ -42,7 +43,8 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
{
foreach (var clause in ifStatementAst.Clauses)
{
var assignmentStatementAst = clause.Item1.Find(testAst => testAst is AssignmentStatementAst, searchNestedScriptBlocks: false) as AssignmentStatementAst;
var ifStatementCondition = clause.Item1;
var assignmentStatementAst = ifStatementCondition.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#.
Expand All @@ -55,18 +57,39 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
}
else
{
// 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)
var leftVariableExpressionAstOfAssignment = assignmentStatementAst.Left as VariableExpressionAst;
if (leftVariableExpressionAstOfAssignment != null)
{
yield return new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Information, fileName);
var statementBlockOfIfStatenent = clause.Item2;
var variableExpressionAstsInStatementBlockOfIfStatement = statementBlockOfIfStatenent.FindAll(testAst => testAst is VariableExpressionAst, searchNestedScriptBlocks: true);
if (variableExpressionAstsInStatementBlockOfIfStatement == null) // no variable uages
Copy link
Owner Author

@bergmeister bergmeister Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in comment

{
yield return new DiagnosticRecord(
Strings.PossibleIncorrectUsageOfComparisonOperatorAssignmentOperatorError, assignmentStatementAst.ErrorPosition,
GetName(), DiagnosticSeverity.Information, fileName);
}
else
{
var variableOnLHSIsBeingUsed = variableExpressionAstsInStatementBlockOfIfStatement.Where(x => x is VariableExpressionAst)
.Any(x => ((VariableExpressionAst)x).VariablePath.UserPath.Equals(leftVariableExpressionAstOfAssignment.VariablePath.UserPath, StringComparison.OrdinalIgnoreCase));

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);
}
}
}
}
}
}

}
var fileRedirectionAst = clause.Item1.Find(testAst => testAst is FileRedirectionAst, searchNestedScriptBlocks: false) as FileRedirectionAst;
if (fileRedirectionAst != null)
{
Expand Down
5 changes: 5 additions & 0 deletions Tests/Rules/PossibleIncorrectUsageOfComparisonOperator.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ Describe "PossibleIncorrectUsageOfComparisonOperator" {
$warnings.Count | Should Be 0
}

It "returns no violations when the variable assigned 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}
$warnings.Count | Should Be 0
Expand Down