Remove CompletionHelpers `escape` parameter from `CompletionRequiresQuotes` #25178

ArmaanMcleod · 2025-03-15T03:46:37Z

PR Summary

Just minor refactor to removed unused escape parameter from CompletionRequiresQuotes method. Given this parameter was meant to be used for escaping globbing characters [ & ] which is not set to true anywhere in the code base, it makes sense to remove it to clean up the code and simplify how this method gets used.

PR Context

This code came from Windows PowerShell 1bd9919 as dead and has not been used until now.

PR Checklist

iSazonov · 2025-03-15T06:40:53Z

src/System.Management.Automation/engine/CommandCompletion/CompletionHelpers.cs

@@ -14,7 +14,7 @@ internal static class CompletionHelpers
    {
        private static readonly SearchValues<char> s_defaultCharsToCheck = SearchValues.Create("$`");

-        private static readonly SearchValues<char> s_escapeCharsToCheck = SearchValues.Create("$[]`");
+        private static readonly SearchValues<char> s_escapeGlobbingPathCharsToCheck = SearchValues.Create("$[]`");


I wonder why do you name the chars "GlobbingPathChars"?

Interesting, is this used at all?

So currently it is not used, but it is from the CompletionRequiresQuotes method. Most usage I see does not use it and keeps it false.

Perhaps we just remove it if not used instead of refacoring? Or we just decide to escape all special characters without this extra flag.

Could you please look history of the file, maybe it was used in past?
It would be nice to understand why it is in code.
If it is dead code we should remove it.

@iSazonov So it seemed the original author wanted to be able to have the choice to escape globbed chars. To be honest I am not sure why it was ever flagged, there is no current code relying on it. But also making it the default could be done. Just not sure if it would introduce regression.

I think for backwards compatibility and less risk of introducing regression, it seems fine to just rename this parameter to something more meaningful like in this PR. Currently escape on its own does not explain much what it is doing.

But we can also completely remove it and just escape $ and backtick. We can reintroduce it later if it is needed again for [ and ]. I agree right now it is dead code.

We use WildcardPattern for filtering.
WildcardPattern uses brackets and backtick as special chars. So I'd expect we escape them for example for file paths but they escaped already so it is in another code path. So It is an open question.
Why should we escape $? Comment says about variables but variables are processed with another method ContainsCharactersRequiringQuotes. So it is an open question too.

@MartinGC94 Have you any thoughts?

ArmaanMcleod · 2025-03-16T08:29:49Z

@iSazonov I think we can safely remove this code: 1bd9919

Escape globbing path chars is not being used anywhere so it really is dead code. Let me know if you you are ok with repurposing this PR.

iSazonov · 2025-03-16T08:44:02Z

I do not have a complete understanding of what needs to be escaped and whether this is always a general rule. Or since we are talking about a common method, the question is whether there can be completers for which we need to escape a different set of characters.

I'd want to get a feedback from @MartinGC94.

This reverts commit 1bd9919.

MartinGC94 · 2025-03-16T15:09:20Z

I agree that the current name is bad, though I'd remove "Path" from the new name as this is not exclusive to paths. However, I don't see much value in renaming a parameter that is unused. Why not just rename it when you start using it for the first time? Or remove it if there's no plans to use it?

As for the code itself, it is flawed and I question its usefulness even if it was fixed.
1: It doesn't check for every problematic character. If I for example create a function like this: New-Item -Path 'function:\-G' -Value {'Hello'} and try to complete it: Get-Command -Name <Tab> it won't be quoted in the list of results because it doesn't check for the dash.
2: Even for the characters it does check it's not useful to quote something just because it has wildcard characters. For example if I created this function: New-Item -Path 'function:\G[a-z]' -Value {'Hello'} it would return true (that quotes are needed) but quoting doesn't stop it from being interpreted as a wildcard so what use is that?
3: Parsing the completion value on its own (meaning it's parsed in Expression Mode) to determine whether or not quotes are needed is not useful when it's an argument because the rules are slightly different. For example an IP address like: 192.168.1.1 is invalid in Expression mode as it sees the first 2 octets as a number, and the last 2 octets as accessing a member of that first number. Whereas as an argument: Get-NetIPAddress -IPAddress 192.168.1.1 it's perfectly valid.

iSazonov · 2025-03-17T05:05:39Z

@MartinGC94 Thanks!

A comment for the code says about paths and variables. I guess the code was used for file paths and variables many years ago. Later specific code was created for file paths and variables.
So I think we should remove the old code and parameter.

Looking as CompletionRequiresQuotes is used (14 call sites) I see it is for arguments only. So questions are:

Currently we check only $ and backtick. Does it make sense to check the chars? What chars are it makes sense to check for follow argument quoting?
I see globbing for one char works in tab completion New-Item -Path c:\W[i]<Tab>. But all globbing features don't work. Is it by design or accidentally? Should we support globbing in tab completion for arguments? I guess it is only a side effect of WildcardPattern using.
Should we continue to use Parser for arguments? And does we use it in right way?

@ArmaanMcleod @MartinGC94 What do you think?

ArmaanMcleod · 2025-03-17T09:39:58Z

So I think we should remove the old code and parameter.

@iSazonov I personally think if we have old code which is not used and doesn't provide much value, we should remove it and perhaps revisit how it is done.

Currently we check only $ and backtick. Does it make sense to check the chars? What chars are it makes sense to check for follow argument quoting?

I think we should check any char which causes problems for escaping single/double quotes. Like I understand the basic examples which need escaping:

single quotes in single quote e.g. ' another, 'quote'
backticks in double quotes e.g. "another, `backtick"

I like how @MartinGC94 has done this with bad chars in https://github.com/MartinGC94/UsefulArgumentCompleters/blob/bd94bb0b5fadc92e95ea32f70b22eeb35ab4b0b7/Classes/CompletionHelper.psm1#L21-L46. I am wondering if we can use a similar list for this repo so we can at least reduce all the obvious bad cases which come up. I guess I'm just not sure how to expose all these bad cases.

Should we continue to use Parser for arguments? And does we use it in right way?

I guess my understanding is that when you parse input for tokens you can get errors, so it was an easy way to check a string is not valid and requires quotes.

It's hard to check if it's the right way or if there were suggested alternatives, a lot of the CompletionCompleters.cs GIT history is from the 60b3b30 when the repository was opensourced 😄.

GitHub
UsefulArgumentCompleters/Classes/CompletionHelper.psm1 at bd94bb0b5fadc92e95ea32f70b22eeb35ab4b0b7 · MartinGC94/UsefulArgumentCompleters
Contribute to MartinGC94/UsefulArgumentCompleters development by creating an account on GitHub.

iSazonov · 2025-03-17T10:12:58Z

@ArmaanMcleod Thanks for referencing the module!

If it is possible to create a test (I mainly assume that we should scenarios where this can happen) for every checked char I believe we can accept the checks from @MartinGC94 module. The same for Parser checks. It is for follow PRs.

As for removing old code, we can continue in the PR.

This reverts commit e6ae71c.

ArmaanMcleod · 2025-03-17T11:13:45Z

@iSazonov Thanks. I have repurposed this PR to just remove the escape parameter.

iSazonov · 2025-03-17T16:56:22Z

@MartinGC94 Do you have any interest in transferring the escaping capabilities of your module here in the form of several PRs?

MartinGC94 · 2025-03-17T17:23:56Z

My module doesn't really do any escaping. It just adds quotes if needed just like the current code (though obviously I handle a few more special characters in my checks). Any new code for this should ideally also take escaping of globbing characters into account, and it sounds like @ArmaanMcleod is already working on that.

iSazonov · 2025-03-18T04:15:54Z

@MartinGC94

My module doesn't really do any escaping.

I was referring to quoting, although escaping may also be necessary.

it sounds like @ArmaanMcleod is already working on that.

He's working on separating the shared code, which we can make public. This is very similar to how your module works.
But neither he nor I have a complete understanding of handling special characters correctly as it is done in your module. Your cooperation would be helpful. If you have the interest and time, it would be great to get a PR with tests and scenario descriptions.

microsoft-github-policy-service · 2025-03-18T04:40:14Z

📣 Hey @ArmaanMcleod, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms

Rename escape to escapeGlobbingPathChars

37362e1

iSazonov reviewed Mar 15, 2025

View reviewed changes

Remove escape globbing path chars code

1bd9919

Revert "Remove escape globbing path chars code"

e6ae71c

This reverts commit 1bd9919.

Revert "Revert "Remove escape globbing path chars code""

72b7346

This reverts commit e6ae71c.

ArmaanMcleod changed the title ~~Refactor CompletionHelpers escape parameter to escapeGlobbingPathChars~~ Remove CompletionHelpers escape parameter from CompletionRequiresQuotes Mar 17, 2025

Merge branch 'master' into refactor-completion-helpers-escape

0aab91d

ArmaanMcleod requested a review from iSazonov March 17, 2025 11:14

This comment was marked as outdated.

Sign in to view

iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Mar 17, 2025

iSazonov self-assigned this Mar 17, 2025

iSazonov approved these changes Mar 17, 2025

View reviewed changes

iSazonov merged commit 377f7e9 into PowerShell:master Mar 18, 2025
38 of 42 checks passed

Remove CompletionHelpers escape parameter from CompletionRequiresQuotes #25178

Remove CompletionHelpers escape parameter from CompletionRequiresQuotes #25178

Uh oh!

Conversation

ArmaanMcleod commented Mar 15, 2025 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

iSazonov Mar 15, 2025

Choose a reason for hiding this comment

Uh oh!

ArmaanMcleod Mar 15, 2025

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArmaanMcleod Mar 15, 2025

Choose a reason for hiding this comment

Uh oh!

ArmaanMcleod Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 15, 2025

Choose a reason for hiding this comment

Uh oh!

ArmaanMcleod commented Mar 16, 2025

Uh oh!

iSazonov commented Mar 16, 2025

Uh oh!

MartinGC94 commented Mar 16, 2025

Uh oh!

iSazonov commented Mar 17, 2025

Uh oh!

ArmaanMcleod commented Mar 17, 2025 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 17, 2025

Uh oh!

ArmaanMcleod commented Mar 17, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

iSazonov commented Mar 17, 2025

Uh oh!

MartinGC94 commented Mar 17, 2025

Uh oh!

iSazonov commented Mar 18, 2025

Uh oh!

Uh oh!

microsoft-github-policy-service bot commented Mar 18, 2025 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Remove CompletionHelpers `escape` parameter from `CompletionRequiresQuotes` #25178

Remove CompletionHelpers `escape` parameter from `CompletionRequiresQuotes` #25178

ArmaanMcleod commented Mar 15, 2025 •

edited by iSazonov

Loading

iSazonov Mar 15, 2025 •

edited

Loading

ArmaanMcleod Mar 15, 2025 •

edited

Loading

ArmaanMcleod commented Mar 17, 2025 •

edited by unfurl-links bot

Loading

microsoft-github-policy-service bot commented Mar 18, 2025 •

edited by unfurl-links bot

Loading