Skip to content

Fix tab completion for filename contains literal wildcard character #7407

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

Closed
kwkam wants to merge 10 commits into from
Closed

Fix tab completion for filename contains literal wildcard character #7407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
931f073
[Feature] Fix file completion with literal wildcard char
kwkam Jul 30, 2018
b8ce02f
Do not use alias in TabCompletion test
kwkam Aug 31, 2018
27cd737
Also test the [WildcardPattern]::Escape function
kwkam Aug 31, 2018
7ab5699
Merge branch 'master' into wc-tabcomp
kwkam Jan 11, 2019
c710d39
Merge branch 'master' into wc-tabcomp
kwkam Mar 6, 2019
40fd00a
Update for ExperimentalFeature
kwkam Mar 6, 2019
98086fc
Merge branch 'master' into wc-tabcomp
kwkam May 8, 2019
8b8c2ed
Add copyright header
kwkam Jun 22, 2019
811b574
Insert blank line after header
kwkam Jun 24, 2019
a09ac29
Merge branch 'master' into wc-tabcomp
kwkam Jun 4, 2020
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
20 changes: 6 additions & 14 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 @@ -4398,6 +4398,12 @@ internal static IEnumerable<CompletionResult> CompleteFilename(CompletionContext
if (CompletionRequiresQuotes(completionText, !useLiteralPath))
{
var quoteInUse = quote == string.Empty ? "'" : quote;

Copy link
Member

@TravisEz13 TravisEz13 Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

should all changes be behind the experimental feature?

Copy link
Member

@TravisEz13 TravisEz13 May 27, 2020

Choose a reason for hiding this comment

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

This was a condition of the committee

Copy link
Contributor Author

@kwkam kwkam Jun 4, 2020

Choose a reason for hiding this comment

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

Except filename completion will now also escape * and ? in non-literal path, the behaviour should be identical unless the experimental feature (the breaking part) is enabled.

if (!useLiteralPath)
{
completionText = WildcardPattern.Escape(completionText);
}

if (quoteInUse == "'")
{
completionText = completionText.Replace("'", "''");
Expand All @@ -4410,20 +4416,6 @@ internal static IEnumerable<CompletionResult> CompleteFilename(CompletionContext
completionText = completionText.Replace("$", "`$");
}

if (!useLiteralPath)
{
if (quoteInUse == "'")
{
completionText = completionText.Replace("[", "`[");
completionText = completionText.Replace("]", "`]");
}
else
{
completionText = completionText.Replace("[", "``[");
completionText = completionText.Replace("]", "``]");
}
}

completionText = quoteInUse + completionText + quoteInUse;
}
else if (quote != string.Empty)
Expand Down
3 changes: 3 additions & 0 deletions src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ static ExperimentalFeature()
new ExperimentalFeature(
name: "PSCommandNotFoundSuggestion",
description: "Recommend potential commands based on fuzzy search on a CommandNotFoundException"),
new ExperimentalFeature(
name: "PSWildcardEscapeEscape",
description: "Fix WildcardPattern API: escape the escape character"),
#if UNIX
new ExperimentalFeature(
name: "PSUnixFileStat",
Expand Down
21 changes: 17 additions & 4 deletions src/System.Management.Automation/engine/regex.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,28 @@ internal static string Escape(string pattern, char[] charsNotToEscape)

Span<char> temp = pattern.Length < StackAllocThreshold ? stackalloc char[pattern.Length * 2 + 1] : new char[pattern.Length * 2 + 1];
int tempIndex = 0;
bool charNeedsEscaping = false;

for (int i = 0; i < pattern.Length; i++)
{
char ch = pattern[i];

//
// if it is a wildcard char, escape it
//
if (IsWildcardChar(ch) && !charsNotToEscape.Contains(ch))
if (ExperimentalFeature.IsEnabled("PSWildcardEscapeEscape"))
{
//
// if it is a wildcard char or escape char, escape it
//
charNeedsEscaping = IsWildcardChar(ch) || ch == escapeChar;
}
else
{
//
// if it is a wildcard char, escape it
//
charNeedsEscaping = IsWildcardChar(ch);
}

if (charNeedsEscaping && !charsNotToEscape.Contains(ch))
{
temp[tempIndex++] = escapeChar;
}
Expand Down
8 changes: 2 additions & 6 deletions src/System.Management.Automation/namespaces/LocationGlobber.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3360,13 +3360,11 @@ private List<string> GenerateNewPSPathsWithGlobLeaf(
StringContainsGlobCharacters(leafElement) ||
isLastLeaf)
{
string regexEscapedLeafElement = ConvertMshEscapeToRegexEscape(leafElement);
Copy link
Member

@TravisEz13 TravisEz13 Jun 21, 2019

Choose a reason for hiding this comment

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

Can you explain this change?

Copy link
Contributor Author

@kwkam kwkam Jun 22, 2019

Choose a reason for hiding this comment

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

As explained in the PR Summary, regexEscapedLeafElement is passed to WildcardPattern::Get, which does not make sense since in PowerShell, regex escaped string is \[abc\], while wildcard escaped string is `[abc`].
Moreover, why should we escape/alter the pattern string in methods GenerateNew*PathsWithGlobLeaf which is supposed to glob? This behaviour is causing issue on tab completion when filename contains bracket.

Copy link
Contributor Author

@kwkam kwkam Jun 4, 2020

Choose a reason for hiding this comment

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

Can I mark this resolved?


// Construct the glob filter

WildcardPattern stringMatcher =
WildcardPattern.Get(
regexEscapedLeafElement,
leafElement,
WildcardOptions.IgnoreCase);

// Construct the include filter
Expand Down Expand Up @@ -3966,13 +3964,11 @@ internal List<string> GenerateNewPathsWithGlobLeaf(
(StringContainsGlobCharacters(leafElement) ||
isLastLeaf))
{
string regexEscapedLeafElement = ConvertMshEscapeToRegexEscape(leafElement);

// Construct the glob filter

WildcardPattern stringMatcher =
WildcardPattern.Get(
regexEscapedLeafElement,
leafElement,
WildcardOptions.IgnoreCase);

// Construct the include filter
Expand Down
3 changes: 3 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 @@ -1055,6 +1055,9 @@ dir -Recurse `
@{ inputStr = "Get-Process >'.\My ``[Path``]\'"; expected = "'.${separator}My ``[Path``]${separator}test.ps1'" }
@{ inputStr = "Get-Process >${tempDir}\My"; expected = "'${tempDir}${separator}My ``[Path``]'" }
@{ inputStr = "Get-Process > '${tempDir}\My ``[Path``]\'"; expected = "'${tempDir}${separator}My ``[Path``]${separator}test.ps1'" }
@{ inputStr = "Set-Location -Path 'My ``["; expected = "'.${separator}My ``[Path``]'" }
@{ inputStr = "Get-Process > 'My ``["; expected = "'.${separator}My ``[Path``]'" }
@{ inputStr = "Get-Process > '${tempDir}\My ``["; expected = "'${tempDir}${separator}My ``[Path``]'" }
)

Push-Location -Path $tempDir
Expand Down
68 changes: 68 additions & 0 deletions test/powershell/engine/Api/WildcardPattern.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Member

@TravisEz13 TravisEz13 Oct 27, 2020

Choose a reason for hiding this comment

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

Suggested change
# Copyright (c) Microsoft Corporation. All rights reserved.
# Copyright (c) Microsoft Corporation.

# Licensed under the MIT License.

Describe "WildcardPattern Escape - Experimental-Feature-Disabled" -Tags "CI" {

BeforeAll {
$testName = 'PSWildcardEscapeEscape'
$skipTest = $EnabledExperimentalFeatures.Contains($testName)

if ($skipTest) {
Write-Verbose "Test Suite Skipped. The test suite requires the experimental feature '$testName' to be disabled." -Verbose
$originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
$PSDefaultParameterValues["it:skip"] = $true
}
}

AfterAll {
if ($skipTest) {
$global:PSDefaultParameterValues = $originalDefaultParameterValues
}
}

It "Unescaping '<escapedStr>' which escaped from '<inputStr>' should get the original" -TestCases @(
@{inputStr = '*This'; escapedStr = '`*This'}
@{inputStr = 'Is?'; escapedStr = 'Is`?'}
@{inputStr = 'Real[ly]'; escapedStr = 'Real`[ly`]'}
@{inputStr = 'Ba`sic'; escapedStr = 'Ba`sic'}
@{inputStr = 'Test `[more`]?'; escapedStr = 'Test ``[more``]`?'}
) {
param($inputStr, $escapedStr)

[WildcardPattern]::Escape($inputStr) | Should -BeExactly $escapedStr
[WildcardPattern]::Unescape($escapedStr) | Should -BeExactly $inputStr
}
}

Describe "WildcardPattern Escape - Experimental-Feature-Enabled" -Tags "CI" {

BeforeAll {
$testName = 'PSWildcardEscapeEscape'
$skipTest = -not $EnabledExperimentalFeatures.Contains($testName)

if ($skipTest) {
Write-Verbose "Test Suite Skipped. The test suite requires the experimental feature '$testName' to be enabled." -Verbose
$originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
$PSDefaultParameterValues["it:skip"] = $true
}
}

AfterAll {
if ($skipTest) {
$global:PSDefaultParameterValues = $originalDefaultParameterValues
}
}

It "Unescaping '<escapedStr>' which escaped from '<inputStr>' should get the original" -TestCases @(
@{inputStr = '*This'; escapedStr = '`*This'}
@{inputStr = 'Is?'; escapedStr = 'Is`?'}
@{inputStr = 'Real[ly]'; escapedStr = 'Real`[ly`]'}
@{inputStr = 'Ba`sic'; escapedStr = 'Ba``sic'}
@{inputStr = 'Test `[more`]?'; escapedStr = 'Test ```[more```]`?'}
) {
param($inputStr, $escapedStr)

[WildcardPattern]::Escape($inputStr) | Should -BeExactly $escapedStr
[WildcardPattern]::Unescape($escapedStr) | Should -BeExactly $inputStr
}
}
1 change: 1 addition & 0 deletions test/tools/TestMetadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"ExperimentalFeatures": {
"PSWildcardEscapeEscape": [ "test/powershell/engine/Api/WildcardPattern.Tests.ps1" ],
"ExpTest.FeatureOne": [ "test/powershell/engine/ExperimentalFeature/ExperimentalFeature.Basic.Tests.ps1" ],
"PSNullConditionalOperators": [
"test/powershell/Language/Operators/NullConditional.Tests.ps1",
Expand Down