GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||
GeezerVison is showing github.com — Add new MissingTryBlock rule by iRon7 · Pull Request #2179 · PowerShell/PSScriptAnalyzer Conversation
|
|
👋 This rule has a severity of This has the same copy-pasta as your other PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…in the rule definition and resolved copy-pasta in the rule description. Co-authored-by: Copilot <[email protected]>
…m Error to Warning.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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:
MissingTryBlock built-in rule implementation and localized strings.Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
| 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <[email protected]>
…iptAnalyzer into #2098MissingTryBlock
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks again iRon7 for working through this with Liam Peters (@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 Liam Peters (@liamjpeters) to take a quick second look since he did the bulk of the original review.
Drafted by Copilot (Claude Opus 4.7)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Sean Wheeler <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but agree to fix things from previous comment
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…iptAnalyzer into #2098MissingTryBlock
… 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.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Just some small cosmetic/hygiene changes. The actual rule and test coverage is looking really good 👍
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
iRon7 - You could update your PR description to include the issues it resolves so they're auto-resolved when your PR is merged. You can make links using comments in the PR description such as: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Liam Peters <[email protected]>
Co-authored-by: Liam Peters <[email protected]>
Co-authored-by: Liam Peters <[email protected]>
Co-authored-by: Liam Peters <[email protected]>
|
Thanks again for your comments, I have tested and committed them. Added to the PR description (I wasn't even aware of the duplicate |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks iRon7 — and thanks Liam Peters (@liamjpeters) for the careful second pass. All five of Liam's 2026-05-15 line-level asks landed at the new head: the PSMissingTryBlock = @{ settings example, the "Doesn't emit a violation" rename, the duplicate <summary> block above the class, the dead helper, and Closes #1700 and Fixes #2098 in the PR body.
CI is green on Linux, macOS, and Windows. Approving.
Drafted by Copilot (Claude Opus 4.7)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I believe they've been addressed.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Rule request: The Catch (or Finally) statement is missing its Try block. Missing try{} block not reported as a problem
There was an error while loading. Please reload this page.