Skip to content

Fix Progress Preference variable in adv functions #24591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
daxian-dbw merged 7 commits into from
Jun 3, 2025
Merged

Fix Progress Preference variable in adv functions #24591

daxian-dbw merged 7 commits into from
Jun 3, 2025
+49 −1

Conversation

@cmkb3
Copy link
Contributor

@cmkb3 cmkb3 commented Nov 15, 2024

PR Summary

!Resubmission

This fixes an array out of bounds/type error that occurs when using the -ProgressAction common parameter in advanced functions. The reference variable and variable type arrays are missing the ProgressPreference entry which causes various engine errors including index out of bounds and invalid typecast from index collision.

PR Context

Fixes: #21074 'The -ProgressAction parameter doesn't work (PowerShell 7.4.1 only)'
Fixes: #20657 'The new -ProgressAction common parameter breaks calls to advanced scripts and functions. '

PR Checklist

@cmkb3 cmkb3 requested a review from daxian-dbw as a code owner November 15, 2024 04:46
@cmkb3
Copy link
Contributor Author

cmkb3 commented Nov 15, 2024

@iSazonov -- Resubmitted, included changes to test names as requested
@jborean93 -- Resubmitted; thanks for generating the initial tests and guiding me through my Git mess 🙃

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 15, 2024
@iSazonov iSazonov requested a review from SteveL-MSFT November 15, 2024 09:47
@iSazonov iSazonov added the WG-Engine-Performance core PowerShell engine, interpreter, and runtime performance label Nov 15, 2024
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 15, 2024

Request for Engine-Performance group. The PR updates code with performance optimizations. Please make conclusion about performance risks in some (maybe edge) scenarios.

@iSazonov iSazonov changed the title Fix Progress Preference handling, add tests/assert Fix Progress Preference variable in adv functions Nov 15, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Nov 23, 2024
@RylandDeGregory
Copy link

RylandDeGregory commented Mar 18, 2025

When should we expect this to be merged? It’s been an open PR for 4 months now…

@SeeminglyScience SeeminglyScience added the WG-NeedsReview Needs a review by the labeled Working Group label May 21, 2025
@daxian-dbw daxian-dbw self-assigned this May 23, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label May 23, 2025
@daxian-dbw daxian-dbw moved this from PR Todo to PR In Progress in PowerShell Team Reviews/Investigations May 23, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label May 30, 2025
@daxian-dbw
Copy link
Member

daxian-dbw commented Jun 3, 2025 •
edited
Loading

@cmkb3 Sorry it took so long to review this PR!

The fix looks good, but I pushed a commit for 2 minor changes:

  1. Move the assertions to the static constructor of Compiler. This is where AutomaticVariableTypes and PreferenceVariableTypes get used, and more importantly, this code path will always be hit, so if they are out of sync, we will know immediately.
  2. Update the tests a little -- a) $ps is created before each It test and disposed afterwards, so the newly added tests should not create new PowerShell instances. b) also add a parameter in the 2nd test case.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jun 3, 2025
SeeminglyScience
SeeminglyScience approved these changes Jun 3, 2025
View reviewed changes
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@daxian-dbw daxian-dbw merged commit b502098 into PowerShell:master Jun 3, 2025
34 checks passed
@github-project-automation github-project-automation bot moved this from PR In Progress to Done in PowerShell Team Reviews/Investigations Jun 3, 2025
@daxian-dbw daxian-dbw removed WG-Engine-Performance core PowerShell engine, interpreter, and runtime performance WG-NeedsReview Needs a review by the labeled Working Group labels Jun 3, 2025
@jborean93
Copy link
Collaborator

jborean93 commented Jun 3, 2025

Thanks for reviewing and merging this bugfix. Hopefully we can have it backported as well!

@mklement0 mklement0 mentioned this pull request Sep 15, 2025
Closed
5 tasks
@jborean93 jborean93 mentioned this pull request Dec 7, 2025
Closed
5 tasks
SIRMARGIN pushed a commit to SIRMARGIN/PowerShell that referenced this pull request Dec 12, 2025
@cmkb3 @daxian-dbw @SIRMARGIN
Fix the common parameter -ProgressAction for advanced functions (Po…
f52b336 
…werShell#24591)

Co-authored-by: Dongbo Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

Assignees

@daxian-dbw daxian-dbw

Labels

BackPort-7.4.x-Consider BackPort-7.5.x-Consider CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

PowerShell Team Reviews/Investigations
Status: Done

Milestone

No milestone

6 participants

@cmkb3 @iSazonov @RylandDeGregory @daxian-dbw @jborean93 @SeeminglyScience