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

Update PossibleIncorrectComparisonWithNull documentation with better example #1244

Merged
merged 3 commits into from
Jun 5, 2019
Merged

Update PossibleIncorrectComparisonWithNull documentation with better example #1244

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adb5089
Update PossibleIncorrectComparisonWithNull documentation with better …
bergmeister May 29, 2019
4f26261
Update RuleDocumentation/PossibleIncorrectComparisonWithNull.md
bergmeister May 30, 2019
1901be3
Add example from Jim
bergmeister May 30, 2019
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
5 changes: 3 additions & 2 deletions RuleDocumentation/PossibleIncorrectComparisonWithNull.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ function Test-CompareWithNull
## Try it Yourself

``` PowerShell
if (@() -eq $null) { 'true' } else { 'false' } # Returns false
if ($null -ne @()) { 'true' } else { 'false' } # Returns true
# Both expressions below return 'false' because the comparison does not return an object and therefore the if statement always falls through:
if (@() -eq $null) { 'true' }else { 'false' }
bergmeister marked this conversation as resolved.
Show resolved Hide resolved
if (@() -ne $null) { 'true' }else { 'false' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a repetition of the line above? I think the existing examples make sense and are illustrative of the difference between a naive comparison with $null and the suggested fix.

It might possibly be worth a more complicated example, but the purpose of the documentation here really isn't to teach people PowerShell concepts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The point of changing the example was that the existing example was too confusing for people because it changed 2 things at once, the example in this PR changes only 1 thing (eq to ne) to demonstrate the behaviour that might be unexpected for some people (yes, I know this is how the comparison operator works but most people do not use it with this intention and rather just want a null checks)

Copy link
Contributor

Choose a reason for hiding this comment

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

Two points there:

  1. Unrelated to the example complexity discussion: it looks like this change just uses the same example twice. Is that intended? It seems unhelpful (and arguably confusing) to have the same example twice. Maybe I've missed something.

  2. I don't think taking an example away prevents confusion. I think adding another simpler example as a stepping stone will be much more helpful. In the original issue this PR addresses, I don't think the issue was "the current examples are confusing" so much as "the current examples don't look like what I have"; @PrzemyslawKlys understood the behaviour of the -eq comparison, but not why he was being warned. So I think the answer is more examples and some discussion about the effects, and what to do if you really do intend null comparison like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At first, yes it looks like it is the same example twice (if one already understands the root cause) but the point of the example is to also show on a high level (if the LHS and RHS were a complex, unknown expression) that changing -eq to -ne does not return in inverting the result, which is basic assumption and thereby shows in what kind of tricky situations one can land

Copy link
Contributor

@rjmholt rjmholt Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I honestly missed the operator change. Without trying to blame my own failing on an extrinsic factor, I suspect others might also miss the important difference.

Maybe it's worth breaking up the comment so there's one for each example, like:

# In PowerShell, `$array -operator $value` works the same as `$array | Where-Object -operator $value` because of pipeline enumeration
# For example:
@(1, 2, 3, 4) -ge 3             # Emits `3, 4`
'a','b','c','ab' -like 'a*'     # Emits 'a','ab'
'hi',$null,'bye' -ne $null      # Emits 'hi','bye'
@() -ne $null`                  # Emits nothing (an empty array)

# Equality comparison doesn't work as often intended, because `@() -ne $null` results in a falsey empty value
if (@() -ne $null) { 'true' } else { 'false' }     # Emits 'false'

# Comparison misleadingly appears to work as intended with -eq, but again because a falsey empty value is emitted (not because the `@()` value is non-null)
if (@() -eq $null) { 'true' } else { 'false' }     # Emits 'false'

# Because of this caveat, PSScriptAnalyzer warns about comparisons with $null by default
# If you want to use a filter behaviour, it's recommended to explicitly filter instead:

# Don't use
if ($array -ne $null) { ... }

# DO use
if ($array | Where-Object -ne $null) { ... }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yee, I agree, maybe it is best to have an additional section with more examples so people can go deep and look at different scenarios and root causes, please open a PR for it since this PR got already merged. It makes me wonder if PowerShell would benefit e.g. from a == operator that would be more intuitive than the -eq operator..

```