impl PathBuf::add_extension and Path::with_extra_extension

[tisonkun]

Proposal

Problem statement

Sometimes, the program can generate files with its own suffix (extension) to identify specific purpose files.

For example, in my tools there is a dry-run mode for formatting files (link):

            let mut extension = doc.filepath.extension().unwrap_or_default().to_os_string();
            extension.push(".formatted");
            let copied = doc.filepath.with_extension(extension);
            doc.save(Some(&copied))

If we have an add_extension method here to append extra extension, the code can be simplified as:

            let copied = doc.filepath.with_extra_extension(".formatted");
            doc.save(Some(&copied))

This situation can be applied for .bak or other cases.

PathBuf::add_extension(&mut self, extension: impl AsRef<OsStr>) is an additional method to modify the PathBuf in place without construct a brand-new instance.

Motivating examples or use cases

Included above.

Solution sketch

It's more expressive with a patch; see https://github.com/rust-lang/rust/pull/123600:

    pub fn add_extension<S: AsRef<OsStr>>(&mut self, extension: S) -> bool {
        self._add_extension(extension.as_ref())
    }

    fn _add_extension(&mut self, extension: &OsStr) -> bool {
        let file_name = match self.file_name() {
            None => return false,
            Some(f) => f.as_encoded_bytes(),
        };

        let new = extension.as_encoded_bytes();
        if !new.is_empty() {
            // truncate until right after the file name
            // this is necessary for trimming the trailing slash
            let end_file_name = file_name[file_name.len()..].as_ptr().addr();
            let start = self.inner.as_encoded_bytes().as_ptr().addr();
            let v = self.as_mut_vec();
            v.truncate(end_file_name.wrapping_sub(start));

            // append the new extension
            v.reserve_exact(new.len() + 1);
            v.push(b'.');
            v.extend_from_slice(new);
        }

        true
    }

Alternatives

Not applicable. This is a trivial case somewhat.

Links and related work

https://github.com/rust-lang/rust/pull/123600:

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[kennytm]

+1 but your "dry-run" code expected copied being a PathBuf, yet your proposed add_extension() returns a bool. I suppose the simplified code should be:

let mut copied = doc.filepath.clone();
copied.add_extension("formatted"); // should actually handle a `false` result
doc.save(Some(&copied));

Alternatively (additionally?) maybe you actually want a &Path -> PathBuf method

impl Path {
    fn with_extra_extension(&self, extension: impl AsRef<OsStr>) -> PathBuf { ... }
}

let copied = doc.filepath.with_extra_extension("formatted");
doc.save(Some(&copied));

[tisonkun]

@kennytm Thanks for your input. Correct that my sample code has a bug.

I agree that we can additionally add a Path:: with_extra_extension method, since if the case is it can modify the PathBuf in place, the user can avoid construct a new instance.

Let me update in the PR and issue description.

[tisonkun]

@joboet is there a specific timeline that lib-team would pick up this issue? Or how can I add this issue in the schedule?

[pitaj]

Just be patient. There's quite a backlog but they'll get to it eventually.

[joshtriplett]

We discussed this in today's libs-api meeting. We agreed that we do want to add these.

One naming tweak: we'd like to name the Path method with_added_extension, for consistency with PathBuf::add_extension.

[tisonkun]

Thanks for your updates! Let me create a tracking issue in the main repo and update the patch correspondingly.

[tisonkun]

Somehow I found this code snippet:

pub trait PathBufExt {
    /// Append an extension to the path, even if it already has one.
    fn with_extra_extension<S: AsRef<OsStr>>(&self, extension: S) -> PathBuf;
}

impl PathBufExt for PathBuf {
    fn with_extra_extension<S: AsRef<OsStr>>(&self, extension: S) -> PathBuf {
        if extension.as_ref().is_empty() {
            self.clone()
        } else {
            let mut fname = self.file_name().unwrap().to_os_string();
            if !extension.as_ref().to_str().unwrap().starts_with('.') {
                fname.push(".");
            }
            fname.push(extension);
            self.with_file_name(fname)
        }
    }
}

So I'll keep the name with_extra_extension for now and open to comments.

[tisonkun]

Updated at https://github.com/rust-lang/rust/pull/123600. PTAL.

[kennytm]

Source of that code snippet: https://github.com/rust-lang/rust/blob/66b4f0021bfb11a8c20d084c99a40f4a78ce1d38/src/tools/compiletest/src/util.rs#L36-L54

A name chosen by the internal "compiletest" tool should not be used as justification to "keep the name with_extra_extension".

[tisonkun]

OK. Then I can update the method name. It just to reduce the changeset in the first place.