Skip to content

Fix the regression in tab completing positional parameters #27857

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

Draft
Dongbo Wang (daxian-dbw) wants to merge 1 commit into
base: master
Choose a base branch
from
+180 −52
Draft

Fix the regression in tab completing positional parameters #27857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 87 additions & 43 deletions src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1738,15 +1738,22 @@ private static void CompletePositionalArgument(
int position,
Dictionary<string, AstParameterArgumentPair> 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 <Tab>' should complete member names for '-MemberName'
// (PropertyAndMethodSet) because '-Process' (the default 'ScriptBlockSet') has nothing to offer.
int bestPosition = int.MaxValue;
MergedCompiledCommandParameter defaultSetParam = null;
List<MergedCompiledCommandParameter> 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;
Expand All @@ -1755,6 +1762,7 @@ private static void CompletePositionalArgument(
continue;
}

bool addedToAltParamList = false;
var parameterSetDataCollection = param.Parameter.GetMatchingParameterSetData(validParameterSetFlags);

foreach (ParameterSetSpecificMetadata parameterSetData in parameterSetDataCollection)
Expand All @@ -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;
}
}
}
Expand Down
102 changes: 93 additions & 9 deletions test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 <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'
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1191,7 +1191,7 @@ param([ValidatePattern(
[Parameter(ParameterSetName = 'SetWithoutHelp')]
[string]
$ParamWithHelp,

[Parameter(ParameterSetName = 'SetWithHelp')]
[switch]
$ParamWithoutHelp
Expand Down Expand Up @@ -1733,7 +1733,7 @@ param([ValidatePattern(

$commaSeparators = "',' ', '"
$semiColonSeparators = "';' '; '"

$squareBracketFormatString = "'[{0}]'"
$curlyBraceFormatString = "'{0:N2}'"
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
Loading