Multiple improvements by CodeRush Static Analysis by Himura2la · Pull Request #5132 · PowerShell/PowerShell

Himura2la · 2017-10-16T14:37:36Z

Hi guys!
I have checked the PowerShell solution with the CodeRush Static Analyzer and fixed several code issues. Here's the summary:

The most common issue is CRR0012: Logical OR expression has opposite operand, such expressions can be simplified.
In Words.cs our analyzers detect CRR0025: This conditional's code block does not appear to be reachable, it's obviously false positive, but these searching while loops caught my attention. They look very similar to nested conditionals and I decided to fold them into for loops without bodies, they look clearer in this form.
Several places had CRR0008: If-block matches else-block, I saved the original conditionals in comments in case they need in future
I have found the CRR0029: The ConfigureAwait(true) is called implicitly issue. The simple delay does not require saving the synchronization context, plus it's better always add an explicit ConfigureAwait() call to awaited calls.
In WSMan I found CRR0017: Expression checks 'nodelist' for null after usage and one more small issue, maybe I should add another pull request for that repository.

I hope this will help, here's the full report: CodeIssuesReport.txt. Thanks for being open source!

msftclas · 2017-10-16T14:37:44Z

All CLA requirements met.

PaulHigin

Thanks for finding and fixing these!

PaulHigin · 2017-10-16T17:48:56Z

src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/Reader/EventLogReader.cs

                    else
                    {
-                        //if inside the current buffer, we still cannot read the events, as the handles.
+                        //if inside the current buffer (_currentIndex + offset >= 0), we still cannot read the events, as the handles.


This comment still doesn't make sense to me. Maybe just remove it?

OK, removing the whole comment

PaulHigin · 2017-10-16T18:05:07Z

src/Microsoft.PowerShell.PSReadLine/Words.cs

-                }
-                i += 1;
-            }
+            for (; i < _singleton._buffer.Length && !InWord(i, wordDelimiters); i++) ;


I feel this is easier to read (with the additional parentheses):
for (; (i < _singleton._buffer.Length) && !InWord(I, wordDelimiters); I++);

Submit a PR to https://github.com/lzybkr/PSReadLine for any changes in the PSReadLine directory.

And I'll likely reject the change - In general I'm not a fan of empty loop bodies.

I reverted the file, this change is a anyway just a preference. Maybe I'll try to push it to the correct repository, thanks for the link.

PaulHigin · 2017-10-16T18:16:26Z

src/System.Management.Automation/engine/DataStoreAdapter.cs

-                    // Since drive1 is null it is less than drive2 which is not null
-                    return false;
-                }
+                // Since both drives are null, they are equal


This doesn't look right. I believe it should be.
return (drive2Object != null);

Oh, thanks, I didn't notice that the refactoring can go even further

PaulHigin · 2017-10-16T18:18:57Z

src/System.Management.Automation/engine/DataStoreAdapterProvider.cs

-            {
-                throw PSTraceSource.NewArgumentException("name");
-            }
+            // FIXME: Other checks?


Please remove this comment and verify that helpfile, description, psSnapIn parameters can be null.

Sorry, I think I didn't catch whether I should ensure the helpfile, description and psSnapIn are not null or just remove the comment.

@Himura2la Can you either remove this comment or file an issue and update the comment to reference the issue?

PaulHigin · 2017-10-16T18:22:31Z

src/System.Management.Automation/engine/Modules/AnalysisCache.cs

                    // writing the cache out in a timely matter isn't too important
                    // now anyway.
-                    await Task.Delay(10000);
+                    await Task.Delay(10000).ConfigureAwait(false);


It is not clear to me that this is needed. Isn't the Task always configured as "false" be default?

It seems right though, so I think it's good to keep it.

@PaulHigin As far as I know, the default option is 'true'. And that's the reason for adding an explicit ConfigureAwait() everywhere.

PaulHigin · 2017-10-16T18:38:19Z

src/System.Management.Automation/security/Authenticode.cs

+            Diagnostics.Assert(error != 0 || signature != null, "GetSignatureFromWintrustData: general crypto failure");

-            if ((signature == null) && (error != 0))
+            if (signature == null && error != 0)


I feel the code is more readable with parentheses back in. I am not sure what our coding guidelines say.

In this area, coding guidelines say - be consistent, but we also ask for no formatting changes mixed with other changes.

In reply to: 144931951 [](ancestors = 144931951)

I reverted this.
Sorry, I should have kept myself from removing parentheses everywhere. I think I write too much Python

lzybkr · 2017-10-16T19:09:07Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs

                            {
-                                Dbg.Assert((inputRecords[0].KeyEvent.KeyDown && inputRecords[0].KeyEvent.RepeatCount != 0) ||
-                                    !inputRecords[0].KeyEvent.KeyDown,
+                                Dbg.Assert(inputRecords[0].KeyEvent.RepeatCount != 0 || !inputRecords[0].KeyEvent.KeyDown,


This changes the order of evaluation - probably safe here, but before, RepeatCount would not be evaluated if KeyDown was false.

I swapped them, thanks for noticing. Although it seems really safe assuming there's even no null-checks

lzybkr · 2017-10-16T19:09:35Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs

                    {
-                        Dbg.Assert((inputRecords[i].KeyEvent.KeyDown && inputRecords[i].KeyEvent.RepeatCount != 0) ||
-                            !inputRecords[i].KeyEvent.KeyDown,
+                        Dbg.Assert(inputRecords[i].KeyEvent.RepeatCount != 0 || !inputRecords[i].KeyEvent.KeyDown,


This changes the order of evaluation - probably safe here, but before, RepeatCount would not be evaluated if KeyDown was false.

lzybkr · 2017-10-16T19:16:17Z

src/Microsoft.WSMan.Management/WsManHelper.cs

-            }
+            //if (!(resUri.ToLowerInvariant().Contains(URI_IPMI) || resUri.ToLowerInvariant().Contains(URI_WMI)))
+            //    tmpNs += ".xsd";
+            //This was reported by Intel as an interop issue. So now we are not appending a .xsd in the end.


This comment can probably be deleted.

OK, I also inlined the tmpNs variable. I hope it's OK

lzybkr · 2017-10-16T19:46:32Z

src/System.Management.Automation/engine/remoting/client/remotepipeline.cs

            RemotePipeline currentPipeline = (RemotePipeline)((RemoteRunspace)_runspace).GetCurrentlyRunningPipeline();
-            if (currentPipeline == null ||
-                currentPipeline != null && !ReferenceEquals(currentPipeline, this))
+            if (currentPipeline == null || !ReferenceEquals(currentPipeline, this))


I think currentPipeline == null is redundant with ReferenceEquals here.

Seems like.

TravisEz13 · 2017-10-16T22:37:42Z

@Himura2la What is your status on signing the CLA?

Himura2la · 2017-10-17T12:09:37Z

I'm done with the changes and CLA. The only thing left is extra checks here

TravisEz13 · 2017-10-17T17:58:18Z

@PaulHigin Just a reminder to update your review.

Restarted macOS due to random brew download failure.

PaulHigin

LGTM

TravisEz13 · 2017-10-18T17:42:39Z

src/System.Management.Automation/engine/DataStoreAdapterProvider.cs

-            {
-                throw PSTraceSource.NewArgumentException("name");
-            }
+            // FIXME: Other checks?


@Himura2la Can you either remove this comment or file an issue and update the comment to reference the issue?

TravisEz13 · 2017-10-18T17:52:47Z

I'm following up on one issue. Hopefully there will be no more delays once I have the answer.

Himura2la · 2017-10-18T18:14:58Z

@TravisEz13, I've removed the comment in my second commit

TravisEz13 · 2017-10-18T19:10:17Z

@Himura2la Yeah, that is not the issue.

iSazonov · 2017-11-08T12:47:05Z

Is the PR ready to merge?

Himura2la · 2017-11-08T15:58:46Z

I wonder

SteveL-MSFT · 2017-11-08T18:06:02Z

@TravisEz13 is this ready to merge?

Multiple improvements by CodeRush Static Analysis

b829325

Himura2la requested review from BrucePay, PaulHigin, adityapatwardhan, anmenaga, charub, dantraMSFT, daxian-dbw, lzybkr, mirichmo and vors as code owners October 16, 2017 14:37

daxian-dbw assigned TravisEz13 Oct 16, 2017

PaulHigin suggested changes Oct 16, 2017

View reviewed changes

lzybkr reviewed Oct 16, 2017

View reviewed changes

Changes to PR from the Team

4effc48

lzybkr approved these changes Oct 17, 2017

View reviewed changes

PaulHigin approved these changes Oct 17, 2017

View reviewed changes

TravisEz13 requested changes Oct 18, 2017

View reviewed changes

TravisEz13 approved these changes Oct 18, 2017

View reviewed changes

TravisEz13 merged commit 6530b77 into PowerShell:master Nov 10, 2017

Conversation

Himura2la commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Himura2la Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Himura2la Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Himura2la Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Himura2la Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Himura2la Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Oct 16, 2017

Uh oh!

Himura2la commented Oct 17, 2017

Uh oh!

TravisEz13 commented Oct 17, 2017

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Oct 18, 2017

Himura2la commented Oct 16, 2017 •

edited

Loading

msftclas commented Oct 16, 2017 •

edited

Loading

Himura2la Oct 17, 2017 •

edited

Loading

Himura2la Oct 17, 2017 •

edited

Loading

Himura2la Oct 17, 2017 •

edited

Loading

Himura2la Oct 17, 2017 •

edited

Loading

Himura2la Oct 17, 2017 •

edited

Loading

Himura2la commented Oct 18, 2017 •

edited

Loading