Rust: Tracking issue for allowing overlapping implementations for marker trait

[nikomatsakis]

Tracking issue for rust-lang/rfcs#1268.

Status

• [x] Initial implementation: #41309
• [ ] Documentation
• [ ] Move to stabilize
• [ ] Stabilization PR

Known bugs

• [ ] [cross-crate support lacking?](https://github.com/rust-lang/rust/issues/29864#issuecomment-295907958)

History

• initial implementation #41309
• https://github.com/rust-lang/rust/pull/53693

Prior to stabilization

1. Is it ok that adding items to a previously empty trait is a breaking change? Should we make declaring something a marker trait more explicit somehow? --> resolved by adding explicit #[marker] annotation, see https://github.com/rust-lang/rust/pull/53693

[nikomatsakis]

With respect to the second question of whether we should make a more explicit notion of marker trait, I have been pondering this since the RFC discussion. On the one hand, there are a number of places in Rust today where going from nothing to something is a breaking change: adding a private field to a struct, for example.

The difference here I think is that:

1. Given the ability to specify defaults, adding an item to a trait does not necessarily have to be a breaking change otherwise (though methods and fns can cause new ambiguities etc).
2. If there are impls that are taking advantage of the ability to overlap, there may just be no way to port older code forward, because it would require overlap.

However, the second point depends a lot on the fate of specialization. If we wind up adopting specialization, and in particular a model that does not require strict subsets, then it would still be possible to adapt existing impls. This makes me a lot less worried. Of course, implementing that form of specialization is tricky, but not (I don't think...) that much trickier than implementing THIS suggestion. The hard part in both cases is the potential for overlap.

[sgrif]

@nrc This has the RFC implemented label, but I don't think it has been. If it has, is there a feature gate that I'm missing? If it hasn't been implemented, is there anything I can do to help push this along? It's now been a year since the RFC was accepted.

[nikomatsakis]

@sgrif swapped labels

[frewsxcv]

Initial implementation just merged in https://github.com/rust-lang/rust/pull/41309.

[sgrif]

Looks like this doesn't work when the overlapping impls are provided by two separate crates, even if both crates have #![feature(overlapping_marker_traits)] enabled (ideally only one of the two crates would need the feature)

[nikomatsakis]

@sgrif interesting, what is the example you have in mind?

[cramertj]

One drawback mentioned in the RFC is that this feature makes it a breaking change to add default methods to a trait. Forgive me if this issue was already discussed and resolved, but I didn't see it mentioned in the RFC discussion. Prior to hearing this RFC mentioned, I never thought of "marker traits" (i.e. traits without any items) as anything special or distinct from regular traits, and I would not have expected it to be a breaking change to add default methods to them.

Personally, I'd prefer if the user had to opt-in to making a trait a "marker" trait so that they didn't unwittingly cause breakage.

[Ixrec]

One alternative I don't see in the RFC or its discussion thread is doing this only to auto traits, not to all "marker traits". Unless I missed something, all of the motivating examples (including hypotheticals) appear to be auto traits, and afaik you can't add a method to an auto trait anyway so that breaking change issue would just evaporate.

[cramertj]

Yeah, I'd be totally fine with just doing this on auto traits (whose definition is already unstable).

[Vurich]

You could explicitly seperate marker and "normal" traits by having marker traits have a different syntax, my suggestion would be:

trait MyMarker;

Similar to ZSTs. This makes it clearer when you're turning a marker trait into a non-marker trait.

[nikomatsakis]

@cramertj I would be in favor, I think, of a #[marker] attribute to signifify this behavior. There is always this line about whether attributes ought to be "semantically meaningful", but I kind of think it's ok.

[nikomatsakis]

I also don't believe this is particularly tied to auto traits.

[cramertj]

@nikomatsakis There's also precedence for semantically meaningful attributes in derives and in #[nonexhaustive] (or whatever it's called now).

[mtak-]

I needed this feature for a single trait in a personal project, and turning it on means overlapping is allowed for _all_ of my marker traits. I'd personally like a warning issued for overlapping impls which can then be allow'ed or deny'ed. This would be similar to haskell's per instance {-# OVERLAPPING #-}.

Also, I assume this is a bug in the current implementation, but order seems to matter:

#![feature(overlapping_marker_traits)]

pub trait F {}
impl<T> F for T where T: Copy {}
impl<T> F for T where T: 'static {}

Gives the error:

error[E0310]: the parameter type `T` may not live long enough
 --> src/lib.rs:5:9
  |
5 | impl<T> F for T where T: Copy {}
  |      -  ^
  |      |
  |      help: consider adding an explicit lifetime bound `T: 'static`...
  |
note: ...so that the type `T` will meet its required lifetime bounds
 --> src/lib.rs:5:9
  |
5 | impl<T> F for T where T: Copy {}

While this (same code, different order) compiles and works as expected:

pub trait F {}
impl<T> F for T where T: 'static {}
impl<T> F for T where T: Copy {}

[mtak-]

Also the error messages when using the above trait aren't great.
https://play.rust-lang.org/?gist=707605bff9858839c0d870a5c380ab2e&version=nightly

The errors always indicate that the type needs a 'static lifetime, when it could also be Copy.

This appears to be a problem solely with lifetimes (which is my use case). If 'static is changed to Debug in the above, a less confusing error is given.

[nikomatsakis]

That particular example will be complex to make work as desired in any case, due to how region handling is done in rustc at present (e.g., https://github.com/rust-lang/rust/issues/21974). In general, integrating "or" constraints and region solving is a bit tricky I suppose. I imagine we can do it by making the region checker smarter, which we have to do eventually.

[Vurich]

No matter what the actual syntax is (either an annotation or ;-vs-{}) there definitely has to be a difference between possibly-overlapping marker traits and nonoverlapping-but-empty-by-circumstance traits otherwise we'll get a host of forward-compatibility-ensuring const __UNUSED: () = (); items like we already do to prevent constructing empty-by-circumstance structs.

[scottmcm]

A thought in favour of #[marker] over trait Foo;: one could allow defaulted associated items on marker traits, and just prohibit any of the impls from overriding them.

(Inspired by a discussion with the bunny about having something like #[marker] unsafe trait Zeroable { fn zeroed() -> Self { unsafe { std::mem::zeroed() } } } in winapi.)

Edit: Also, using an attribute fits the generally-suggested "do it with a macro or attribute first, then add syntax once we have more experience" procedure.

[Vurich]

I actually quite like that idea, since it's not an uncommon pattern to have convenience functions in traits and often these will only ever be implemented in the defining crate and it might be a useful tradeoff to allow overlapping implementations at the cost of disallowing consuming crates from overriding the function body. If that was to be allowed, though, I'd prefer the name #[overlapping] instead of #[marker].

[nikomatsakis]

It seems obvious that we should make a #[marker] annotation.

[cramertj]

I was just thinking about a way to prevent users from implementing the Unpin trait for types defined by a local macro invocation, and one idea I had was to always generate an Unpin implementation that was conditional so as to prevent the user from creating their own unconditional impl. However, this would be made unsound if this feature ever stabilized. I don't know that my idea was a good one, or that it's a pattern worth encouraging, but I thought it was interesting enough to share.

[scottmcm]

@cramertj Agreed; the discussion seems to have decided that this needs to be opt-in on the trait, not something that just happens. I've started a PR towards doing so: https://github.com/rust-lang/rust/pull/53693

[Centril]

cc https://github.com/rust-lang/rust/issues/60784#issuecomment-491901601

[Centril]

cc https://github.com/rust-lang/rust/issues/61651

[RalfJung]

So is the current status that we have both #[feature(overlapping_marker_traits)] which applies to any trait with no items, and #[marker]?

Based on https://github.com/rust-lang/rust/issues/29864#issuecomment-409398581, it seems like consensus is that overlapping_marker_traits shouldn't happen without opt-in from the trait, and indeed with https://github.com/taiki-e/pin-project/pull/18 we now have a concrete example for code that would be unsound if overlapping impls were allowed for Unpin. Is that correct? Does that mean we could remove the code that can permit any item-less trait to have overlapping impls?

[taiki-e]

The current #[feature(overlapping_marker_traits)] certainly seems to be able to break the guarantee that pin-project provides: playground

cc https://github.com/taiki-e/pin-project/issues/105

[Vurich]

I think reverting to having the trait opt-in is better. This also means that you don't have to break backwards compatibility to add items to a trait.

[scottmcm]

@Vurich note that the opt-in is the new direction (FCP), so there isn't any reverting needed.

So I think there's a PR opportunity here of removing overlapping_marker_traits (the new one is marker_trait_attr), if anyone's interested in making one.

[Others]

I'd like to see this stabilized. The next check box on the issue is documentation, which I'm happy to contribute if it moves this feature forward. Could someone point me to where we'd want to document this feature?

[nikomatsakis]

@Others I think moving forward here would be reasonable. I think a first step would be to do a bit of "archival research", documenting the history and current design (which is now using e.g. a #[marker] attribute) and looking for any unresolved questions. We've not done a very good job of tracking that on this issue, it seems, though I just made a few edits to the main header.

One thing I just remembered is that https://github.com/rust-lang/rust/pull/68004 we were discussing some interaction between marker traits and negative impls (see this comment for more details). In particular there was a sort of unresolved question (I can't find the issue just now) that was asking whether marker traits should be exempt from the orphan rules (i.e., I should be able to implement a marker trait for any type, not only my own types).

I think the answer to this is probably no -- or at least I would not feel comfortable stabilizing those orphan rule extensions until we talk more about negative reasoning.

[jswrenn]

I've relied on #[marker] extensively for prototyping new features in typic. The instability of #[marker] is a major barrier to adopting typic in zerocopy. I cannot overstate how much I'd like to see this stabilized. I'm happy to chat about the use cases I've encountered!

[Aaron1011]

In particular there was a sort of unresolved question (I can't find the issue just now) that was asking whether marker traits should be exempt from the orphan rules (i.e., I should be able to implement a marker trait for any type, not only my own types).

I think the answer to this is probably no -- or at least I would not feel comfortable stabilizing those orphan rule extensions until we talk more about negative reasoning.

I agree - I think any kind of 'orphan rule exemption' for #[marker] should require an explicit opt-in.

Imagine you have a 'safe marker trait', like std::cmp::Eq. If you're willing to commit to having it always have no associated items, it might make sense to make it #[marker]. However, disabling the orphan rules would allow downstream crates to break a (safe) API contract. For example (in a world where std::cmp::eq is #[marker]):

// crate1
struct NotReflexive;
impl std::cmp::PartialEq for NotReflexive {
    fn eq(&self, _other_: &Self) -> bool { false }
}

// crate 2
impl std::cmp::Eq for crate1::NotReflexive {} // uh oh

Here, we've violated the (safe) API contract for std::cmp::Eq, and there's nothing that crate1 can do to prevent this. While this can't lead to unsoundness, I think it's a good idea to be able to prevent this kind of code from compiling (via the orphan rule).

[RalfJung]

With Unpin, we even have a safe marker trait where this would lead to unsoundness.

[nikomatsakis]

@jswrenn would you be interested in working with me to stabilize the marker trait annotation?

[jswrenn]

@nikomatsakis Currently on my honeymoon, but I'd be thrilled to help after July 1!

[rylev]

@nikomatsakis @jswrenn I'd be interested in helping with this. How do we get started?

[WaffleLapkin]

Is it possible for marker-trait implementations to ignore the unconstrained type parameter error? Marker-trait impl can't use type parameters in any way, so I would expect this to compile:

#![feature(marker_trait_attr)]
#[marker]
trait Fun {}
impl<F, T, R> Fun for T where F: FnOnce(T) -> R { 
    // In difference with common traits you can't use T or R here 
    // so this can't bring ambiguity "which type param to use for a concrete type"
}

[playground]
However, today it gives an error:

error[E0207]: the type parameter `F` is not constrained by the impl trait, self type, or predicates
 --> src/lib.rs:4:6
  |
4 | impl<F, T, R> Fun for T where F: FnOnce(T) -> R { 
  |      ^ unconstrained type parameter

error[E0207]: the type parameter `R` is not constrained by the impl trait, self type, or predicates
 --> src/lib.rs:4:12
  |
4 | impl<F, T, R> Fun for T where F: FnOnce(T) -> R { 
  |            ^ unconstrained type parameter

[nikomatsakis]

@WaffleLapkin I would not want to change something that fundamental without more thought, although I can't give an immediate reason why it is strictly necessary. Without that rule, though, you would have fundamentally "unconstrained" types that result from type check and which we cannot recover -- as things are, we have enough information to fully recover all the information we need for any trait match.

[nikomatsakis]

@rylev basically we just need to create a stabilization PR with a write-up that includes

• quick summary of the feature
• any changes since the RFC
• some tests that highlight key aspects of the feature

There is some documentation here about writing a report and also preparing a PR.

[rylev]

I've tried to collect the known open questions and issues here.

Questions

• Bike-shedding the name. Does this require an RFC? @nikomatsakis thoughts on this?
• Is there any unsoundness around Unpin now that you are required to explicitly opt into marker traits with #[marker]? I haven't able to completely follow this topic yet, but it seems that @RalfJung was able to convince himself that there is no unsoundness although his latest comment does not indicate as such. I _believe_ however that this concern along with others are resolved by the fact that the mechanism is now opt-in.
• How does this interact with negative trait impls? The discussion of this seemed to have good ideas on how to handle this, but was consensus reached? This is tied to how #[marker] traits interact with the orphan rule. Currently #[marker] traits fully respect the orphan rule.

Bugs

• Does the implementation work cross crate? I'm not fully sure what this means. This might be a left over from before #[marker] explicit opt-in was introduced.
• The order of impl declarations matter.

I think most of these should be answered before we move forward with stabilization.

[RalfJung]

I believe however that this concern along with others are resolved by the fact that the mechanism is now opt-in.

Yes, opt-in is key. Likely none of the existing auto traits can be made #[marker], but certainly not Unpin.

[rylev]

@nikomatsakis do you have thoughts on whether an RFC is required for stabilizing the name #[marker]?
@Centril brought up good concerns on negative trait impls, which was never fully resolved. How can we continue the discussion? Are Niko's ideas what we want to stabilize?