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

Windows verbatim paths shouldn't normalize path when .push-ed #11594

Closed
erickt opened this issue Jan 16, 2014 · 3 comments · Fixed by #21759
Closed

Windows verbatim paths shouldn't normalize path when .push-ed #11594

erickt opened this issue Jan 16, 2014 · 3 comments · Fixed by #21759
Labels
O-windows Operating system: Windows

Comments

@erickt
Copy link
Contributor

erickt commented Jan 16, 2014

cc: @kballard

If I understand windows verbatim paths correctly, a path like \\?\a\b\ should not normalize away that last trailing slash. WindowsPath::new does this correctly. However, if we break up the paths into \\?a and b\ and use push to join the paths, that trailing slash is removed:

fn main() {
    let path = WindowsPath::new("\\\\?\\a\\b\\");
    println!("{:?}\n{}\n{:?}", path, path.display(), path.str_components().to_owned_vec());

    let mut path = WindowsPath::new("\\\\?\\a");
    path.push("b\\");
    println!("{:?}\n{}\n{:?}", path, path.display(), path.str_components().to_owned_vec());
}
std::path::windows::Path{repr: ~"\\\\?\\a\\b\\", prefix: Some(VerbatimPrefix(1u)), sepidx: Some(5u)}
\\?\a\b\
~[Some("b")]
std::path::windows::Path{repr: ~"\\\\?\\a\\b", prefix: Some(VerbatimPrefix(1u)), sepidx: Some(5u)}
\\?\a\b
~[Some("b")]
@lilyball
Copy link
Contributor

@erickt When pushing anything onto a verbatim path, we explicitly normalize the pushed argument first. When pushing onto a non-verbatim path, we concatenate them and normalize the result. The idea here is that we're pushing paths, not arbitrary strings. p.push(s) should behave identically to p.push(Path::new(s)). In most cases, there's no difference, but verbatim paths are kind of wonky.

This makes for an argument that we should have another method .push_component(), which treats the argument as a mere path component instead of a path in its own right, doing things such as skipping any parsing of the path prefix and skipping any normalization when pushing onto a verbatim path. The only difference in PosixPath is that p.push_component("/foo") would presumably ignore the leading / and treat it the same as p.push("foo"). This is in addition to the use-case we talked about on IRC, where you want to be able to split up a path by its .components() and then re-create it and get the same path back (which should work today for all paths except for verbatim windows paths).

However, I am leery about the idea of adding 4 more API methods (.push_component(), .join_component(), .push_many_components(), .join_many_components()).

@erickt
Copy link
Contributor Author

erickt commented Jan 17, 2014

Yeah, I don't love the idea of adding more API methods either. What do you think about changing how normalization is done? If we didn't normalize by default, the end user could decide that they didn't want to run normalization on a verbatim path.

@lilyball
Copy link
Contributor

@erickt We need to normalize. The entire way the path module is implemented assumes that all Path instances are normalized internally. The only quirk here is that, when pushing onto a verbatim path, you may not want to normalize the pushed component (but the resulting path would still need to be a "normalized" verbatim path, for the certain value of normalization that verbatim paths have [e.g. no double-backslashes in between components]).

Another option is to add a Component type that wraps a &[u8] and implements BytesContainer, which will allow it to be pushed. Then we can detect if it's a Component and skip the pre-normalization when pushing onto a verbatim path. But I'm not thrilled with this idea either, because it means adding yet another specialized type-detection method .is_component() to BytesContainer, and is just generally kind of awkward. And it means .components() would probably want to return this Component value (but then what about .str_components()? Do we make Component into an enum with ByteComponent and StrComponent, or just live with string components not being able to opt into this behavior without converting back to &[u8]?)

There's a third option which might solve your specific use-case, which is a .parent_iter() that yields successive paths starting from the root and adding each component dir in turn, until it yields the parent of the original path (if the original path is the root folder already, it would yield nothing). Then you could use .path_relative_from() to construct the other half of the path, for your pair. However, this is a somewhat specialized use-case.

@aturon aturon added the A-libs label Jun 3, 2014
alexcrichton added a commit to alexcrichton/rust that referenced this issue Feb 4, 2015
This PR implements [path reform](rust-lang/rfcs#474), and motivation and details for the change can be found there.

For convenience, the old path API is being kept as `old_path` for the time being. Updating after this PR is just a matter of changing imports to `old_path` (which is likely not needed, since the prelude entries still export the old path API).

This initial PR does not include additional normalization or platform-specific path extensions. These will be done in follow up commits or PRs.

[breaking-change]

Closes rust-lang#20034
Closes rust-lang#12056
Closes rust-lang#11594
Closes rust-lang#14028
Closes rust-lang#14049
Closes rust-lang#10035
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 21, 2023
…fate

[`get_first`]: lint on non-primitive slices

Fixes rust-lang#11594

I left the issue open for a couple days before making the PR to see if anyone has something to say, but it looks like there aren't any objections to removing this check that prevented linting on non-primitive slices, so here's the PR now.
There's a couple of instances in clippy itself where we now emit the lint. The actual relevant change is in the first commit and fixing the `.get(0)` instances in clippy itself is in the 2nd commit.

changelog: [`get_first`]: lint on non-primitive slices
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants