Skip to content
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.

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

ArgumentOutOfRangeException: Completion of Splat, but Not $ #983

Closed
msftrncs opened this issue Jul 31, 2019 · 11 comments · Fixed by #984
Closed

ArgumentOutOfRangeException: Completion of Splat, but Not $ #983

msftrncs opened this issue Jul 31, 2019 · 11 comments · Fixed by #984

Comments

@msftrncs
Copy link
Collaborator

msftrncs commented Jul 31, 2019

Using a custom build of PS 7 Preview, but also applies to 5.1 and 6/6.1/6.2 in the right conditions.

I have been working with the PowerShell code to try to get completion starting at a splat @ to work. With that working, the current completion code completes the $ variable as @{$}, which is knowingly invalid, and it causes an ArgumentOutOfRange exception.

Steps to reproduce or exception report

This reproduction is for the released builds of PowerShell, as they will not currently produce completions for just a @ argument.

Complete the following:

echo @?<CTRL SPACE> # crashes after list is displayed, but before the first result is inserted
echo $<CTRL SPACE> # notice selection varies with variable names that require a brace

Note, the window must be large enough as to be able to display the entire list without the input line scrolling off the top. Also, I've only tested this in Windows.

Code in Question

I traced down the code that is calling the exception:

private int FindUserCompletionTextPosition(CompletionResult match, string userCompletionText)
{
return match.ResultType == CompletionResultType.Variable && match.CompletionText[1] == '{'
? match.CompletionText.IndexOf(userCompletionText.Substring(1), StringComparison.OrdinalIgnoreCase) - 1
: match.CompletionText.IndexOf(userCompletionText, StringComparison.OrdinalIgnoreCase);
}

I think the check for a { is incorrect here, and that there is no reason to ever substring the original text, removing the prefix, which causes the completion to look like its replacing the prefix as well.

@msftrncs
Copy link
Collaborator Author

I'm also pretty sure there is some code somewhere else dealing specifically with the @ condition, and its not just the linked to function above that's at problem, because, why else would it exist.

@msftrncs
Copy link
Collaborator Author

Also reported at PowerShell/PowerShell#10243

@msftrncs
Copy link
Collaborator Author

I think the error is here:

private static string GetUnquotedText(CompletionResult match, bool consistentQuoting)
{
var s = match.CompletionText;
if (match.ResultType == CompletionResultType.Variable)
{
if (IsQuotedVariable(s))
{
return '$' + s.Substring(2, s.Length - 3);
}
return s;
}
return GetUnquotedText(s, consistentQuoting);
}

The CompletionResultType is Variable for a splat, and so PSReadLine for some reason is tearing off the braces, but while doing so changes the @ to a $, and then later attempts to compare that with each completion to see how much of the command its completing for highlighting purposes, but since the @ is now a $, the comparison determines that no text changes (or stays the same or whatever), which results in an empty string which then ends up as the userCompletionText in FindUserCompletionTextPosition which because the second character of the match.CompletionText is a '{' results in a substring(1) of an empty string, ArgumentOutOfRangeException.

msftrncs added a commit to msftrncs/PSReadLine that referenced this issue Jul 31, 2019
Do not adjust userCompletionText while finding userCompletion text position
when match contains `{` in CompletionText[1], and preserve the splat sigil
at start of completions of type `Variable`.

Adjusting userCompletionText led to selection box consuming sigil for
braced variables, or crash on incorrectly braced splats.
@daxian-dbw
Copy link
Member

echo @? # crashes after list is displayed, but before the first result is inserted
echo $ # notice selection varies with variable names that require a brace

@msftrncs I cannot repro the crash with these steps. I'm using 7.0-preview.1 (see the screen gif below).
Does the console buffer size need to be somehow configured to repro this?

crash-not-repro
crash-not-repro2

@msftrncs
Copy link
Collaborator Author

@daxian-dbw, yes, the buffer needs enough lines visible so that the list can populate without the input line scrolling off the top, so that the first item in the list can replace the current input.

image

@daxian-dbw
Copy link
Member

yes, the buffer needs enough lines visible so that the list can populate without the input line scrolling off the top, so that the first item in the list can replace the current input.

Thanks, that helps. I can repro the issue now.

@msftrncs
Copy link
Collaborator Author

msftrncs commented Jul 31, 2019

I think I forgot to include that echo @?<TAB> is also a quick way to produce the error, as long as the first completion returned is the typical @{$}.

oops, guess it doesn't, that works...

msftrncs added a commit to msftrncs/PSReadLine that referenced this issue Aug 1, 2019
Prevent adjustment of userCompletionText while finding userCompletion text
position when match contains `{` in CompletionText[1] if the
userCompletionText is not longer than 1 char, and preserve the splat sigil
at start of completions of type `Variable`.

Adjusting userCompletionText blindly led to ArgumentOutOfRangeException on
incorrectly braced splats on input `@?`.
@msftrncs
Copy link
Collaborator Author

msftrncs commented Aug 1, 2019

I think I figured out what the check for a { is doing …

The detection for a { appears to be to handle the situation where @en is the input, and then when @{env:blah(blah)} is the completion, the selection becomes 'v:blah(blah)}`, preventing the matching process from selecting from the first brace.

@daxian-dbw
Copy link
Member

@msftrncs I can reproduce the crash using echo @?<CTRL SPACE>, but not exactly sure what you mean with the second repro echo $<CTRL SPACE>.

# notice selection varies with variable names that require a brace

The selection looks fine to me for variables that require a brace. Can you please clarify it a bit more?

image

@msftrncs
Copy link
Collaborator Author

msftrncs commented Sep 6, 2019

echo $<CTRL SPACE> # notice selection varies with variable names that require a brace

The selection looks fine to me for variables that require a brace. Can you please clarify it a bit more?

Hopefully these snips clarify the comment, but because it could be said better: 'the selection of the leading $ varies depending on whether the variable name requires a brace, or not.'

image
image
image

Because the $ sigil is there in all cases, it should not be part of the selection. The fix in PR #984 acts like this:
image
image
image

@daxian-dbw
Copy link
Member

Thanks for the further clarification. You really have a pair of sharp eyes to find this inconsistency 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants