Prevent duplicate rule ID suppression errors in -Fix and DSC scenarios#2181
Prevent duplicate rule ID suppression errors in -Fix and DSC scenarios#2181liamjpeters wants to merge 2 commits intoPowerShell:mainfrom
-Fix and DSC scenarios#2181Conversation
…r unapplied suppression errors
andyleejordan
left a comment
andyleejordan
left a comment
There was a problem hiding this comment.
Thanks for picking this up — the repro in #2177 was solid and this fix matches it cleanly.
For the -Fix case, threading emitSuppressionErrors: false through the Fix() loop's AnalyzeSyntaxTree / AnalyzeScriptDefinition calls is the right shape: every iteration inside the loop suppresses, the post-loop AnalyzeFile emits once, no duplicates. For the DSC case, swapping the second if to else if so the class-DSC and module-DSC handler branches are mutually exclusive is exactly the right minimal fix. Both regression tests pass locally for me.
LGTM — approving pending CI passing.
— Drafted by Copilot (Claude Opus 4.7)
The `Library Usage` describe block in `LibraryUsage.tests.ps1` (only
active on Windows PowerShell 5.1, since it's gated `-Skip:$IsCoreCLR`)
re-runs `RuleSuppression.tests.ps1` against a hand-rolled
`Invoke-ScriptAnalyzer` wrapper that drives the analyzer as a .NET
library. That wrapper plugs in `PesterTestOutputWriter`, whose
`WriteError` is intentionally a no-op:
public void WriteError(ErrorRecord error)
{
// We don't write errors to avoid misleading
// error messages in test output
}
So the unapplied-suppression `ErrorRecord` we now emit during the final
`-Fix` pass never reaches `-ErrorVariable`, and `$fixErr | Should
-HaveCount 1` fails with "Expected a collection with size 1, but got an
empty collection". The behaviour itself is correct - the assertion is
just unobservable through this test harness.
Mark the new `It` block `-Skip:$testingLibraryUsage`, matching the
existing pattern already used by the `Bad Rule Suppression` and
`External Rule Suppression` contexts in the same file for the same
reason. The regular pwsh and WinPS runs of `RuleSuppression.tests.ps1`
(which `[+]` in the failing CI log) continue to exercise the assertion.
The new `UseDSCResourceFunctions.tests.ps1` test isn't dot-sourced by
`LibraryUsage.tests.ps1`, so it doesn't need the same guard.
Drafted by Copilot (Claude Opus 4.7).
|
@bergmeister I'll need your review since the super stringent code review requirements now bar me from merging after having Claude fix the failing test in CI. |
Sure. Will look at this and others tomorrow |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR prevents duplicate “unapplied suppression” errors that can occur during -Fix reanalysis passes and in DSC scenarios where both class-based and module-layout DSC logic apply to the same file.
Changes:
- Adds an
emitSuppressionErrorsswitch to avoid emitting suppression errors during intermediate-Fixpasses. - Makes DSC class-based vs module-layout analysis mutually exclusive to avoid duplicate suppression error emission.
- Adds regression tests for
-Fixreanalysis and mixed DSC layout/class scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Tests/Rules/UseDSCResourceFunctions.tests.ps1 | Adds regression test ensuring DSC suppression errors aren’t duplicated when both DSC detection paths apply. |
| Tests/Engine/RuleSuppression.tests.ps1 | Adds regression test ensuring -Fix reanalysis doesn’t emit multiple unapplied suppression errors. |
| Engine/ScriptAnalyzer.cs | Introduces emitSuppressionErrors plumbing and makes DSC analysis branches mutually exclusive to prevent duplicate suppression errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public List<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, out ScriptBlockAst scriptAst, out Token[] scriptTokens, bool skipVariableAnalysis = false, bool emitSuppressionErrors = true) | ||
| { |
| bool skipVariableAnalysis = false, | ||
| bool emitSuppressionErrors = true) | ||
| { |
| /// <param name="scriptTokens">Parsed tokens of <paramref name="scriptDefinition"/.></param> | ||
| /// <param name="skipVariableAnalysis">Whether variable analysis can be skipped (applicable if rules do not use variable analysis APIs).</param> |
PR Summary
-Fix, only emit errors for unused rule suppressions IDs on the final pass - not intermediateFix()passes.Added regression tests covering these 2 scenarios.
Fixes #2177
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.