Fix the regression in tab completing positional parameters - #27857
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in positional-parameter tab completion introduced by earlier changes to positional argument selection/prioritization. It restores the ability to try positional parameters from alternative parameter sets when the default parameter set’s positional parameter yields no completions, while keeping the “closest position” behavior.
Changes:
- Refactors
CompletePositionalArgumentto track all best-position candidates across valid parameter sets, trying the default-set candidate first and then alternatives until a completion is produced. - Adjusts fallback behavior so “remaining arguments” parameters are only considered when no applicable positional parameter is found.
- Adds tests to validate fallback across parameter sets and correct behavior when positions differ between parameter sets (with/without a default set).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs |
Refactors positional argument candidate discovery and processing order (default set first, then alternative sets), and tightens fallback to remaining-argument parameters. |
test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 |
Adds coverage for positional-parameter fallback across parameter sets and scenarios with differing positional declarations across sets. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Ilya (iSazonov)
left a comment
There was a problem hiding this comment.
LGTM.
Many thanks!
Have you plans to backport?
| if (defaultSetParam is not null) | ||
| { | ||
| ProcessParameter(commandName, commandAst, context, result, defaultSetParam, boundArguments); | ||
| if (result.Count > 0) |
There was a problem hiding this comment.
the result.Count could be 1 here where result[0] could be CompletionResult.Null. In that case we should continue down the line and not return.
There was a problem hiding this comment.
CompletionResult.Null is an indicator that means stopping subsequent fallback. We should follow the existing behavior -- only fall back when there is no result.
PR Summary
The PR #17796 and #18755 introduced a regression in tab completion --
dir | % <tab>completes with member names ofFileInfoobject before that change (v7.2 v7.3 behavior), but after that PR, it completes with file names within the current directory (v7.4 + behavior).This is because the original code that handled positional parameter completion was able to preserve an alternative non-default-set positional parameter, which it would fall back when the default-set positional parameter had no completion result. For
ForEach <tab>, the default-set positional parameter applicable for the position is-Process [ScriptBlock], which has no completion result, so it falls back to the non-default-set parameter-MemberName [string].However, the PR #17796 + #18755 removed the alternative positional parameter preservation, so for
ForEach <tab>, it will complete for-Process [ScriptBlock]only. That caused the regression.Improvements to positional parameter completion
The implementation before #17796 + #18755 is also inconsistent:
This pull request improves the logic for positional parameter completion in PowerShell. It fixed the regression and those inconsistencies. It keeps the improvement by #17796 + #18755 -- find the positional parameter with the closest position instead of the exact position. In addition, it prioritizes the default-set positional parameter and preserve non-default-set parameters with the same best position.
It refactored the logic in
CompletePositionalArgument(inCompletionCompleters.cs) to:Enhanced test coverage
Added new tests in
TabCompletion.Tests.ps1to verify:PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header