Skip to content
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.

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

Pushing an empty path to a verbatim path no longer adds a separator #89658

Closed
dylni opened this issue Oct 8, 2021 · 5 comments · Fixed by #89665
Closed

Pushing an empty path to a verbatim path no longer adds a separator #89658

dylni opened this issue Oct 8, 2021 · 5 comments · Fixed by #89665
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@dylni
Copy link
Contributor

dylni commented Oct 8, 2021

Code

I tried this code:

use std::path::Path;

fn main() {
    let mut path = Path::new(r"C:\Users").to_owned();
    path.push("");
    assert_eq!(r"C:\Users\", path.as_os_str());

    path = Path::new(r"\\?\C:\Users").to_owned();
    path.push("");
    assert_eq!(r"\\?\C:\Users\", path.as_os_str());
}

I expected to see this happen: Both assertions succeed.

Instead, this happened: The second assertion fails, likely due to #89270:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"\\\\?\\C:\\Users\\"`,
 right: `"\\\\?\\C:\\Users"`', src\main.rs:10:5

Although adding a separator when pushing an empty path is debatable, the behavior should probably be consistent for verbatim and non-verbatim paths.

This change caused normpath to break. I can add an inefficient workaround, but is this change intentional?

Version it worked on

It most recently worked on: 1.56.0-beta.4

Version with regression

rustc --version --verbose:

rustc 1.57.0-nightly (485ced56b 2021-10-07)
binary: rustc
commit-hash: 485ced56b8753ec86936903f2a8c95e9be8996a1
commit-date: 2021-10-07
host: x86_64-pc-windows-msvc
release: 1.57.0-nightly
LLVM version: 13.0.0

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged +O-windows

cc @seanyoung

@dylni dylni added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Oct 8, 2021
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. O-windows Operating system: Windows regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. labels Oct 8, 2021
seanyoung added a commit to seanyoung/normpath that referenced this issue Oct 8, 2021
Since rust-lang/rust#89270 rust's pathbuf
push() normalizes the path if it is a verbatim path, so that the result
is still a valid verbatim path. This involves parsing reconstructing the
path.

Appending a path separator using push("") will no longer work, and if
it did, it would be very inefficient.

rust-lang/rust#89658

Signed-off-by: Sean Young <[email protected]>
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 8, 2021
@seanyoung
Copy link
Contributor

This is an interesting case. The normpath crate has its own path push function to work around the issue of verbatim paths not working with PathBuf::push(), which is exactly the same issue that my PR #89270 was trying to solve upstream.

The normpath push() function uses pathbuf push function to append a directory separator by calling .push(""). Since the pathbuf push now already returns normalized paths (for verbatim paths) this no longer works.

I've created a PR on normpath to fix the issue there: dylni/normpath#4

So since verbatim paths have special handling to ensure the returned path normalized, yes there are differences. If you call .push(".") on a verbatim path, then the path will remain unchanged, otherwise it would not be valid any more. We cannot expect push to work the same on verbatim paths as on non-verbatim paths, verbatim paths must be normalized.

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 8, 2021
@ChrisDenton
Copy link
Member

Hm, should this not be special cased so that it works the same for all paths? Otherwise the only way to push a trailing separator is to convert to an OsString, push the separator and then convert back to a PathBuf.

IMHO, ideally this would be fixed in components so that trailing slashes are never normalized away but I'm uncertain if that change will be accepted.

@seanyoung
Copy link
Contributor

seanyoung commented Oct 8, 2021

We could special case it so that .push("") is a no-op in all cases.

@ChrisDenton
Copy link
Member

Would this break existing code though?

@seanyoung
Copy link
Contributor

@ChrisDenton on second thought, maybe you are right. Let me see what I can do.

dylni added a commit to dylni/normpath that referenced this issue Oct 11, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.57.0 milestone Oct 14, 2021
@m-ou-se m-ou-se added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 20, 2021
@joshtriplett joshtriplett added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Oct 20, 2021
@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Oct 20, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 22, 2021
Ensure that pushing empty path works as before on verbatim paths

Fixes: rust-lang#89658

Signed-off-by: Sean Young <[email protected]>
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 22, 2021
Ensure that pushing empty path works as before on verbatim paths

Fixes: rust-lang#89658

Signed-off-by: Sean Young <[email protected]>
@bors bors closed this as completed in 1bb399c Oct 22, 2021
dylni added a commit to dylni/normpath that referenced this issue Oct 24, 2021
dylni added a commit to dylni/normpath that referenced this issue Oct 24, 2021
dylni added a commit to dylni/normpath that referenced this issue Oct 24, 2021
This reverts commit da9b7b2.

This workaround is no longer necessary: rust-lang/rust#89665
@m-ou-se m-ou-se added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Oct 25, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 25, 2021
@m-ou-se m-ou-se added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 25, 2021
cuviper pushed a commit to cuviper/rust that referenced this issue Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants