From 56519a40deca28289b8edc95f5041f2ed1679131 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 14 Aug 2026 17:06:00 -0700 Subject: [PATCH] Fix the regression in tab completing positional parameters --- .../CommandCompletion/CompletionCompleters.cs | 130 ++++++++++++------ .../TabCompletion/TabCompletion.Tests.ps1 | 102 ++++++++++++-- 2 files changed, 180 insertions(+), 52 deletions(-) diff --git a/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs b/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs index 80cead788d3..7ba2e418b55 100644 --- a/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs +++ b/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs @@ -1738,15 +1738,22 @@ private static void CompletePositionalArgument( int position, Dictionary boundArguments = null) { - bool isProcessedAsPositional = false; bool isDefaultParameterSetValid = defaultParameterSetFlag != 0 && (defaultParameterSetFlag & validParameterSetFlags) != 0; - MergedCompiledCommandParameter positionalParam = null; - MergedCompiledCommandParameter bestMatchParam = null; - ParameterSetSpecificMetadata bestMatchSet = null; + // Find all the parameters with the position closest to the specified position. Different parameter + // sets can declare a parameter at the same position, so there can be more than one candidate. + // Only parameters from the still valid parameter sets are considered: 'GetMatchingParameterSetData' + // filters the parameter set data by 'validParameterSetFlags', and the default parameter set is + // given priority only when it's still valid. + // The candidate from the default parameter set is tried first, and when it doesn't produce any + // completion results, the candidates from the other parameter sets are tried in turn. + // For example, 'Get-Process | ForEach-Object ' should complete member names for '-MemberName' + // (PropertyAndMethodSet) because '-Process' (the default 'ScriptBlockSet') has nothing to offer. + int bestPosition = int.MaxValue; + MergedCompiledCommandParameter defaultSetParam = null; + List alternativeParams = null; - // Finds the parameter with the position closest to the specified position foreach (MergedCompiledCommandParameter param in parameters) { bool isInParameterSet = (param.Parameter.ParameterSetFlags & validParameterSetFlags) != 0 || param.Parameter.IsInAllSets; @@ -1755,6 +1762,7 @@ private static void CompletePositionalArgument( continue; } + bool addedToAltParamList = false; var parameterSetDataCollection = param.Parameter.GetMatchingParameterSetData(validParameterSetFlags); foreach (ParameterSetSpecificMetadata parameterSetData in parameterSetDataCollection) @@ -1774,64 +1782,100 @@ private static void CompletePositionalArgument( continue; } - if (bestMatchSet is null - || bestMatchSet.Position > positionInParameterSet - || (isDefaultParameterSetValid && positionInParameterSet == bestMatchSet.Position && defaultParameterSetFlag == parameterSetData.ParameterSetFlag)) + if (positionInParameterSet > bestPosition) { - bestMatchParam = param; - bestMatchSet = parameterSetData; - if (positionInParameterSet == position) + // A parameter closer to the specified position was already found. + continue; + } + + if (positionInParameterSet < bestPosition) + { + // This parameter is closer to the specified position, so the candidates found so far are no longer relevant. + bestPosition = positionInParameterSet; + defaultSetParam = null; + alternativeParams?.Clear(); + addedToAltParamList = false; + } + + // Prioritize the parameter from the default set. A parameter in all sets is considered from the default set if the + // default set is still valid. But, we prioritize it only if we have not found a default param yet. + // If 'defaultSetParam' is not null, then that means there are 2 parameters in the default set with the same position. + // That would be invalid parameter declaration, but we tolerate that in tab completion. + if (isDefaultParameterSetValid + && (parameterSetData.IsInAllSets || parameterSetData.ParameterSetFlag == defaultParameterSetFlag) + && defaultSetParam is null) + { + defaultSetParam = param; + + if (addedToAltParamList) + { + // If we already added the param to the list when processing a previous set, remove it from the list. + alternativeParams.RemoveAt(alternativeParams.Count - 1); + } + + if (bestPosition == position) { + // If it's the exact position, no need to go through the rest of the sets for this parameter. break; } - } - } - } - if (bestMatchParam is not null) - { - if (isDefaultParameterSetValid) - { - if (bestMatchSet.ParameterSetFlag == defaultParameterSetFlag) - { - ProcessParameter(commandName, commandAst, context, result, bestMatchParam, boundArguments); - isProcessedAsPositional = result.Count > 0; + // We still need to go through the rest of the sets in case that the position in any of them is closer. + // But for the same position from a different set, no need to add the param to the list anymore. + addedToAltParamList = true; } - else + else if (!addedToAltParamList) { - positionalParam ??= bestMatchParam; + addedToAltParamList = true; + (alternativeParams ??= new()).Add(param); } } - else + } + + if (defaultSetParam is not null) + { + ProcessParameter(commandName, commandAst, context, result, defaultSetParam, boundArguments); + if (result.Count > 0) { - isProcessedAsPositional = true; - ProcessParameter(commandName, commandAst, context, result, bestMatchParam, boundArguments); + return; } } - if (!isProcessedAsPositional && positionalParam != null) + if (alternativeParams?.Count > 0) { - isProcessedAsPositional = true; - ProcessParameter(commandName, commandAst, context, result, positionalParam, boundArguments); + // There are alternative parameters at the same best position. + // Process them in the discovery order. + foreach (MergedCompiledCommandParameter param in alternativeParams) + { + ProcessParameter(commandName, commandAst, context, result, param, boundArguments); + if (result.Count > 0) + { + // Stop at the first one that we successfully get completion results. + break; + } + } + + // We will return here no matter we get completion results or not, because we did find + // applicable positional parameters, and we have done processing them. + return; } - if (!isProcessedAsPositional) + // If we found no applicable positional parameter, then try the remaining argument parameters. + foreach (MergedCompiledCommandParameter param in parameters) { - foreach (MergedCompiledCommandParameter param in parameters) + bool isInParameterSet = (param.Parameter.ParameterSetFlags & validParameterSetFlags) != 0 || param.Parameter.IsInAllSets; + if (!isInParameterSet) { - bool isInParameterSet = (param.Parameter.ParameterSetFlags & validParameterSetFlags) != 0 || param.Parameter.IsInAllSets; - if (!isInParameterSet) - continue; + continue; + } - var parameterSetDataCollection = param.Parameter.GetMatchingParameterSetData(validParameterSetFlags); - foreach (ParameterSetSpecificMetadata parameterSetData in parameterSetDataCollection) + var parameterSetDataCollection = param.Parameter.GetMatchingParameterSetData(validParameterSetFlags); + foreach (ParameterSetSpecificMetadata parameterSetData in parameterSetDataCollection) + { + // in the second pass, we check the remaining argument ones + if (parameterSetData.ValueFromRemainingArguments) { - // in the second pass, we check the remaining argument ones - if (parameterSetData.ValueFromRemainingArguments) - { - ProcessParameter(commandName, commandAst, context, result, param, boundArguments); - break; - } + ProcessParameter(commandName, commandAst, context, result, param, boundArguments); + break; } } } diff --git a/test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 b/test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 index f8762a63929..27e948074aa 100644 --- a/test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 +++ b/test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1 @@ -51,7 +51,7 @@ Describe "TabCompletion" -Tags CI { New-ModuleManifest -Path "$($NewDir.FullName)\$ModuleName.psd1" -RootModule "$ModuleName.psm1" -FunctionsToExport "MyTestFunction" -ModuleVersion $NewDir.Name } - $env:PSModulePath += [System.IO.Path]::PathSeparator + $tempDir + $env:PSModulePath += [System.IO.Path]::PathSeparator + $tempDir $Res = TabExpansion2 -inputScript MyTestFunction $Res.CompletionMatches.Count | Should -Be 2 $SortedMatches = $Res.CompletionMatches.CompletionText | Sort-Object @@ -82,7 +82,7 @@ Describe "TabCompletion" -Tags CI { New-ModuleManifest -Path "$($NewDir.FullName)\$ModuleName.psd1" -RootModule "$ModuleName.psm1" -FunctionsToExport "MyTestFunction" -ModuleVersion $NewDir.Name } - $env:PSModulePath += [System.IO.Path]::PathSeparator + $tempDir + $env:PSModulePath += [System.IO.Path]::PathSeparator + $tempDir $Res = TabExpansion2 -inputScript 'Import-Module -Name TestModule' $Res.CompletionMatches.Count | Should -Be 1 $Res.CompletionMatches[0].CompletionText | Should -Be TestModule1 @@ -165,21 +165,21 @@ Describe "TabCompletion" -Tags CI { $res = TabExpansion2 -inputScript 'param($PS = $P' $res.CompletionMatches.Count | Should -BeGreaterThan 0 } - + It 'Should complete variable with description and value ' -TestCases @( @{ Value = 1; Expected = '[int]$VariableWithDescription - Variable description' } @{ Value = 'string'; Expected = '[string]$VariableWithDescription - Variable description' } @{ Value = $null; Expected = 'VariableWithDescription - Variable description' } ) { param ($Value, $Expected) - + New-Variable -Name VariableWithDescription -Value $Value -Description 'Variable description' -Force $res = TabExpansion2 -inputScript '$VariableWithDescription' $res.CompletionMatches.Count | Should -Be 1 $res.CompletionMatches[0].CompletionText | Should -BeExactly '$VariableWithDescription' $res.CompletionMatches[0].ToolTip | Should -BeExactly $Expected } - + It 'Should complete environment variable' { try { $env:PWSH_TEST_1 = 'value 1' @@ -226,7 +226,7 @@ Describe "TabCompletion" -Tags CI { @{ Value = $null; Expected = 'VariableWithDescription - Variable description' } ) { param ($Value, $Expected) - + New-Variable -Name VariableWithDescription -Value $Value -Description 'Variable description' -Force $res = TabExpansion2 -inputScript '$local:VariableWithDescription' $res.CompletionMatches.Count | Should -Be 1 @@ -1191,7 +1191,7 @@ param([ValidatePattern( [Parameter(ParameterSetName = 'SetWithoutHelp')] [string] $ParamWithHelp, - + [Parameter(ParameterSetName = 'SetWithHelp')] [switch] $ParamWithoutHelp @@ -1733,7 +1733,7 @@ param([ValidatePattern( $commaSeparators = "',' ', '" $semiColonSeparators = "';' '; '" - + $squareBracketFormatString = "'[{0}]'" $curlyBraceFormatString = "'{0:N2}'" } @@ -2097,6 +2097,90 @@ Verb-Noun -Param1 Hello ^ $res.CompletionMatches[0].CompletionText | Should -Be "Attributes" } + it 'Should fall back to a positional parameter in another parameterset when the default parameterset has no completions' { + $ScriptInput = 'Get-Process | ForEach-Object ' + $res = TabExpansion2 -inputScript $ScriptInput -cursorColumn $ScriptInput.Length + # '-Process' is positional in the default 'ScriptBlockSet' but has no completions to offer, + # so the completion should fall back to '-MemberName' in the 'PropertyAndMethodSet'. + $res.CompletionMatches.CompletionText | Should -Contain "ProcessName" + # Make sure we didn't fall through to file name completion. + $res.CompletionMatches.ResultType | Should -Not -Contain ([System.Management.Automation.CompletionResultType]::ProviderItem) + } + + it 'Should keep trying the positional parameters from other parameter sets until completions are found' { + $TestString = @' +function Verb-Noun +{ + [CmdletBinding(DefaultParameterSetName = 'ScriptBlockSet')] + Param + ( + [Parameter(ParameterSetName = 'ScriptBlockSet', Position = 0)] + [scriptblock] + $ScriptBlockParam, + [Parameter(ParameterSetName = 'StringSet', Position = 0)] + [string] + $StringParam, + [Parameter(ParameterSetName = 'ValidateSetSet', Position = 0)] + [ValidateSet('Alpha', 'Beta')] + [string] + $ValidateSetParam + ) +} +Verb-Noun ^ +'@ + $CursorIndex = $TestString.IndexOf('^') + $res = TabExpansion2 -inputScript $TestString.Remove($CursorIndex, 1) -cursorColumn $CursorIndex + # Neither the default set nor the 'StringSet' parameter has completions to offer, + # so we should keep going until the 'ValidateSetSet' parameter is reached. + $res.CompletionMatches.CompletionText -join ',' | Should -BeExactly 'Alpha,Beta' + } + + it 'Should complete a positional parameter declared at different positions in different parameter sets' { + $TestString = @' +function Verb-Noun +{ + [CmdletBinding(DefaultParameterSetName = 'SetB')] + Param + ( + [Parameter(ParameterSetName = 'SetB', Position = 0)] + [Parameter(ParameterSetName = 'SetA', Position = 1)] + [ValidateSet('Alpha', 'Beta')] + [string] + $Param1 + ) +} +Verb-Noun ^ +'@ + $CursorIndex = $TestString.IndexOf('^') + $res = TabExpansion2 -inputScript $TestString.Remove($CursorIndex, 1) -cursorColumn $CursorIndex + # The parameter set data is enumerated in the reverse order of the declaration, so 'SetA' with the + # position 1 is seen first, and then 'SetB' with the closer position 0. The parameter must still be + # a candidate after the closer position is found. + $res.CompletionMatches.CompletionText -join ',' | Should -BeExactly 'Alpha,Beta' + } + + it 'Should complete a positional parameter declared at different positions when there is no default parameterset' { + $TestString = @' +function Verb-Noun +{ + Param + ( + [Parameter(ParameterSetName = 'SetB', Position = 0)] + [Parameter(ParameterSetName = 'SetA', Position = 1)] + [ValidateSet('Alpha', 'Beta')] + [string] + $Param1 + ) +} +Verb-Noun ^ +'@ + $CursorIndex = $TestString.IndexOf('^') + $res = TabExpansion2 -inputScript $TestString.Remove($CursorIndex, 1) -cursorColumn $CursorIndex + # Same as above, but without a default parameter set the parameter is tracked as an alternative + # candidate instead, and it must survive finding the closer position. + $res.CompletionMatches.CompletionText -join ',' | Should -BeExactly 'Alpha,Beta' + } + it 'Should complete base class members of types without type definition AST' { $res = TabExpansion2 -inputScript @' class InheritedClassTest : System.Attribute @@ -2408,7 +2492,7 @@ param ($Param1) $null = New-Item -Path $TestFile $res = TabExpansion2 -ast $scriptAst -tokens $tokens -positionOfCursor $cursorPosition Pop-Location - + $ExpectedPath = Join-Path -Path '.\' -ChildPath $ExpectedFileName $res.CompletionMatches.CompletionText | Where-Object {$_ -Like "*$ExpectedFileName"} | Should -Be $ExpectedPath }