Skip to content

Commit 4b94c24

Browse files
iRon7iRon7
committed
Applied feedback from @andrewconnell to add a note about the rule not being enabled by default, and to add a note about potential false positives with functions named "catch" or "finally". Also added a test context for when the rule is disabled. Updated the rule implementation to inherit from ConfigurableRule and set Enable to false in the constructor. Updated the AnalyzeScript method to be an override, and added overrides for GetCommonName, GetDescription, GetName, GetSeverity, and GetSourceName.
1 parent 7c22919 commit 4b94c24

4 files changed

Lines changed: 139 additions & 30 deletions

File tree

‎Rules/MissingTryBlock.cs‎

Lines changed: 74 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,45 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
54
using System;
65
using System.Collections.Generic;
7-
using System.Globalization;
8-
using System.Management.Automation.Language;
9-
106
#if !CORECLR
117
using System.ComponentModel.Composition;
128
#endif
9+
using System.Globalization;
10+
using System.Management.Automation.Language;
11+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
1312

1413
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
1514
{
15+
/// <summary>
16+
/// Find bare word "catch" or "finally" tokens that are not part of a TryStatementAst
17+
/// </summary>
1618
#if !CORECLR
1719
[Export(typeof(IScriptRule))]
1820
#endif
1921

2022
/// <summary>
2123
/// Rule that warns when catch or finally blocks are used without a corresponding try block
2224
/// </summary>
23-
public class MissingTryBlock : IScriptRule
25+
26+
public class MissingTryBlock : ConfigurableRule
2427
{
28+
2529
/// <summary>
26-
/// Analyzes the PowerShell AST for catch or finally blocks that miss the try block.
30+
/// Construct an object of MissingTryBlock type.
2731
/// </summary>
28-
/// <param name="ast">The PowerShell Abstract Syntax Tree to analyze.</param>
29-
/// <param name="fileName">The name of the file being analyzed (for diagnostic reporting).</param>
30-
/// <returns>A collection of diagnostic records for each violation.</returns>
31-
public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
32+
public MissingTryBlock() {
33+
Enable = false;
34+
}
35+
36+
/// <summary>
37+
/// Find bare word "catch" or "finally" tokens that are not part of a TryStatementAst
38+
/// </summary>
39+
/// <param name="ast">AST to be analyzed. This should be non-null</param>
40+
/// <param name="fileName">Name of file that corresponds to the input AST.</param>
41+
/// <returns>A an enumerable type containing the violations</returns>
42+
public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
3243
{
3344
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
3445

@@ -64,20 +75,66 @@ stringAst.Parent is CommandAst commandAst &&
6475
}
6576
}
6677

67-
public string GetCommonName() => Strings.MissingTryBlockCommonName;
78+
/// <summary>
79+
/// Retrieves the common name of this rule.
80+
/// </summary>
81+
public override string GetCommonName()
82+
{
83+
return string.Format(CultureInfo.CurrentCulture, Strings.MissingTryBlockCommonName);
84+
}
6885

69-
public string GetDescription() => Strings.MissingTryBlockDescription;
86+
/// <summary>
87+
/// Retrieves the description of this rule.
88+
/// </summary>
89+
public override string GetDescription()
90+
{
91+
return string.Format(CultureInfo.CurrentCulture, Strings.MissingTryBlockDescription);
92+
}
7093

71-
public string GetName() => string.Format(
94+
/// <summary>
95+
/// Retrieves the name of this rule.
96+
/// </summary>
97+
public override string GetName()
98+
{
99+
return string.Format(
72100
CultureInfo.CurrentCulture,
73101
Strings.NameSpaceFormat,
74102
GetSourceName(),
75103
Strings.MissingTryBlockName);
104+
}
76105

77-
public RuleSeverity GetSeverity() => RuleSeverity.Warning;
106+
/// <summary>
107+
/// Retrieves the severity of the rule: error, warning or information.
108+
/// </summary>
109+
public override RuleSeverity GetSeverity()
110+
{
111+
return RuleSeverity.Warning;
112+
}
113+
114+
/// <summary>
115+
/// Gets the severity of the returned diagnostic record: error, warning, or information.
116+
/// </summary>
117+
/// <returns></returns>
118+
public DiagnosticSeverity GetDiagnosticSeverity()
119+
{
120+
return DiagnosticSeverity.Warning;
121+
}
78122

79-
public string GetSourceName() => Strings.SourceName;
123+
/// <summary>
124+
/// Retrieves the name of the module/assembly the rule is from.
125+
/// </summary>
126+
public override string GetSourceName()
127+
{
128+
return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
129+
}
80130

81-
public SourceType GetSourceType() => SourceType.Builtin;
131+
/// <summary>
132+
/// Retrieves the type of the rule, Builtin, Managed or Module.
133+
/// </summary>
134+
public override SourceType GetSourceType()
135+
{
136+
return SourceType.Builtin;
137+
}
82138
}
83-
}
139+
}
140+

‎Tests/Rules/MissingTryBlock.tests.ps1‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,18 @@ BeforeAll {
99
}
1010

1111
Describe "MissingTryBlock" {
12+
13+
BeforeAll {
14+
$Settings = @{
15+
IncludeRules = @($ruleName)
16+
Rules = @{ $ruleName = @{ Enable = $true } }
17+
}
18+
}
19+
1220
Context "Violates" {
1321
It "Catch is missing a try block" {
1422
$scriptDefinition = { catch { "An error occurred." } }.ToString()
15-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
23+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
1624
$violations.Count | Should -Be 1
1725
$violations.Severity | Should -Be Warning
1826
$violations.Extent.Text | Should -Be catch
@@ -22,7 +30,7 @@ Describe "MissingTryBlock" {
2230

2331
It "Finally is missing a try block" {
2432
$scriptDefinition = { finally { "Finalizing..." } }.ToString()
25-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
33+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
2634
$violations.Count | Should -Be 1
2735
$violations.Severity | Should -Be Warning
2836
$violations.Extent.Text | Should -Be finally
@@ -34,7 +42,7 @@ Describe "MissingTryBlock" {
3442
$scriptDefinition = {
3543
catch { "An error occurred." } finally { "Finalizing..." }
3644
}.ToString()
37-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
45+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
3846
$violations.Count | Should -Be 1
3947
$violations.Severity | Should -Be Warning
4048
$violations.Extent.Text | Should -Be catch
@@ -47,7 +55,7 @@ Describe "MissingTryBlock" {
4755
catch { "An error occurred." }
4856
finally { "Finalizing..." }
4957
}.ToString()
50-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
58+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
5159
$violations.Count | Should -Be 2
5260
$violations[0].Severity | Should -Be Warning
5361
$violations[0].Extent.Text | Should -Be catch
@@ -66,7 +74,7 @@ Describe "MissingTryBlock" {
6674
try { NonsenseString }
6775
catch { "An error occurred." }
6876
}.ToString()
69-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
77+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
7078
$violations | Should -BeNullOrEmpty
7179
}
7280

@@ -76,33 +84,33 @@ Describe "MissingTryBlock" {
7684
catch { "An error occurred." }
7785
finally { "Finalizing..." }
7886
}.ToString()
79-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
87+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
8088
$violations | Should -BeNullOrEmpty
8189
}
8290

8391
It "Single line try statement" {
8492
$scriptDefinition = {
8593
try { NonsenseString } catch { "An error occurred." } finally { "Finalizing..." }
8694
}.ToString()
87-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
95+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
8896
$violations | Should -BeNullOrEmpty
8997
}
9098

9199
It "Catch as parameter" {
92100
$scriptDefinition = { Write-Host Catch }.ToString()
93-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
101+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
94102
$violations | Should -BeNullOrEmpty
95103
}
96104

97105
It "Catch as double quoted string" {
98106
$scriptDefinition = { "Catch" }.ToString()
99-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
107+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
100108
$violations | Should -BeNullOrEmpty
101109
}
102110

103111
It "Catch as single quoted string" {
104112
$scriptDefinition = { 'Catch' }.ToString()
105-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
113+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
106114
$violations | Should -BeNullOrEmpty
107115
}
108116
}
@@ -115,7 +123,7 @@ Describe "MissingTryBlock" {
115123
catch { "An error occurred." }
116124
finally { "Finalizing..." }
117125
}.ToString()
118-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
126+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
119127
$violations | Should -BeNullOrEmpty
120128
}
121129

@@ -126,8 +134,25 @@ Describe "MissingTryBlock" {
126134
catch { "An error occurred." }
127135
finally { "Finalizing..." }
128136
}.ToString()
129-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -IncludeRule @($ruleName)
137+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
130138
$violations.Count | Should -Be 1
131139
}
132140
}
141+
142+
Context "Disabled" {
143+
144+
BeforeAll {
145+
$Settings = @{
146+
IncludeRules = @($ruleName)
147+
Rules = @{ $ruleName = @{ Enable = $false } }
148+
}
149+
}
150+
151+
It "ConvertFrom-SecureString -AsPlainText" {
152+
$scriptDefinition = { catch { "An error occurred." } }.ToString()
153+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
154+
$violations | Should -BeNullOrEmpty
155+
}
156+
}
157+
133158
}

‎docs/Rules/MissingTryBlock.md‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,19 @@ The `catch` and `finally` blocks must be preceded by a `try` block. Without a `t
1818
This rule identifies instances where `catch` or `finally` blocks are present with out an associated
1919
`try` block.
2020

21+
> [!NOTE]
22+
> This rule is not enabled by default. The user needs to enable it through settings.
23+
2124
## How
2225

2326
Add a `try` block before the `catch` and `finally` blocks.
2427

28+
> [!NOTE]
29+
> This rule could result in a false positive as it will fire on user code that violates the rule
30+
> [AvoidReservedWordsAsFunctionNames][1] for functions named `catch` or `finally`:
31+
> If you have functions named `catch` or `finally`, you can either rename the function or disable
32+
> this rule.
33+
2534
## Example
2635

2736
### Wrong
@@ -36,3 +45,21 @@ catch { "An error occurred." }
3645
try { $a = 1 / $b }
3746
catch { "Attempted to divide by zero." }
3847
```
48+
49+
## Configuration
50+
51+
```powershell
52+
Rules = @{
53+
PSAvoidExclaimOperator = @{
54+
Enable = $true
55+
}
56+
}
57+
```
58+
59+
### Parameters
60+
61+
- `Enable`: **bool** (Default value is `$false`)
62+
63+
Enable or disable the rule during ScriptAnalyzer invocation.
64+
65+
[1]: AvoidReservedWordsAsFunctionNames.md "Avoid using reserved words as function names."

‎docs/Rules/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ The PSScriptAnalyzer contains the following rule definitions.
5050
| [DSCUseVerboseMessageInDSCResource](./DSCUseVerboseMessageInDSCResource.md) | Error | Yes | |
5151
| [MisleadingBacktick](./MisleadingBacktick.md) | Warning | Yes | |
5252
| [MissingModuleManifestField](./MissingModuleManifestField.md) | Warning | Yes | |
53-
| [MissingTryBlock](./MissingTryBlock.md) | Warning | Yes | |
53+
| [MissingTryBlock](./MissingTryBlock.md) | Warning | No | Yes |
5454
| [PlaceCloseBrace](./PlaceCloseBrace.md) | Warning | No | Yes |
5555
| [PlaceOpenBrace](./PlaceOpenBrace.md) | Warning | No | Yes |
5656
| [PossibleIncorrectComparisonWithNull](./PossibleIncorrectComparisonWithNull.md) | Warning | Yes | |

0 commit comments

Comments
 (0)