Add more useful PSSA rules that should be enabled by default

[bergmeister]

Since the majority of people do not customize the PSSA settings inside VSCode and just use the vs code extension how it comes out of the box, therefore enabling more useful PSSA rules by default, especially PSPossibleIncorrectComparisonWithNull (this was what drove me to change the defaults because I really expected this to be on by default when I was doing a demo)
The parallel nature of PSSA (that executes all rules in parallel) means that the time to analyse a script will be always bound by the slowest rule.

[bergmeister]

Locally, the tests passed on Windows PowerShell 5.1 on Windows 1803, therefore I guess this AppVeyor failure must be sporadic?

[rkeithhill]

That test failure happens intermittently. RE the PR, I'm a bit concerned that folks will see quite a few more warnings in their script after updating PSES. OTOH I'm sympathetic to having a good default experience. I was thinking that at one time we enabled more rules by default but in looking through the commit history I'm not seeing that. Regardless, I just want to point out that this PR will have a noticeable impact on users.

BTW I thought that using Write-Host was considered OK in >= v5?

[rjmholt]

@bergmeister I was watching your PSConf EU talk yesterday and was thinking about exactly this

[bergmeister]

Yes, the lack of default rules annoyed me at my talk because after the first 2 rules that I had to enable at my talk I could not be bothered to enable PSPossibleIncorrectComparisonWithNull as well and so I just explained the reasoning behind the rule and gave an example of the rule (being sarcastic, you could probably also say that I should've used a PSSA settings file instead...). I also had a classic demo failure moment where auto-formatting was not working any more because I made live changes during the demo that lead to invalid syntax (missing closing parenthesis), which caused the formatter to not work without any message.... But hey ho, it was just a spontaneous presentation/demo that I did so no harm done.
The PSPossibleIncorrectComparisonWithNull rule brings the most value to the user imho in this PR so if you have any concerns then maybe take only this one for a starter.
I am O( to drop the WriteHost rule or others but most people still do not know when to use Write-Verbose and write-output, so the rule is more of a reminder of 'did you actually want that'? A good example of misuse is the Start-PSBuild function of the PowerShell team which writes to Host instead of returning an object, if it returned an object, then I could just do something like ii (Start-PsBuild) or & (Start-PSBuild)...
Im general: more warnings are good, otherwise what is the point of the tool, as always we have to remind people that sophisticated code analysis always requires human judgement. As I said in my talk, PSSA should not be seen as the gold standard or a quality milestone (because let's be honest, the tool is still pretty stupid compared to C# analyzers) but rather as a nice helper on the side

[rkeithhill]

In general I'm with you. But be careful about this:

more warnings are good, otherwise what is the point of the tool

I've been through this with other languages/linters in the past. With our .NET development, the initial crush of warnings from FxCop was so bad most folks wanted it turned off. Fortunately, we prevailed by the efforts of a few folks who went in and did a fair amount configuration i.e. turning off rules we did not care about.

There's a phenomenon known as "broken window" theory that states (when applied to sw) roughly that if you have too many warnings for too long, it will be hard to see new warnings and people will stop caring about fixing warnings. IOW we don't want folks to go and set "powershell.scriptAnalysis.enable": false" because they don't want to deal with all the visual noise of too many warnings.

[rkeithhill]

FYI we had a discussion related to this several years ago on the PSSA project. See PowerShell/PSScriptAnalyzer#214

[bergmeister]

Yeah, I know what you mean @rkeithhill but FXCop has really a lot of nonsense rules when it is not configured and moans even about the style of a new default class file as produced by VS...
Hence why I added only rules that I find kind of generally useful. The biggest producer of false warnings (but still a very useful rule that I would never disable) is PSUseDeclaredVarsMoreThanAssignment, which is already enabled.
I think it would be nice to also be able to use the lightbulb on a warning in VSCode to suppress it on the current function (and it would be nice if PSSA supported suppression on a per line/statement basis but this is not trivial..)

[SeeminglyScience]

PSAvoidDefaultValueForMandatoryParameter should definitely be default, I'm surprised that isn't already. PSPossibleIncorrectComparisonWithNull makes sense to me too. As for the others, I think there would be a lot of backlash. Personally I think we should keep any new default rules as close to "almost definitely a mistake" as possible.

[bergmeister]

OK, as per feedback, we all agree that PSAvoidDefaultValueForMandatoryParameter and PSPossibleIncorrectComparisonWithNull should be added, I have edited the PR accordingly.
In PSSA 1.17.0, PSAvoidTrailingWhitespace and PossibleIncorrectUsageOfRedirectionOperator, therefore I added those 2 in addition to the other 2 because they are generic, very useful imho and are not controversial. Especially PossibleIncorrectUsageOfRedirectionOperator should save a lot of people a lot of mistakes (I wrote this rule because not just I but also my colleagues constantly made the mistake of using > for a 'greater than' comparison because most of the day we program in C#).

[rjmholt]

Is there a simple way to find out what each rule does? Is it worth documenting in the comments what the default rules do and a rationale for their inclusion by default?

[bergmeister]

@rjmholt Rule documentation is here, could you add hyperlinks to those (when one sees the warning)?

[rjmholt]

@bergmeister Good idea. Do the rules record such a hyperlink within themselves at runtime?

[bergmeister]

@rjmholt Unfortunately no, there is PowerShell/PSScriptAnalyzer#816 and PowerShell/PSScriptAnalyzer#730 for that, at the moment, PSSA has only a Get-ScriptAnalyzerRule cmdlet that tells you the available rules and their severity.

[rkeithhill]

The new ruleset looks good to me. BTW why isn't PossibleIncorrectUsageOfRedirectionOperator prefixed with a PS?

[bergmeister]

@rkeithhill Thanks for spotting the typo, I fixed it.

[rkeithhill]

LGTM

[SeeminglyScience]

LGTM, love the addition of PSPossibleIncorrectUsageOfRedirectionOperator :)

Thanks @bergmeister!

[TylerLeonhardt]

LGTM :) merging this 👍

[bergmeister]

Great, and PSSA just re-released as 1.17.1