-
Notifications
You must be signed in to change notification settings - Fork 394
Add 'UseCorrectCasing' formatting rule for cmdlet/function name #1117
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
Changes from 8 commits
01b7a99
b613a92
d2aa9fc
db83ede
0f7e48f
2012a26
42c54b0
ecfeb23
3208a8d
ea5f379
e9440ca
8c9e44f
5dae0a2
4e42c3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# UseCorrectCasing | ||
|
||
**Severity Level: Information** | ||
|
||
## Description | ||
|
||
This is a style/formatting rule. PowerShell is case insensitive where applicable. The casing of cmdlet names does not matter but this rule ensures that the casing matches for consistency and also because most cmdlets start with an upper case and using that improves readability to the human eye. | ||
|
||
## How | ||
|
||
Use exact casing of the cmdlet, e.g. `Invoke-Command`. | ||
|
||
## Example | ||
|
||
### Wrong | ||
|
||
``` PowerShell | ||
invoke-command { 'foo' } | ||
} | ||
``` | ||
|
||
### Correct | ||
|
||
``` PowerShell | ||
Invoke-Command { 'foo' } | ||
} | ||
``` |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,176 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Management.Automation.Language; | ||
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; | ||
#if !CORECLR | ||
using System.ComponentModel.Composition; | ||
#endif | ||
using System.Globalization; | ||
using System.Linq; | ||
using System.Management.Automation; | ||
|
||
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules | ||
{ | ||
/// <summary> | ||
/// UseCorrectCasing: Check if cmdlet is cased correctly. | ||
/// </summary> | ||
#if !CORECLR | ||
[Export(typeof(IScriptRule))] | ||
#endif | ||
public class UseCorrectCasing : ConfigurableRule //IScriptRule | ||
{ | ||
/// <summary> | ||
/// AnalyzeScript: Analyze the script to check if cmdlet alias is used. | ||
/// </summary> | ||
public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) | ||
{ | ||
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); | ||
|
||
IEnumerable<Ast> commandAsts = ast.FindAll(testAst => testAst is CommandAst, true); | ||
|
||
// Iterates all CommandAsts and check the command name. | ||
foreach (CommandAst commandAst in commandAsts) | ||
{ | ||
string commandName = commandAst.GetCommandName(); | ||
|
||
// Handles the exception caused by commands like, {& $PLINK $args 2> $TempErrorFile}. | ||
// You can also review the remark section in following document, | ||
// MSDN: CommandAst.GetCommandName Method | ||
if (commandName == null) | ||
{ | ||
continue; | ||
} | ||
|
||
using (var powershell = System.Management.Automation.PowerShell.Create()) | ||
{ | ||
var getCommand = powershell.AddCommand("Get-Command") | ||
.AddParameter("Name", commandName) | ||
.AddParameter("ErrorAction", "SilentlyContinue"); | ||
|
||
//if (commandName != null) | ||
//{ | ||
// psCommand.AddParameter("CommandType", commandType); | ||
//} | ||
|
||
var commandInfo = getCommand.Invoke<CommandInfo>().FirstOrDefault(); | ||
if (commandInfo != null) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do we do about things which look like a command, but can't be found by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing |
||
var shortName = commandInfo.Name; | ||
var fullyqualifiedName = $"{commandInfo.ModuleName}\\{shortName}"; | ||
|
||
var isFullyQualified = commandName.Equals(fullyqualifiedName, StringComparison.OrdinalIgnoreCase); | ||
var correctlyCasedCommandName = isFullyQualified ? fullyqualifiedName : shortName; | ||
|
||
|
||
if (!commandName.Equals(correctlyCasedCommandName, StringComparison.Ordinal)) | ||
{ | ||
yield return new DiagnosticRecord( | ||
string.Format(CultureInfo.CurrentCulture, Strings.UseCorrectCasingError, commandName, shortName), | ||
GetCommandExtent(commandAst), | ||
GetName(), | ||
DiagnosticSeverity.Warning, | ||
fileName, | ||
commandName, | ||
suggestedCorrections: GetCorrectionExtent(commandAst, correctlyCasedCommandName)); | ||
} | ||
} | ||
} | ||
|
||
} | ||
} | ||
|
||
/// <summary> | ||
/// For a command like "gci -path c:", returns the extent of "gci" in the command | ||
/// </summary> | ||
private IScriptExtent GetCommandExtent(CommandAst commandAst) | ||
{ | ||
var cmdName = commandAst.GetCommandName(); | ||
foreach (var cmdElement in commandAst.CommandElements) | ||
{ | ||
var stringConstExpressinAst = cmdElement as StringConstantExpressionAst; | ||
if (stringConstExpressinAst != null) | ||
{ | ||
if (stringConstExpressinAst.Value.Equals(cmdName)) | ||
{ | ||
return stringConstExpressinAst.Extent; | ||
} | ||
} | ||
} | ||
return commandAst.Extent; | ||
} | ||
|
||
private IEnumerable<CorrectionExtent> GetCorrectionExtent(CommandAst commandAst, string correctlyCaseName) | ||
{ | ||
var description = string.Format( | ||
CultureInfo.CurrentCulture, | ||
Strings.UseCorrectCasingDescription, | ||
correctlyCaseName, | ||
correctlyCaseName); | ||
var cmdExtent = GetCommandExtent(commandAst); | ||
var correction = new CorrectionExtent( | ||
cmdExtent.StartLineNumber, | ||
cmdExtent.EndLineNumber, | ||
cmdExtent.StartColumnNumber, | ||
cmdExtent.EndColumnNumber, | ||
correctlyCaseName, | ||
commandAst.Extent.File, | ||
description); | ||
yield return correction; | ||
} | ||
|
||
/// <summary> | ||
/// GetName: Retrieves the name of this rule. | ||
/// </summary> | ||
/// <returns>The name of this rule</returns> | ||
public override string GetName() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseCorrectCasingName); | ||
} | ||
|
||
/// <summary> | ||
/// GetCommonName: Retrieves the common name of this rule. | ||
/// </summary> | ||
/// <returns>The common name of this rule</returns> | ||
public override string GetCommonName() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.UseCorrectCasingCommonName); | ||
} | ||
|
||
/// <summary> | ||
/// GetDescription: Retrieves the description of this rule. | ||
/// </summary> | ||
/// <returns>The description of this rule</returns> | ||
public override string GetDescription() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.UseCorrectCasingDescription); | ||
} | ||
|
||
/// <summary> | ||
/// GetSourceType: Retrieves the type of the rule, Builtin, Managed or Module. | ||
/// </summary> | ||
public override SourceType GetSourceType() | ||
{ | ||
return SourceType.Builtin; | ||
} | ||
|
||
/// <summary> | ||
/// GetSeverity: Retrieves the severity of the rule: error, warning of information. | ||
/// </summary> | ||
/// <returns></returns> | ||
public override RuleSeverity GetSeverity() | ||
{ | ||
return RuleSeverity.Information; | ||
} | ||
|
||
/// <summary> | ||
/// GetSourceName: Retrieves the name of the module/assembly the rule is from. | ||
/// </summary> | ||
public override string GetSourceName() | ||
{ | ||
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be much happier to use a single instance of PowerShell rather than to create a new instance of PowerShell for each found command.
Also, what do we do about things which look like a command, but can't be found by Get-Command. Should we attempt to validate those?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to use the
Helper.Instance.CommandInfo
method for the moment because this method has commandinfo caching (but the disadvantage of this method is that it has uses a lock internally). I measured it before and after this change and could not measure a difference though, even compared to even with a 3rd version where I cached thePowerShell
instance instead of always creating it... I also measured the difference betweendevelopment
and this branch and for a 600 line script file, I'd say it increased only a bit (from 370ms to 400ms), so I suspect there is something else that is the bottleneck. Formatting of a whole document is also not a task that a user would run frequently.Would performance be improved if we used the async Execution APIs of PowerShell instead? Would it be acceptable to not enable this rule by default until we've found a better solution? Via the PR in the vscode extension we could run the rule by default for
Invoke-Formatter
but not with the default settings of the vscode extension.I'm also working on a prototype in the background that speeds up PSSA by a factor of 10 in general by calling
Get-Command
in the Helper class once initially in the background to have a much richer cache, the only disadvantage is thatGet-Command
does not populate certain properties that are needed by other rules (I raised issue 8910 in thePowerShell/PowerShell
repo for this), therefore until I figure out a solution to it, this is the performance bottleneck.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon for the moment this is ok - it's something that I'll watch though.