-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Supertrait Auto-impl #3851
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
base: master
Are you sure you want to change the base?
Supertrait Auto-impl #3851
Conversation
The explanation states that the traits need to have a super/sub trait relation. But some examples use traits e.g. |
Signed-off-by: Xiangfei Ding <[email protected]>
6c7841c
to
6114f50
Compare
@dingxiangfei2009 Please don't force-push in this repo, that's heavily discouraged. From the README.md:
|
Quick question for @fmease, should I rename the RFC file from serial |
@dingxiangfei2009 the name won't get updated automatically. We've merged the 2025H2 RFC, forgot to update and had to open a separate PR to fix the name: #3860. |
It's possible to declare that an auto implementation is unsafe. | ||
```rs | ||
trait MySubTrait: MyTrait { | ||
unsafe auto impl MyTrait; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, does this create an unsafe
requirement or discharge an existing unsafe
requirement? you seem to have it do both which is generally not how Rust uses unsafe
anymore, now that we have unsafe_op_in_unsafe_fn
enabled by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this can be confusing.
- Inside a
trait Subtrait: .. { .. }
block,unsafe auto impl
creates anunsafe
requirement. - Inside a
impl Subtrait { .. }
block,unsafe auto impl
discharges the correspondingunsafe
requirement.
Should we adjust the "keyword" orders, so that the distinction can be more obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems less useful than what I would have expected needing unsafe
for -- something like:
/// safety: `returns_even` must always return an even integer
// unsafe means we create a requirement that impls must fulfill
pub unsafe Supertrait {
fn returns_even(&self) -> u8;
}
pub trait Subtrait: Supertrait {
/// safety: returns_even always returns an even integer
// unsafe here means we're fulfilling the requirement since this is an impl of Supertrait
unsafe auto impl Supertrait {
fn returns_even(&self) -> u8 {
self.get_int().wrapping_mul(2)
}
};
fn get_int(&self) -> u8;
}
impl Subtrait for u8 {
fn get_int(&self) -> u8 {
*self
}
}
pub struct Even(u8);
impl Subtrait for Even {
// no unsafe needed here because this isn't fulfilling or creating any safety requirements,
// the unsafe impl Supertrait below does that,
// all we're doing here is saying this impl doesn't also impl Supertrait
extern impl Supertrait;
fn get_int(&self) -> u8 {
self.0 / 2
}
}
// safety: returns_even always returns an even integer because Even contains an even integer
// unsafe here means we're fulfilling the requirement since this is an impl of Supertrait
unsafe impl Supertrait for Even {
fn returns_even(&self) -> u8 {
self.0
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @programmerjake 's expectations-- I would've expected unsafe auto impl
to discharge an unsafe requirement-- that is, guarante that the provided impl of the supertrait fulfills the requirements of the unsafe trait SuperTrait
. This is the usual meaning of unsafe impl
in Rust.
I wouldn't have expected there to be an option to explicitly specify an auto impl as unsafe to apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So to not overload the meaning of unsafe
, I will stick with the stable Rust's interpretation of unsafe
.
On the contrary, is it more acceptable if we use attributes as signals? It could be an attribute like #[require(unusal_override)]
on the def-site auto impl Trait { .. }
and an attribute like #[allow(unusual_override)]
on the overriding auto impl
in the downstream implementor. I am happy to take suggestions, including using the existing linting machinery.
As an example,
auto impl Subtrait: Supertrait {
#[require(unusual_override)]
auto impl Supertrait {
type Target = Self;
}
}
impl Subtrait for MyType {
#[allow(unusual_override)] // <-- this is required
auto impl Supertrait {
type Target = Ref<MyType>;
}
}
|
||
|
||
|
||
<details> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another good hypothetical example which I think Rust should implement:
splitting ToOwned
so you can use Cow
as a reference-or-owned type even if you have an unclonable type:
pub trait AsOwned {
type Owned: Borrow<Self>;
}
pub trait ToOwned: AsOwned {
auto impl AsOwned;
fn to_owned(&self) -> Self::Owned;
fn clone_into(&self, target: &mut Self::Owned) {
*target = self.to_owned();
}
}
that way Cow
can be:
pub enum Cow<'a, B: AsOwned + ?Sized> {
Borrowed(&'a B),
Owned(B::Owned),
}
impl<B: AsOwned + ?Sized> Deref for Cow<'_, B> {
type Target = B;
fn deref(&self) -> &Self::Target {
match self {
Self::Borrowed(v) => v,
Self::Owned(v) => v.borrow(),
}
}
}
impl<B: ToOwned + ?Sized> Cow<'_, B> {
pub fn into_owned(self) -> B::Owned {
match self {
Self::Borrowed(v) => v.to_owned(),
Self::Owned(v) => v,
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll note that @scottmcm came up with a much better idea that doesn't need to split ToOwned
-- essentially adding another type parameter to Cow
for the owned type too. That said, the split could still be useful when you don't want to write out the owned type and use AsOwned::Owned
but your borrowed type can't implement ToOwned
.
} | ||
``` | ||
|
||
However, this practice will not be encouraged eventually under provision of this RFC. For this reason, we also propose a future-compatibility lint, which will be escalated on a future Edition boundary to denial. The lint shall highlight the existing `auto impl` block in the subtrait definition and suggest an explicit `extern impl` statement in the subtrait implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what the motivation is for making this a warning, and I'm not sure how we'd ever translate this into a hard error. Crates may want to add new auto impl
s for existing supertrait relationships. If we're able to detect and allow this via #[probe_extern_impl]
, I'm not convinced that this shouldn't be the default behavior.
That is, in order to support #[probe_extern_impl]
, we have to solve the impl detection problem. If we've successfully solved that problem, I think I would prefer that this be the default behavior: it's more ergonomic, and it avoids introducing unnecessary warnings into existing working code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @cramertj I thought I pressed comment button but it is still here as draft. Oh well...
The shortest path to success here is to invoke the trait solving machinery to tell us if an external candidate impl
exists. However, it puts us under the mercy of the power of the trait solver and the effort to formulate its capability, what is expected to work and what is not, in this or a future RFC is insurmountable. Making it a default behaviour would probably force the compiler to work a lot harder.
I propose that we could just start an experiment early, to see if we can make it work; or whether we need to resort to a simpler test due to potential performance regression and any others to follow.
// and we deduce based on some applicability criterion ... | ||
impl AutoMyTrait for Foo {} // <-- generates MyTrait | ||
|
||
// Suppose this item is added at one point of time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankfully, the addition of a blanket impl is already a breaking change. Given that, we can check when deciding to implement a trait whether there exists another potentially-overlapping impl. Even if the blanket impl does not overlap, we should treat it as potentially-overlapping.
I believe this analysis can also be done with only a conservative "is it possible that these Self
types may overlap"-style check, just as coherence does today. For example:
trait MyTrait: SuperTrait {
auto impl SuperTrait;
};
struct MyType;
impl MyTrait for MyType {}
// Turns off `auto impl` for *all* types, regardless of whether the bound applies.
//
// Note that, because blanket impls are only allowed in the crate that defines the trait,
// this blanket impl must be in the same crate defining `SuperTrait`, so it is visible to
// the definition of `MyTrait`.
//
// This means that we could even make it a hard error to use `auto impl SuperTrait` for
// any `SuperTrait` that has a blanket impl for an uncovered type parameter `T`.
impl<T: ...> SuperTrait for T { ... }
// Turns off `auto impl` for any type that can may unify with `&mut T`.
impl<T: ...> SuperTrait for &mut T { ... }
// Turns off `auto impl` for any type that may unify with `MyType<T>`.
//
// Note that this is not *purely* a syntactic analysis, but requires a coherence-like check:
// If `MyType` is a normal ADT generic over `T`, this is only other `MyType` values, but
// if `MyType` is e.g. `type MyType<T> = T;`, then this is all types.
impl<T: ...> SuperTrait for MyType<T> { ... }
``` | ||
|
||
### Example: relaxed bounds via new supertraits | ||
A common use case of supertraits is weaken bounds involved in associated items. There are occassions that a weakend supertrait could be useful. Suppose that we have a factory trait in the following example. In this example, the `async fn make` factory method could be weakened so that the future returned could be used in the context where the future is not required to be of `Send`. This has been enabled through the use of [the `trait_variant` crate](https://docs.rs/trait-variant/latest/trait_variant/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It could be useful to specify concretely that this is a reference to the issues encountered by the tower::Service
API: https://docs.rs/tower/latest/tower/trait.Service.html
This is a very interesting proposal! As maintainer of Would you mind adding a |
@obi1kenobi Yes, I remember that. I will tag you in the "changelog" when I add the SemVer hazard section for your reviews. |
|
||
It is still up for discussion. | ||
|
||
## What is the SemVer implication? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@obi1kenobi what do you think? Should we add any further consideration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the information provided here is not sufficient to evaluate this proposal's SemVer impact, and it is also not sufficient to write a SemVer reference entry nor to write an exhaustive set of lints in cargo-semver-checks
for this feature.
We need to have more details, consider more edge cases, and ideally set up more structure in the section.
Example edge cases we should consider in detail:
- The text considers introducing
auto impl
into an existingtrait Trait
. What are the implications if we removeauto impl
fromtrait Trait
, essentially undoing that addition? - When we
auto impl
a supertrait and supply default implementations for the supertrait's methods, presumably deleting such a default implementation is SemVer major? For example, this might still compile if the supertrait also had a default implementation, and ourauto impl
used to override that default impl with a new default impl. - Is adding or removing
unsafe
aroundunsafe auto impl
or similar able to cause compilation failures in downstream code? - Is switching between
extern impl
andauto impl
ever possibly breaking toward users of the type ultimately implementing such traits, or is this merely a syntactic difference? - Can adding or removing an item in a supertrait cause a downstream trait that uses
auto impl
to be broken? - Can renaming an item in a supertrait cause a downstream trait that uses
auto impl
to be broken? For example, if the new item name is the same / ambiguous with an existing item in the downstream trait?
Based on my skim through the doc, it would appear that at least some of these cases are at least SemVer hazards if not outright SemVer-major. It would be useful to have them enumerated in this section, so they can be considered specifically from a SemVer angle. This is because most of our RFCs consider language design in the point-in-time setting ("I gave the compiler some code and it did X") while SemVer is about language design across time ("my code used to compile fine, then I updated a dependency and now it doesn't") and thus requires a different mindset.
We would like to propose
auto impl
syntax for supertraits for a few language enhancement, to ultimately facilitate easier trait evolution and refactoring and easier trait authoring when trait hierarchy is in consideration.24/09/2025: revision published with typo fixes and dropping the
unsafe
qualifier forprobe_extern_impl
attribute.Past edit history
21/09/2025: revision published with the following changes. - Address [comments from @cramertj](https://github.com//pull/3851#discussion_r2301552062) by including the worked examples mentioned in the [design meeting](https://hackmd.io/@1n2nRkvSTd-QgK8cCPds1w/HkBmeCE7xx). A new proposal is included so that an unsafe attribute may be used to allow compiler to probe further and decide if `auto impl` default implementation would be conflicting. This might need consultation with the types team as its effectiveness and stability depends very much on the capabilities of the trait solver. - Address [comments from @ElchananHaas](https://github.com//pull/3851#discussion_r2306004253) and [comments from @N4tus](https://github.com//pull/3851#issuecomment-3239334843) on several deficiencies in the worked examples, such as missing superbounds and complexity in the motivating examples. - Address [a comment from @programmerjake](https://github.com//pull/3851#discussion_r2302020617) by removing the template text.Rendered