Add new MissingTryBlock rule#2179
Conversation
|
👋 This rule has a severity of This has the same copy-pasta as your other PR. |
…in the rule definition and resolved copy-pasta in the rule description. Co-authored-by: Copilot <copilot@github.com>
…m Error to Warning.
There was a problem hiding this comment.
Pull request overview
Adds a new built-in PSScriptAnalyzer rule to detect catch/finally blocks used without a preceding try, with accompanying documentation and Pester tests to validate behavior.
Changes:
- Introduces the new
MissingTryBlockbuilt-in rule implementation and localized strings. - Adds rule documentation and registers it in the rules documentation index.
- Adds a dedicated Pester test suite for violation, compliant, and suppression scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Rules/README.md | Adds MissingTryBlock to the published rules index table. |
| docs/Rules/MissingTryBlock.md | New rule documentation page describing intent, guidance, and examples. |
| Tests/Rules/MissingTryBlock.tests.ps1 | New Pester coverage for violations, compliant cases, and suppression behavior. |
| Rules/Strings.resx | Adds name/common-name/description/error message resources for the new rule. |
| Rules/MissingTryBlock.cs | Implements AST-based detection and emits diagnostics for missing try before catch/finally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…iptAnalyzer into #2098MissingTryBlock
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Thanks again @iRon7 for working through this with @liamjpeters. The severity mismatch is fixed and the implementation is tight (using StringConstantExpressionAst as the bareword detection is the right approach for this scenario).
One context note that may be useful: this rule was explicitly endorsed upstream in PowerShell/PowerShell#25491 by the engine working group as something that belongs in PSSA rather than the parser. Good signal that there's appetite for landing it.
Three things before I'm comfortable merging:
The // Find all FunctionDefinitionAst in the Ast comment. GitHub's review bot flagged this and you replied "Outdated?" — but the comment is still in Rules/MissingTryBlock.cs on the current head and it's misleading: the code is finding StringConstantExpressionAst, not FunctionDefinitionAst. Could you replace it with something like // Find bareword "catch" or "finally" tokens that are not part of a TryStatementAst?
Opt-in by default. Same convention point as the other rules: please derive from ConfigurableRule with Enable = false so users can opt in, and update docs/Rules/README.md.
Doc the false-positive. This rule will fire on user code like:
function catch { param([scriptblock]$body) & $body }
catch { Write-Host 'hi' }which is legal PowerShell (catch is not a reserved word at the command position). Worth a short note in docs/Rules/MissingTryBlock.md so users know they may need to suppress in unusual cases. Doesn't change the implementation.
Once those are sorted, I'd like @liamjpeters to take a quick second look since he did the bulk of the original review.
Drafted by Copilot (Claude Opus 4.7)
PR Summary
Although #2098 didn't get an up-for-graps label, I did create the suggested Add new MissingTryBlock rule to warn when catch or finally blocks are not preceded by a try block because:
catchblock that misses atryblock is less valid than atryblock that misses acatchblock which causes an parse error by the enginetryblock might easily get unnoticed during design time but will very likely fail during runtimetryblockPR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.