Warn when using FileRedirection operator inside if/while statements and improve new PossibleIncorrectUsageOfAssignmentOperator rule #881
Conversation
…perator inside if statements. TODO: rename rule, adapt documentation and error message strings
| @@ -0,0 +1,7 @@ | |||
| # PossibleIncorrectUsageOfComparisonOperator | |||
|
|
|||
| **Severity Level: Information** | |||
There was a problem hiding this comment.
I was wondering because we can determine pretty accurately know that the usage of the comparison operator inside if statements is unintentional, if we should up the level to warning because some people ignore/disable warnings of informational level?
|
About the renaming of the rule,I think there are two different cases. This causes parsing errors: if ($a > = $b){ "ok"} else { "nok"}And this is valid: if ($a >=){ "ok"} else { "nok"}This rule can also be applied to 'While' and 'Do' statements. For the level of the rule, it depends on the level of knowledge of the person. For a beginner the level 'Warning' would be useful and the level 'Information' would be too risky if it configures the triggering of the rules on 'Warning'. And it is easier with PSSA to exlude a rule than to configure its level of criticality. "Since assignment inside if statements are very rare," Test with a module triggering 38 times this rule: $r=Invoke-ScriptAnalyzer -path 'C:\Program Files\WindowsPowerShell\Modules\InvokeBuild\5.1.0'|
where-object rulename -eq 'PSPossibleIncorrectUsageOfAssignmentOperator'
#------
$_ = ${*}.Data['Checkpoint.Import']
$r = ${*checkpoint}.Result
$file = $b['File']
$log = $b['Log']
$up = $PSCmdlet.SessionState.PSVariable.Get('*')
$f = [System.IO.Directory]::GetFiles($Path, '*.build.ps1')
$_ = $f -match '[\\/]\.build\.ps1$'
$_ = $env:InvokeBuildGetFile
$_ = $PSCmdlet.SessionState.PSVariable.Get('*')
$_ = $PSBoundParameters['Result']
$_ = $p.Name
$_ = ${*}.All[$Name]
$_ = ${*}.All[$Task]
$_ = $PSCmdlet.GetVariableValue(${*n})
$d = $Hash[$f]
$c = $d.C[$n]
$_ = $PSBoundParameters['Variable']
$$ = $PSCmdlet.GetVariableValue($_)
$_ = $PSBoundParameters['Environment']
$_ = $PSBoundParameters['Property']
$t = ${*}.All[$r]
$t = ${*}.All[$r]
$r = ${*}.All[$_]
${private:*i} = $Task.Inputs
${private:*x} = $Task.If
${*}.Q = $BuildTask -eq '?' -or $BuildTask -eq '??'
${**} = ${*}.All
$_ = $_.Error
$w = ${*}.Warnings
$_ = ${*}.P
$root = ${env:ProgramFiles(x86)}
$r = $paths -like '*\Enterprise\*'
$r = $paths -like '*\Professional\*'
$r = $paths -like '*\Community\*'
$r = $r.parameters
$r = $r.parameter
$d = $Hash[$f]
$c = $d.C[$n] |
|
while |
The rule is for cases where the user simply forgets the right PowerShell syntax (due to context switching or because he/she is a newbie) and therefore I do not understand why someone would enable/disable only one of them if the rule were to be split into 2 rules. Can you please give an example/scenario where a split would be needed? Parsing errors are OK and by design (they come from the PowerShell parses itself) and are a good signal to raise an early red flag because if a script does not parse then there is no point running the script at all and the error messages of the parses are good as well and even point to the right line where the problem happens. There is no point to catch parsing errors and do our own logic, we would be re-inventing the wheel.
The implementation of the rule is clever enough to not warn in those special cases scenarios where assignment is intentional.
That's a good point. We should consider implementing the rule for those statements as well. Your 38 examples do not trigger the rule for me (I even tried putting some of the lines in an if statement), but they do trigger |
|
@bergmeister if ($x = <something>) {When I deliver code to a customer, I prefer it for maintenance / knowledge reasons : $x = <something>
if ($x) {For that I would like to have this rule with a level 'warning'. For this case : if ($a > $b){ "ok"} else { "nok"}I assume that the majority of people will consider this case as being of the warning level. This is what makes me think that splitting these cases into two rules,the both with a 'warning' level , with could satisfy as many people as possible : Invoke-ScriptAnalyzer -ExcludeRule 'PSPossibleIncorrectUsageOfAssignmentOperator' -IncludeRule 'PSPossibleIncorrectUsageOfRedirectionOperator'
Invoke-ScriptAnalyzer -IncludeRule 'PSPossibleIncorrectUsageOfAssignmentOperator','PSPossibleIncorrectUsageOfRedirectionOperator'
#or
Invoke-ScriptAnalyzer #default There is no point to catch parsing errors and do our own logic, we would be re-inventing the wheel. Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a > = $b){ "ok"} else { "nok"}'
# Invoke-ScriptAnalyzer : Parse error in script definition: Unexpected token '$b' in expression or statement at line 1
# column 12.
# At line:1 char:1
# + Invoke-ScriptAnalyzer -ScriptDefinition 'if ($a > = $b){ "ok"} else { ...
# + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# + CategoryInfo : ParserError: (UnexpectedToken:String) [Invoke-ScriptAnalyzer], ParseException
# + FullyQualifiedErrorId : Parse error in script definition: Unexpected token '$b' in expression or statement at l
# ine 1 column 12.,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommandWhat I wanted to say : it is not necessary to put this case in the tests because PSSA is waiting for a syntactically correct script. Your 38 examples do not trigger the rule for me ipmo PSScriptAnalyzer
$r=Invoke-ScriptAnalyzer -path 'C:\Program Files\WindowsPowerShell\Modules\InvokeBuild\5.1.0'|
where rulename -eq 'PSPossibleIncorrectUsageOfAssignmentOperator'
$r.count
#38
$r[0]|select *
#
# Line : 59
# Column : 7
# Message : Did you really mean to make an assignment inside an if statement? If you rather meant to check
# for equality, use the '-eq' operator.
# Extent : $_ = ${*}.Data['Checkpoint.Import']
# RuleName : PSPossibleIncorrectUsageOfAssignmentOperator
# Severity : Information
# ScriptName : Build-Checkpoint.ps1
# ScriptPath : C:\Program Files\WindowsPowerShell\Modules\InvokeBuild\5.1.0\Build-Checkpoint.ps1
# RuleSuppressionID :
# SuggestedCorrections : |
|
@LaurentDardenne I think you are missing a detail: The legitimate case Import-Module : Could not load file or assembly 'file:///C:\Users\cberg\Downloads\psscriptanalyzer\out\PSScriptAnalyzer
\Microsoft.Windows.PowerShell.ScriptAnalyzer.dll' or one of its dependencies. Operation is not supported. (Exception
from HRESULT: 0x80131515)
At C:\Users\cberg\Downloads\psscriptanalyzer\out\PSScriptAnalyzer\PSScriptAnalyzer.psm1:24 char:17
+ $binaryModule = Import-Module -Name $binaryModulePath -PassThru
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (:) [Import-Module], FileLoadException
+ FullyQualifiedErrorId : System.IO.FileLoadException,Microsoft.PowerShell.Commands.ImportModuleCommand |
|
... this is why I don't see the need for the split... How are you building/using the PSSA version of my branch? cd C:\temp\pssa\out\PSScriptAnalyzer
ipmo .\PSScriptAnalyzer.psd1
Invoke-ScriptAnalyzer -ScriptDefinition 'if ($root = ${env:ProgramFiles(x86)}) {}'
# RuleName Severity ScriptName Line Message
# -------- -------- ---------- ---- -------
# PSPossibleIncorrectUsageOfCompariso Information 1 Did you really mean to use the assignment operator
# nOperator '=' inside an if statement? If you rather meant to
# check for equality, use the '-eq' operator.
# PSUseDeclaredVarsMoreThanAssignment Warning 1 The variable 'root' is assigned but never used.
# s
Invoke-ScriptAnalyzer -ScriptDefinition 'if ($root = get-nothing) {}'
# RuleName Severity ScriptName Line Message
# -------- -------- ---------- ---- -------
# PSUseDeclaredVarsMoreThanAssignment Warning 1 The variable 'root' is assigned but never used.
# s
Invoke-ScriptAnalyzer -ScriptDefinition 'if ($root > $b) {}'
# RuleName Severity ScriptName Line Message
# -------- -------- ---------- ---- -------
# PSPossibleIncorrectUsageOfCompariso Warning 1 Did you really mean to use the redirection operator
# nOperator '>'? If you wanted to use an equality comparer then
# use the '-gt' (greater than) or '-ge' (greater or
# equal) operators.
Invoke-ScriptAnalyzer -ScriptDefinition 'if ($root > get-nothing) {}'
# RuleName Severity ScriptName Line Message
# -------- -------- ---------- ---- -------
# PSPossibleIncorrectUsageOfCompariso Warning 1 Did you really mean to use the redirection operator
# nOperator '>'? ...
Invoke-ScriptAnalyzer -ScriptDefinition 'if (get-nothing > $root) {}'
#
# RuleName Severity ScriptName Line Message
# -------- -------- ---------- ---- -------
# PSPossibleIncorrectUsageOfAssignmen Warning 1 Did you really mean to use the redirection operator
# tOperator '>'? ... |
…t to the equal sign (instead of the RHS only) and fore file redirections it will be the redirection ast (e.g. '> $b')
|
@LaurentDardenne Thanks for clarifying the repro, very helpful, I can repro now and will take a look at them. |
bergmeister
changed the title
|
One last remark on the presence of two rules. $sb={
if ($root > $b) {}
if ($root = ${env:ProgramFiles(x86)}) {}
}I have to filter this way : Invoke-ScriptAnalyzer -ScriptDefinition "$sb"|
Where {($_.RuleName -eq 'PSPossibleIncorrectUsageOfComparisonOperator') -and ($_.Severity -ne 'Information')}
# RuleName Severity ScriptName Line Message
# -------- -------- ---------- ---- -------
# PSPossibleIncorrectUsageOfCompariso Warning 2 Did you really mean to use the redirection operator
# nOperator '>'? If you wanted to use an equality comparer thenUntil now this was enough : Invoke-ScriptAnalyzer -ScriptDefinition "$sb" -ExcludeRule 'PSPossibleIncorrectUsageOf_AssignmentOperator'Finally the deletion of the application of the rule, for one of the two cases, by the attribute SuppressMessage is not possible: $sb={
function Test{
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSPossibleIncorrectUsageOfComparisonOperator', '')]
[CmdletBinding()]
param ()
if ($root > $b) {}
if ($root = ${env:ProgramFiles(x86)}) {}
}
}
Invoke-ScriptAnalyzer -ScriptDefinition "$sb"
#nothing |
|
@LaurentDardenne I think I slowly start to understand your point of view and can see now how a split into 2 rules would be beneficial.
|
|
@bergmeister Can you give an example usage where using the > operator is intentional AND useful? But maybe this for the assignment can occur : $Resgen="$psScriptRoot\ResGen.exe"
if ($ResultExec=.$Resgen $Source $Destination 2>&1) {}It's not written like that in the original code and I wonder if this writing has an interest ... What do you think about this approach of avoiding false positives? if ( $files = get-childitem )@JamesWTruher consider that "There is a pattern which is very useful which will cause warnings to appear" if ( $A = $B )is also a very useful pattern (see InvokeBuilder module) and others that it will always be an error. Consider a trigger level configuration could cover these cases :
So we are no longer concerned about the intention it is the user who declares it. |
…is used in assignment block to avoid false positives for assignments: check if variable on LHS is used in statement block update documentation and change level to warning since we are now much more certain that a violation. is happening Commits: * prototype of detecting variable usage on LHS to avoid false positives * simplify and reduce nesting * fix regression by continue statements * fix last regression * add simple test case for new improvemeent * finish it off and adapt tests * update docs * minor style update * fix typo * fix test due to warning level change
bergmeister
changed the title
|
To avoid false positives for assignments I refactored the rule to check if the variable on LHS is used in statement block and if not, then warn. This should then not leave any false positive cases, therefore there is no need for a rule split any more and the level was bumped to be a warning. @LaurentDardenne Let me know if this works well for you now as well. |
|
@bergmeister |
…nalyzer into WarnPipelineInsideIfStatement
|
The highlighting of the $w=Invoke-ScriptAnalyzer -ScriptDefinition 'if($a > $b){}'
$w.Extent.Text
> $b
$w=Invoke-ScriptAnalyzer -ScriptDefinition 'if($a > $b){}'
$w.Extent.Text
=
$w=Invoke-ScriptAnalyzer -ScriptDefinition 'if($a == $b){}'
$w.Extent.Text
==I have already added examples to the markdown file. |
Is the exception of -match because it sometimes returns a boolean and sometimes it returns an array? If so, what about operators that are purely arithmetic and operators that are purely boolean? I think those lines should be flagged. |
|
@imfrancisd The current exceptions to
|
…ance that it is by design is much smaller, therefore rather having some false positives is preferred
|
I do not find this use case in the test file : Invoke-ScriptAnalyzer -ScriptDefinition 'if ( ($a=$b) ){}' |
bergmeister
changed the title
|
Thanks @LaurentDardenne for spotting that, I fixed it. It seems I was unlucky to introduce a regression both in the implementation and test when doing the final refactoring. |
bergmeister
changed the title
|
@bergmeister |
|
Should not the second example fail, even if we use () ? Invoke-ScriptAnalyzer -ScriptDefinition 'if ($_ == $_.Error) {}'
# RuleName Severity ScriptName Line Message
# -------- -------- ---------- ---- -------
# PSPossibleIncorrectUsageOfAssignmen Warning 1 Did you mean to use the assignment operator '='? The
# tOperator equals operator in PowerShell is 'eq'.
Invoke-ScriptAnalyzer -ScriptDefinition 'if (($_ == $_.Error)) {}'
#no warningtypo in the error message : |
|
@LaurentDardenne The clang style extra parenthesis are a way of letting the author state implicitly that assignment is deliberate and by design for |
|
@bergmeister What do you think about "if ($null = )"? Shouldn't they always be flagged? |
|
@imfrancisd The rule currently does not inspect the LHS of the assignment, only the RHS. But it would be a good improvement because a common (recommended pattern) is to do comparison via |
I didn't know I had a say in the matter! :-) Not my place to say. I just wander around GitHub issues from time to time and one day found myself here in pull request land. |
…e analysis for while and do-while statements as well, which is the exact same use case as if statements
|
@JamesWTruher After community feedback I addressed it and implemented additional enhancements for special cases. To me the rule looks very polished now. Can you please re-review and give your opinion? |
bergmeister
changed the title
| ```` | ||
|
|
||
| ```` PowerShell | ||
| if ($a = Get-Something) # Only execute action if command returns something and assign result to variable |
There was a problem hiding this comment.
Still even in this case we might want to only compare the variable with the function result. Shouldn't we give some warning message as well?
There was a problem hiding this comment.
I know what you mean but it was a strong desire of the community to not see warnings when assignment is by design as this is a common use case. See referenced issue and there was also a discussion on the testing channel in Slack. I can sort of understand this fear/annoyance of people about false positives, especially given that one cannot suppress on a per-line basis in PSSA. I guess it is better to warn in most cases and help save the developer time rather than always warning and annoying the developer.
| ### Correct | ||
|
|
||
| ```` PowerShell | ||
| if ($a -eq $b) |
There was a problem hiding this comment.
I think it should be -gt here?
There was a problem hiding this comment.
Yes, this makes more sense to someone reading the document and comparing wrong vs right. I will change that.
Rules/ClangSuppresion.cs
Outdated
| 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 |
There was a problem hiding this comment.
Please finish the sentence with '.'
| namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules | ||
| { | ||
| /// <summary> | ||
| /// PossibleIncorrectUsageOfRedirectionOperator: Warn if someone uses '>' or '>=' inside an if or elseif statement because in most cases that is not the intention. |
There was a problem hiding this comment.
Don't we want it to work as well for While statements as PossibleIncorrectUsageOfAssignmentOperator ?
There was a problem hiding this comment.
We actually do, but the comment got out of date. I will fix that. I am following the current comment style but my feeling is that the codebase has too many repeated comments.
…nalyzer into WarnPipelineInsideIfStatement
…nalyzer into WarnPipelineInsideIfStatement resolved by taking both changes # Conflicts: # Rules/Strings.resx
…nalyzer into WarnPipelineInsideIfStatement resolve by taking head # Conflicts: # Tests/Rules/PossibleIncorrectUsageOfAssignmentOperator.tests.ps1
PR Summary
Closes #878
[NameSpace]::Method()or$b -match $con the RHS of the if expression and add optional, implicit clang suppression when extra parenthesis are used.PossibleIncorrectUsageOfRedirectionOperatorrule to warn when using>unintentionally insideifandelseifstatements (e.gif ($a > $){ }).PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready