Skip to content

Commit ca4221f

Browse files
bergmeisterChristoph Bergmeisterrjmholt
authored
New rule (disabled by default): AvoidUsingDoubleQuotesForConstantString (#1470)
* AvoidUsingDoubleQuotesForConstantString rule. TODO: tests and docs * cleanup * add tests * tidy up and don't warn on strings containing single quote or @' * exclude escape sequences and add docs * fix tests * fix last tests * Apply suggestions from code review Co-authored-by: Robert Holt <rjmholt@gmail.com> * update comment * Apply suggestions from code review Co-authored-by: Robert Holt <rjmholt@gmail.com> * add code documentation, add formatter tests and make test file itself not have PSAvoidUsingDoubleQuotesForConstantString violations itself Co-authored-by: Christoph Bergmeister <christoph.bergmeister@bjss.com> Co-authored-by: Robert Holt <rjmholt@gmail.com>
1 parent 8e6182f commit ca4221f

File tree

8 files changed

+337
-3
lines changed

8 files changed

+337
-3
lines changed

‎Engine/Formatter.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ public static string Format<TCmdlet>(
4444
"PSAlignAssignmentStatement",
4545
"PSUseCorrectCasing",
4646
"PSAvoidUsingCmdletAliases",
47+
"PSAvoidUsingDoubleQuotesForConstantString",
4748
};
4849

4950
var text = new EditableText(scriptDefinition);

‎RuleDocumentation/AvoidUsingDoubleQuotesForConstantString.md‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
# AvoidUsingDoubleQuotesForConstantString
2+
3+
**Severity Level: Information**
4+
5+
## Description
6+
7+
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.
8+
9+
## Example
10+
11+
### Wrong
12+
13+
``` PowerShell
14+
$constantValue = "I Love PowerShell"
15+
```
16+
17+
### Correct
18+
19+
``` PowerShell
20+
$constantValue = 'I Love PowerShell'
21+
```

‎RuleDocumentation/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
|[AvoidNullOrEmptyHelpMessageAttribute](./AvoidNullOrEmptyHelpMessageAttribute.md) | Warning | |
1818
|[AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | |
1919
|[UseUsingScopeModifierInNewRunspaces](./UseUsingScopeModifierInNewRunspaces.md) | Warning | |
20+
|[AvoidUsingDoubleQuotesForConstantString](./AvoidUsingDoubleQuotesForConstantString.md) | Warning | Yes |
2021
|[AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md) | Warning | Yes |
2122
|[AvoidUsingComputerNameHardcoded](./AvoidUsingComputerNameHardcoded.md) | Error | |
2223
|[AvoidUsingConvertToSecureStringWithPlainText](./AvoidUsingConvertToSecureStringWithPlainText.md) | Error | |

‎Rules/AvoidUsingDoubleQuotesForConstantString.cs‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
#if !CORECLR
7+
using System.ComponentModel.Composition;
8+
#endif
9+
using System.Globalization;
10+
using System.Management.Automation.Language;
11+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
12+
13+
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
14+
{
15+
/// <summary>
16+
/// AvoidUsingDoubleQuotesForConstantStrings: Checks if a string that uses double quotes contains a constant string, which could be simplified to a single quote.
17+
/// </summary>
18+
#if !CORECLR
19+
[Export(typeof(IScriptRule))]
20+
#endif
21+
public class AvoidUsingDoubleQuotesForConstantString : ConfigurableRule
22+
{
23+
/// <summary>
24+
/// Construct an object of type <seealso cref="AvoidUsingDoubleQuotesForConstantStrings"/>.
25+
/// </summary>
26+
public AvoidUsingDoubleQuotesForConstantString()
27+
{
28+
Enable = false;
29+
}
30+
31+
/// <summary>
32+
/// Analyzes the given ast to find occurences of StringConstantExpressionAst where double quotes are used
33+
/// and could be replaced with single quotes.
34+
/// </summary>
35+
/// <param name="ast">AST to be analyzed. This should be non-null</param>
36+
/// <param name="fileName">Name of file that corresponds to the input AST.</param>
37+
/// <returns>A an enumerable type containing the violations</returns>
38+
public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
39+
{
40+
if (ast == null)
41+
{
42+
throw new ArgumentNullException(nameof(ast));
43+
}
44+
45+
var stringConstantExpressionAsts = ast.FindAll(testAst => testAst is StringConstantExpressionAst, searchNestedScriptBlocks: true);
46+
foreach (StringConstantExpressionAst stringConstantExpressionAst in stringConstantExpressionAsts)
47+
{
48+
switch (stringConstantExpressionAst.StringConstantType)
49+
{
50+
/*
51+
If an interpolated string (i.e. using double quotes) uses single quotes (or @' in case of a here-string), then do not warn.
52+
This because one would then have to escape this single quote, which would make the string less readable.
53+
If an interpolated string contains a backtick character, then do not warn as well.
54+
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.
55+
Therefore to keep it simple, all interpolated strings with a backtick are being excluded.
56+
*/
57+
case StringConstantType.DoubleQuoted:
58+
if (stringConstantExpressionAst.Value.Contains("'") || stringConstantExpressionAst.Extent.Text.Contains("`"))
59+
{
60+
break;
61+
}
62+
yield return GetDiagnosticRecord(stringConstantExpressionAst,
63+
$"'{stringConstantExpressionAst.Value}'");
64+
break;
65+
66+
case StringConstantType.DoubleQuotedHereString:
67+
if (stringConstantExpressionAst.Value.Contains("@'") || stringConstantExpressionAst.Extent.Text.Contains("`"))
68+
{
69+
break;
70+
}
71+
yield return GetDiagnosticRecord(stringConstantExpressionAst,
72+
$"@'{Environment.NewLine}{stringConstantExpressionAst.Value}{Environment.NewLine}'@");
73+
break;
74+
75+
default:
76+
break;
77+
}
78+
}
79+
}
80+
81+
private DiagnosticRecord GetDiagnosticRecord(StringConstantExpressionAst stringConstantExpressionAst,
82+
string suggestedCorrection)
83+
{
84+
return new DiagnosticRecord(
85+
Strings.AvoidUsingDoubleQuotesForConstantStringError,
86+
stringConstantExpressionAst.Extent,
87+
GetName(),
88+
GetDiagnosticSeverity(),
89+
stringConstantExpressionAst.Extent.File,
90+
suggestedCorrections: new[] {
91+
new CorrectionExtent(
92+
stringConstantExpressionAst.Extent,
93+
suggestedCorrection,
94+
stringConstantExpressionAst.Extent.File
95+
)
96+
}
97+
);
98+
}
99+
100+
/// <summary>
101+
/// Retrieves the common name of this rule.
102+
/// </summary>
103+
public override string GetCommonName()
104+
{
105+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingDoubleQuotesForConstantStringCommonName);
106+
}
107+
108+
/// <summary>
109+
/// Retrieves the description of this rule.
110+
/// </summary>
111+
public override string GetDescription()
112+
{
113+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingDoubleQuotesForConstantStringDescription);
114+
}
115+
116+
/// <summary>
117+
/// Retrieves the name of this rule.
118+
/// </summary>
119+
public override string GetName()
120+
{
121+
return string.Format(
122+
CultureInfo.CurrentCulture,
123+
Strings.NameSpaceFormat,
124+
GetSourceName(),
125+
Strings.AvoidUsingDoubleQuotesForConstantStringName);
126+
}
127+
128+
/// <summary>
129+
/// Retrieves the severity of the rule: error, warning or information.
130+
/// </summary>
131+
public override RuleSeverity GetSeverity()
132+
{
133+
return RuleSeverity.Information;
134+
}
135+
136+
/// <summary>
137+
/// Gets the severity of the returned diagnostic record: error, warning, or information.
138+
/// </summary>
139+
/// <returns></returns>
140+
public DiagnosticSeverity GetDiagnosticSeverity()
141+
{
142+
return DiagnosticSeverity.Information;
143+
}
144+
145+
/// <summary>
146+
/// Retrieves the name of the module/assembly the rule is from.
147+
/// </summary>
148+
public override string GetSourceName()
149+
{
150+
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
151+
}
152+
153+
/// <summary>
154+
/// Retrieves the type of the rule, Builtin, Managed or Module.
155+
/// </summary>
156+
public override SourceType GetSourceType()
157+
{
158+
return SourceType.Builtin;
159+
}
160+
}
161+
}

‎Rules/Strings.Designer.cs‎

Lines changed: 36 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Rules/Strings.resx‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1137,4 +1137,16 @@
11371137
<data name="UseUsingScopeModifierInNewRunspacesCorrectionDescription" xml:space="preserve">
11381138
<value>Replace {0} with {1}</value>
11391139
</data>
1140-
</root>
1140+
<data name="AvoidUsingDoubleQuotesForConstantStringCommonName" xml:space="preserve">
1141+
<value>Avoid using double quotes if the string is constant.</value>
1142+
</data>
1143+
<data name="AvoidUsingDoubleQuotesForConstantStringDescription" xml:space="preserve">
1144+
<value>Use single quotes if the string is constant.</value>
1145+
</data>
1146+
<data name="AvoidUsingDoubleQuotesForConstantStringName" xml:space="preserve">
1147+
<value>AvoidUsingDoubleQuotesForConstantString</value>
1148+
</data>
1149+
<data name="AvoidUsingDoubleQuotesForConstantStringError" xml:space="preserve">
1150+
<value>Use single quotes when a string is constant.</value>
1151+
</data>
1152+
</root>

‎Tests/Engine/GetScriptAnalyzerRule.tests.ps1‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ Describe "Test Name parameters" {
6363

6464
It "get Rules with no parameters supplied" {
6565
$defaultRules = Get-ScriptAnalyzerRule
66-
$expectedNumRules = 64
66+
$expectedNumRules = 65
6767
if ($IsCoreCLR -or ($PSVersionTable.PSVersion.Major -eq 3) -or ($PSVersionTable.PSVersion.Major -eq 4))
6868
{
6969
# for PSv3 PSAvoidGlobalAliases is not shipped because
@@ -162,7 +162,7 @@ Describe "TestSeverity" {
162162

163163
It "filters rules based on multiple severity inputs"{
164164
$rules = Get-ScriptAnalyzerRule -Severity Error,Information
165-
$rules.Count | Should -Be 17
165+
$rules.Count | Should -Be 18
166166
}
167167

168168
It "takes lower case inputs" {

‎Tests/Rules/AvoidUsingDoubleQuotesForConstantString.tests.ps1‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
$settings = @{
2+
IncludeRules = @('PSAvoidUsingDoubleQuotesForConstantString')
3+
Rules = @{
4+
PSAvoidUsingDoubleQuotesForConstantString = @{
5+
Enable = $true
6+
}
7+
}
8+
}
9+
10+
Describe 'AvoidUsingDoubleQuotesForConstantString' {
11+
Context 'One line string' {
12+
It 'Should warn if string is constant and double quotes are used' {
13+
(Invoke-ScriptAnalyzer -ScriptDefinition '$item = "value"' -Settings $settings).Count | Should -Be 1
14+
}
15+
16+
It 'Should correctly format if string is constant and double quotes are used' {
17+
Invoke-Formatter -ScriptDefinition '$item = "value"' -Settings $settings | Should -Be "`$item = 'value'"
18+
}
19+
20+
It 'Should not warn if string is interpolated and double quotes are used but single quotes are in value' {
21+
Invoke-ScriptAnalyzer -ScriptDefinition "`$item = 'value'" -Settings $settings | Should -BeNullOrEmpty
22+
}
23+
24+
It 'Should not warn if string is interpolated and double quotes are used but backtick character is in value' {
25+
Invoke-ScriptAnalyzer -ScriptDefinition '$item = "foo-`$-bar"' -Settings $settings | Should -BeNullOrEmpty
26+
}
27+
28+
It 'Should not warn if string is constant and single quotes are used' {
29+
Invoke-ScriptAnalyzer -ScriptDefinition "`$item = 'value'" -Settings $settings | Should -BeNullOrEmpty
30+
}
31+
32+
It 'Should not warn if string is interpolated and double quotes are used' {
33+
Invoke-ScriptAnalyzer -ScriptDefinition '$item = "$(Get-Item)"' -Settings $settings | Should -BeNullOrEmpty
34+
}
35+
36+
It 'Should not warn if string is interpolated and double quotes are used' {
37+
Invoke-ScriptAnalyzer -ScriptDefinition '$item = "$(Get-Item)"' -Settings $settings | Should -BeNullOrEmpty
38+
}
39+
}
40+
41+
# TODO: check escape strings
42+
43+
Context 'Here string' {
44+
It 'Should warn if string is constant and double quotes are used' {
45+
$scriptDefinition = @'
46+
$item=@"
47+
value
48+
"@
49+
'@
50+
(Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings).Count | Should -Be 1
51+
}
52+
53+
It 'Should correctly format if string is constant and double quotes are used' {
54+
$scriptDefinition = @'
55+
$item=@"
56+
value
57+
"@
58+
'@
59+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be @"
60+
`$item=@'
61+
value
62+
'@
63+
"@
64+
}
65+
66+
It 'Should not warn if string is constant and double quotes are used but @'' is used in value' {
67+
$scriptDefinition = @'
68+
$item=@"
69+
value1@'value2
70+
"@
71+
'@
72+
Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty
73+
}
74+
It 'Should not warn if string is constant and double quotes are used but backtick is used in value' {
75+
$scriptDefinition = @'
76+
$item=@"
77+
foo-`$-bar
78+
"@
79+
'@
80+
Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty
81+
}
82+
83+
It 'Should not warn if string is constant and single quotes are used' {
84+
$scriptDefinition = @"
85+
`$item=@'
86+
value
87+
'@
88+
"@
89+
Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty
90+
}
91+
92+
It 'Should not warn if string is interpolated' {
93+
$scriptDefinition = @'
94+
$item=@"
95+
$(Get-Process)
96+
"@
97+
'@
98+
Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $settings | Should -BeNullOrEmpty
99+
}
100+
}
101+
102+
}

0 commit comments

Comments
 (0)