X Tutup
The Wayback Machine - https://web.archive.org/web/20200920010924/https://github.com/PowerShell/PowerShell-RFC/pull/151
Skip to content

/ PowerShell-RFC

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.

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

PSScriptAnalyzer aggressive mode & default rules #151

Open
wants to merge 4 commits into
base: master
from
Open

PSScriptAnalyzer aggressive mode & default rules #151

wants to merge 4 commits into from

Conversation

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 16, 2018

No description provided.

@TylerLeonhardt
pssa aggressive mode
Loading status checks…
95655fb
@bergmeister
bergmeister reviewed Nov 21, 2018
View changes
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

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

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018 •
edited

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 ;-)

This comment has been minimized.

Sign in to view
@edyoung

edyoung Nov 21, 2018

Or 'Strict'
@robertiansweetman
robertiansweetman reviewed Nov 21, 2018
View changes
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
This can be set to:

- None - disables script analysis
- Default - some crutial rules but nothing too crazy

This comment has been minimized.

Sign in to view
@robertiansweetman

robertiansweetman Nov 21, 2018

Suggested change
- Default - some crutial rules but nothing too crazy
- Default - some crucial rules but nothing too crazy
@essentialexch
Copy link

essentialexch commented Nov 21, 2018

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
Copy link

bergmeister commented Nov 21, 2018 •
edited

@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
SteveL-MSFT requested changes Nov 21, 2018
View changes
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

This comment has been minimized.

Sign in to view
@SteveL-MSFT

SteveL-MSFT Nov 21, 2018 Member

Perhaps call it Strict instead of Aggressive?
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md Outdated Show resolved Hide resolved
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md Outdated Show resolved Hide resolved
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md Outdated Show resolved Hide resolved
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| AvoidAssignmentToAutomaticVariable | Warning | Aggressive | |
| AvoidDefaultValueForMandatoryParameter | Warning | Default | |
| AvoidDefaultValueSwitchParameter | Warning | Default | |
| AvoidGlobalAliases* | Warning | Aggressive | x |

This comment has been minimized.

Sign in to view
@SteveL-MSFT

SteveL-MSFT Nov 21, 2018 Member

Is this asterisk supposed to be there? Doesn't ref anything.
@bergmeister
bergmeister reviewed Nov 21, 2018
View changes
Copy link

bergmeister left a comment •
edited

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
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md

This can be set to:

- None - disables script analysis

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

Yes, but then the powershell.scriptanalysis.enable setting should either be supported in some back-compat mode or removed
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| Rule | PSSA Type | Mode | Add to default |
|----------------------------------------------|-------------|------------|----------------|
| AlignAssignmentStatement | Warning | | |
| AvoidAssignmentToAutomaticVariable | Warning | Aggressive | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

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.
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| AvoidInvokingEmptyMembers | Warning | Aggressive | x |
| AvoidNullOrEmptyHelpMessageAttribute | Warning | | |
| AvoidShouldContinueWithoutForce | Warning | Aggressive | |
| AvoidUsingCmdletAliases | Warning | Default | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

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)
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| AvoidNullOrEmptyHelpMessageAttribute | Warning | | |
| AvoidShouldContinueWithoutForce | Warning | Aggressive | |
| AvoidUsingCmdletAliases | Warning | Default | |
| AvoidUsingComputerNameHardcoded | Error | Default | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

I would not enable this by default but in the more thorough mode
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| AvoidShouldContinueWithoutForce | Warning | Aggressive | |
| AvoidUsingCmdletAliases | Warning | Default | |
| AvoidUsingComputerNameHardcoded | Error | Default | |
| AvoidUsingConvertToSecureStringWithPlainText | Error | Default | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

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.
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| AvoidUsingDeprecatedManifestFields | Warning | Aggressive | x |
| AvoidUsingEmptyCatchBlock | Warning | | |
| AvoidUsingInvokeExpression | Warning | | |
| AvoidUsingPlainTextForPassword | Warning | Default | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

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
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| ReservedCmdletChar | Error | Default | |
| ReservedParams | Error | Default | |
| ShouldProcess | Error | Default | |
| UseApprovedVerbs | Warning | Default | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

I would not enable this by default but in the more thorough mode
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| UseApprovedVerbs | Warning | Default | |
| UseBOMForUnicodeEncodedFile | Warning | | |
| UseCmdletCorrectly | Warning | Aggressive | |
| UseDeclaredVarsMoreThanAssignments | Warning | Default | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

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.
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| UsePSCredentialType | Warning | Aggressive | |
| UseShouldProcessForStateChangingFunctions | Warning | Aggressive | |
| UseSingularNouns | Warning | Aggressive | x |
| UseSupportsShouldProcess | Warning | Aggressive | x |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

I would put this rule not even into the thorough mode. This rule annoys me the most personally tbh.
1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
| UseSingularNouns | Warning | Aggressive | x |
| UseSupportsShouldProcess | Warning | Aggressive | x |
| UseToExportFieldsInManifest | Warning | Default | |
| UseCompatibleCmdlets | Warning | | |

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Nov 21, 2018

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
Copy link

essentialexch commented Nov 21, 2018

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
Copy link

bergmeister commented Nov 21, 2018

For completeness, the rules used by the PSGallery are those here
@rkeithhill
Copy link

rkeithhill commented Nov 21, 2018

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
Copy link

bergmeister commented Nov 21, 2018

@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
Copy link

rkeithhill commented Nov 21, 2018

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?
SteveL-MSFT and others added 3 commits Nov 22, 2018
@SteveL-MSFT @TylerLeonhardt
Update 1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
52f0246 
Co-Authored-By: TylerLeonhardt <tylerl0706@gmail.com>
@SteveL-MSFT @TylerLeonhardt
Update 1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
03a0a3a 
Co-Authored-By: TylerLeonhardt <tylerl0706@gmail.com>
@SteveL-MSFT @TylerLeonhardt
Update 1-Draft/RFCNNNN-VSCode-PSScriptAnalyzer-Rules.md
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8594534 
Co-Authored-By: TylerLeonhardt <tylerl0706@gmail.com>
@TylerLeonhardt
Copy link
Member Author

TylerLeonhardt commented Nov 22, 2018

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?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edyoung edyoung

@robertiansweetman robertiansweetman

@bergmeister bergmeister

@SteveL-MSFT SteveL-MSFT

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@TylerLeonhardt @essentialexch @bergmeister @rkeithhill @edyoung @robertiansweetman @SteveL-MSFT
You can’t perform that action at this time.
X Tutup