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 — Distinguish script-defined functions from DSC resources by Copilot · Pull Request #2297 · PowerShell/PSScriptAnalyzer Conversation
Contributor
PR SummaryFollow-up to #2270, targeting its branch rather than The review finding is reproducible: a resource-shaped call to a script-defined function is treated as a DSC declaration because command discovery uses a separate runspace. Commands inside its script block arguments are then incorrectly suppressed. Both new regression cases returned zero diagnostics before the fix instead of the two expected diagnostics.
Validation: .NET 8 and .NET Framework 4.6.2 builds succeeded. Alias tests: 23 passed, 2 Windows-only tests skipped on Linux. Formatter integration tests: 10 passed. Executed using PowerShell 7.4.13 and Pester 5.7.1; the configured Pester 6.0.1 package was unavailable from the accessible mirror. PR Checklist
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsResolve visible function declarations from the AST when classifying DSC resource syntax and checking implicit Get-prefix aliases. Preserve analysis of executable function arguments for both brace layouts. Co-authored-by: Copilot <[email protected]>
Copilot CLI created this pull request from a session on behalf of
Christoph Bergmeister (bergmeister)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedScoped function declarations are mishandled, causing incorrect alias rewrites and suppressed diagnostics, while repeated scope scans introduce quadratic analysis overhead. 2 open issuesWhat changed in this PRThis follow-up to #2270 distinguishes script-defined function calls from DSC resource declarations so alias analysis preserves function names while checking commands in their script-block arguments. Changes:
Review effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedScope handling still changes command resolution for dot-sourced private functions and incorrectly treats script/global declarations in unexecuted nested functions as visible, causing missed diagnostics and unsafe corrections. 2 open issues2 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedDot-sourced scope handling still misindexes declarations and incorrectly skips script-block argument scopes, causing both erroneous alias corrections and missed diagnostics. 2 open issues2 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedThe argument-count restriction incorrectly treats parameterized dot-sourced blocks as child scopes, causing erroneous alias formatting and missed declarations in the caller’s scope. 1 open issue2 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedUnresolved execution-order and scope-analysis errors cause valid function calls to be incorrectly rewritten as Get-Verb and required alias diagnostics to be suppressed. 4 open issuesReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedScope-boundary searches incorrectly isolate nested blocks in DSC callbacks and background pipelines, hiding enclosing local functions and causing erroneous alias rewrites. 2 open issues4 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedThe ancestor walk incorrectly treats a nested hashtable callback as an isolated DSC resource callback, causing a false Get-Verb suggestion; it needs to stop at an intervening ScriptBlockAst and gain regression coverage. 1 open issue2 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]> Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Needs a closer lookThe scope traversal incorrectly promotes global: and script: function declarations from background jobs into the foreground scope, suppressing required diagnostics for implicit Get-Verb calls. No open issues 🎉 1 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedUnresolved scope-isolation and execution-order errors in AvoidAlias can suppress required diagnostics or incorrectly rewrite valid function calls. 4 open issuesReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedThe pipeline check incorrectly rejects function declarations that execute before later calls within the same pipeline element, causing valid calls such as 1 open issue4 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Needs a closer lookHuman assessment is needed because subtle scope and execution-order interactions involving scriptblock casts, eager arguments, and background callbacks can cause incorrect alias resolution. No open issues 🎉 1 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedUnwrapping stops at 1 open issueReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Needs a closer lookPositional binding fails to exclude parameters supplied by name, causing incorrect execution-order analysis and an erroneous rewrite of No open issues 🎉 1 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedUnresolved callback-binding and execution-flow errors produce incorrect alias diagnostics by misclassifying positional callbacks and indexing declarations from nonexecuting iterators or discarded script blocks. 2 open issuesReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Needs a closer lookNamed-argument binding can cause incorrect alias rewrites, while oscillating scope resolution makes diagnostics depend on unrelated function declarations. No open issues 🎉 2 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedUnresolved execution-order and reachability errors in AvoidAlias cause valid function calls to be rewritten and required implicit-alias diagnostics to be suppressed. 6 open issues
Review effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedAvoidAlias misjudges function visibility across pipeline callbacks and eagerly evaluated arguments, causing incorrect alias rewrites and suppressed diagnostics. 2 open issues6 resolved since last review
Review effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedTwo critical pipeline callback-ordering errors can discard visible function declarations and cause the formatter to incorrectly replace script-defined 2 open issues2 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedPhase inference misclassifies script-block arguments and omits wrapped or array-valued callbacks, which can suppress expected diagnostics or incorrectly rewrite valid function calls. 1 open issue2 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Co-authored-by: bergmeister <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Needs a closer lookHuman assessment is needed because pipeline execution-order and background-job scoping interactions can cause incorrect function-visibility decisions and alias rewrites. No open issues 🎉 1 resolved since last reviewReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Changes recommendedFunction resolution still mishandles background runspaces, callback execution order, and discarded script blocks, suppressing required diagnostics or incorrectly rewriting real function calls. 3 open issuesReview effort: Balanced
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Comment on lines
+397
to
+399
Comment on lines
+1779
to
+1787
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Copilot code review
Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
At least 1 approving review is required to merge this pull request. Uh oh!
There was an error while loading. Please reload this page. Labels
None yet
ProjectsMilestoneNo milestoneDevelopmentSuccessfully merging this pull request may close these issues. Uh oh!
There was an error while loading. Please reload this page. Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| You are visitor 01402503 — GeezerVison does not store the pages you look up. Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required. |