From b7d8f007584f86faa8dd99b96cd209d05c273a67 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 3 May 2020 22:27:06 +0100 Subject: [PATCH 01/11] AvoidUsingDoubleQuotesForConstantString rule. TODO: tests and docs --- Engine/Formatter.cs | 1 + ...AvoidUsingDoubleQuotesForConstantString.cs | 179 ++++++++++++++++++ Rules/Strings.Designer.cs | 27 +++ Rules/Strings.resx | 11 +- 4 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 Rules/AvoidUsingDoubleQuotesForConstantString.cs 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/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs new file mode 100644 index 000000000..745ae559f --- /dev/null +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -0,0 +1,179 @@ +// 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.Linq; +using System.Management; +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; // Disabled by default + } + + /// + /// Analyzes the given ast to find the [violation] + /// + /// 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) + { + case StringConstantType.DoubleQuoted: + yield return GetDiagnosticRecord(stringConstantExpressionAst, + $"'{stringConstantExpressionAst.Value}'"); + break; + + case StringConstantType.DoubleQuotedHereString: + yield return GetDiagnosticRecord(stringConstantExpressionAst, + $"@'{Environment.NewLine}{stringConstantExpressionAst.Value}{Environment.NewLine}'@"); + break; + // yield return new DiagnosticRecord( + // Strings.AvoidOverwritingBuiltInCmdletsError, + // stringConstantExpressionAst.Extent, + // GetName(), + // GetDiagnosticSeverity(), + // stringConstantExpressionAst.Extent.File, + // suggestedCorrections: new[] { + // new CorrectionExtent( + // stringConstantExpressionAst.Extent, + // $"@'{Environment.NewLine}{stringConstantExpressionAst.Value}{Environment.NewLine}'@", + // stringConstantExpressionAst.Extent.File + // ) + // } + // ); + + default: + break; + } + + // if (stringConstantExpressionAst.StringConstantType == StringConstantType.DoubleQuoted) + // { + // yield return new DiagnosticRecord( + // Strings.AvoidOverwritingBuiltInCmdletsError, + // stringConstantExpressionAst.Extent, + // GetName(), + // GetDiagnosticSeverity(), + // stringConstantExpressionAst.Extent.File, + // suggestedCorrections: new[] { + // new CorrectionExtent( + // stringConstantExpressionAst.Extent, + // $"'{stringConstantExpressionAst.Value}'", + // stringConstantExpressionAst.Extent.File + // ) + // } + // ); + // } + } + } + + private DiagnosticRecord GetDiagnosticRecord(StringConstantExpressionAst stringConstantExpressionAst, + string suggestedCorrection) + { + return new DiagnosticRecord( + Strings.AvoidOverwritingBuiltInCmdletsError, + 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; + } + } +} \ No newline at end of file diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 4b253c71a..d983518e1 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -861,6 +861,33 @@ 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 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..d00ecda6c 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1137,4 +1137,13 @@ Replace {0} with {1} - + + Avoid using double quotes if the string is constant. + + + Use single quotes if the string is constant. + + + AvoidUsingDoubleQuotesForConstantString + + \ No newline at end of file From 50da832260b7776e328aa4c52165b095bf224e6d Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 3 May 2020 22:28:38 +0100 Subject: [PATCH 02/11] cleanup --- ...AvoidUsingDoubleQuotesForConstantString.cs | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index 745ae559f..9289b9ffd 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -7,8 +7,6 @@ using System.ComponentModel.Composition; #endif using System.Globalization; -using System.Linq; -using System.Management; using System.Management.Automation.Language; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; @@ -57,42 +55,10 @@ public override IEnumerable AnalyzeScript(Ast ast, string file yield return GetDiagnosticRecord(stringConstantExpressionAst, $"@'{Environment.NewLine}{stringConstantExpressionAst.Value}{Environment.NewLine}'@"); break; - // yield return new DiagnosticRecord( - // Strings.AvoidOverwritingBuiltInCmdletsError, - // stringConstantExpressionAst.Extent, - // GetName(), - // GetDiagnosticSeverity(), - // stringConstantExpressionAst.Extent.File, - // suggestedCorrections: new[] { - // new CorrectionExtent( - // stringConstantExpressionAst.Extent, - // $"@'{Environment.NewLine}{stringConstantExpressionAst.Value}{Environment.NewLine}'@", - // stringConstantExpressionAst.Extent.File - // ) - // } - // ); default: break; } - - // if (stringConstantExpressionAst.StringConstantType == StringConstantType.DoubleQuoted) - // { - // yield return new DiagnosticRecord( - // Strings.AvoidOverwritingBuiltInCmdletsError, - // stringConstantExpressionAst.Extent, - // GetName(), - // GetDiagnosticSeverity(), - // stringConstantExpressionAst.Extent.File, - // suggestedCorrections: new[] { - // new CorrectionExtent( - // stringConstantExpressionAst.Extent, - // $"'{stringConstantExpressionAst.Value}'", - // stringConstantExpressionAst.Extent.File - // ) - // } - // ); - // } } } From 40acc954ad07e9561ccfee1376c19e3c2d4f4fdc Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 3 May 2020 22:46:41 +0100 Subject: [PATCH 03/11] add tests --- ...ingDoubleQuotesForConstantString.tests.ps1 | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 diff --git a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 new file mode 100644 index 000000000..c8fac161e --- /dev/null +++ b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 @@ -0,0 +1,54 @@ +$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 not warn if string is constant and signle 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 + } + } + + 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 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 + } + } + +} From 13663c368147d82a373d0963b3e63e27ec0f9fe3 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 3 May 2020 22:59:39 +0100 Subject: [PATCH 04/11] tidy up and don't warn on strings containing single quote or @' --- Rules/AvoidUsingDoubleQuotesForConstantString.cs | 12 ++++++++++-- Rules/Strings.Designer.cs | 9 +++++++++ Rules/Strings.resx | 3 +++ ...AvoidUsingDoubleQuotesForConstantString.tests.ps1 | 6 +++++- 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index 9289b9ffd..6250663c5 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -25,7 +25,7 @@ public class AvoidUsingDoubleQuotesForConstantString : ConfigurableRule /// public AvoidUsingDoubleQuotesForConstantString() { - Enable = false; // Disabled by default + Enable = false; } /// @@ -47,11 +47,19 @@ public override IEnumerable AnalyzeScript(Ast ast, string file switch (stringConstantExpressionAst.StringConstantType) { case StringConstantType.DoubleQuoted: + if (stringConstantExpressionAst.Value.Contains("'")) + { + break; + } yield return GetDiagnosticRecord(stringConstantExpressionAst, $"'{stringConstantExpressionAst.Value}'"); break; case StringConstantType.DoubleQuotedHereString: + if (stringConstantExpressionAst.Value.Contains("@'")) + { + break; + } yield return GetDiagnosticRecord(stringConstantExpressionAst, $"@'{Environment.NewLine}{stringConstantExpressionAst.Value}{Environment.NewLine}'@"); break; @@ -66,7 +74,7 @@ private DiagnosticRecord GetDiagnosticRecord(StringConstantExpressionAst stringC string suggestedCorrection) { return new DiagnosticRecord( - Strings.AvoidOverwritingBuiltInCmdletsError, + Strings.AvoidUsingDoubleQuotesForConstantStringError, stringConstantExpressionAst.Extent, GetName(), GetDiagnosticSeverity(), diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index d983518e1..d39ce63fd 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -879,6 +879,15 @@ internal static string AvoidUsingDoubleQuotesForConstantStringDescription { } } + /// + /// 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. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index d00ecda6c..588e9e3dd 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1146,4 +1146,7 @@ AvoidUsingDoubleQuotesForConstantString + + Use single quotes when a string is constant. + \ No newline at end of file diff --git a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 index c8fac161e..d021b2219 100644 --- a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 +++ b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 @@ -13,12 +13,16 @@ Describe 'AvoidUsingDoubleQuotesForConstantString' { (Invoke-ScriptAnalyzer -ScriptDefinition '$item = "value"' -Settings $settings).Count | Should -Be 1 } + 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 constant and signle 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 + Invoke-ScriptAnalyzer -ScriptDefinition '$item = "$(Get-Item)"' -Settings $settings | Should -BeNullOrEmpty } } From 1e00461ab466d080145f653132ebbdc57f57ad1f Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 May 2020 11:58:36 +0100 Subject: [PATCH 05/11] exclude escape sequences and add docs --- ...AvoidUsingDoubleQuotesForConstantString.md | 21 +++++++++++++++ ...AvoidUsingDoubleQuotesForConstantString.cs | 4 +-- ...ingDoubleQuotesForConstantString.tests.ps1 | 27 +++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 RuleDocumentation/AvoidUsingDoubleQuotesForConstantString.md 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/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index 6250663c5..e63adc2de 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -47,7 +47,7 @@ public override IEnumerable AnalyzeScript(Ast ast, string file switch (stringConstantExpressionAst.StringConstantType) { case StringConstantType.DoubleQuoted: - if (stringConstantExpressionAst.Value.Contains("'")) + if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.Value.Contains("`")) { break; } @@ -56,7 +56,7 @@ public override IEnumerable AnalyzeScript(Ast ast, string file break; case StringConstantType.DoubleQuotedHereString: - if (stringConstantExpressionAst.Value.Contains("@'")) + if (stringConstantExpressionAst.Value.Contains("@'") || stringConstantExpressionAst.Value.Contains("`")) { break; } diff --git a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 index d021b2219..fe60cf69c 100644 --- a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 +++ b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 @@ -17,6 +17,10 @@ Describe 'AvoidUsingDoubleQuotesForConstantString' { 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 signle quotes are used' { Invoke-ScriptAnalyzer -ScriptDefinition '$item = ''value''' -Settings $settings | Should -BeNullOrEmpty } @@ -24,8 +28,14 @@ Describe 'AvoidUsingDoubleQuotesForConstantString' { 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 = @' @@ -36,6 +46,23 @@ value (Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings).Count | Should -Be 1 } + 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=@' From 306c36e7d29b4b30968a0fa66e6da8a51df8e30b Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 May 2020 18:26:08 +0100 Subject: [PATCH 06/11] fix tests --- RuleDocumentation/README.md | 1 + Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 4 ++-- Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) 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/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 53ffe5e63..c6ab89831 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -58,7 +58,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 64 + $expectedNumRules = 65 if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4)) { # for PSv3 PSAvoidGlobalAliases is not shipped because @@ -156,7 +156,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 index fe60cf69c..e029e615c 100644 --- a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 +++ b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 @@ -18,7 +18,7 @@ Describe 'AvoidUsingDoubleQuotesForConstantString' { } 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 + Invoke-ScriptAnalyzer -ScriptDefinition '$item = "foo-`$-bar"' -Settings $settings | Should -BeNullOrEmpty } It 'Should not warn if string is constant and signle quotes are used' { From 3362ff90f78faec7c2f5f32d4549e7fea1f1c034 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 May 2020 18:41:54 +0100 Subject: [PATCH 07/11] fix last tests --- Rules/AvoidUsingDoubleQuotesForConstantString.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index e63adc2de..b622c6224 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -47,7 +47,8 @@ public override IEnumerable AnalyzeScript(Ast ast, string file switch (stringConstantExpressionAst.StringConstantType) { case StringConstantType.DoubleQuoted: - if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.Value.Contains("`")) + // Note that .ToString() has to be called in the 2nd case because the Value property already trimmed the ` escape character + if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.ToString().Contains("`")) { break; } @@ -56,7 +57,7 @@ public override IEnumerable AnalyzeScript(Ast ast, string file break; case StringConstantType.DoubleQuotedHereString: - if (stringConstantExpressionAst.Value.Contains("@'") || stringConstantExpressionAst.Value.Contains("`")) + if (stringConstantExpressionAst.Value.Contains("@'") || stringConstantExpressionAst.ToString().Contains("`")) { break; } From 2c93230d75791eccdb8a4afcdb5a64d992c408c0 Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister [MVP]" Date: Mon, 4 May 2020 20:03:12 +0100 Subject: [PATCH 08/11] Apply suggestions from code review Co-authored-by: Robert Holt --- Rules/AvoidUsingDoubleQuotesForConstantString.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index b622c6224..5ccf4031c 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -48,7 +48,7 @@ public override IEnumerable AnalyzeScript(Ast ast, string file { case StringConstantType.DoubleQuoted: // Note that .ToString() has to be called in the 2nd case because the Value property already trimmed the ` escape character - if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.ToString().Contains("`")) + if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.Extent.Text.Contains("`")) { break; } @@ -57,7 +57,7 @@ public override IEnumerable AnalyzeScript(Ast ast, string file break; case StringConstantType.DoubleQuotedHereString: - if (stringConstantExpressionAst.Value.Contains("@'") || stringConstantExpressionAst.ToString().Contains("`")) + if (stringConstantExpressionAst.Value.Contains("@'") || stringConstantExpressionAst.Extent.Text.Contains("`")) { break; } @@ -151,4 +151,4 @@ public override SourceType GetSourceType() return SourceType.Builtin; } } -} \ No newline at end of file +} From 73b8a71ccc9301e69063f4c329e30b853e05bb9a Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 May 2020 20:06:21 +0100 Subject: [PATCH 09/11] update comment --- Rules/AvoidUsingDoubleQuotesForConstantString.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index 5ccf4031c..b4e27e825 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -29,7 +29,8 @@ public AvoidUsingDoubleQuotesForConstantString() } /// - /// Analyzes the given ast to find the [violation] + /// 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. From 43c8d1b1345e994eeacc7c897e9e9bb913378b70 Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister [MVP]" Date: Mon, 4 May 2020 21:57:47 +0100 Subject: [PATCH 10/11] Apply suggestions from code review Co-authored-by: Robert Holt --- Rules/AvoidUsingDoubleQuotesForConstantString.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index b4e27e825..edfe50901 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -48,7 +48,6 @@ public override IEnumerable AnalyzeScript(Ast ast, string file switch (stringConstantExpressionAst.StringConstantType) { case StringConstantType.DoubleQuoted: - // Note that .ToString() has to be called in the 2nd case because the Value property already trimmed the ` escape character if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.Extent.Text.Contains("`")) { break; From 8bb6f032daf4ba9315a5f372881e892a01b10f85 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 5 May 2020 18:00:21 +0100 Subject: [PATCH 11/11] add code documentation, add formatter tests and make test file itself not have PSAvoidUsingDoubleQuotesForConstantString violations itself --- ...AvoidUsingDoubleQuotesForConstantString.cs | 7 ++++++ ...ingDoubleQuotesForConstantString.tests.ps1 | 23 ++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/Rules/AvoidUsingDoubleQuotesForConstantString.cs b/Rules/AvoidUsingDoubleQuotesForConstantString.cs index edfe50901..5b2c232b7 100644 --- a/Rules/AvoidUsingDoubleQuotesForConstantString.cs +++ b/Rules/AvoidUsingDoubleQuotesForConstantString.cs @@ -47,6 +47,13 @@ public override IEnumerable AnalyzeScript(Ast ast, string file { 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("`")) { diff --git a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 index e029e615c..558c67441 100644 --- a/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 +++ b/Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1 @@ -13,16 +13,20 @@ Describe 'AvoidUsingDoubleQuotesForConstantString' { (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 + 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 signle quotes are used' { - Invoke-ScriptAnalyzer -ScriptDefinition '$item = ''value''' -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' { @@ -46,6 +50,19 @@ 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=@"