Implemented DoubledEndedIterator for Ancestors<'_> + added tests and updated doc comments

[asder8215]

View all comments

This PR revolves around the tracking issue for implementing DoubleEndedIterator for Ancestors<'_>.

I tried to minimize the information needed in Ancestors<'_> struct to effectively produce the correct backward and forward paths using a front and back index; I did opt to use a &'a [u8] over &'a Path because it would reduce code in .next()/.next_back() where I wouldn't need to constantly convert the Path into a u8 slice. Due to how relative paths worked previously for Ancestors<'_>, I had to introduce logic that allowed for returning an empty string at the start/end of a relative path for .next_back()/.next(); I'm open to a lot of feedback on how to make the code neater to address these cases.

I tested this pretty extensively on my Linux machine (comparing it with the output of what Ancestors<'_> previously returned); I do have to do more testing for Windows (been struggling a bit to get my Windows VM to run my shared folder containing the rust project through ./x though, so I may need some help on this), which there are some windows-specific test in tests/path_ancestors.rs. As far as I'm aware as well, the create_dir_all tests, which does use Ancestors<'_> underneath the hood, all passes.

I am aware of one breaking change in this code though. I realized for a path like:

let mut path = Path::new("/foo/bar/baz/").ancestors();

Its Ancestor<'_> iterator contains:

"/foo/bar/baz/"
"/foo/bar"
"/foo"
"/"
 (Then it's None afterward)

The previous implementation doesn't strip trailing slashes for the first path, but in subsequent paths it does eliminate trailing slashes. You can see this here.

With how I use ascii separators to determine that we've reached the next component, this does make it a bit difficult to find a neat way to handle this logic (and do this symmetrically as well for .next_back()). For example, consider the two paths:

"/foo/bar/baz/"

"/foo/bar/baz"

With how I'm using a back index approach, I save what the current back idx is in a variable (curr_back), then iterate through the u8 slice until I hit an ascii separator, advance more through potential ascii separators, and then stop reading from there if there are no more adjacent ascii separators, returning what path[..curr_back] is (trimming separators). Doing this on the first path would return "/foo/bar/baz" in the first call (since I trim separators, but otherwise this would be "/foo/bar/baz/") and then return "/foo/bar/baz" again in the next call (this is because curr_back is set to the idx of where the last "/" is at in "/foo/bar/baz/"). Doing this for the second path would return "/foo/bar/baz" in the first call and then "/foo/bar" in the next call. I currently just trim separators from the original path before giving that to the Ancestors<'_> struct when .ancestors() is called.

I'm hoping this change isn't room for big concerns because removing trailing separators for the original path does not break logic in accessing that path. However, if someone were to use Ancestors<'_> for printing paths recursively up, this would cause changes to the first path, which I do not see it being a huge issue if they want to print the original path unchanged since they can opt to consume the first item in the iterator and print the original path. If I do, however, need to account for this logic, then I will try and see what I can do; I'm open to feedback in handling this case as well.

UPDATE

I added a bit more information into Ancestors<'_> (path_len and trailing_seps) that provides more information to the iterator so that we can print out the original path on the first call to .next() or when we reach the last component for .next_back(). Unfortunately, this does mean ancestors() becomes a O(N) function if we just have a usize::MAX trailing root path, but that's really irregular and rare to see. As a result of this, this should prevent any breaking changes.

The way Ancestors<'_> is implemented now operates the same as the previous version (at least on Linux/Unix, I still have to test on Windows to see if Prefix components are okay).

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, joboet

[Mark-Simulacrum]

This looks like a partial implementation of trim_trailing_sep, except without the handling for !self.has_root()...

I added an unresolved question to the tracking issue (#142503) on whether that's the right behavior for trim_trailing_sep/has_trailing_sep, but I think this is probably not what we want here...

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[asder8215]

I ran the fuzz script you made after making some changes to the code, and it's been running without errors for ~20 minutes. I'm pretty confident that the code for all the Ancestors<'_> iterator next and next_back captures the behavior of the original Ancestors<'_> next and shows symmetrical behavior for next_back.

[Mark-Simulacrum]

Re-running the fuzz script for me with what I believe is latest on this branch fails with:

thread '<unnamed>' (8934365) panicked at fuzz_targets/fuzz_target_1.rs:13:5:
assertion `left == right` failed: "/.."
  left: ["/..", "/"]
 right: ["/.."]

i.e., forward returns ["/..", "/"] while a reversed backwards iteration returns ["/.."].

Can you confirm whether that's the case and perhaps spend some more time fuzzing? The failure is near-instant for me, though I did modify the fuzzing target a bit:

#![no_main]

use libfuzzer_sys::fuzz_target;
use std::ffi::OsStr;
use std::os::unix::ffi::OsStrExt;
use std::path::{Path, PathBuf};

fuzz_target!(|path: PathBuf| {
    let path: &Path = &path;
    let forward = path.ancestors().collect::<Vec<_>>();
    let mut backward = path.ancestors().rev().collect::<Vec<_>>();
    backward.reverse();
    assert_eq!(forward, backward, "{:?}", path);

    let mut by_parents = vec![];
    let mut parent = path;
    loop {
        by_parents.push(parent);
        parent = if let Some(p) = parent.parent() {
            p
        } else {
            break;
        };
    }

    assert_eq!(forward, by_parents, "{:?}", path);
});

[asder8215]

i.e., forward returns ["/..", "/"] while a reversed backwards iteration returns ["/.."].

Ah shoot. I'm noticing that I forgot to copy over updating the rebound index in the b'.' comparison conditional (!curr_dir_present branch) within advance_through_trailing_sep_front function.

I copied over the necessary Ancestor code in a separate folder, cargo fuzzed it, and updated the code as I saw issues pop up from cargo fuzz until I didn't see anything for a while. I guess I missed a couple updates I made from the fuzzed code to this PR code; I'll fix this later tonight once I'm back on my PC.

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

  It's recommended to update your branch according to the rustc-dev-guide.

[asder8215]

Okay, so I copied over the Ancestors code I wrote in the fuzz script properly into this PR and diffed it. I don't see any issues running cargo fuzz; I made sure to run it longer, but it didn't produce any errors as far as I can see. Let me know if there are any errors you see on your end running cargo fuzz.

I did try to see if I could get rid of trailing_path_length() within Path::ancestors, so I could make that function effectively O(1) instead of possibly O(N) (though this worst case scenario is very unlikely to occur unless someone for some reason does trailing slashes, i.e. //////, only or trailing current directory path only, i.e. /././././.), but it's proving to be difficult using a front and back index approach as I mentioned earlier in my PR description.

I'm okay with the existing code here for now, and I wouldn't mind coming back to it later to see if I could remove that function (if you are okay with that).

On a separate note, there technically is duplicate code between Ancestors::advance_through_trailing_sep_back and trailing_path_length (and technically Ancestors::advance_through_trailing_sep_front, but that uses front index) on traversing backward in the path to strip away trailing seps + curr dir components. Do you want me to put that in a common function or is it alright as is?

Lastly, I still haven't set up my Windows 10 VM to run the rust test suite, so I may need some assistance testing for that. I tried installing cargo-fuzz to see if I could just extract the Ancestors rust code and run the fuzz script you made, but it's telling me that it had an error calling dlltool.exe in which it couldn't find the program (hence couldn't compile window-sys).

[asder8215]

Forgot to do this:
@rustbot ready

[Mark-Simulacrum]

This implementation looks pretty complicated. I poked around a bit and a non-Iterator (so needs moving state into a struct) implementation that seems to pass fuzzing at least on unix (I don't have Windows on hand to check that). Specifically note that this makes use of the existing std::path::Components code for all of the tricky byte-by-byte inspection of the underlying path. I suspect Component::Prefix would need to get treated the same as RootDir based on the code in Path::parent().

I don't think we should land the PR's current form as-is because even if it's functionally the same (as fuzzing might suggest, though I didn't rerun it against your implementation) it's very hard for me to convince myself it's actually correct as a reviewer. In contrast leaning on Components should broadly work, though it does need verifying for the one assumption I make below ("Any Component boundary should be sufficient to satisfy that condition") -- that needs some inspection of the rules, though I suspect we already implicitly rely on that in Components.

fn fwd_ancestors(path: &Path) -> Vec<&Path> {
    let mut ancestors = vec![];
    let mut components = path.components();
    loop {
        ancestors.push(components.as_path());
        match components.next_back() {
            Some(Component::RootDir) => {
                break;
            }
            None => {
                break;
            }
            _ => {}
        }
    }
    ancestors
}

fn rev_ancestors(path: &Path) -> Vec<&Path> {
    let mut ancestors = vec![];
    let mut components = path.components();
    let trimmed_path = path.components().as_path();
    loop {
        let current_suffix = components.as_path();
        match components.next() {
            Some(Component::RootDir) => {
                // do nothing
            }
            _ => {
                let prefix = strip_delimited_suffix(trimmed_path, current_suffix);
                ancestors.push(prefix);
            }
        }
        if component.is_none() {
            break;
        }
    }
    ancestors
}

// Will panic if `suffix` is not actually a suffix of `path`.
//
// Will also panic if boundary in `path` between itself and `suffix` doesn't respect the conditions
// in OsStr::slice_encoded_bytes. Any `Component` boundary should be sufficient to satisfy that
// condition (TODO: verify that claim).
fn strip_delimited_suffix<'a>(path: &'a Path, suffix: &'a Path) -> &'a Path {
    let path_bytes = path.as_os_str().as_encoded_bytes();
    let suffix_bytes = suffix.as_os_str().as_encoded_bytes();
    let stripped = path_bytes
        .strip_suffix(suffix_bytes)
        .expect("suffix is suffix of path");
    Path::new(path.as_os_str().slice_encoded_bytes(..stripped.len()))
}

[asder8215]

Yea, the implementation is pretty complicated with keeping in mind about normalizing away current directory '.', and repeating '/' pieces, and then you have certain things to keep in mind of like how relative paths end with an empty path "".

There's other behavior here that made this a bit convoluted, like when we use path.components().as_path(), it produces:

"/foo/bar/baz"

However, if we create an Ancestors iterator out of this path and use Ancestors::next(), it produces:

"/foo/bar/baz///"

This behavior caused a little bit of a headache for me in terms of symmetry because it meant that the first path that Ancestors::next produces is (with no Ancestors::next_back being called) the entire path untrimmed (everything else is trimmed however). Vice versa, that means that the last path that Ancestors::next_back produces (with no Ancestors::next being invoked) has to also have the entire path untrimmed.

I tried out the code you suggested on rust playground, and as expected it didn't cover the case of preserving the whole path untrimmed, so this isn't an exact equivalent to what Ancestors does (though it effectively does the same thing trimmed/untrimmed).

The other difference I can point out here between this DoubleEndedIterator impl of Ancestors and what Components does comes from how intermingling next() and next_back() does. Using the same path again, for the DoubleEndedIterator impl of Ancestors presents the following:

let path = Path::new("/foo/bar/baz///");
let mut ancestors = path.ancestors();
ancestors.next(); // This shows Some("/")
ancestors.next(); // This shows Some("/foo")
ancestors.next_back(); // This shows Some("/foo/bar/baz///")
ancestors.next_back(); // This shows Some("/foo/bar")
... // Everything else is None

let path = Path::new("/foo/bar/baz///");
let mut components = path.ancestors();
components.next(); // Some(RootDir), as_path() produces "foo/bar/baz" 
components.next(); // Some(Normal("foo"), as_path() produces "bar/baz"
components.next_back(); // Some(Normal("baz"), as_path() produces "bar"
components.next_back(); // Some(Normal("bar"), as_path() produces ""
... // Everything else is None, producing ""

I brought this distinction up because I think it shows a difference in focus in each iterator on how Components presents unconsumed components from Components::as_path while Ancestors presents the consumed forward/backward path from Ancestors::next/Ancestors::next_back.

The intention behind this ACP was to avoid having to store the subpaths of a path (forward or backward direction) into a Vec if we can slice the path accordingly and to also perform this slicing in the most efficient way possible. The added bonus to this ACP is that you don't have iterate through Components and use Components::as_path to grab the paths in forward direction (i.e. "/", "/foo", "/foo/bar", etc.) if everything can be done through iterating through Ancestors::next_back.

The non-iterator approach you suggested, while it's fine for the forward/Ancestor::next direction, it is inefficient for reverse ancestors. From what I'm understanding about strip_suffix, with stripping the delimited suffix, you are repeatedly performing slice equality between path's &'a [u8] and suffix's &'a [u8], which in order to ascertain that the tail/suffix portion of path's slice is equal to the given suffix's slice, you have to check the content byte by byte; this seems to make the reverse ancestor function O(N^2) instead of being O(N). However, I suppose you can directly grab a subslice of the path through subtracting the length of the path by the length of the suffix (avoiding checking the slice for equality byte by byte), but I'm unsure if that would be okay or would not cause any panics.

[asder8215]

If it would make things easier for you to review and not feel like this PR will wildly cause something incorrect to occur with the stable Ancestors iterator (since making the DoubleEndedIterator trait for it would have to be insta stable), would you prefer there to be a separate iterator created that presents the reverse of what Ancestors does and we could have another method similar to Path::ancestors? The code for presenting a separate iterator that does the reverse of Ancestors would be simpler because it only needs a front index, and it could be denoted as unstable.

The libs-api team also mentioned in the ACP that:

The suggested children() method did not gain consensus, so we'd like to leave this out. If it can't be implemented in Ancestors we might reconsider, though possibly with a different name.

[Mark-Simulacrum]

Yes, optimizing out the equality checks should be trivial - my code already panics if that logic is wrong, and indexing there is much easier to explain than the logic in this PR as-is.

My feeling is that doing the trimming that components does shouldn't be a big deal, though it would be good to ask libs-api for an opinion on that, especially if it allows for a simplification for this PR.

[asder8215]

My feeling is that doing the trimming that components does shouldn't be a big deal, though it would be good to ask libs-api for an opinion on that, especially if it allows for a simplification for this PR.

I think that would be best.

@rustbot label +I-libs-nominated

[asder8215]

@Mark-Simulacrum I thought about your approach with Path::components and doing all the calculations through taking the difference between slice lengths, and decided to rewrite the Ancestors::next and Ancestors::next_back implementation using that approach. It should be less complicated to read through it comparatively to before.

It passes all the tests I created + fuzzing. However, I do notice that it's slower assuming from what cargo fuzz output is telling me (~80000 execs/s vs ~60000 execs/sec with this current implementation) probably because using the Path::components approach clones the whole Components<'_> struct every time we do Components::as_path (another reason why I didn't prefer this route due to the unnecessary cloning).

I also added Copy trait to the derive for Components iterator (though I could've also just used clone to have Ancestors<'_> iterator contain the Components<'_> iterator). I can remove that if you want as well.

Let me know what you think about this implementation. If this is still not preferable, I'm down with the idea of creating a whole separate struct that does the reverse of what Ancestors<'_> does depending on what libs-api says.

I have the old implementation in the previous commit to this current one. I can squash that out should you want that implementation off record.