BurntSushi / walkdir

Rust library for walking directories recursively.
The Unlicense
1.3k stars 109 forks source link

Add `AsRef<std::path::Path>` for `DirEntry` #184

Closed cactusdualcore closed 1 year ago

cactusdualcore commented 1 year ago

I was working on another project with a function roughly like this

fn foo<I, P>(iter: I) -> Bar
where
    P: AsRef<std::path::Path>,
    I: IntoIterator<Item = P>,
{
    // implementation goes here
}

Somewhere else I tried calling this function with an iterator of DirEntrys

foo(iter_of_entries)

This does not work, because DirEntry does not implement AsRef<Path>, but Path does. So I tried this

let mapped = iter_of_entries.map(DirEntry::path);
foo(mapped);

This does not compile, because DirEntry::path() returns a reference to the entry, which does not exist after the map The thing that works is this

let owned_entries: Vec<_> = iter_of_entries.collect();
let mapped = owned_entries.iter().map(DirEntry::path);
foo(mapped);

It works, because the entries are now collected in a Vec and the closure's argument is already a reference. This is suboptimal, because it requires the user to allocate a vector that is used exactly once to call a function that doesn't want a Vec.

cactusdualcore commented 1 year ago

Another way to solve the problem I had is to convert return an owned value from the function passed to map i.e. a PathBuf in this case.

let mapped = iter_of_entries.map(|e| e.path().to_path_buf());
foo(mapped);

This allocates a new PathBuf for every entry. These already contain PathBufs with identical contents though, so this really just does a lot of unnecessary work.

cactusdualcore commented 1 year ago

There is the newtype pattern for dealing with such cases.

struct MyDirEntry(pub DirEntry);

impl AsRef<Path> for MyDirEntry {
    fn as_ref(&self) -> &Path {
        self.0.path()
    }
}

foo(iter_of_entries.map(MyDirEntry))

This does not allocate unnecessary memory, but I personally find this awkward and overengineered in this specific case, because AsRef is incredibly straightforward to implement.

cactusdualcore commented 1 year ago

I am sorry, I didn't look through the issues before I made this PR. It seems this was declined before in #146 and if this PR is closed, I will respect that immediately.

For now I will keep this PR open though, because I would like present a final argument for my position. The issue that asked for this feature was closed, because

A DirEntry is more than a path, so it seems inappropriate to impl AsRef

I don't think this contradicts the semantics of AsRef. The documentation of std::borrow::Borrow says

If generic code merely needs to work for all types that can provide a reference to related type T, it is often better to use AsRef<T> as more types can safely implement it.

This implies that AsRef marks types that can provide a reference to their generic Argument. I think DirEntry fits this understanding of what types should implement the trait.

BurntSushi commented 1 year ago

My reasoning in #146 still applies. I admit the std docs aren't totally clear on whether or not an AsRef impl here makes sense or not, but that cuts both ways. IMO, it's weird.

Also, you can seemingly achieve your original goal with DirEntry::into_path.