PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.86k stars 377 forks source link

Avoid using cmdlet aliases. #214

Open yutingc opened 9 years ago

yutingc commented 9 years ago

What aliases should we put on the white-list? This is a open thread for discussion.

GoodOlClint commented 9 years ago

I would assume if a script is being run through the Analyzer it is being prepared for distribution. Aliases should then only be used if they increase readability for the enduser.

Example gl doesn't make much sense, but del would. If you're going to allow aliases at all, maybe only allow the ones that match existing, commonly used, non-powershell commands.

KirkMunro commented 9 years ago

I've been thinking about the idea of a white-list some more, and reviewing my internal scripts and profile, and now that I have done so (and remembered some changes I have made to my environment), I'm going to turn direction on this and suggest the following:

  1. Any use of aliases in a script or module being run through Script Analyzer should generate a warning for all aliases
  2. Authors of modules or scripts that define aliases where use of an alias would be a best practice must be able to disable that warning for the users of that module (this was already proposed in another issue).

The reason I've backpedaled on this is because of how easy it is to change built-in alias behaviour, and why it may even be beneficial for users to do so.

In my profile, I have the following script snippet:

#region Fix important *nix commands.

# PowerShell comes with some predefined aliases built-in that are designed to
# ease the transition from *nx to PowerShell. While this is well-intentioned,
# it hides the true power that is available in these *nx commands, and it
# makes it much more difficult to compare and contrast between native commands
# in PowerShell and their *nx counterparts. This block of code removes the
# predefined aliases that would otherwise hide these important *nx commands
# that are made available through Cygwin.

foreach ($nxCommand in @('cat','cp','curl','diff','echo','kill','ls','man','mount','mv','ps','pwd','rm','sleep','tee','type','wget')) {
    if (Test-Path -LiteralPath alias:${nxCommand}) {
        Remove-Item -LiteralPath alias:${nxCommand} -Force
    }
}

New-Alias -Name type -Value cat

function ls {
    param()
    if ($args -notcontains '--color') {
        $args += '--color'
    }
    & ls.exe @args
}

#endregion

This script snippet is important for me because I use cygwin on my system, and I find it much more convenient if nix commands I enter actually run those nix commands. I don't want curl invoking Invoke-WebRequest. Nor do I want ls invoking Get-ChildItem. I have iwr and gci as aliases for those so that I can save time when using them ad-hoc. Instead when I run curl or ls, I want the actual nix command from Cygwin to run. This allows me to take nix examples from the web and run them, have them work, plus compare and contrast with PowerShell equivalent functionality all from the same shell. It is very convenient. Also, the nix commands run very, very fast, so I can compare performance with PowerShell equivalent functionality as well. That benefit has proven quite advantageous to me already, because there is a lot of functionality behind nix commands that is not in PowerShell. Sure, I don't get objects back, just text, but when I need objects I know how to get those, and I have aliases that I can use that are PowerShell specific. This mixed-mode approach is better for me, as I have never really liked the over-simplified approach of having a *nix command be an alias because command arguments aren't the same at all.

It doesn't matter if aliases are built-in and read-only. You can remove them with a very simple command in PowerShell. And rightly so, because there are scenarios where you would want to (as I identified here). Even if you were to argue that aliases should be allowed for -Object commands, tee is an alias by default for tee-object, but tee is also a nix command and when I invoke that, I want the *nix command to invoke.

If any aliases at all are whitelisted, I would limit it to a very, very small set, like foreach and where (because those commands are rather exceptional in that they are used very commonly, perhaps more commonly than any other command, and because they also have corresponding keywords with the same name, so newcomers can understand them more easily that way -- the idea of foreach vs ForEach-Object can confuse people). But even then, is Script Analysis about making scripts readable and easy to understand by others or is it about making scripts reliable in every environment? I think it's about the latter, in which case I think there should be no whitelist of aliases, and the only exceptions should be when attributes are used to make them exceptions, by a script or module consumer or by a script or module author.

joeyaiello commented 9 years ago

I am in almost complete agreement, @KirkMunro. You've articulated extremely well the point that I was trying to make on Monday. My only nitpick is here:

But even then, is Script Analysis about making scripts readable and easy to understand by others or is it about making scripts reliable in every environment?

I would argue that it's intended for both purposes: manageability of code among many people, and reliability of code among many environments. However, this doesn't change your point which is that there should be no whitelist.

Again, though, I think this becomes less of an issue for those that still love aliases in their scripts when we enable global rule suppression.

rkeithhill commented 9 years ago

Hmm, I kind of like the whitelist approach. We use CA dictionary.xml (https://msdn.microsoft.com/en-us/library/bb514188.aspx) files for almost all of our C# source code. We have any industry specific terms and abbreviations that CA flags as misspellings. I could see a generic PSSA config file that has a section for global suppressions, a section for whitelisted aliases, eventually a section for recognized words (if PSSA ever does spell checking). Over time, this config could be expanded to add new capabilities to configure PSSA.

This config file could also serve as a basis for the "profile" functionality we have discussed. For instance, I can almost guarantee that the folks I work with, who will see dozens to perhaps a hundred alias warnings in their scripts about Where, Foreach and Select, will suppress the whole rule. That is a shame as I think it would be better to allow them to whitelist those three aliases so they won't suppress the rule. I can make a good case that they shouldn't be using ? and % and encourage them to fix those spots in their scripts. But if they globally suppress the rule, well, none of theses will get fixed.

I could see a "Existing Script" profile that is a config that suppresses all but the most serious errors and warnings and is perhaps pre-populated with a few, what I would call "canonical" aliases like Where, Foreach and Select.

KirkMunro commented 9 years ago

I don't think suppression is black or white. For example, for uninitialized variables, you can suppress that warning for individual named variables. So the "whitelisting" concept could come from partial suppression, but whitelisting specific aliases in PSScriptAnalyzer for anyone who uses it doesn't seem like the right approach given how people may change their aliases.

rkeithhill commented 9 years ago

I don't think suppression is black or white.

Let's put it this way, and I've seen this time and time again with C# devs, if there are too many instances of a particular rule firing, they will globally suppress the rule instead of suppressing per instance. That's just human nature (path of least resistance) and I fully expect that to be the case with folks using PSSA. A whitelist/config file provides both finer control and is easy i.e. it is easier to whitelist the foreach alias once than to suppress it individually in dozens of places.

I think a high degree of configurability is important for this tool otherwise folks will write it off as just too noisy. I ran PSSA against our nightly regression test scripts - about 10 scripts with a total of ~13,000 lines. The script ran for 7.5 minute and I got 3758 diagnostic records.

45> $res | group RuleName | sort count -desc

Count Name                      Group
----- ----                      -----
 2179 PSAvoidUninitializedVa... {Microsoft.W
  942 PSAvoidUsingPositional... {Microsoft.W
  230 PSUseDeclaredVarsMoreT... {Microsoft.W
  141 PSAvoidUsingInternalURLs  {Microsoft.W
  108 PSAvoidUsingCmdletAliases {Microsoft.W
   56 PSAvoidUsingWriteHost     {Microsoft.W
   35 PSAvoidUsingInvokeExpr... {Microsoft.W
   32 PSAvoidTrapStatement      {Microsoft.W
   12 PSAvoidUsingWMICmdlet     {Microsoft.W
    8 PSPossibleIncorrectCom... {Microsoft.W
    6 PSUsePSCredentialType     {Microsoft.W
    4 PSUseApprovedVerbs        {Microsoft.W
    2 PSAvoidGlobalVars         {Microsoft.W
    1 PSUseCmdletCorrectly      {Microsoft.W
    1 PSAvoidUsingComputerNa... {Microsoft.W
    1 PSProvideVerboseMessage   {Microsoft.W

Yeah, that could be a bit intimidating to tackle without just globally suppressing say the first six rules. But global suppressions are a very big stick and any new code will not get checked against globally suppressed rules.

Jaykul commented 8 years ago

In my opinion, if users remove Constant or ReadOnly aliases and scripts break on them, that's user error.

Particularly the built-in ones:

Needlessly warning about aliases just validates the complaints about the overly verbose syntax of PowerShell.

For what it's worth, @rkeithhill, I would argue that it would probably logical to disable five of your top six ;-)

The one thing I'd check on is PSUseDeclaredVarsMoreThanAssignments, which in my experience is probably right, but it's basically impossible for that warning to represent actual errors or problems.

kilasuit commented 8 years ago

For those new to the language PSAvoidUsingCmdletAliases is a crucial rule to get them to understand that PowerShell is Object orientated and not text driven especially for those that are from an interchanging multi OS mixed environment.

We should be catering this for the much wider scope than those that have 3+ dedicated years of working with the Language

For what its worth regarding aliases as a whole I see there being a usecase for using them in interactive sessions only - they shouldn't be used in scripts at all for the reasons given by @KirkMunro and as we are analysing scripts and not the interactive sessions I think having a Whitelist of aliases is counter intuitive and more damaging than fully enforcing to use the language in its full manner i.e using the verb-noun convention

Jaykul commented 8 years ago

I definitely don't see the connection between long names and object oriented output.

More to the point, those are the exact people who will be turned off PowerShell as a shell if you insist on writing out fully verbose commands like Get-ChildItem -LiteralPath $Pwd instead of dir

As long as you (and this tool) insists on not using aliases at all in scripts, not to mention positional parameters and short form parameters -- I guess we have very little to talk about. I passionately disagree.

For what it's worth, I think you're trying to use default rules to force people to write things a certain way that you like, without regard for any useful metrics.

@KirkMunro wrote

It doesn't matter if aliases are built-in and read-only. You can remove them with a very simple command in PowerShell. And rightly so, because there are scenarios where you would want to (as I identified here). Even if you were to argue that aliases should be allowed for -Object commands, tee is an alias by default for tee-object, but tee is also a nx command and when I invoke that, I want the *nx command to invoke.

The truth is:

Using Where-Object is no safer than using Where -- any user or module can override Where-Object by just defining a function -- in fact, it's easier to override the command name: they don't have to deliberately override the built-in defaults with -Force and -Option AllScope.

There is just no difference.

One is not inherently more dangerous than the other, nor more inherently built-in.

In point of fact, if Where is defined with the fully qualified name (as it should be):

Set-Alias -Name Where -Value Microsoft.PowerShell.Core\Where-Object -Force -Option AllScope, ReadOnly, Constant

The alias "where" is now safer than the simple name Where-Object because it won't be broken if another module exports Where-Object....

KirkMunro commented 8 years ago

@Jaykul. I still stand by my last point.

But even then, is Script Analysis about making scripts readable and easy to understand by others or is it about making scripts reliable in every environment? I think it's about the latter, in which case I think there should be no whitelist of aliases, and the only exceptions should be when attributes are used to make them exceptions, by a script or module consumer or by a script or module author.

Sure people can still cause stuff to break by creating functions with the same names as cmdlets. Even Microsoft employees do that sometimes. But that's a completely separate issue that is outside of the scope of what PSScriptAnalyzer should deal with.

Intellisense and other tools make it trivial to use full command names. Scripts using full command names (not fully qualified, just no aliases) have a much higher chance of success when run on any system than scripts using aliases.

For examples shared on the web and in repositories like the Gallery, I think that is the better way to go for everyone involved, regardless of individual personal feelings on what aliases are safe vs what are not.

For other stuff (scripts that you write for private or corporate use that are not broadly shared, where you have more control over the reliability of aliases because you control the environment), you don't have to use this rule, do you? Because you can configure a profile ("profile" is a bad name, it's really just configuration) that controls which rule sets get used by script analyzer. For things like Code integration, if it's to run script analyzer all the time, maybe they should just provide better support for script analyzer profiles so that you can define one that works for content you produce for your environment.

rkeithhill commented 8 years ago

@jaykul wrote:

in fact, it's easier to override the command name

Indeed. I think it is far more likely that you will run into an issue using a resolved, but not fully qualified cmdlet name such as Where-Object or in a recent example Expand-Archive.

PSCX has had an Expand-Archive command for a long time. Recently, PowerShell V5 added an Expand-Archive command. The vscode-powershell extension has a bit of PowerShell script that was used to install the extenion before VSCode had proper extension installation support. That script would fail on machines with PSCX installed because the PSCX version of Expand-Archive took precedence over the built-in one (and used different parameter names). However the script was written with the built-in command in mind.

With PSCX we can deprecate our Expand-Archive in deference to the built-in one. Still, I think this is likely to continue to be a problem down the road. The safe way to reference built-in commands is to always use a module-qualified name. Personally I think the resulting script would be nigh unreadable if every command were module-qualified. :-) So how do you protect yourself from this real danger?

Scripts executing in a user's PowerShell console are executing in dangerous waters

Personally, I wish that once you got into script scope, PowerShell would A) no longer auto-load modules and B) would not give a command precedence unless it was explicitly imported by the script. If you take defensive programming to the extreme, you have to protect your script against every possible thing thousands of users have done to their global enviornments including (but not limited to):

Do we need more of a wall between a script's execution environment and the user's potentially polluted global scope? You could run a script using powershell.exe -noprofile that helps with a lot of the above issues. I wish the equivalent of a "clean, no-profile" global scope could be enforced for script execution yet still allowed be run from a user's polluted global scope. Perhaps a #Requires NoGlobalSession or something like that?

WRT to auto-loaded modules, I think that some of these features that were added to help with CLI discovery of commands are hurting the reliability of scripts. Once you are writing a script, you should have more control over what is loaded and used by your script.

KirkMunro commented 8 years ago

Maybe something like:

requires -Modules ... -Preferred

or

using ModuleName

where either of these would set the preferred command resolution order (look in the specified modules first).

I'd gladly welcome such a change.

It would be very nice if there was a way to disable auto-loading as well (a new parameter for Set-StrictMode maybe).

This idea is similar to PSDefaultParameterValues -- it's a powerful, very useful feature, but you don't want to inherit the values that are set in it inside of your module because it can break things. In v5 they changed it so that PSDefaultParameterValues are automatically cleared in script module scope, but modules in v3 and v4 still are vulnerable to it, as are script files themselves.

Keith, have you logged that on Connect? If not, please do, I'll vote it up.

rkeithhill commented 8 years ago

@KirkMunro Here you go Kirk. Please feel free to embellish the description of the issue with a comment.

https://connect.microsoft.com/PowerShell/feedbackdetail/view/2065573/scripts-need-better-isolation-from-the-users-global-session-state

rkeithhill commented 8 years ago

OK, circling back around to the original topic ... :-)

I could see going two ways here.

First option - implement a configurable whitelist (perhaps empty by default) in which case I would add the aliases: Where, Foreach, Select, Group, Sort, Tee, Measure, Compare, Measure. I would also add cd, pushd, popd. Sorry but I see scripts littered with cd, I just don't see much benefit in changing a bunch of working scripts to use Set-Location. Ultimately I think this works best if the whitelist is configurable by the user. Having used FxCop and StyleCop for about a decade on C# source, I can tell you that we would have stopped using these tools if they weren't pretty configurable. A tool like this has to be careful about being too opinionated or you'll just turn off folks to using it - especially for rules of debatable value.

Second option - no whitelist but have a "Minimum" profile that does not include this rule. And the minimum profile (or ruleset) should be the default. I think it should be up to the user to move up the chain of more rigorous profiles. This will give a better, "first use" experience of showing warnings/errors on what would truly be problems in the user's script. Next step up is perhaps a "Normal" profile that checks for more stuff. Then perhaps there is a "PSGallery" profile that is even more stricter.

At the end of the day, you want to provide a tool that folks can get value out of, that has a positive "out of the box" experience and doesn't force debatable policies down your throat. Yes I know I can disable the "avoid using cmdlet aliases" rule and if nothing changes that is exactly what I will do for my team. However that is a shame as I would like to avoid using all other aliases except for those I listed above.

Jaykul commented 8 years ago

I'm resisting the urge to repeat arguments which have already been made above, so let me just say that the fact that this rule is still in the default set after all this time makes me feel we're being ignored, and is the number two reason why I don't like ScriptAnalyzer.

kilasuit commented 8 years ago

@Jaykul You may not agree with the rule and I understand your reasons behind it - however I find it important for those that aren't as experienced with the language to be able to run it and understand that they are using aliases.

I'd be one for having a separate rule that allows us to define whitelisted aliases but would expect the defacto standard rule to just kick out a warning on all alias uses.

You choice on which rule to use is then upto you but I know that I'd personally use the defacto rule.

@joeyaiello / @raghushantha - could we get a new Rule PSAvoidNonWhitelistedCmdletAliases to combat @Jaykul concerns about ability to be configured appropriately for the more advanced user whilst being able to be a tool that is useful to the casual/newcomer to the PowerShell World

kilasuit commented 8 years ago

PS Created new issue for the PSAvoidNonWhitelistedCmdletAliases rule #580

Jaykul commented 8 years ago

@kilasuit In my opinion, you're repeating the same mistake made earlier in the conversation. Why does it matter if they are using built-in aliases?

rismoney commented 6 years ago

I'm submitting a PR to get deprecated of this ill formed rule. It is bad for the community and people are abusing the rationale 'aliases are bad' instead of embracing idiomatic powershell as awesome.

bergmeister commented 6 years ago

@rismoney Times have moved on since this issue was created. PowerShell Core came out and as part of it some alias had to be removed on some platforms, which makes it much more important to not use aliases in scripts. Aliases are meant to be used when typing command but not in scripts due to readability and the fact that the alias might not exist in the given version of PowerShell or platform. The future direction is even to take out aliases completely and provide them via modules. You can call Invoke-ScriptAnalyzer with -ExcludeRule PSAvoidUsingCmdletAliases if the rule is not suitable for your environment but I think the rule still makes sense to be there. This needs further discussion and approval before PR #970 that you opened can continue. From me it is a no to this change but I'm happy to be convinced otherwise. Can you please comment @SteveL-MSFT

rismoney commented 6 years ago

Times have moved on since this issue was created.

True but in the duration this issue has caused irreparable harm to the ecosystem in favor of removing aliases from scripts for poor foundational reasoning. This now accounts for unneccesary verbose scripts that don't embrace a powershell's idiomatic style capabilities.

PowerShell Core came out and as part of it some alias had to be removed on some platforms, which makes it much more important to not use aliases in scripts.

Disagree. Powershell core can do what it wants. It is clear that Microsoft has been ambiguous on what to provide in core. Feature parity isn't implied through the Core nomenclature. Plenty of scripts have no intention for ever running on Core yet are perfectly compliant on their current platform. When other linters and analyzers for other languages have cross version incompatibilities these exceptions are managed as such and not excluded via a misleading catch-all category. Specific alias usage discouragement is then understood as a deprecated feature, and not as all aliases are bad. If my exception rule was conformtoPSstandard that would make sense. But if all aliases included in core are banned that's a new wave of broken.

Aliases are meant to be used when typing command but not in scripts due to readability and the fact that the alias might not exist in the given version of PowerShell or platform

Disagree with first part entirely. It's idiomatic powershell that you are rejecting. ? and % are wonderful and should be encouraged everywhere. The latter as I previous stated should be addressed via versioning incompatibilities not on the alias usage perspective.

The future direction is even to take out aliases completely and provide them via modules.

The future direction is machine names > 15 characters... Largely irrelevant today.

You can call Invoke-ScriptAnalyzer with ExcludeRule PSAvoidUsingCmdletAliases

Exceptions imply you are breaking a generally accepted rule. In this case as I've stated the rule should be cross version alias compatibility not using aliases in general. Aliases are great and you should embrace them in scripts everywhere. When I see an Invoke-Expression instead of iex I cringe that people see this as a horrific language of really verbose cmdlet names that are encouraged. Embrace the idioms! Now if Microsoft regrets aliasing curl to iwr that's a different story. Their intentions haven't been pure on that. At the time they wanted to publically demo powershell as friendly to Linux folks, albeit deceptively. Look curl gets a url on Powershell!! But iwr has been in the product for 6 versions and it shouldn't, like many other aliases be discouraged for misguided reasons and people spouting mantras like 'aliases are bad'

SeeminglyScience commented 6 years ago

Exceptions imply you are breaking a generally accepted rule. In this case as I've stated the rule should be cross version alias compatibility not using aliases in general.

I agree that the version compatibility argument isn't the best one when it comes to aliases. I'd like to see a separate rule that warns about specific aliases if you've already excluded PSAvoidUsingCmdletAliases, for those who prefer aliases.

The stronger argument is readability for those just starting to learn PowerShell. You can instantly tell what Invoke-Expression does, but you're much more likely to need to google iex if you haven't come across it. Honestly I absolutely love aliases and use them a bunch at the prompt, but if I saw sujb 1 in script it'd take me at least a couple seconds to guess that was Suspend-Job. There's definitely arguments to be made that some aliases are more readable to new comers than their referenced commands, but I don't think many would argue that all of them are.

Now, it may be the case that none of that is important to you. That's perfectly fine, that's why you can customize rules. If you are more productive using aliases as much as possible, then absolutely continue to do so.

In fact, if you're so inclined a better approach to this might be to submit an alternate opt in rule that requires the use of aliases. I don't speak for PSSA, but I can't see any reason why that shouldn't be an option.

like many other aliases be discouraged for misguided reasons and people spouting mantras like 'aliases are bad'

I will say that the PowerShell community is one of the most welcoming communities I've seen, but yeah the pitchforks do come out when a few specific things come up (aliases, Write-Host, backticks). It can suck to disagree with the crowd sometimes, but what's important is that you're doing what works for you. There's nothing wrong with disabling a rule you disagree with, you may get some flack for it, but that's just the cost of going against the grain.

rkeithhill commented 6 years ago

? and % are wonderful and should be encouraged everywhere

I use these interactively all the time but I also work with folks that have to tweak my scripts. These folks are not PowerShell experts but can Google with the best of them. Cryptic syntax puts those people off where-as seeing Where-Object and Foreach-Object is significantly more "obvious" to them. That said, folks are never going to agree on what rules should be enabled/disabled by default which is why PSSA is configurable in that regard. Disable that rule and party on. :-)

rismoney commented 6 years ago

It's hardly cryptic. It's idiomatic. If anything the rule should be reversed. Anyone who endorses or uses Foreach-object should use %. foreach is not even a legit verb or word for that matter. I foreach'nt a clue who came up with this one.

Give people credit. People CAN learn simplistic symbols and shorthand used in very common cases. The language has some really rediculous cmdlet names and constructs (operators for example) which clearly add more confusion to end users than aliases ever did. It's overblown.

Overall the recommendation to the community should be to embrace them instead of curtail. The trend I see -people point to PSSA as the community's best practice voice when there is a lot misguidance here.

The subsequent issue is being labeled a rule breaker by not conforming pssa and this becomes toxic.

Deprecation notices for aliases are the way to go to bridge version gaps. But alias usage is a foundational capability since the get go.

majkinetor commented 6 years ago

Totall support for @rismoney and the patch !

Times have moved on since this issue was created. PowerShell Core came out and as part of it some alias had to be removed on some platforms, which makes it much more important to not use aliases in scripts

This is invalid argument. Everyting changed, including bunch of cmdlets. Some cmdlets do not exist just the same as aliases, and some are substantially different like iwr.

Aliases are meant to be used when typing command but not in scripts due to readability and the fact that the alias might not exist in the given version of PowerShell or platform.

Another one when developers thought that they know how people use their tool, but the life thought differently, even with all that "no aliases" propaganda. I see people all around offereing 'contributions' to remove aliases. On each posh project I mantain there is always an army of little hobbits trying to remove aliases from my world (and the universe), multiple times (1 typical example)

Aliases are WAY MORE READABLE then full verbose strings. To bring only few points:

In my company, default aliases are basically mandatory. Working with many and large teams, I can't remember when we had alias problem. But I remember all sort of cmdlet problems when somebody is using non-current posh version or multiple module versions and so on. If you want to judge aliases we need facts, metrics. Not phylosophy, not intutition, not projections. Otherwise, arch desisions are baseless in reality, serve nobody and are not worthy an engineer.

The future direction is even to take out aliases completely and provide them via modules.

Amazingly bad direction really - its total insanity. Aliases are first class citizens in any shell.

majkinetor commented 6 years ago

I use these interactively all the time but I also work with folks that have to tweak my scripts. These folks are not PowerShell experts but can Google with the best of them. Cryptic syntax puts those people off where-as seeing Where-Object and Foreach-Object is significantly more "obvious" to them

Really @rkeithhill ? Is that even happening, someobdy searching on Google for % and ? and not finding them and abandoning powershell forever ? \ So we lost potential valuable member of community or invaluable IT worker \

People that want to use Powershell usually just open some tutorial or book or quick intro first and % and ? are on the first page, probably in the first few paragraphs.

This line of thinking isn't IT, its babysitting buisnis.

bergmeister commented 6 years ago

Of course one can have different views on aliases, but a matter of fact is:

Therefore I do not think that a deprecation of the rule is going to happen. Rather think about something that could add value to it like for example adding a customization setting of the rule to whitelist common aliases that are available in every PowerShell version and platform like e.g. select.

rismoney commented 6 years ago

Whitelist common aliases from a rule that discourages use? This is backwards. If you want to block a particular alias then you should cite a reason such as incompatibility across versions (your first bullet) But I stand by, if you can't substantiate a valid reason not to use a particular alias, beyond noob understanding I think the rule is a farce and a disservice.

If you are writing code you do not understand how it works I strongly discourage you from publishing it. Aliases need not apply.

Jaykul commented 6 years ago

For the record, this is the opposite of cleverness:

PSSA has some cleverness built in and can also see if you have aliases defined in your session, for example the following will trigger a warning: New-Alias foo Get-ChildItem; Invoke-ScriptAnalyzer -ScriptDefinition foo. This is very helpful to avoid having a script that just works on the developer's machine because of the developer's profile/session.

Analysis of aliases based entirely on the user's current environment is fragile, brittle, and pretty useless ...

# NO ERRORS, even though the command doesn't exist, and the script will THROW
Invoke-ScriptAnalyzer -ScriptDefinition NoSuchCommand 

Set-Alias NoSuchCommand Where-Object
# ERRORS, even though this script now actually WORKS
Invoke-ScriptAnalyzer -ScriptDefinition NoSuchCommand 

Set-Alias Where-Object NoSuchCommand
# ERRORS, because the local scope has a dumb alias -- NOT because the script is bad
Invoke-ScriptAnalyzer -ScriptDefinition Where-Object
Jaykul commented 6 years ago

Honestly, @bergmeister maybe it's worth revisiting all those points, now that I've thought about it..

PSSA has to support all PowerShell versions and ... some aliases do not exist on some platforms

Unless we have hardcoded the lists of pre-defined aliases into it, PSSA does not know anything about aliases -- except that the current session has some. Therefore, all PSSA can reasonably do is make sure I know that my SESSION has aliases which might affect the behavior of this code. Something like:

INFO: This session has an alias Where -> Where-Object which will be used by this code. This may cause this code to behave differently in this session than in other environments.

It's hard to say that this should be more than informational since PSSA is not making a determination of whether this is a common alias present on all systems, or a built-in alias exported by the same module that exports the command, a local alias defined by the code under test, or a personal alias defined by the developer and used in error. Only the latter aliases are legitimate errors, the rest are all fine, and constitute the vast majority of aliases in real use.

Perhaps PSSA could upgrade it to a warning if it could determine the alias is shadowing a command of some other type, or is unlikely to exist on other systems.

PSSA has some cleverness built in and can also see if you have aliases defined in your session

I don't believe this is cleverness: if PSSA is a STATIC analysis tool, it shouldn't be concerned with the state of my session.

A lot of people have to be able to read languages nowadays without knowing anything about it...

The readability argument simply doesn't hold water. We've gone over this argument time and time again in threads on this project, and the bottom line is that, readability is simply not something PSSA is capable of judging, and has nothing to do with aliases.

There are many examples of aliases which improve readability to certain user groups.

Additionally, there are many examples of aliases created by module authors because they renamed a command and are using aliases for backwards compatibility.

there will be a -Fix switch that can automatically fix those warnings for you

Changing the command name I have been using for months or years to the new command name that I've never seen before has a negative effect on readability for me and my team -- nevermind any other potential users.

There are some people that find the rule useful

On this we agree. I wouldn't lobby for removing the rule, but I do think it should be INFO only, and that any potential -Fix switch should have a severity or rule-name option...

rismoney commented 6 years ago

I think what you said makes A LOT of sense @Jaykul. I would add that it should be the job of PSSA to indicate (via informational) deprecations notices of aliases changes across versions to discourage particular usage patterns. These might help people code and plan across versions, which I believe to be a very popular CI pattern in other languages. Often times, in ruby, I'll rubocop my code on 3 different ruby versions to find breakages. This is far more valuable than saying 'this is bad, don't do it, for non articulated reasons'. It would be way more powerful for PSSA to become version aware instead of agnostic in cultivating future features and deprecating others.

majkinetor commented 6 years ago

100% agree with @Jaykul too.

If anything, PSSA should have static list of cmdlets and aliases per version (this exists in the wild, for example in ISE on Steroids this is called Compatibilty check). Then it can do useful recommendations suach as this command/alias is deprecated/non-existent in Posh vX.Y

Because that is the most useful thing to know, contrary to flags I get when use % | ? which server no purpose whatsoever other then to irritate on each installed place.

chriskuech commented 5 years ago

I have a very deterministic approach for how I create my alias whitelist:

Get-Alias | ? {$_.DisplayName -like "*-object"} | % Name

Pipelines are the foundation of PowerShell, and all generic pipeline functions use Object as their noun, so whitelisting aliases of *-Object cmdlets encourages non-verbose pipelines in scripts. In that spirit, I hope that Sort alias is loaded by default in the future.

cd, dir, etc are great aliases for ad-hoc commands, but I don't think they distract away from the provider/item paradigm and therefore should be discouraged in well-maintained scripts (any script that adheres to a PSScriptAnalyzer configuration)

roysubs commented 4 years ago

I'm confused on how I should proceed on this. I am not on the "every-script-must-be-assumed-to-be-a-mission-critical-enterprise-tool" wagon. i.e. I have a simple/practical/efficient philosophy when it comes to aliases: a) built-in aliases are always ok to use in scripts, e.g. ls / gci / sls (I mean, wth, these aliases have been unchanged for over 14 years people, I think 14 years is about as reliable as it gets, no?), and b) I never use any aliases that are just temporary, made in some script, I just stick to the built-ins.

So, "Not every script is a Mission Critical Enterprise App", and "I only use built-in aliases", and "I love PSScriptAnalyzer and want to keep using it". Therefore, I would like to do either:

1. disable PSScriptAnalyzer warnings only and exclusively for built-in aliases., or 2. setup a white list (but I would prefer 1, since this would require manual messing about on any system that I work on). Could you advise me how to achieve 1, or 2 please? I found this page, but I've tried putting in the exact syntax on this page and it doesn't seem to work (plus I would really much rather have a simple switch, like in "1." above. https://github.com/PowerShell/PSScriptAnalyzer/blob/master/RuleDocumentation/AvoidUsingCmdletAliases.md

If I want to disable PSScriptAnalyzer warnings only for built-in aliases in VS Code etc, what exact step should I take? Do I add a line into the .psd1 file, or something else?

Also, I would point out that all readability arguments are 100% bunkum, which I can prove as follows: • "you can't use sls, it's less readable than Select-String !!!", vs • "oh, -gt as an abbreviation of 'greater than' is perfectly readable, nothing to see here, move along ..." The whole readability debate has always been complete nonsense (plus points form @Jaykul too) - if it's not, tell me how to make -gt more "readable"? If I come to a language and I don't know what an abbreviation means, I google it. We all know this. The readability """argument""" is disingenuous and meaningless (and of course, in most IDE's you can just hover over an alias and it will tell you what it is an alias for ... so you don't even have to google most of the time).

bergmeister commented 4 years ago

First of all, built in all aliases have NOT remained unchanged in the last 14 years. When PowerShell first became cross platform 4 years ago, one of the very first issues raised by the community was the curl alias and as a result it got completely removed and in addition to that any aliases that exist as binaries on Linux (such as e.g. ls) have been removed as aliases on Linux. Therefore this list of in-built aliases depends not only the PowerShell version but also platform. ScriptAnalyzer is clever and actually checks for what is actually an alias when you run it, i.e. Invoke-ScriptAnalyzer -ScriptDefinition 'ls' returns you a warning on Windows but not on Linux where ls is a native binary.

Regarding setting up an allow-list: As you've already found, PSSA allows you allow-list whatever you think is fine. The documentation gives even an example. Can you elaborate please what you've tried to do so that we can improve documentation to be easier to read/understand. You can define settings either in a file or using the -Settings parameter, for more details see here. Therefore if you wanted to allow e.g. cd as given in the example, you would run

Invoke-ScriptAnalyzer -ScriptDefinition 'cd' -Settings @{
    'Rules' = @{
        'PSAvoidUsingCmdletAliases' = @{
            'Whitelist' = @('cd')
        }
    }
}

Regarding your last point around -gt. PSSA is a linter and acts as a helper and crutch and some of its purpose is to make up for things that either cannot be changed in the product any more or some things that would not be desired/possible in the product. I think having a -greaterThan operator or alias might be possible and the product team might be open about it but probably requires significant engineering and I assume that's why it was never done. I suggest you open an issue in PowerShell itself around this idea.

SeeminglyScience commented 4 years ago

Also, I would point out that all readability arguments are 100% bunkum, which I can prove as follows:

Readability is subjective. Enough of the community agrees that it's less readable in most cases that it's become a best practice recommendation.

If I come to a language and I don't know what an abbreviation means, I google it. We all know this. The readability """argument""" is disingenuous and meaningless (and of course, in most IDE's you can just hover over an alias and it will tell you what it is an alias for ... so you don't even have to google most of the time).

That's the readability issue though. If you have to google (or even hover over) every command, most would agree that makes it a bit harder to read.

roysubs commented 4 years ago

Readability is subjective. Enough of the community agrees that it's less readable in most cases that it's become a best practice recommendation.

If it subjective, then stop using readability as an argument... and please don't use the "speak for the community" thing. I find it to be one of the most saddening things on the internet when people say that they speak for the "community". "Enough people": Give me hard stats showing that it is the consensus view among PowerShell coders (and not just from a "poll of people that hate aliases") that they disapprove of aliases, or please don't make that assumption. Avoiding aliases is only a "best practice recommendation" for Production scenarios. Many many PowerShell users have no need for that in the majority of their scripts. Remember: the vast majority of scripts that people write are written only for themselves and are not used in a Production environment, hence why it's unreasonable to want to enforce Production standards on to how most people work, and think about, and use their scripts (for example: we get an idea, we get it working roughly, and learn from that process and maybe use that in a serious way on a Production environment and clean it up - and how many thousands of small scripts do people just write roughly and then discard after that scripts usefulness has expired?).

If I come to a language and I don't know what an abbreviation means, I google it. We all know this. The readability """argument""" is disingenuous and meaningless (and of course, in most IDE's you can just hover over an alias and it will tell you what it is an alias for ... so you don't even have to google most of the time).

That's the readability issue though. If you have to google (or even hover over) every command, most would agree that makes it a bit harder to read.

Not in the slightest. That is all part and parcel of learning a programming language, which is hard, and in that process we say "ok, what's the syntax for that again? How does that function work again? Oh, I forgot that tricky little way that these two functions differ in their output!" over and over, and we invariably google for examples. That's literally not a readability issue. That's an "all programming languages have complexity and nuance, and if you expect a programming language to read like a children's book, please, go and buy a children's book, or only ever program in Basic or Pascal?" (and even those languages are not readable and have to also be learned). Readability is subjective, so people wanting programming languages to be as ""readable"" as a children's book is a actually a pretty big problem (as it smuggles itself into discussions to subvert "useful discussion" into useless noodling about """readability""" concerns).

You literally said that readability is subjective, and then you went on to conclude that readability issues are important and objectively real (and that you speak for the community). I'm actually floored by that... Readability is meaningless in any serious discussion on programming and the world would be a better place if the old "I speak for the community and the community says that we must not do 'x', because readability is objective and real, and most would agree with this statement" thing never reared its head again. You (or me) don't speak for the "community" and programming is hard with nuance and complexity, hence the need for IDE's that help with syntax and that we google for things, AND that we use convenient aliases to avoid having long sprawling unreadable lines (over-long lines are unreadable in their own way, and remember: it's subjective!). "Programming is not a children's book" is a much better mantra than "we have to eradicate readability wrongthink!" (and "most would agree" / "enough in the community agree" are "appeals to authority", classic fallacious reasoning. https://www.thoughtco.com/logical-fallacies-appeal-to-authority-250336 . Deal with facts, not with appeals to "the community"). Aliases are good and I encourage everyone to use the built-in aliases as much as possible in day to day scripting, and enough in the community agrees with me, since the vast majority of PowerShell scripts on the planet are non-Production code that often contain aliases (since you have no stats to back up any of your claims, I guess that I can do the same, right? - see how fallacious it is to speak for "the community"?).

Please note: I'm not against your opinion, and I think your opinion on how and when to use aliases is great for you and is just as valid an opinions as anyone's(!), but it's just your opinion and you don't speak for the "community" and I'm just hopeful that the whole "speaking for the community" thing and pretending that "best practices" (for Production-ready code) should apply to all peoples code at all times goes away. "Readability" and "the community agrees" are often just code-words for turning any/all "useful discussions" into swamps that go nowhere. Fact: aliases are used and are 100% valid in scripts and they are not going away (and certainly not by "appeals to the community") and aliases-in-scripts are just a fixture of the real-world that many many people are very happy with. i.e. It's good to just accept that people think/work differently to our opinions about what might qualify as good/acceptable/normal for alias usage.

roysubs commented 4 years ago

First of all, built in all aliases have NOT remained unchanged in the last 14 years.

It's a valid point and I remember well when the move to Linux happened, but it only affects about two or three aliases. All the rest have remained pretty much completely consistent for 14 years. And it never could affect me, as I was always sceptical about using "curl" / "ls" as I used cygwin or other linux tools on Windows (and anyway, equating Invoke-WebRequest to Linux "curl" was highly dubious to me, so I only ever used "iwr"; similarly for "ls", I only ever used "gci" in scripts).

Therefore this list of in-built aliases depends not only the PowerShell version but also platform.

Agree. Aliases can be this way, but for 95% (if not 99%!) of my scripts (which, as for most people, are things just written for me, on my systems), only a small fraction are ever code that I release on our Production servers. And consider that ... if I ever want to take a script that I have and prepare it for a Production server or to make it Linux cross-platform compatible (and I'm sure I will do this in future), then I can just use PSSA to achieve that quickly. But the fact remains that 95+% of my scripts will remain just things in my personal toolkit that do the occasional automation of use to me only.

ScriptAnalyzer is clever and actually checks for what is actually an alias when you run it, i.e. Invoke-ScriptAnalyzer -ScriptDefinition 'ls' returns you a warning on Windows but not on Linux where ls is a native binary.

ScriptAnalyzer is amazing, and I would never argue for eliminating the ability to detect aliases, and many many people want that. I'm very grateful that you guys built this tool ...

Regarding setting up an allow-list: The documentation gives even an example. Can you elaborate please what you've tried to do https://github.com/PowerShell/PSScriptAnalyzer#settings-support-in-scriptanalyzer).

I just don't know how to set common parameters in the PSScriptAnalyzer.psd1 settings file. In the link I put above it suggests

'Rules' = @{ 'PSAvoidUsingCmdletAliases' = @{ 'Whitelist' = @('cd', 'gci', 'ls', 'dir') } }

but none of the other entries in that file follow that syntax. They are more like:

Rules = @{ PSAvoidUsingCmdletAliases = @{ Whitelist = @('cd', 'gci', 'ls', 'dir') } }

But however I do it I end up with the Module being unable to load with "import-module : The 'C:\Users\Boss\Documents\WindowsPowerShell\Modules\PSScriptAnalyzer\1.19.1\PSScriptAnalyzer.psd1' module cannot be imported because its manifest contains one or more members that are not valid."

What file should I add the above to to get it to ignore these aliases, and which syntax should I use?

Regarding your last point around -gt. I think having a -greaterThan operator or alias might be possible

My point was that I would be against it. Programming languages are hard and nuanced, and are always going to be that way by their nature, so readability issues seem like non-issues. I care more about functionality and syntax consistency (for example, PSSA explaining why $null should be on the left of an equivalence operation).

the product team might be open about it but probably requires significant engineering and I assume that's why it was never done. I suggest you open an issue in PowerShell itself around this idea.

I would never want that and I shudder a bit at the idea that Microsoft would do it. I would hate having to read code that had -greaterThanOrEqualTo instead of -ge. Other than the broad strokes of readability that Microsoft built into the language syntax (Verb-Noun and common switches etc, which I think were extremely wise design choices), I think everything is fine there. Readability is just in the eye of the beholder (for example, a friend of mine uses Whitesmiths notation for his PowerShell, which I find horribly unreadable, that's his choice: each to their own I say! And some people like aliases and some people don't ...).

roysubs commented 4 years ago
  • There are quite some people that find the rule useful and that is why it still makes sense to have it. PSSA is not a religion but it is customizable and extensible. You can either customize or disable the rule if you disagree.

Oh, I just noticed this in a separate comment above @bergmeister. You say "customize" or "disable". The "disable" part sounds perfect and I would really want that. I would very much like to completely disable all alias checking in all of my VS Code instead of having to customize / define each alias that I want to ignore. Alias checking is very unhelpful for the way I work, but will be useful for the times that I need to promote some script to Production usage. Is there a way to blanket disable the rule for my entire environment in VS Code and then I can turn it on later if/when I need it?

Ideally, I would love to be able to disable it by default and then have a blacklist (instead of a whitelist) for the aliases that I do want to check for if that's doable (this would be a super-convenient option that I think many people would appreciate, as I would like to remove things like 'curl')? 'Rules' = @{ 'PSAvoidUsingCmdletAliases' = @{ 'Blacklist' = @('cd', 'ls', 'curl') } }

roysubs commented 4 years ago

It seems that my opinion is wrongthink? I guess that I've never really properly bent the knee to this stuff. I'd just never tell someone "It is not recommended to use aliases in scripts! The community agrees that it is not best practice!", something that we see over and over hundreds of times on stackoverflow - it just looks so OCD, like a self-proclaimed religious priesthood scorning people for daring to not accept dogma, lol. Hell, I just want to use aliases in my scripts and disable alias checking from PSScriptAnalyzer without the constant and never-ending finger-wagging for daring to use aliases. Is that so evil? Apparently so!

majkinetor commented 4 years ago

Lets have reality check.

I suggest to anyone monitor Hacker News for PowerShell mentions. In at least 30% of all mentions people complain about verbosity of PowerShell, its really eye opening. People usually don't know about aliases because they are systematically avoided by proponents of verbose code. Its a disaster, to have a such a good thing avoided because wrong politics.

Some random samples:

cookiecaper 69 days ago [–]PowerShell is underrated, but it's its own worst enemy, stubbornly insisting on LongForm-VerbNoun syntax for all but the most common commands that anyone familiar with the shell would expect

sonofhans 4 months ago | parent | favorite | on: Curl Wttr.in Serious question -- not a flame war :) I honestly can't tell if this is sarcastic or not. I've been typing curl -O http: ... into command prompts for decades, and that Powershell incantation looks awfully unwieldy. In what way is it the happy path?

franga2000 5 months ago [–]Bash is a good shell with a fairly unimpressive pipeline. Powershell is bad shell with a good pipeline. For me, as a programmer, the shell part of a shell is more important, as I can do the rest in a more powerful language. Powershell is too verbose for shell usage (see New-Item), brings nothing new to the table in terms of scripting and is ultimately not worth using just for the better pipeline.(another commenter pointed me to Nushell, which seems to be a very good middle ground, and I've heard of a few more similar efforts, so we hopefully won't be stuck with just bash for too long)

GordonS 6 months ago | parent | favorite | on: Linux Bash vs. Windows PowerShellI know about the lack of case sensitivity, but in idiomatic Powershell, it's Pascal-Kebab-Case. If I use a language, almost everyone else's code is going to be idiomatic, so I would write idiomatic code too. I've really tried to like it, but I just can't get past it. It's just my personal preference, but I've come across quite a few others who dislike the naming of Powershell commands too - seems to be a real love/hate kind of thing.

Its also VERY annoying: on any powershell project I maintain there were like X PR's to replace aliases. Here is a sample from a single project

image

And those were aliases like rm or ls.

Madness!

I must add, that my stance on this somewhat changed with linux entering a game - I avoid now ls, select , sort, rm, etc.. because they go for the native and can break the script.

Jaykul commented 4 years ago

Since actual indisputable facts are in short supply in recent comments:

On Windows, there are only 16 aliases in PowerShell 5.1 that aren't in PowerShell 7 -- and of those, 10 are because the entire functionality was removed (e.g. Snapins, WMI, Jobs, ise, print).

On Linus, there are FOURTY SIX missing.

You can check by opening powershell 7 and then running:

$Aliases = powershell.exe -nolo -noni -nopro -out xml -com '&{$ProgressPreference=0;gal}'
$Aliases | where { -not (gal $_.Name -ea 0) } | measure

I won't add any further comment, because nothing has really changed since I wrote this 2 years ago (or for that matter 5 years ago).

Now as then, there's no value in debating the rule further --especially in this thread-- since the maintainers are unwilling to change it to make it useful.

@roysubs you probably should have opened a new issue to ask about how to do the whitelist, but since we're all here ...

The settings file should have a hashtable in it. The quotes are optional because it's just a hashtable like most PowerShell metadata files (.psd1). Try running something like this:

# Get built-in aliases from Windows PowerShell:
$Aliases = powershell.exe -nolo -noni -nopro -out xml -com '&{$ProgressPreference=0;gal}'

# And exclude them all
Set-Content PSScriptAnalyzerSettings.psd1 "@{
Rules = @{ PSAvoidUsingCmdletAliases = @{ Whitelist = @('$($aliases -join "','")') } }
}"

Do you want to know the easiest way to avoid tripping over this rule?

Just delete all your aliases before you run ScriptAnalyzer (you can put them back afterward). This works as well as any whitelist, because the rule is still only based on the aliases right now on your system when it runs. For compatibility purposes, you need a totally different rule.

# Remove all aliases, but don't forget them
Get-Alias -ov global:aliases | Remove-Alias -Force

# Then run ScriptAnalyzer. For instance:
Invoke-ScriptAnalyzer -ScriptDefinition "gal | measure"

# Then put back the aliases. In case you put this in a script, we'll set the alias scope...
$aliases | Select-Object Name, @{ N="Value"; E={ $_.Definition }} | Set-Alias -Scope Global
hrimhari commented 3 years ago

I got here after having my usage of cd flagged by this rule. I was surprised about that, so I did what I think professional people would do: I started to investigate the subject.

I align with @JayKul et al regarding this rule, but what bothers me most is to realize that depending on what OS I run PSSA, it may or may not flag an alias because it may or may not exist in that OS.

This is particularly troublesome in CI/CD environments where steps like static analysis may be run on different OSs (I.e. Docker images) than the one intended for the software to which the analysis is being run.

I also scratched my head at having cd flagged in Linux, but not ls. I now understand why, but the main purpose that seems to be avoiding incompatibilities doesn't work here. If ls is not flagged because it's not an alias in Linux, what happens when I run this script in Windows? What if I use curl -UseBasicParsing?

B4Art commented 3 years ago

Lets have reality check. .... Madness!

Lets keep it to One and NOT go to SHELLHELL like: dash | bash | ksh | tcsh | zsh

I think this is the same preference discussion as with Linux Shells. You probably prefer one above the other. Nowadays I learn to type English corrected automatically and that is a good thing. I like Powershell and the way it works. Lets all keep it that way.

markonop93 commented 1 year ago

alias should be default to be honest. its bad that powershell doesn't support it fully on all systems

rismoney commented 7 months ago

https://twitter.com/jsnover/status/1774810011866161536?s=19