Remove static constants #18154

iSazonov · 2022-09-24T10:07:07Z

PR Summary

Please review commit by commit.

Historically PowerShell code contains a lot of static constants because of limitations in old .Net APIs. For example, now we have string.Split(char) and can use it instead of string.Split(char[]).
I had to refactor some methods.

Escape() method now is more short, simple, clear and fast - now we exactly point chars we want to escape.
GetModuleNameAndVersion() method is refactored a bit. Since all paths have been collected from file system it makes no sense to take into account alternative directory separator.
PathSearchTrimEnd is removed based on Why TrimEndChars was removed? dotnet/runtime#76122 (comment)
(for ex., now we don't remove trail spaces that are valid in file names)

PR Context

PR Checklist

iSazonov · 2022-09-25T08:26:02Z

src/System.Management.Automation/engine/CommandSearcher.cs

@@ -1473,7 +1473,7 @@ private static CanDoPathLookupResult CanDoPathLookup(string possiblePath)
        /// <summary>
        /// The command name to search for.
        /// </summary>
-        private string _commandName;
+        private readonly string _commandName;


It is requested by analyzer.

xtqqczze · 2022-09-26T00:18:57Z

I suggest splitting PathSearchTrimEnd changes.

xtqqczze · 2022-09-26T20:48:02Z

We could avoid some string allocations with the following IndexOfAny<T> overloads, available since .NET Core 2.1:

daxian-dbw

Left 2 comments. A lot of changes will be gone once addressing them, so I will wait after that to review the rest changes.

src/Microsoft.PowerShell.Security/security/SecureStringCommands.cs

daxian-dbw · 2022-09-26T22:02:13Z

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

@@ -1263,7 +1263,7 @@ private static string GetFirstLineSubString(string stringToComplete, out bool ha
            hasNewLine = false;
            if (!string.IsNullOrEmpty(stringToComplete))
            {
-                var index = stringToComplete.IndexOfAny(Utils.Separators.CrLf);
+                var index = stringToComplete.AsSpan().IndexOfAny("\r\n");


This is less readable than the existing code.

You will need to reason that "\r\n" will be cast to ReadOnlySpan and then the chars are used instead of the string itself. Also, I don't think the saving is measurable.

It's helpful to keep all separators together in Utils.Separators, for looking up and referencing.

@daxian-dbw

We can get better codegen using the char overloads:

Suggested change

var index = stringToComplete.AsSpan().IndexOfAny("\r\n");

var index = stringToComplete.AsSpan().IndexOfAny('\r', '\n');

https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcDMACR2DC2A3utmbjgJYB2w2AsgAwAUcSj2EAlMaef3ADsnAHQBBCAGUADmGrMuIgJLUAJgFMAHgHkAZmOoBPZgCIAOgCcz1E1wDcfMgF9HFbDTr0krdpx4k0fgFhCHEpWXlFFQ0dfSNmAHJLBIAabCTqBPtXFzQnIA===

Yes, we should use this overload.

xtqqczze · 2022-09-28T16:19:14Z

src/System.Management.Automation/engine/CommandSearcher.cs

@@ -140,7 +140,7 @@ public bool MoveNext()

                // For security reasons, if the command is coming from outside the runspace and it looks like a path,
                // we want to pre-check that path before doing any probing of the network or drives
-                if (_commandOrigin == CommandOrigin.Runspace && _commandName.IndexOfAny(Utils.Separators.DirectoryOrDrive) >= 0)
+                if (_commandOrigin == CommandOrigin.Runspace && _commandName.AsSpan().IndexOfAny("\\/:") >= 0)


Suggested change

if (_commandOrigin == CommandOrigin.Runspace && _commandName.AsSpan().IndexOfAny("\\/:") >= 0)

if (_commandOrigin == CommandOrigin.Runspace && _commandName.AsSpan().IndexOfAny('\\', '/', ':') >= 0)

https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcDMACR2DC2A3utmbjgJYB2w2AsgAwAUcSj2EAlMaef3ADsnAHQBBCAGUADmGrMuIgJLUAJgFMAHgHkAZmOoBPZgCIAOmYD0IE1wDcfMgF9HFbDTr0krdpx4k0fgFhCHEpWXlFFQ0dfSNmAHILBIAabATLVPSQBPtXFzQnIA===

It is a question for .Net team why these options have so different codegen.

One reason must be IndexOfAny("a-string") requires implicit casting from string to ReadOnlySpan<char>.
We should use the explicit overloads instead.

Done.

I opened dotnet/runtime#76354

src/System.Management.Automation/engine/CommandDiscovery.cs

xtqqczze · 2022-09-28T16:19:14Z

src/System.Management.Automation/engine/CommandDiscovery.cs

@@ -1150,7 +1150,7 @@ private static CommandInfo TryModuleAutoLoading(string commandName, ExecutionCon
            string moduleName;

            // Now we check if there exists the second '\'
-            var secondBackslash = moduleCommandName.IndexOfAny(Utils.Separators.Backslash);
+            var secondBackslash = moduleCommandName.IndexOf('\\');


Suggested change

var secondBackslash = moduleCommandName.IndexOf('\\');

int secondBackslash = moduleCommandName.IndexOf('\\');

This PR already touches this line so we can fix code style too?

There is an enormous number of var. It is better to fix in another PR if we would want.

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

iSazonov · 2022-09-29T09:19:20Z

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

                {
-                    wordToComplete = WildcardPattern.Escape(wordToComplete, Utils.Separators.StarOrQuestion);


You point to removed line.

daxian-dbw · 2022-09-29T16:51:19Z

src/System.Management.Automation/engine/CommandSearcher.cs

@@ -1536,7 +1536,6 @@ private void setupPathSearcher()
                if (_canDoPathLookupResult == CanDoPathLookupResult.Yes)
                {
                    _canDoPathLookup = true;
-                    _commandName = _commandName.TrimEnd(Utils.Separators.PathSearchTrimEnd);


Please revert changes related to TrimEnd since it was moved to #18166.

This reverts commit d0eccf5.

pull-request-quantifier · 2022-09-29T16:58:29Z

This PR has 175 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +82 -93
Percentile : 55%

Total files changed: 30

Change summary by file extension:
.cs : +82 -93

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 👌 👎 (Email)
Customize PullRequestQuantifier for this repository.

xtqqczze · 2022-09-29T20:32:24Z

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

                {
-                    wordToComplete = WildcardPattern.Escape(wordToComplete, Utils.Separators.StarOrQuestion);
+                    wordToComplete = WildcardPattern.Escape(wordToComplete, "[]");


@iSazonov Why is Utils.Separators.StarOrQuestion replaced with "[]", not "*?"

xtqqczze · 2022-09-29T20:38:22Z

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

@@ -905,7 +905,7 @@ internal List<CompletionResult> GetResultHelper(CompletionContext completionCont
                        {
                            switch (completionContext.TokenBeforeCursor.Kind)
                            {
-


Can we run dotnet-format on the repo to fix whitespace, so we don't see these changes in PRs?

We can 😄

xtqqczze · 2022-09-29T20:41:39Z

I suggest changes to CompletionCompleters.NativeCompletionVariableCommands are split as well.

iSazonov added 2 commits Sep 24, 2022

Replace Utils.Separators.Backslash

1ca2e7a

Replace Utils.Separators.Directory

541b6f2

iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Sep 24, 2022

iSazonov assigned daxian-dbw Sep 24, 2022

pull-request-quantifier bot added the Medium label Sep 24, 2022

iSazonov force-pushed the remove-static branch 5 times, most recently from 3c8bb42 to 1c39590 Compare Sep 24, 2022

iSazonov marked this pull request as ready for review Sep 25, 2022

iSazonov requested review from PaulHigin, anmenaga, adityapatwardhan, TravisEz13 and SteveL-MSFT as code owners Sep 25, 2022

iSazonov commented Sep 25, 2022

View changes

iSazonov force-pushed the remove-static branch 2 times, most recently from c5dc0f3 to 05022a1 Compare Sep 26, 2022

daxian-dbw requested changes Sep 26, 2022

View changes

msftbot bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Sep 26, 2022

iSazonov mentioned this pull request Sep 27, 2022

Remove PathSearchTrimEnd #18166

Open

22 tasks

xtqqczze reviewed Sep 28, 2022

View changes

iSazonov added 4 commits Sep 29, 2022

Replace Utils.Separators.DirectoryOrDrive

45a8277

Replace Utils.Separators.Colon

77426e4

Replace Utils.Separators.Dot

d11e4e8

Replace Utils.Separators.Pipe

c84ff3f

iSazonov added 12 commits Sep 29, 2022

Replace Utils.Separators.Comma

620538c

Replace Utils.Separators.Semicolon

6f20453

Replace Utils.Separators.StarOrQuestion

26e5414

Replace Utils.Separators.ColonOrBackslash

395d0b5

Replace Utils.Separators.PathSeparator

6cf65f5

Replace Utils.Separators.Space

f456b9e

Replace Utils.Separators.SpaceOrTab

56a5c19

Replace Utils.Separators.Newline

cf97db1

Replace Utils.Separators.CrLf

9884e9a

Remove unused Utils.Separators.*

2eae4c8

Replace Utils.Separators.Directory

e5cc020

Remove Utils.Separators.PathSearchTrimEnd

d0eccf5

iSazonov force-pushed the remove-static branch from 05022a1 to d0eccf5 Compare Sep 29, 2022

iSazonov commented Sep 29, 2022

View changes

daxian-dbw requested changes Sep 29, 2022

View changes

msftbot bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 29, 2022

Revert "Remove Utils.Separators.PathSearchTrimEnd"

851b776

This reverts commit d0eccf5.

msftbot bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 29, 2022

xtqqczze reviewed Sep 29, 2022

View changes

Sep	OCT	Nov
	03
2021	2022	2023

Remove static constants #18154

Remove static constants #18154

iSazonov commented Sep 24, 2022 •

edited

iSazonov Sep 25, 2022

xtqqczze commented Sep 26, 2022

xtqqczze commented Sep 26, 2022 •

edited

daxian-dbw left a comment

daxian-dbw Sep 26, 2022

xtqqczze Sep 28, 2022

daxian-dbw Sep 28, 2022

iSazonov Sep 29, 2022

xtqqczze Sep 28, 2022

xtqqczze Sep 28, 2022 •

edited

iSazonov Sep 28, 2022

daxian-dbw Sep 28, 2022

iSazonov Sep 29, 2022

xtqqczze Sep 28, 2022

xtqqczze Sep 29, 2022

iSazonov Sep 29, 2022

iSazonov Sep 29, 2022

daxian-dbw Sep 29, 2022

iSazonov Sep 29, 2022

pull-request-quantifier bot commented Sep 29, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

xtqqczze Sep 29, 2022 •

edited

xtqqczze Sep 29, 2022

iSazonov Sep 30, 2022

xtqqczze commented Sep 29, 2022

	var index = stringToComplete.AsSpan().IndexOfAny("\r\n");
	var index = stringToComplete.AsSpan().IndexOfAny('\r', '\n');

	if (_commandOrigin == CommandOrigin.Runspace && _commandName.AsSpan().IndexOfAny("\\/:") >= 0)
	if (_commandOrigin == CommandOrigin.Runspace && _commandName.AsSpan().IndexOfAny('\\', '/', ':') >= 0)

	var secondBackslash = moduleCommandName.IndexOf('\\');
	int secondBackslash = moduleCommandName.IndexOf('\\');

		{
		wordToComplete = WildcardPattern.Escape(wordToComplete, Utils.Separators.StarOrQuestion);

Remove static constants #18154

Are you sure you want to change the base?

Remove static constants #18154

Conversation

iSazonov commented Sep 24, 2022 • edited

PR Summary

PR Context

PR Checklist

Choose a reason for hiding this comment

xtqqczze commented Sep 26, 2022

xtqqczze commented Sep 26, 2022 • edited

daxian-dbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtqqczze Sep 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-request-quantifier bot commented Sep 29, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

xtqqczze Sep 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtqqczze commented Sep 29, 2022

iSazonov commented Sep 24, 2022 •

edited

xtqqczze commented Sep 26, 2022 •

edited

xtqqczze Sep 28, 2022 •

edited

xtqqczze Sep 29, 2022 •

edited