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

Tracking issue for allowing overlapping implementations for marker trait #29864

Open
nikomatsakis opened this Issue Nov 16, 2015 · 22 comments

Comments

Projects
None yet
10 participants
@nikomatsakis
Contributor

nikomatsakis commented Nov 16, 2015

Tracking issue for rust-lang/rfcs#1268.

Status

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

Known bugs

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?
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 18, 2015

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 added a commit to diesel-rs/diesel that referenced this issue Nov 25, 2015

Add the ability to work with boxed expressions
There's a few limitations that I'm not happy with at the moment:

- The type of the expression has to be repeated twice
  (https://github.com/rust-lang/rust/isues/30056)
- It won't work with joins yet. I might be able to work around this in
  the short term, but it may require
  rust-lang/rust#29864 or specialization to
  express properly

sgrif added a commit to diesel-rs/diesel that referenced this issue Nov 26, 2015

Add basic support for additional expressions in from clause
This is useful for things like full text search, where you need to do an
expensive calculation to a parameter going into the where clause, but
doing it inline would repeat the calculation for each loop.

I had to expose a lot of internals here, because this is yet another
case where we can't properly express what we want without either
specialization or rust-lang/rust#29864. I
could not find a way to properly enforce the selectability of `Aliased`
when `with` was called more than once (this is ultimately the same
problem as joining multiple times). In the interim, we'll allow the
alias to be used anywhere.

@sgrif sgrif referenced this issue Nov 28, 2015

Closed

TODO #1

28 of 57 tasks complete
@sgrif

This comment has been minimized.

Contributor

sgrif commented Nov 15, 2016

@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

This comment has been minimized.

Contributor

nikomatsakis commented Nov 15, 2016

@sgrif swapped labels

sgrif added a commit to diesel-rs/diesel that referenced this issue Dec 8, 2016

Add a function for SQL `EXISTS` expressions.
While this is useful for some cases where you don't want to load the
rows, this won't fill every use case for the expression, as right now
you wouldn't be able to build a query that references the outer table.
For us to do that and have it be type safe we'd need overlapping impls
for `SelectableExpression` (story of my life), which requires
rust-lang/rust#29864 being implemented.

Fixes #414.

sgrif added a commit to diesel-rs/diesel that referenced this issue Dec 8, 2016

Add a function for SQL `EXISTS` expressions.
While this is useful for some cases where you don't want to load the
rows, this won't fill every use case for the expression, as right now
you wouldn't be able to build a query that references the outer table.
For us to do that and have it be type safe we'd need overlapping impls
for `SelectableExpression` (story of my life), which requires
rust-lang/rust#29864 being implemented.

Fixes #414.

sgrif added a commit to sgrif/rust that referenced this issue Feb 25, 2017

Implement RFC 1268
This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864

sgrif added a commit to sgrif/rust that referenced this issue Feb 25, 2017

Implement RFC 1268
This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864

sgrif added a commit to sgrif/rust that referenced this issue Feb 25, 2017

Implement RFC 1268
This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864

sgrif added a commit to sgrif/rust that referenced this issue Mar 17, 2017

Implement RFC 1268
This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864

sgrif added a commit to sgrif/rust that referenced this issue Mar 19, 2017

Implement RFC 1268
This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864

nikomatsakis added a commit to sgrif/rust that referenced this issue Apr 3, 2017

Implement RFC 1268
This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864

bors added a commit that referenced this issue Apr 3, 2017

Auto merge of #40097 - sgrif:sg-implement-rfc-1268, r=nikomatsakis
Implement RFC 1268

This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref #29864

bors added a commit that referenced this issue Apr 4, 2017

Auto merge of #40097 - sgrif:sg-implement-rfc-1268, r=nikomatsakis
Implement RFC 1268

This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref #29864

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 4, 2017

Rollup merge of rust-lang#40097 - sgrif:sg-implement-rfc-1268, r=niko…
…matsakis

Implement RFC 1268

This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 15, 2017

Implement RFC 1268
This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

Ref rust-lang#29864
@frewsxcv

This comment has been minimized.

Member

frewsxcv commented Apr 15, 2017

Initial implementation just merged in #41309.

@sgrif

This comment has been minimized.

Contributor

sgrif commented Apr 20, 2017

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

This comment has been minimized.

Contributor

nikomatsakis commented Apr 21, 2017

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

@cramertj

This comment has been minimized.

Member

cramertj commented Nov 6, 2017

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

This comment has been minimized.

Contributor

Ixrec commented Nov 8, 2017

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

This comment has been minimized.

Member

cramertj commented Nov 8, 2017

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

@Vurich

This comment has been minimized.

Vurich commented Jan 11, 2018

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

This comment has been minimized.

Contributor

nikomatsakis commented Jan 11, 2018

@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

This comment has been minimized.

Contributor

nikomatsakis commented Jan 11, 2018

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

@cramertj

This comment has been minimized.

Member

cramertj commented Jan 11, 2018

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

@mtak-

This comment has been minimized.

mtak- commented Feb 21, 2018

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-

This comment has been minimized.

mtak- commented Feb 21, 2018

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

This comment has been minimized.

Contributor

nikomatsakis commented Feb 21, 2018

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., #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

This comment has been minimized.

Vurich commented Feb 26, 2018

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

This comment has been minimized.

Member

scottmcm commented Feb 26, 2018

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

This comment has been minimized.

Vurich commented Feb 26, 2018

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

This comment has been minimized.

Contributor

nikomatsakis commented Feb 27, 2018

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

@cramertj

This comment has been minimized.

Member

cramertj commented Jul 31, 2018

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

This comment has been minimized.

Member

scottmcm commented Aug 26, 2018

@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: #53693

bors added a commit that referenced this issue Sep 25, 2018

Auto merge of #53693 - scottmcm:marker-trait-attribute, r=nikomatsakis
Support an explicit annotation for marker traits

From the tracking issue for rust-lang/rfcs#1268:
> It seems obvious that we should make a `#[marker]` annotation. ~ #29864 (comment)

This PR allows you to put `#[marker]` on a trait, at which point:
- [x] The trait must not have any items ~~All of the trait's items must have defaults~~
- [x] Any impl of the trait must be empty (not override any items)
- [x] But impls of the trait are allowed to overlap

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment