diff --git a/Engine/Formatter.cs b/Engine/Formatter.cs index 15db1b39d..0067757cd 100644 --- a/Engine/Formatter.cs +++ b/Engine/Formatter.cs @@ -44,6 +44,7 @@ public static string Format( "PSAlignAssignmentStatement", "PSUseCorrectCasing", "PSAvoidUsingCmdletAliases", + "PSAvoidUsingDoubleQuotesForConstantString", }; var text = new EditableText(scriptDefinition); diff --git a/RuleDocumentation/AvoidUsingDoubleQuotesForConstantString.md b/RuleDocumentation/AvoidUsingDoubleQuotesForConstantString.md new file mode 100644 index 000000000..ec6813b86 --- /dev/null +++ b/RuleDocumentation/AvoidUsingDoubleQuotesForConstantString.md @@ -0,0 +1,21 @@ +# AvoidUsingDoubleQuotesForConstantString + +**Severity Level: Information** + +## Description + +When the value of a string is constant (i.e. not being interpolated by injecting variables or expression into such as e.g. `"$PID-$(hostname)"`), then single quotes should be used to express the constant nature of the string. This is not only to make the intent clearer that the string is a constant and makes it easier to use some special characters such as e.g. `$` within that string expression without the need to escape them. There are exceptions to that when double quoted strings are more readable though, e.g. when the string value itself has to contain a single quote (which would require a double single quotes to escape the character itself) or certain very special characters such as e.g. `"\n"` are already being escaped, the rule would not warn in this case as it is up to the author to decide on what is more readable in those cases. + +## Example + +### Wrong + +``` PowerShell +$constantValue = "I Love PowerShell" +``` + +### Correct + +``` PowerShell +$constantValue = 'I Love PowerShell' +``` diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md index 2ed360e88..fbe0f5cbd 100644 --- a/RuleDocumentation/README.md +++ b/RuleDocumentation/README.md @@ -17,6 +17,7 @@ |[AvoidNullOrEmptyHelpMessageAttribute](./AvoidNullOrEmptyHelpMessageAttribute.md) | Warning | | |[AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | | |[UseUsingScopeModifierInNewRunspaces](./UseUsingScopeModifierInNewRunspaces.md) | Warning | | +|[AvoidUsingDoubleQuotesForConstantString](./AvoidUsingDoubleQuotesForConstantString.md) | Warning | Yes | |[AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md) | Warning | Yes | |[AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md) | Error | | |[AvoidUsingConvertToSecureStringWithPlainText](./AvoidUsingConvertToSecureStringWithPlainText.md) | Error | | diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs new file mode 100644 index 000000000..5b2c232b7 --- /dev/null +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -0,0 +1,161 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; +using System.Management.Automation.Language; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// AvoidUsingDoubleQuotesForConstantStrings: Checks if a string that uses double quotes contains a constant string, which could be simplified to a single quote. + /// +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + public class AvoidUsingDoubleQuotesForConstantString : ConfigurableRule + { + /// + /// Construct an object of type . + /// + public AvoidUsingDoubleQuotesForConstantString() + { + Enable = false; + } + + /// + /// Analyzes the given ast to find occurences of StringConstantExpressionAst where double quotes are used + /// and could be replaced with single quotes. + /// + /// AST to be analyzed. This should be non-null + /// Name of file that corresponds to the input AST. + /// A an enumerable type containing the violations + public override IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException(nameof(ast)); + } + + var stringConstantExpressionAsts = ast.FindAll(testAst => testAst is StringConstantExpressionAst, searchNestedScriptBlocks: true); + foreach (StringConstantExpressionAst stringConstantExpressionAst in stringConstantExpressionAsts) + { + switch (stringConstantExpressionAst.StringConstantType) + { + /* + If an interpolated string (i.e. using double quotes) uses single quotes (or @' in case of a here-string), then do not warn. + This because one would then have to escape this single quote, which would make the string less readable. + If an interpolated string contains a backtick character, then do not warn as well. + This is because we'd have to replace the backtick in some cases like `$ and in others like `a it would not be possible at all. + Therefore to keep it simple, all interpolated strings with a backtick are being excluded. + */ + case StringConstantType.DoubleQuoted: + if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.Extent.Text.Contains("`")) + { + break; + } + yield return GetDiagnosticRecord(stringConstantExpressionAst, + $"'{stringConstantExpressionAst.Value}'"); + break; + + case StringConstantType.DoubleQuotedHereString: + if (stringConstantExpressionAst.Value.Contains("@'") || stringConstantExpressionAst.Extent.Text.Contains("`")) + { + break; + } + yield return GetDiagnosticRecord(stringConstantExpressionAst, + $"@'{Environment.NewLine}{stringConstantExpressionAst.Value}{Environment.NewLine}'@"); + break; + + default: + break; + } + } + } + + private DiagnosticRecord GetDiagnosticRecord(StringConstantExpressionAst stringConstantExpressionAst, + string suggestedCorrection) + { + return new DiagnosticRecord( + Strings.AvoidUsingDoubleQuotesForConstantStringError, + stringConstantExpressionAst.Extent, + GetName(), + GetDiagnosticSeverity(), + stringConstantExpressionAst.Extent.File, + suggestedCorrections: new[] { + new CorrectionExtent( + stringConstantExpressionAst.Extent, + suggestedCorrection, + stringConstantExpressionAst.Extent.File + ) + } + ); + } + + /// + /// Retrieves the common name of this rule. + /// + public override string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingDoubleQuotesForConstantStringCommonName); + } + + /// + /// Retrieves the description of this rule. + /// + public override string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingDoubleQuotesForConstantStringDescription); + } + + /// + /// Retrieves the name of this rule. + /// + public override string GetName() + { + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AvoidUsingDoubleQuotesForConstantStringName); + } + + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public override RuleSeverity GetSeverity() + { + return RuleSeverity.Information; + } + + /// + /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// + /// + public DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Information; + } + + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public override string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public override SourceType GetSourceType() + { + return SourceType.Builtin; + } + } +} diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 4b253c71a..d39ce63fd 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -861,6 +861,42 @@ internal static string AvoidUsingDeprecatedManifestFieldsName { } } + /// + /// Looks up a localized string similar to Avoid using double quotes if the string is constant.. + /// + internal static string AvoidUsingDoubleQuotesForConstantStringCommonName { + get { + return ResourceManager.GetString("AvoidUsingDoubleQuotesForConstantStringCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Use single quotes if the string is constant.. + /// + internal static string AvoidUsingDoubleQuotesForConstantStringDescription { + get { + return ResourceManager.GetString("AvoidUsingDoubleQuotesForConstantStringDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Use single quotes when a string is constant.. + /// + internal static string AvoidUsingDoubleQuotesForConstantStringError { + get { + return ResourceManager.GetString("AvoidUsingDoubleQuotesForConstantStringError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidUsingDoubleQuotesForConstantString. + /// + internal static string AvoidUsingDoubleQuotesForConstantStringName { + get { + return ResourceManager.GetString("AvoidUsingDoubleQuotesForConstantStringName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using Empty Catch Block. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index e64cecd6b..588e9e3dd 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1137,4 +1137,16 @@ Replace {0} with {1} - + + Avoid using double quotes if the string is constant. + + + Use single quotes if the string is constant. + + + AvoidUsingDoubleQuotesForConstantString + + + Use single quotes when a string is constant. + + \ No newline at end of file diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 9f3cdb9c0..664be838a 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -63,7 +63,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 64 + $expectedNumRules = 65 if ($IsCoreCLR -or ($PSVersionTable.PSVersion.Major -eq 3) -or ($PSVersionTable.PSVersion.Major -eq 4)) { # for PSv3 PSAvoidGlobalAliases is not shipped because @@ -162,7 +162,7 @@ Describe "TestSeverity" { It "filters rules based on multiple severity inputs"{ $rules = Get-ScriptAnalyzerRule -Severity Error,Information - $rules.Count | Should -Be 17 + $rules.Count | Should -Be 18 } It "takes lower case inputs" { diff --git a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 new file mode 100644 index 000000000..558c67441 --- /dev/null +++ b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 @@ -0,0 +1,102 @@ +$settings = @{ + IncludeRules = @('PSAvoidUsingDoubleQuotesForConstantString') + Rules = @{ + PSAvoidUsingDoubleQuotesForConstantString = @{ + Enable = $true + } + } +} + +Describe 'AvoidUsingDoubleQuotesForConstantString' { + Context 'One line string' { + It 'Should warn if string is constant and double quotes are used' { + (Invoke-ScriptAnalyzer -ScriptDefinition '$item = "value"' -Settings $settings).Count | Should -Be 1 + } + + It 'Should correctly format if string is constant and double quotes are used' { + Invoke-Formatter -ScriptDefinition '$item = "value"' -Settings $settings | Should -Be "`$item = 'value'" + } + + It 'Should not warn if string is interpolated and double quotes are used but single quotes are in value' { + Invoke-ScriptAnalyzer -ScriptDefinition "`$item = 'value'" -Settings $settings | Should -BeNullOrEmpty + } + + It 'Should not warn if string is interpolated and double quotes are used but backtick character is in value' { + Invoke-ScriptAnalyzer -ScriptDefinition '$item = "foo-`$-bar"' -Settings $settings | Should -BeNullOrEmpty + } + + It 'Should not warn if string is constant and single quotes are used' { + Invoke-ScriptAnalyzer -ScriptDefinition "`$item = 'value'" -Settings $settings | Should -BeNullOrEmpty + } + + It 'Should not warn if string is interpolated and double quotes are used' { + Invoke-ScriptAnalyzer -ScriptDefinition '$item = "$(Get-Item)"' -Settings $settings | Should -BeNullOrEmpty + } + + It 'Should not warn if string is interpolated and double quotes are used' { + Invoke-ScriptAnalyzer -ScriptDefinition '$item = "$(Get-Item)"' -Settings $settings | Should -BeNullOrEmpty + } + } + + # TODO: check escape strings + + Context 'Here string' { + It 'Should warn if string is constant and double quotes are used' { + $scriptDefinition = @' +$item=@" +value +"@ +'@ + (Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings).Count | Should -Be 1 + } + + It 'Should correctly format if string is constant and double quotes are used' { + $scriptDefinition = @' +$item=@" +value +"@ +'@ + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be @" +`$item=@' +value +'@ +"@ + } + + It 'Should not warn if string is constant and double quotes are used but @'' is used in value' { + $scriptDefinition = @' +$item=@" +value1@'value2 +"@ +'@ + Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty + } + It 'Should not warn if string is constant and double quotes are used but backtick is used in value' { + $scriptDefinition = @' +$item=@" +foo-`$-bar +"@ +'@ + Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty + } + + It 'Should not warn if string is constant and single quotes are used' { + $scriptDefinition = @" +`$item=@' +value +'@ +"@ + Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty + } + + It 'Should not warn if string is interpolated' { + $scriptDefinition = @' +$item=@" +$(Get-Process) +"@ +'@ + Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty + } + } + +}