Skip to content

Fix positional argument completion #17796

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

Merged
daxian-dbw merged 7 commits into from
Aug 22, 2022
Merged

Fix positional argument completion #17796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6a162f6
Fix positional argument completion
MartinGC94 Jul 28, 2022
b8f20b8
Merge branch 'master' into FixPositionalArgumentCompletion
MartinGC94 Jul 28, 2022
5f4ace3
Fix merge conflict
MartinGC94 Jul 28, 2022
25e2f9b
Update src/System.Management.Automation/engine/CommandCompletion/Comp…
MartinGC94 Aug 11, 2022
0e9d688
Minor update to a comment
daxian-dbw Aug 22, 2022
54d073a
Merge branch 'PowerShell:master' into FixPositionalArgumentCompletion
MartinGC94 Aug 22, 2022
ee31af5
Merge branch 'PowerShell:master' into FixPositionalArgumentCompletion
MartinGC94 Aug 22, 2022
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
47 changes: 31 additions & 16 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 @@ -1586,13 +1586,20 @@ private static void CompletePositionalArgument(
(defaultParameterSetFlag & validParameterSetFlags) != 0;
MergedCompiledCommandParameter positionalParam = null;

MergedCompiledCommandParameter bestMatchParam = null;
ParameterSetSpecificMetadata bestMatchSet = 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;
if (!isInParameterSet)
{
continue;
}

var parameterSetDataCollection = param.Parameter.GetMatchingParameterSetData(validParameterSetFlags);

foreach (ParameterSetSpecificMetadata parameterSetData in parameterSetDataCollection)
{
// in the first pass, we skip the remaining argument ones
Expand All @@ -1604,35 +1611,43 @@ private static void CompletePositionalArgument(
// Check the position
int positionInParameterSet = parameterSetData.Position;

if (positionInParameterSet == int.MinValue || positionInParameterSet != position)
if (positionInParameterSet < position)
{
// The parameter is not positional, or its position is not what we want
// The parameter is not positional (position == int.MinValue), or its position is lower than what we want.
continue;
}

if (isDefaultParameterSetValid)
if (bestMatchSet is null || bestMatchSet.Position > positionInParameterSet)
{
if (parameterSetData.ParameterSetFlag == defaultParameterSetFlag)
bestMatchParam = param;
bestMatchSet = parameterSetData;
if (positionInParameterSet == position)
{
ProcessParameter(commandName, commandAst, context, result, param, boundArguments);
isProcessedAsPositional = result.Count > 0;
break;
}
else
{
positionalParam ??= param;
}
}
}
}

if (bestMatchParam is not null)
{
if (isDefaultParameterSetValid)
{
if (bestMatchSet.ParameterSetFlag == defaultParameterSetFlag)
{
ProcessParameter(commandName, commandAst, context, result, bestMatchParam, boundArguments);
isProcessedAsPositional = result.Count > 0;
}
else
{
isProcessedAsPositional = true;
ProcessParameter(commandName, commandAst, context, result, param, boundArguments);
break;
positionalParam ??= bestMatchParam;
}
}

if (isProcessedAsPositional)
break;
else
{
isProcessedAsPositional = true;
ProcessParameter(commandName, commandAst, context, result, bestMatchParam, boundArguments);
}
}

if (!isProcessedAsPositional && positionalParam != null)
Expand Down
21 changes: 21 additions & 0 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 @@ -766,6 +766,27 @@ ConstructorTestClass(int i, bool b)
$res.CompletionMatches.CompletionText | Should -Contain "-Path"
}

it 'Should find the closest positional parameter match' {
$TestString = @'
function Verb-Noun
{
Param
(
[Parameter(Position = 0)]
[string]
$Param1,
[Parameter(Position = 1)]
[System.Management.Automation.ActionPreference]
$Param2
)
}
Verb-Noun -Param1 Hello ^
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Jul 29, 2022

Choose a reason for hiding this comment

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

What about more complex test like (expect Value3 in parameter position 3)
Verb-Noun -ParamPos0 Value0 ParamPos1Value1 -ParamPos2 Value2 ^

Copy link
Copy Markdown
Contributor Author

@MartinGC94 MartinGC94 Jul 29, 2022

Choose a reason for hiding this comment

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

Is it necessary? The logic would be the same as in the previous test: It takes the next available positional parameter. It doesn't really matter if it's the first, third or even the 30th parameter position it's looking for.

Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Jul 31, 2022

Choose a reason for hiding this comment

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

The code is too big to observe its logic in its entirety, and the test I'm asking about is a case I thought it might not work.

Copy link
Copy Markdown
Contributor Author

@MartinGC94 MartinGC94 Jul 31, 2022

Choose a reason for hiding this comment

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

That's fair. Here's how it works: The target position is found elsewhere and is based on how many positionally bound parameters have been specified, in the above example it would look for the second positional parameter (position 1) because one other positional parameter has been found.
The loop I've modified loops over the unbound parameters and parametersets that are valid based on the currently bound parameters, here it looks for any positional parameters with a position greater than or equal to the target location. It then saves whichever parameter has the lowest position and binds the argument to that.
Because it's looping over unbound parameters all the previous parameters "ParamPos0", "ParamPos1" and "ParamPos2" are automatically filtered out so the next available parameter would be "ParamPos3".

'@
$CursorIndex = $TestString.IndexOf('^')
$res = TabExpansion2 -inputScript $TestString.Remove($CursorIndex, 1) -cursorColumn $CursorIndex
$res.CompletionMatches[0].CompletionText | Should -Be "Break"
}

Context "Script name completion" {
BeforeAll {
Setup -f 'install-powershell.ps1' -Content ""
Expand Down