PSScriptAnalyzer aggressive mode & default rules #151

TylerLeonhardt · 2018-11-16T02:45:35Z

No description provided.

bergmeister · 2018-11-21T17:17:44Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+
+- None - disables script analysis
+- Default - some crutial rules but nothing too crazy
+- Aggressive - more rules, more fun


I would prefer the word Thorough over Aggressive and would also like to have an All mode. When compared to ReSharper or FxCop, PSSA is definitely not aggressive ;-)

Or 'Strict'

robertsweetman · 2018-11-21T17:25:36Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+This can be set to:
+
+- None - disables script analysis
+- Default - some crutial rules but nothing too crazy


Suggested change

- Default - some crutial rules but nothing too crazy

- Default - some crucial rules but nothing too crazy

essentialexch · 2018-11-21T17:39:01Z

I really like this idea, but I also really think that PSSA needs #pragma once/disable/restore before this is done.

@bergmeister is this (or something similar) on the radar?

bergmeister · 2018-11-21T18:08:26Z

@swngdnz Yes, the item is being tracked here. From the Microsoft side, I see that they are quite busy and want to implement more analysis to know if something will not work on certain platforms or ps versions at the moment, therefore it might take them some time to approach that. However, implementing it right now is up to the community. I am currently in the process of wrapping up the last fixes and features for the next release in a few weeks, not sure if I can squeeze that one in. I agree that this feature would be very use useful though but the implementation is not going to be easy due to the nature of the PowerShell language definition. I'll have more time next week and might give it a stab but cannot guarantee anything.

SteveL-MSFT · 2018-11-21T18:22:51Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+Plan to implement: Yes
+---
+
+# New default rules in the VSCode Extension and a new "Aggressive Analysis mode" rules


Perhaps call it Strict instead of Aggressive?

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

SteveL-MSFT · 2018-11-21T18:27:24Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| AvoidAssignmentToAutomaticVariable           | Warning     | Aggressive |                |
+| AvoidDefaultValueForMandatoryParameter       | Warning     | Default    |                |
+| AvoidDefaultValueSwitchParameter             | Warning     | Default    |                |
+| AvoidGlobalAliases*                          | Warning     | Aggressive | x              |


Is this asterisk supposed to be there? Doesn't ref anything.

bergmeister

I'd prefer the following modes:

Off
Gentle. This is what you describe as default but I think the editor should not be gentle by default because 90% of the people do not know or do not have the time or do not care about changing this setting to customise the level of PSSA analysis.
Default: This is what you refer to as 'aggressive' and should be the default, again to give the majority of people the best out of the box analysis for free.
AllRules (by that I mean the code analysis rules, not the code style rules like e.g. OpenBrace)
In this definition

All comments below use your definition and idea of default and aggressive in case you do not agree with my idea/proposal

bergmeister · 2018-11-21T18:13:24Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+
+This can be set to:
+
+- None - disables script analysis


Yes, but then the powershell.scriptanalysis.enable setting should either be supported in some back-compat mode or removed

bergmeister · 2018-11-21T18:14:55Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| Rule                                         | PSSA Type   | Mode       | Add to default |
+|----------------------------------------------|-------------|------------|----------------|
+| AlignAssignmentStatement                     | Warning     |            |                |
+| AvoidAssignmentToAutomaticVariable           | Warning     | Aggressive |                |


I would mark this as default because the rule was written very mildly to only warn about read-only automatic variables that will 100% cause a runtime error.

bergmeister · 2018-11-21T18:16:22Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| AvoidInvokingEmptyMembers                    | Warning     | Aggressive | x              |
+| AvoidNullOrEmptyHelpMessageAttribute         | Warning     |            |                |
+| AvoidShouldContinueWithoutForce              | Warning     | Aggressive |                |
+| AvoidUsingCmdletAliases                      | Warning     | Default    |                |


yes, but we should whitelist common aliases that exist on all platformsps versions such as where, foreach, select etc. (i.e. where the short version is just more readable)

bergmeister · 2018-11-21T18:17:08Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| AvoidNullOrEmptyHelpMessageAttribute         | Warning     |            |                |
+| AvoidShouldContinueWithoutForce              | Warning     | Aggressive |                |
+| AvoidUsingCmdletAliases                      | Warning     | Default    |                |
+| AvoidUsingComputerNameHardcoded              | Error       | Default    |                |


I would not enable this by default but in the more thorough mode

bergmeister · 2018-11-21T18:18:00Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| AvoidShouldContinueWithoutForce              | Warning     | Aggressive |                |
+| AvoidUsingCmdletAliases                      | Warning     | Default    |                |
+| AvoidUsingComputerNameHardcoded              | Error       | Default    |                |
+| AvoidUsingConvertToSecureStringWithPlainText | Error       | Default    |                |


I would not enable this by default but in the more thorough mode. Even systems like Azure DevOps do not support Secure strings yet and strings are being injected as plaintext strings.

bergmeister · 2018-11-21T18:19:54Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| AvoidUsingDeprecatedManifestFields           | Warning     | Aggressive | x              |
+| AvoidUsingEmptyCatchBlock                    | Warning     |            |                |
+| AvoidUsingInvokeExpression                   | Warning     |            |                |
+| AvoidUsingPlainTextForPassword               | Warning     | Default    |                |


I would not enable this by default but in the more thorough mode due to the same reasoning about SecureString not having enough support coverage by CI systems

bergmeister · 2018-11-21T18:21:38Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| ReservedCmdletChar                           | Error       | Default    |                |
+| ReservedParams                               | Error       | Default    |                |
+| ShouldProcess                                | Error       | Default    |                |
+| UseApprovedVerbs                             | Warning     | Default    |                |


I would not enable this by default but in the more thorough mode

bergmeister · 2018-11-21T18:22:41Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| UseApprovedVerbs                             | Warning     | Default    |                |
+| UseBOMForUnicodeEncodedFile                  | Warning     |            |                |
+| UseCmdletCorrectly                           | Warning     | Aggressive |                |
+| UseDeclaredVarsMoreThanAssignments           | Warning     | Default    |                |


I would not enable this by default but in the more thorough mode. Or alternatively not use it in .tests.ps1 files as PSSA is not Pester aware and the rule is limited to a per scriptblock basis.

bergmeister · 2018-11-21T18:23:51Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| UsePSCredentialType                          | Warning     | Aggressive |                |
+| UseShouldProcessForStateChangingFunctions    | Warning     | Aggressive |                |
+| UseSingularNouns                             | Warning     | Aggressive | x              |
+| UseSupportsShouldProcess                     | Warning     | Aggressive | x              |


I would put this rule not even into the thorough mode. This rule annoys me the most personally tbh.

bergmeister · 2018-11-21T18:25:22Z

1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

+| UseSingularNouns                             | Warning     | Aggressive | x              |
+| UseSupportsShouldProcess                     | Warning     | Aggressive | x              |
+| UseToExportFieldsInManifest                  | Warning     | Default    |                |
+| UseCompatibleCmdlets                         | Warning     |            |                |


Correct decision because the rule does nothing by default. In the future we could tweak it to use the current ps version/platform but the rule is not ready for that yet.

essentialexch · 2018-11-21T18:40:25Z

Another item that popped to mind - changes to the "default" rule set probably have a wide impact. For example, I know the DSC team requires all PSSA warnings to be eliminated (and I think they base that on the default rule set, although I haven't read their Coding Requirements document in at least six months). Changes to the default should probably follow the "least surprise" rule.

bergmeister · 2018-11-21T18:45:10Z

For completeness, the rules used by the PSGallery are those here

rkeithhill · 2018-11-21T18:50:25Z

I'd like to see something like this ship with PSSA. An additional parameter set that allows the user to select a set of "canned" settings that ship with PSSA. For example:

Invoke-ScriptAnalyzer -Path ./src -Profile PSGallery
Invoke-ScriptAnalyzer -Path ./src -Profile CriticalOnly

This is very analogous to the named rule sets that ship with FxCop. PSSA already covers the customizable rule set functionality.

bergmeister · 2018-11-21T18:59:58Z

@rkeithhill You already can, just press tab when using the -Settings parameter to go through the available pre-defined sets of rule-lists

Invoke-ScriptAnalyzer -Path $path -Settings PSGallery

rkeithhill · 2018-11-21T19:02:30Z

Excellent. I remember this getting talked about but never heard that it got implemented. So these new suggestions (All, Thorough/Aggressive) just extend this mechanism, right?

Co-Authored-By: TylerLeonhardt <tylerl0706@gmail.com>

TylerLeonhardt · 2018-11-22T03:44:00Z

Yeah if we can extend the mechanism already available in PSSA, that makes this really nice and reusable elsewhere.

Can you comment on the extensibility @bergmeister?

joeyaiello · 2021-03-09T20:07:11Z

@PowerShell/powershell-committee discussed this and agreed that, while it's a worthwhile discussion being held here, we think that it belongs more in an issue discussion on PowerShell/vscode-powershell

pssa aggressive mode

95655fb

bergmeister reviewed Nov 21, 2018

View reviewed changes

robertsweetman reviewed Nov 21, 2018

View reviewed changes

SteveL-MSFT requested changes Nov 21, 2018

View reviewed changes

bergmeister reviewed Nov 21, 2018

View reviewed changes

SteveL-MSFT and others added 3 commits November 21, 2018 19:41

Update 1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

52f0246

Co-Authored-By: TylerLeonhardt <tylerl0706@gmail.com>

Update 1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

03a0a3a

Co-Authored-By: TylerLeonhardt <tylerl0706@gmail.com>

Update 1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

8594534

Co-Authored-By: TylerLeonhardt <tylerl0706@gmail.com>

joeyaiello closed this Mar 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSScriptAnalyzer aggressive mode & default rules #151

PSScriptAnalyzer aggressive mode & default rules #151

TylerLeonhardt commented Nov 16, 2018

bergmeister Nov 21, 2018 •

edited

Loading

edyoung Nov 21, 2018

robertsweetman Nov 21, 2018

essentialexch commented Nov 21, 2018

bergmeister commented Nov 21, 2018 •

edited

Loading

SteveL-MSFT Nov 21, 2018

SteveL-MSFT Nov 21, 2018

bergmeister left a comment •

edited

Loading

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

bergmeister Nov 21, 2018

essentialexch commented Nov 21, 2018

bergmeister commented Nov 21, 2018

rkeithhill commented Nov 21, 2018

bergmeister commented Nov 21, 2018

rkeithhill commented Nov 21, 2018

TylerLeonhardt commented Nov 22, 2018

joeyaiello commented Mar 9, 2021

	- Default - some crutial rules but nothing too crazy
	- Default - some crucial rules but nothing too crazy

PSScriptAnalyzer aggressive mode & default rules #151

PSScriptAnalyzer aggressive mode & default rules #151

Conversation

TylerLeonhardt commented Nov 16, 2018

bergmeister Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

essentialexch commented Nov 21, 2018

bergmeister commented Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

essentialexch commented Nov 21, 2018

bergmeister commented Nov 21, 2018

rkeithhill commented Nov 21, 2018

bergmeister commented Nov 21, 2018

rkeithhill commented Nov 21, 2018

TylerLeonhardt commented Nov 22, 2018

joeyaiello commented Mar 9, 2021

bergmeister Nov 21, 2018 •

edited

Loading

bergmeister commented Nov 21, 2018 •

edited

Loading

bergmeister left a comment •

edited

Loading