refining_impl_trait: Decide whether explicit opt-in is needed

[tmandry]

What is refining_impl_trait?

The refining_impl_trait lints detect usages of return-position impl traits in trait signatures which are refined by implementations, meaning the implementation adds information about the return type that is not present in the trait.

Example

pub trait AsDisplay {
    fn as_display(&self) -> impl Display;
}

impl<'s> AsDisplay for &'s str {
    fn as_display(&self) -> Self {
        *self
    }
}

fn main() {
    // users can observe that the return type of
    // `<&str as AsDisplay>::as_display()` is `&str`.
    let x: &str = "".as_display();
}

The above code compiles, but produces a warning:

warning: impl trait in impl method signature does not match trait method signature
 --> src/main.rs:8:29
  |
4 |     fn as_display(&self) -> impl Display;
  |                             ------------ return type from trait method defined here
...
8 |     fn as_display(&self) -> Self {
  |                             ^^^^
  |
  = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
  = note: `#[warn(refining_impl_trait)]` on by default
help: replace the return type so that it matches the trait
  |
8 |     fn as_display(&self) -> impl std::fmt::Display {
  |                             ~~~~~~~~~~~~~~~~~~~~~~

Explanation

Callers of methods for types where the implementation is known are able to observe the types written in the impl signature. This may be intended behavior, but may also lead to implementation details being revealed unintentionally. In particular, it may pose a semver hazard for authors of libraries who do not wish to make stronger guarantees about the types than what is written in the trait signature.

Future possibilities

Add an explicit #[refine] attribute. This would make it easier to opt in to refining on a per-method basis and make the refine check feel more like a builtin feature of the language.

Make refining_impl_trait error-by-default in a future edition. This would mean we are committed to explicit refinement for impl Traits. Since this would happen over an edition, we could choose to go even further and make it a hard error so you could not blanket-allow it in your crate.

Keep all refining_impl_trait warn-by-default.

Make refining_impl_trait_internal allow-by-default, but keep refining_impl_trait_reachable warn-by-default. This would mean that we think semver hazards for publicly-reachable traits in a crate are important enough to lint against. But for uses internal to a crate, opting in is unnecessary overhead for the user.

Make all refining_impl_trait allow-by-default. This would mean that we think most uses of refinement for return-position impl Trait are deliberate and unlikely to cause semver hazards or other unexpected behavior.

Discussion

The language team is collecting feedback about this lint. We would like to know

• Whether users expect refinement to require an explicit opt-in.
• Whether users see value in the refinement check to prevent accidental leakage of implementation details.
• Whether users expect refinement to work the same between publicly-reachable impls and internal-only impls.
• Whether users would like to see a #[refine] attribute in the future.

[schuelermine]

User feedback:
Given my experience in other languages I expected refinement to be available by default and not linted against.
I don’t care how it works, I just want it to exist (so #[refine] is fine by me).

[dhardy]

Personally I like #[refine], but...

... is it refinement when the impl is on a private type, not in any way observable outside the current module? In this particular case I don't see any need to warn (assuming the lint remains just a warning).

[rdrpenguin04]

I don't really care; I'd prefer not needing to opt-in, but I'm fine either way.

[thynson]

Make refining_impl_trait_internal allow-by-default, but keep refining_impl_trait_reachable warn-by-default.

Prefered in this way with #[refine] introduced.

[jean-pierreBoth]

#[allow(refining_impl_trait)] is ok for me

[JarredAllen]

(not sure if this is the right thread for this) I just hit a funky case that caused this lint where I didn't expect it, in code like this:

trait DoSomething {
    fn do_something() -> impl Future;
}

struct SomethingDoer;
impl DoSomething for SomethingDoer {
    async fn do_something() {}
}

If I specify the output type of do_something (e.g. fn do_something() -> impl Future<Output = ()>), then the async fn implementation for the trait is allowed (I guess the implementation here desugars to -> impl Future<Output = ()>), but then the trait has to impose a return type on the do_something() method.

Generally, I'm fine with whatever #[refine]/#[allow(..)] combination is deemed desirable, but I'd expect the code snippet I put here to work without requiring any annotations (either through refining or through changing the desugaring for async fn in this case, though it's probably too late to change the desugaring).

[maurer]

Ran into this dealing with impl Trait as an unconstrained part of a trait function signature.

Simplified case

Because the Ok branch is never constructed, the Rust compiler doesn't know what type to pick for that branch. Refining the type gives Rust an option. This isn't quite the same as what's being described, as we don't actually necessarily want users to have access to this type in the signature, we just want to give Rust a candidate for the existential so that it can finish compilation.

This can be worked around by making an explicit internal copy of the function, but this doesn't seem great.

[JarredAllen]

Ran into this dealing with impl Trait as an unconstrained part of a trait function signature.

Simplified case

Because the Ok branch is never constructed, the Rust compiler doesn't know what type to pick for that branch. Refining the type gives Rust an option. This isn't quite the same as what's being described, as we don't actually necessarily want users to have access to this type in the signature, we just want to give Rust a candidate for the existential so that it can finish compilation.

This can be worked around by making an explicit internal copy of the function, but this doesn't seem great.

You can also get around it by annotating the None with an explicit type for the generic: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a46cbfd8d7abda90d9862dc413bacbb9

impl P for str {
    fn try_iter(&self) -> Option<impl Iterator<Item = u32>> {
        None::<std::array::IntoIter<u32, 0>>
    }
}

Still not perfect that you have to annotate the None with an explicit type even though it's never used, but that lets you avoid refining without writing another function.

[obi1kenobi]

Generic bounds applied to trait methods also have some unexpected quirky behavior under refinement — I opened #129251 to avoid cluttering this issue with a tangent.

[noakodaren]

I think it should warn for both public and internal impls; leaking internal implementation details to oneself should also be a conscious decision. It would be nice if it would become a hard error, but it is probably not worth it. In any case, #[refine] would be nice.

[j-a-m-l]

For me refinements are useful, and I expect that crates that use my library could avoid unnecessary conversions.
This is an example of my current use case:

pub trait Whatever {
    fn id(&self) -> impl ToString;
}

pub struct InternalObjectString<'a> {
    id: &'a str,
}

impl<'a> Whatever for InternalObjectString<'a> {
    fn id(&self) -> String {
        self.id.to_string()
    }
}

pub struct InternalObjectInt {
    id: u32
}

impl<'a> Whatever for InternalObjectInt {
    fn id(&self) -> u64 {
        self.id as u64
    }
}

// In other crate
use whatever::Whatever;

struct ExternalCrateObject {
    id: u32,
}

impl Whatever for ExternalCrateObject {
    fn id(&self) -> u32 {
        self.id
    }
}

In my case, types that implement Whatever could be used by the user to unify different implementations or they could be used alone, so it is not necessary to transform them to the same type always.
For instance, they may be interested only in InternalObjectInt or they may need to implement their own version (ExternalCrateObject).

I also prefer that the type that implements the trait could be stricter and more explicit about the types of its methods.

So:

• Refinements should be allowed in the public API, using an attribute or whatever.
• Annotating them for the internal API maybe is unnecessary and adds more overhead, but it could be useful for the public API and the compiler could detect those leaks.

About introducing #[refine]:
Using a new attribute such as #[refine] would be nice, but using #[allow(refining_impl_trait)] is fine.

I'd introduce it only if it is necessary for the internal API too, or if it can be used in more contexts. Some ideas:

• Using it to limit the feature to the public API #[refine(pub)] or internal API #[refinement(crate)] in case that it must be set explicitly.
• Exposing the public API as an specific type:

pub trait Whatever {
    #[refine(String)]
    fn id(&self) -> impl ToString;
}

• Using #[refine] at other levels, such evaluating the return at compile time when possible:

trait Number {
    #[refine(return < 0)]
    const fn negative(&self) -> impl Into<i32> {
        -1
    }
}