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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

cactusdualcore
Copy link

@cactusdualcore cactusdualcore commented Jul 28, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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.

@BurntSushi BurntSushi closed this Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants