Added Default impl to PathBuf

[XAMPPRocky]

No description provided.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[GuillaumeGomez]

👍

[sfackler]

@rfcbot fcp merge

[rfcbot]

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

Concerns:

• there's no obvious default for paths per above (Added Default impl to PathBuf #38764 (comment))

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[brson]

I feel like we've discussed this before and decided not to have a default for paths because it's not clear what it means to be a 'default' path. This implementation I think creates an empty path, but semantically that does not strike me as a 'default path', it strikes me more like a null path and a source of errors.

[brson]

@rfcbot concern there's no obvious default for paths per above

[aturon]

@brson Yes, I believe this came up for Path previously, with the conclusion you mention. But the story is different for PathBuf, as it's common to create an empty PathBuf and then push components onto it.

[sfackler]

A generally good rule of thumb is that if a type has a no-arg new method, it probably makes sense to have a Default impl that does the same thing. I think clippy may even have a lint for that?

[clarfonthey]

@sfackler it does: https://github.com/Manishearth/rust-clippy/wiki#new_without_default

[sfackler]

@brson ping?

[clarfonthey]

Also note that this has precedent in Iterator::unzip which requires Default.

[brson]

@brson Yes, I believe this came up for Path previously, with the conclusion you mention. But the story is different for PathBuf, as it's common to create an empty PathBuf and then push components onto it.

And you would use default for that, not new?

[brson]

Also note that this has precedent in Iterator::unzip which requires Default.

I don't quite follow how that is precedent for Path::default.

[brson]

I'm afraid I still don't see any obvious motivation here except for @sfackler's rule of thumb that types that implement new should implement default. I've stated my concern that empty PathBuf's, which could accidentally slip in to arbitrary deriving-default data structures, are bogus, and prone to error. I don't see a use case for defaulting empty paths.

[clarfonthey]

@brson: the unzip iterator adapter assumes Default and Extend instead of FromIterator, basically stating that the Default implementation for a container is its empty version.

Empty strings might not be a reasonable default for a lot of containers too, but that's the way stdlib seems to point.

[BurntSushi]

I've stated my concern that empty PathBuf's, which could accidentally slip in to arbitrary deriving-default data structures, are bogus, and prone to error.

I feel like this is a pretty good point. Paths have additional semantics in the environment that strings don't have.

I don't feel strongly either way on this given @brson's concerns. It does feel like a possible footgun, but I think adding a Default impl so that PathBuf works with things like unzip is pretty nice. Neither seem like a big deal though.

[aturon]

I agree with @brson that there's a potential footgun here, but I think it's relatively minor, and OTOH not being able to work easily with things like unzip is a pain. So on balance I'm still 👍.

[XAMPPRocky]

I've stated my concern that empty PathBuf's, which could accidentally slip in to arbitrary deriving-default data structures, are bogus, and prone to error.

@brson I'm not really sure if I see the footgun. What errors could arise from the PathBuf that wouldn't already have to be handled by the user?

[tbu-]

See also #32990.

The empty path makes sense for both Path and PathBuf -- when you accumulate over (concatenate) many of these, the empty path is the neutral element, much like the Default implementation for many other types.

[aturon]

Ping @brson, are you satisfied by the recent arguments?

[brson]

@aturon No, but I clicked the box anyway.

[aturon]

Thanks all, and thanks @Aaronepower for sticking in there!

@bors: r+ rollup

[bors]

📌 Commit 108293d has been approved by aturon