WIP: Correct completions for quoting and escaping

[msftrncs]

PR Summary

Correct completions (Tab/Intellisense) for quoting and escaping for the various kinds of completions available.

PR Context

Completers of all types seem to have inconsistent quoting and escaping behaviors that often left the resulting completion still unusable, most specially when the completion contained:

• potential wildcard patterns
• escape sequence characters
• curly quotes
• variable interpolation patterns
• specific file system conditions

This PR, through a series of commits, brings unity to the completers, implementing methods that were already available in the code and additional methods, where by each type of completer (variable name, member name, argument) utilizes a common quoting and escaping method.

Fix #10006 - Variable ($ and @) name bracing and escaping and empty scope prefixing
Fix #10239 - Variable completion has no results with input $:
Fix #10198 - Member name completion quoting and escaping
Fix #9881 - General argument completion quoting and escaping
Fix #10218 - Argument completion of ValidateSetAttribute values quoting and escaping
Fix #7569 - \\Server\Share With Space quoting and escaping

This PR is still WIP, as it lacks any changes for testing, and it is unknown if it causes any tests to fail.

To quote @bergmeister, be gentle. :) This is my first work in C#.

@rkeithhill, from dahlbyk/posh-git#683 (comment), this includes such a method, [System.Management.Automation.Language.CodeGeneration]::QuoteArgument().

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• [] This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

[iSazonov]

The pattern quote == string.Empty ? (char)0 : quote[0] is used 16 time - it should be moved in QuoteArgument() method.

[msftrncs]

Would it be acceptable to rewrite HandleSingleAndDoubleQuote():

PowerShell/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

Lines 6489 to 6519 in 8f5b2c2

	internal static string HandleDoubleAndSingleQuote(ref string wordToComplete)
	{
	string quote = string.Empty;
	if (!string.IsNullOrEmpty(wordToComplete) && (wordToComplete[0].IsSingleQuote() || wordToComplete[0].IsDoubleQuote()))
	{
	char frontQuote = wordToComplete[0];
	int length = wordToComplete.Length;
	if (length == 1)
	{
	wordToComplete = string.Empty;
	quote = frontQuote.IsSingleQuote() ? "'" : "\"";
	}
	else if (length > 1)
	{
	if ((wordToComplete[length - 1].IsDoubleQuote() && frontQuote.IsDoubleQuote()) || (wordToComplete[length - 1].IsSingleQuote() && frontQuote.IsSingleQuote()))
	{
	wordToComplete = wordToComplete.Substring(1, length - 2);
	quote = frontQuote.IsSingleQuote() ? "'" : "\"";
	}
	else if (!wordToComplete[length - 1].IsDoubleQuote() && !wordToComplete[length - 1].IsSingleQuote())
	{
	wordToComplete = wordToComplete.Substring(1);
	quote = frontQuote.IsSingleQuote() ? "'" : "\"";
	}
	}
	}
	return quote;
	}

with this:

        internal static char HandleDoubleAndSingleQuote(ref string wordToComplete)
        {
            char quote = (char)0;

            if (!string.IsNullOrEmpty(wordToComplete) && (wordToComplete[0].IsSingleQuote() || wordToComplete[0].IsDoubleQuote()))
            {
                quote = wordToComplete[0].IsSingleQuote() ? '\'' : '"';
                int length = wordToComplete.Length;

                wordToComplete = length == 1 ? string.Empty :
                    (quote.IsSingleQuote() ? wordToComplete[length - 1].IsSingleQuote() : wordToComplete[length - 1].IsDoubleQuote()) ?
                        wordToComplete.Substring(1, length - 2) : wordToComplete.Substring(1);

            }

            return quote;
        }

and then eliminate the kludge? This would change the entire code over to using char to pass the quote.

[iSazonov]

You check quote == 0 only once in "QuoteArgument". I guess you could use quote.Empty and don't use the zero value.

[msftrncs]

It comes down to, do you use a string to store just three values, empty, ' or ". or is there a better way? Using a string means you have to use [0] references to get at the character (after you've confirmed that the string isn't empty) for using IsSingleQuote and IsDoubleQuote.

In a sense, its a glorified enum. Is(Single/Double)Quote's are overkill because at this point it can only contain 3 values, not any of the other 7 quotes (at least as defined by HandleSingleAndDoubleQuote).

[iSazonov]

Please add comment why we need the change.

[msftrncs]

per 033ef6c:

allow completion starting with `$:`
Enhances variable completion allowing completions to start with `$:`

[msftrncs]

Added issue #10239.

[iSazonov]

Please use for cycle.

[msftrncs]

Is there a lint I am supposed to be using? I don't think any of the CodeFactor / Codacy issues would be there if there if I had the linting warnings. I didn't find any references to a lint in the contributing guide.

[msftrncs]

Noting, this PR may conflict with #7407, as this PR attempts to do some of the same things, but not exclusively to file paths.

[msftrncs]

Broken Tests

Two test patterns are currently broke. The reason for this is that this PR implements a more specific $ variable interpolation detection to require the argument to be quoted. The implementation is that, if a $ character is not followed by a legitimate variable character, that $ will not itself require the argument to be quoted.

Patterns failing:

• a file path completion test where $ appears at the end of the completion
• a Windows only miscellaneous completion test where $ appears followed by a \ in the completion.

Both test patterns involve command completions, so the quoting of the path resulted in the completion using the invoke operator &. With this PR's implementation, the quoting and the invoke operator no longer occur.

Tests Needed

• Member completion quoting tests, these should probably be similar to the two examples that were previously commented in the code, as I haven't found any tests for them yet, in addition to tests relating to issue Member tab completion fails to quote member names not directly acceptable #10198.
• Variable completion tests
  • test completions of variable names that require braces and quoting and escaping both for access and for use as an argument.
  • test completions of variable names that have colons in their names, both with and without a scope specified.
  • test completions of variable names that start with ? but with more than one character, with scope, with complex character requring braces, with scope and complex character, without scope and without complex character.
• Argument value completion tests
  • test escaping of wildcard patterns in a completion for a parameter which accepts them. ** IMPORTANT for other work being done around the WildCardPattern class **
  • test quoting and escaping of curly quoting characters.
  • On Unix OS's test completion of a file name with wildcard characters * and ? (with a parameter supporting wildcards) and doublequotes. Tests should also be added to other areas to insure these files or folders are accessible, but that is not the scope of this PR (these tests will need to create said files though).
• test ValidateSetAttribute completed arguments that require quoting, especially regarding those that could be confused as a parameter name when not quoted. Include variants related to issue completions of [validateset] parameters fails to escape special characters #10218.

We'll keep adding to this list to track the tests fixed, needed and added.

[iSazonov]

Is there a lint I am supposed to be using?

CodeFactor is based on StyleCop - you can run it locally. Usually we don't request to fix style issue which is not in your code. If there is a lot of style issues please pull new PR to ifx them - we don't want to mess fuctional and style changes in one PR.

[iSazonov]

@kwkam Could you please review the PR too?

[msftrncs]

CodeFactor is based on StyleCop - you can run it locally. Usually we don't request to fix style issue which is not in your code.

I did notice that a lot of the style errors were not from my code changes (at first I thought they were all in my code, probably because it showed new issues first), and that would make it difficult to let a format-document command run, but it would sure help if I could format-selection and get the easy ones taken care of, and then a lint would show the other suggestions. That's how the TS/JS repositories I have contributed to work, and its all in VS Code.

I was able to turn on the 'editor settings' feature of the C# extension and it did change some of the formatting, but it still doesn't warn me that this repository would prefer all IF statements braced, comments spaced out, statements after IF statements spaced out, etc...

[msftrncs]

I'm considering putting the call to GenerateVariableCompletionText inside AddUniqueVariable, because if the variable is not unique, there is no reason to determine if it needs braced.

[msftrncs]

I think I need to have an exception in the argument quoting/bareword checker for commands, as they have a slightly different bareword syntax and I am not accounting for that.

[msftrncs]

That's weird, the Windows test must be skipping the completion tests, because its not failing them, where as the MacOS and Linux tests are failing completion tests (which are currently expected at this time).

[iSazonov]

@msftrncs It is Newline difference.

[iSazonov]

@msftrncs Have you plans to continue?

[msftrncs]

I do, having trouble dedicating time. I do have new commits for improved use of the parser to determine when quoting is required, specially for command name values.

[iSazonov]

I see 9-11 tests fails. Is it expected?

[msftrncs]

I see 9-11 tests fails. Is it expected?

Yes and no, I expected the '$' tests to fail, because an ending $ doesn't need to be quoted, but the ending ` should have triggered a requirement to quote. Those tests previously had not failed.

I also have not yet handled testing, as I am not sure how much of these changes can be specifically tested.

[msftrncs]

At this time, I do expect the completion tests that involve a $ to fail, as the parser doesn't see them as separate tokens so no need to quote. However, I need to understand the result regarding the configuration keyword. It appears to be 1 of 3 keywords receiving special treatment.

[ghost]

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

[3052]

FYI #9881 this seems to be fixed as of version 7.4.0

[iSazonov]

Sorry, I'm closing it, because it's a long time ago. If there is something that has not been fixed yet in recent version, it is better to open a new PR. Thanks!