Should Path::display return impl Debug + Display instead of concrete type?

[camelid]

Currently, the Path::display() function returns the concrete
std::path::Display type. As far as I can tell, this type's public API consists
of only its implementations of Debug and Display. Should Path::display()
return a generic impl Debug + Display instead of allowing users to rely on it
being a particular concrete type?

I think the potential breakage is probably none since you can't do anything with
std::path::Display short of using its Debug and Display impls. You could
theoretically have a function that requires its argument to be of type
std::path::Display, but I don't see why that would be useful. If there are
such functions, they could likely be changed to take impl Debug + Display.

Right now, there's not necessarily a reason to do this, but in general we want
to hide implementation details in case we need to make changes in the future
(cc #80634).

[tesuji]

Isn't it a breaking change if we change return type by now?

[oli-obk]

Isn't it a breaking change if we change return type by now?

if we use pub type Display = impl fmt::Display + fmt::Debug; and rename path::Display, then all the imports should still line up, but I'm not sure we are at a stage where we can use type alias impl trait this way

[m-ou-se]

Changing it to -> impl '_ + fmt::Display + fmt::Debug makes this line in sys_common/backtrace.rs fail:

rust/library/std/src/sys_common/backtrace.rs

Line 230 in a4cbb44

fmt::Display::fmt(&file.display(), fmt)

With this error:

error[E0597]: `file` does not live long enough
   --> library/std/src/sys_common/backtrace.rs:230:24
    |
230 |     fmt::Display::fmt(&file.display(), fmt)
    |                        ^^^^----------
    |                        |
    |                        borrowed value does not live long enough
    |                        a temporary with access to the borrow is created here ...
231 | }
    | -
    | |
    | `file` dropped here while still borrowed
    | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `impl core::fmt::Debug+core::fmt::Display`
    |
    = note: the temporary is part of an expression at the end of a block;
            consider forcing this temporary to be dropped sooner, before the block's local variables are dropped
help: for example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block
    |
230 |     let x = fmt::Display::fmt(&file.display(), fmt); x
    |     ^^^^^^^                                        ^^^

The problem is very subtle. For path::Display<'a>, it is known 'a will not be relevant when it gets dropped, so may be dangling at that point. But that's not the case for impl 'a + fmt::Display + fmt::Debug, which might access things with lifetime 'a in its Drop implementation.

There's no such thing as -> impl #[may_dangle] 'a + ... or -> impl ... + !Drop. However, since Copy implies !Drop, making path::Display: Copy and returning impl '_ + fmt::Display + fmt::Debug + Copy makes the error disappear again.

[m-ou-se]

Looks like thiserror names the path::Display type directly, which would fail with this change: https://github.com/dtolnay/thiserror/blob/c09ddc2241516b8c5859b8ef1f70305f762f2d0a/src/display.rs#L15

[camelid]

Would it be worth a crater check run, or no? Since this is pretty low-priority, maybe we could consider a crater run on just some popular crates?

[m-ou-se]

We can't run a crater run if the try build won't even succeed. thiserror is a dependency of rustfmt.

[BurntSushi]

I'm not sure what the motivation for something like this is, even if backcompat weren't a concern. Returning an impl Trait is awfully inflexible in today's Rust. I don't think it should be in a public API anywhere unless it has to be.

[camelid]

Returning an impl Trait is awfully inflexible in today's Rust.

But there's pretty much nothing you can do with std::path::Display anyway. Can you explain what you mean about it being inflexible?

[BurntSushi]

There are a lot fewer things you can do with an impl Trait as opposed to a normal concrete type. See: rust-lang/api-guidelines#60

And you absolutely might want to do something with the type. Like maybe wrap it up on another type.

[camelid]

And you absolutely might want to do something with the type. Like maybe wrap it up on another type.

I guess I would think you could just wrap it like struct MyWrapper<T: fmt::Display>(T);. But I guess then you can't ensure it was created from Path::display()?

[BurntSushi]

That, and now you've introduced a type parameter where one really isn't needed. This dance is kind of supporting my stance. If you return an impl Trait, then there are fewer things you can do with it. Therefore, in public APIs, you shouldn't use it unless you have to or if there are some other set of material advantages that I'm not anticipating here.

[m-ou-se]

In the case of these display adapters, this wrapper could just contain the Path directly though. You could always create the path::Display right where you need it, and use the Path itself if you need to keep it around. (path::Display isn't even Copy, so not a great type to wrap anyway.)

[sfackler]

Why would we make a breaking change to the standard library when "there's not necessarily a reason to do this"? What is the expected outcome of this issue, other than answering the question in the title with "no"?

[m-ou-se]

Oh we definitely shouldn't change Path::display. That's just a breaking change with no benefit. But the question is relevant to new similar adapters that might be added in the future. E.g. for OsStr, where this question popped up.

[jyn514]

I'm going to close this since we're not going to change Path::display.