Skip to content

Add tilde expansion for windows native executables - #20402

Merged
Dongbo Wang (daxian-dbw) merged 4 commits into
PowerShell:masterfrom
domsleee:feature/windows-tilde
Feb 12, 2024
Merged

Dongbo Wang (daxian-dbw) merged 4 commits into
PowerShell:masterfrom
domsleee:feature/windows-tilde

Conversation

@domsleee

@domsleee Dominic Slee (domsleee) commented Sep 29, 2023 •

Copy link
Copy Markdown
Contributor

PR Summary

Attempted implementation for #20031.

Tilde expansion here is the same on windows as it is on unix, except that it also expands back slash.
i.e. ~, ~/...., and ~\.... are expanded.

PR Context

This comment: #20031 (comment)

My understanding is that tilde expansion was briefly introduced as part of a larger PR.
But that PR didn't handle double quotes properly, specifically "~" should not be expanded on windows, so it was removed.

There were also other issues with the PSNativePSPathResolution that lead to its removal, specifically around determining if the PSDrive argument was a file or not.

The failing example (from the previous PR) is:

$arg = 'temp:foo'; /bin/echo $arg | Should -be $arg

Expected: 'temp:foo' But was: '/var/folde...'

For tilde expansion, we don't need to consider this information. Tilde (with no quotes) should be always expanded.

We can use an experimental feature here if required, not sure if it is needed 👍

PR Checklist

@domsleee

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@domsleee

Copy link
Copy Markdown
Contributor Author

The place it is done for cmdlets like Get-ChildItem is here, and it supports ~, startsWith("~/") and startsWith("~\"):

if (path.IndexOf(StringLiterals.HomePath, StringComparison.Ordinal) == 0)
{
// Support the single "~"
if (path.Length == 1)
result = true;
// Support "~/" or "~\"
else if ((path.Length > 1) &&
(path[1] == '\\' ||
path[1] == '/'))
result = true;
}

@domsleee
Dominic Slee (domsleee) marked this pull request as ready for review September 30, 2023 03:50
@SteveL-MSFT Steve Lee (SteveL-MSFT) added the CommunityDay-Small A small PR that the PS team has identified to prioritize to review label Oct 2, 2023

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution! Please wrap as an ExperimentalFeature as it also helps advertise the new capability and allow changes on feedback not to be a breaking change.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 2, 2023
@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 3, 2023
@StevenBucher98 Steven Bucher (StevenBucher98) added the PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update label Oct 9, 2023
@microsoft-github-policy-service microsoft-github-policy-service Bot added Review - Needed The PR is being reviewed labels Oct 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is where this feature gets a bit complicated and the original implementation was abandoned. Double quotes are expected to be expanded (like if your path has whitespace), but single quotes are treated as literal and never expanded. So this needs to be differentiated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would you expect tilde expansion if it was surrounded with double quotes? Currently pwsh on linux doesn't expand tilde for double quotes:

/bin/echo '~' | Should -BeExactly '~'
/bin/echo "~" | Should -BeExactly '~'
/bin/echo '~/foo' | Should -BeExactly '~/foo'
/bin/echo "~/foo" | Should -BeExactly '~/foo'

Also I checked bash, fish, zsh with /bin/echo "~/" and none of them expand it (whereas /bin/echo ~ is expanded). So I think it's reasonable to just follow every other shell here, are there other considerations for how tilde should behave on windows?

Also in a related area, the tab completion around tilde doesn't seem quite right - the behaviour from powershell 5 seems more correct to me.
Some cases I tried out, where $ is cursor position:

Case bash --norc (5.2) pwsh -NoProfile (7.4.0) powershell -NoProfile (5.1)
ls "~/my<tab> ls "/Users/user/my folder"/$ ls "~\my folder\$" ls "C:\Users\user\my folder\$"
ls ~/my<tab> ls ~/my\ folder/$ ls '~\my folder\$' ls 'C:\Users\user\my folder\$'

Seems like it is new (undesirable) behaviour in 7.4.0, some discussion about it here: #20750

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

POSIX-like shells only ever expand an unquoted argument-initial ~, either in isolation or if followed by a an unquoted username (not implemented in PowerShell - see #12387) or an unquoted /, and then possibly followed by an mix of unquoted and quoted strings (any mix of '...', "...", with their usual semantics), so I think that's what PowerShell should do as well - with the addition of also recognizing unquoted \, as stated in the initial post.

Thus, to ensure expansion of ~-prefixed paths parts of which require quoting, only the part after ~/ must be quoted; e.g. /bin/echo ~/'a test' (ditto in PowerShell) or /bin/echo ~/a\ test (/bin/echo ~/a` test in PowerShell)

Note that these cases are currently broken in PowerShell (which currently surface on Unix only):

Indeed, the problem described in #20750 effectively broke tab-completion of ~ on Windows for native-program calls, due to - by well-meaning design - no longer instantly expanding ~ to the value of $HOME.

This PR would make that problem go away - but to resolve #20750 this way (which I personally prefer) requires it to become a stable feature right away.

Find a summary of the solution options at:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Steve Lee (@SteveL-MSFT) i think this change just makes the UNIX and Windows behaviors similar, where ~ or ~/ would now behave consistently regardless of platform (it's cribbed from the code in lines 399-418). I would probably suggest #else rather than #endif at line 419 which might be clearer. We disambiguate between quoted and non-quoted ~ in lines after 344, so that would remain consistent. We don't currently treat ~ with regard to single and double quotes differently; if it's quoted, it's not expanded.

Now that I think on it, the concern I do have is that we would now have duplicated code which means a fix in one spot may not happen in the other. Perhaps this method should be refactored where we don't duplicate the code.

@microsoft-github-policy-service microsoft-github-policy-service Bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Dec 4, 2023
@domsleee
Dominic Slee (domsleee) force-pushed the feature/windows-tilde branch 3 times, most recently from 600b89d to a7270cf Compare December 14, 2023 11:25
@pull-request-quantifier-deprecated

Copy link
Copy Markdown

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


Quantification details

Label      : Small
Size       : +48 -11
Percentile : 23.6%

Total files changed: 4

Change summary by file extension:
.cs : +23 -10
.ps1 : +23 -0
.json : +2 -1

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? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Dec 21, 2023

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nice!

@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit c56a52b into PowerShell:master Feb 12, 2024
@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Review - Needed The PR is being reviewed label Feb 12, 2024
@daxian-dbw Dongbo Wang (daxian-dbw) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 12, 2024
@bradwilson

Copy link
Copy Markdown

Steve Lee (@SteveL-MSFT) James Truher (@JamesWTruher) Dominic Slee (@domsleee) The fact that PowerShell on Linux quotes space-containing paths during tab expansion is broken behavior since 7.4.0, because it breaks translation of ~ when calling executables.

This PR will now break Windows in the same way, because of the same tab expansion behavior.

Additionally, trying to use PowerShell's native escaping for spaces to work around this behavior on Linux is also currently broken:

image

In my unvarnished opinion, the dumpster fire around tab expansion behavior with ~ introduced in 7.4.0 was ill thought out, and this one PR is not remotely a complete fix, even on Windows. Frankly: where are the test cases for all of this? How was this feature allowed out into the wild without realizing how many things were being broken, and how is the team working to resolve them before 7.5.0? The fact that the team has been 100% silent on #20750 is extremely disappointing, given that the issue has been open since November 2023. Will anybody ever respond?

@mklement0

Copy link
Copy Markdown
Contributor

While I share your frustration around the lack of responses / a decision on #20750,
what you're describing are known bugs that must be fixed separately and their existence is not an argument against the value or implementation of this PR:

@bradwilson

Copy link
Copy Markdown

Michael Klement (@mklement0) Not to be overly pessimistic, but neither of your issues have any comment from anybody on the team, either. Right now you're we're basically hoping they agree and create (or allow) a fix in time for 7.5.0.

@mklement0

Copy link
Copy Markdown
Contributor

Fair point, Brad Wilson (@bradwilson) - let's hope that this exchange helps.

@MatejKafka

Copy link
Copy Markdown
Contributor

The behavior of this implementation of tilde expansion seems utterly unintuitive to me and breaks basic assumptions that I (and I assume many other PowerShell users) have:

$Path = "~"
cmd /c echo $Path # C:\Users\admin
cmd /c echo "$Path" # ~

One of the things I like about PowerShell is that I don't have to think about quoting rules – if I want to pass a variable as a parameter, I just pass a parameter and don't worry about quoting. This feature breaks that, and means that whenever I'm passing a variable to a native command, I have to actively think whether I want to expand ~ and quote accordingly. PowerShell is complex enough as it is, and I don't see any way this feature can pull its weight. I disagree with the original implementation of this feature, where ~ was always expanded, but at least it was consistent, unlike this implementation.

I first encountered this feature when doing something like

ssh <host> cp ... ~/...

as part of a longer command and receiving mysterious errors that C:Usersadmin... is not an existing directory, which took me quite a lot of time to debug. I would not intuitively expect that ~ will be expanded in this context, much less so when it wrapped in a PowerShell function and the ~ is passed as a parameter.


In POSIX shells, ~ is expanded immediately when written, and not when passed to a native command:

x1=~
x2="~"
echo $x1 # /home/...
echo "$x1" # /home/...
echo $x2 # ~
echo "$x2" # ~

This is not ideal, but it is much more intuitive than expanding ~ in the command invocation. I understand why PowerShell cannot resolve ~ in the same way.

I'd personally happily sacrifice the resolution of ~ when calling native commands if it means that PowerShell quoting rules will remain simple and interact nicely with remote ssh calls and other places where literal ~ is passed. I understand that this is a bigger problem on Linux compared to Windows, but tab-expanding ~ seems like a safer, simpler and well-tested alternative.

@mklement0

Copy link
Copy Markdown
Contributor

The ssh example is an interesting one, as it mixes the worlds of Windows and Unix-like platforms.

Were you to run your example command on Unix, you would have to escape or quote the ~ to prevent instant expansion or - and with this feature you'll now need to do it on Windows too.

Similarly, expansion still happening via variables - $p = "~"; /usr/bin/echo $p / $p = "~"; cmd /c echo $p should be considered a bug too, so I've created:

@MatejKafka

Matěj Kafka (MatejKafka) commented Mar 10, 2024 •

Copy link
Copy Markdown
Contributor

Were you to run your example command on Unix, you would have to escape or quote the ~ to prevent instant expansion or - and with this feature you'll now need to do it on Windows too.

Yes, but I use PowerShell because it's better than Unix shells. :)

More seriously, PowerShell takes different approach than Unix-style shells, and not having to quote variables is an important part of what makes PowerShell more intuitive and comfortable for daily usage. In Unix shells, you're expected to quote more-or-less everything, which is not expected in PowerShell, and this PR makes PowerShell quoting more complex, for imo lesser benefit.

I understand the desire for consistent ~ behavior between cmdlets and native commands, but adding special cases for quoting (a much more commonly used and core feature of PowerShell) to achieve that seems too great of a sacrifice to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log CommunityDay-Small A small PR that the PS team has identified to prioritize to review PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update Small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants