search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.03k stars 12.68k forks source link

Tracking Issue for auto traits (auto_traits) -- formerly called opt-in built-in traits (optin_builtin_traits) #13231

Open flaper87 opened 10 years ago

flaper87 commented 10 years ago

This is the tracking issue for RFC 19.

Checklist

Here is a check-list of code to write and tricky scenarios to be sure we handle:

pnkfelix commented 10 years ago

Marking 1.0, P-backcompat-lang.

nikomatsakis commented 10 years ago

cc me

flaper87 commented 10 years ago

I'm actively working on this. Unfortunately, I've moved quite slowly because of other bugs I had to fix. I hope to be able to submit a PR this week.

pcwalton commented 10 years ago

Per https://github.com/rust-lang/rfcs/pull/127

aturon commented 9 years ago

Nominating for removal from milestone; I believe the backcompat issues have been handled.

pnkfelix commented 9 years ago

removing from 1.0 milestone.

alexcrichton commented 8 years ago

I believe this has basically all been implemented to the fullest extent it will for now, so closing.

steveklabnik commented 8 years ago

For posterity's sake, what hasn't been?

One question I have about a lot of these older RFCs is how much I can actually rely on their text for documenting semantics. For really old stuff like this, I worry that the RFCs might not be useful for people. This might just be the reality, though.

On Feb 18, 2016, 13:51 -0500, Alex Crichtonnotifications@github.com, wrote:

I believe this has basically all been implemented to the fullest extent it will for now, so closing.

— Reply to this email directly orview it on GitHub(https://github.com/rust-lang/rust/issues/13231#issuecomment-185857012).

aturon commented 8 years ago

There's definitely more work to do here (see the checklist -- there may be more), and this should ultimately become the tracker for stabilization.

cc @nikomatsakis

nikomatsakis commented 8 years ago

In this particular case, the RFC is definitely out of sync with the current implementation. One of my pending worklist items has been to write an update to the text.

@steveklabnik I also think that, as part of the stabilization process, we should go back and update the RFCs to bring them inline with the final results.

On Thu, Feb 18, 2016 at 2:42 PM, Aaron Turon notifications@github.com wrote:

There's definitely more work to do here (see the checklist -- there may be more), and this should ultimately become the tracker for stabilization.

cc @nikomatsakis https://github.com/nikomatsakis

— Reply to this email directly or view it on GitHub https://github.com/rust-lang/rust/issues/13231#issuecomment-185885630.

dylanede commented 8 years ago

I'm writing a library that depends on OIBIT. In particular, it depends on the following coding pattern working:

trait NotSame {}
impl NotSame for .. {}
impl<T> !NotSame for (T, T) {}

trait Foo<A> {}

struct Bar<B>(B);

impl<A, B> Foo<A> for Bar<B> where (A, B): NotSame {
    // stuff
}

impl<A> Foo<A> for Bar<A> {
    // other stuff
}

This works on nightly at the moment. Is this relying on any behaviour that is disallowed by the intended functionality? I.e. will this break by the time this is stabilised? Thanks.

brson commented 8 years ago

RFC needs to be ammended, not sure for what.

nikomatsakis commented 8 years ago

Roughly speaking I wanted to formalize the following things:

The first two are mostly reflecting the current semantics, though.

nikomatsakis commented 8 years ago

@dylanede hmm, sorry I failed to answer your question before. I'm sorry to say this, but your code snippet is not expected to work.

In particular, the way to think about it is that, by adding a negative impl for (T, T), you have suppressed the default impl that would have been provided -- so now there is no positive impl! Therefore, (X, Y): NotSame cannot hold for any tuple. (But, (X, Y): !NotSame, if we ever added negative impls, would only hold if X == Y; otherwise, (X, Y) neither implements NotSame nor !NotSame, a situation which @withoutboats artfully described as (X, Y): ?NotSame.)

Note that the behavior for your example is inconsistent already. For example, while your code compiles, this similar snippet does not.

I had to dig a bit into the code to see why your example worked at all: the answer is that the "first pass" test which tries to unify with various impls fails to unify for types like u32 and i32 -- it will basically succeed "modulo regions". So then later when we check to see if there were any negative impls that applied, we find out that there were not (in the case of u32, i32) but that there are (in the case of &'static u32, &'a u32). If we were properly implementing the RFC the way that I had intended to amend it, we would always find applicable negative impls (in fact, until such time as we add general negative reasoning, I had intended to make the negative impl you wrote illegal -- you would have to write negative impls that are "fully general", much like the rules we enforce for Drop.)

chriskrycho commented 7 years ago

This is… beyond me. Someday maybe. 😬

But I'm trying to document all the things, so: can someone take a gander at the reference and the book and give me a reasonable summary of the extent to which this is or isn't documented?

Zoxc commented 7 years ago

I just wanted to note that if we add immovable types in it's current form, default traits would have to be declared with trait Send: ?Move {} to avoid having Move as a supertrait (which has soundness bugs).

bstrie commented 6 years ago

Resuming the bikeshed which I had previously missed, not quite sold on the new auto trait syntax as of https://github.com/rust-lang/rust/pull/45247 (though nevertheless an improvement over the old). Not only is it a new keyword (a heavyweight addition) for a feature that is intended to be incredibly obscure, but it risks confusion for people coming from C++.

In https://github.com/rust-lang/rust/pull/45247 an alternative of #[autoderive] was floated and seemed to have quite some support. I think it makes sense as a general policy for us to prefer attributes/macros for obscure features, and later offer keywords/operators if anyone demands (which was the trajectory for ?, and the possible trajectory for async/await, and unlikely to ever occur for autotraits IMO).

clarfonthey commented 6 years ago

Random thing I noticed is that OIBIT (hackily) allows checking for type equality (and hence type inequality):

// sealed for good measure
mod private {
    pub auto trait NotSame {}
    impl<T> !NotSame for (T, T) {}
}
use NotSame;

trait ReqDifferent<T> where (T, Self): NotSame {}
trait ReqSame<T> where (T, Self): !NotSame {}

I assume that this is not desired.

dylanede commented 6 years ago

No, it's not. This behaviour is a bug, See the responses to https://github.com/rust-lang/rust/issues/13231#issuecomment-187302270

clarfonthey commented 6 years ago

Wow, I completely missed that comment. Appears that this has already been discussed

mitsuhiko commented 6 years ago

I ran into this issue when trying to implement !Sync for a type of mine. I know you can work around this with PhantomData well but it makes for ugly error messages. I wish I could tell my users that my type is not Sync and not that *mut () or something similar contained in my type is not sync.

arielb1 commented 5 years ago

Added #56934

Robbepop commented 5 years ago

With the current implementation (that might be buggy) one can solve some simple specialization cases with this feature which is nice: https://gist.github.com/rust-play/4ea94f766d49abdbc9b2c55e49a9d2d9

WaffleLapkin commented 4 years ago

@clarfon note that type equality (but sadly not inequality) can be reached with simple traits on stable:

trait Id {
    type This: ?Sized;
}

impl<T: ?Sized> Id for T {
    type This = T;
}

fn assert_same<A, B>() where A: Id<This = B> {}

(playground)

jdahlstrom commented 4 years ago

The following code (playground link) is rejected by the compiler because Something is not implemented for the type dyn std::error::Error + std::marker::Send + std::marker::Sync + 'static. Providing an explicit impl makes the code compile. Is this a bug or intended behavior?

#![feature(optin_builtin_traits)]

pub auto trait Something {}

// Uncomment this to make the code compile
//
//impl Something for dyn std::error::Error + std::marker::Send + std::marker::Sync + 'static {}

pub fn foo<T: Something>(_: T) {}

#[test]
fn test() {
    use std::io::*;
    foo(Error::new(ErrorKind::Other, ""));
}
dtolnay commented 4 years ago

That is the correct behavior @jdahlstrom. std::io::Error does not get a Something impl because it holds something that does not necessarily implement Something. In general you would only get the auto trait impl if all the fields are known to implement that auto trait.

For example:

struct S;
impl std::error::Error for S {}
impl std::fmt::Display for S {fn fmt(&self, _f: &mut std::fmt::Formatter) -> std::fmt::Result {Ok(())}}
impl !Something for S {}
foo(Error::new(ErrorKind::Other, S));  // io::Error containing an S.
jdahlstrom commented 4 years ago

@dtolnay Ah, of course. Thanks.

camelid commented 3 years ago

I updated the name of this issue to be consistent with the new feature name (see #79336).

sourcefrog commented 3 years ago

The RFC doc talks about the Share trait, but I think that is now renamed to Sync? (Or is it / was it different?) Could I suggest the RFC text be updated, at least with a note if not a search-and-replace?

camelid commented 3 years ago

@sourcefrog Usually RFCs aren't updated since there are so many and it would be too much work to keep them up-to-date. But you can open a PR on rust-lang/rfcs and see if indeed it was renamed and they think it's a good idea to update the RFC.

dtolnay commented 1 year ago

107082 proposes an adjustment to the coherence rules involving autotraits.

dtolnay commented 1 year ago

I am adding a note to the checklist that the interaction between auto traits and str needs to be revisited as part of any eventual stabilization.

Currently with the following code:

auto trait AutoTrait {}

impl<T> !AutoTrait for [T] {}

we get str implements AutoTrait but [u8] does not.

Under a different libcore-based str library implementation something like https://github.com/rust-lang/rust/pull/70911, we'd get that neither implements AutoTrait.

If we want to ever be able to push something like https://github.com/rust-lang/rust/pull/70911 through in the future, there may need to be str-specific special cases in the autotrait feature for now.

dhardy commented 1 year ago

This RFC is a little dated and concerns multiple topics ("auto trait", other automatically-implemented marker traits, unsafe traits). Lets summarise the RFC:

Motivation

To paraphrase (with comments):

  1. Send and Share (is this the old Sync?) should not be opt-in. This issue is solved.
  2. Move Send and Share out of the language into the standard library.
  3. Allow user-defined classification traits. Suggested uses are: "tainted", Snapshot, NoManaged, NoDrop (now !Drop).

Detailed design

Default and negative impls

We add a notion of a default impl, written:

impl Trait for .. { }

(This is now just auto trait Trait {} IIUC.)

Negative impls are only permitted if Trait has a default impl.

This is reliant on having a specified "default impl", as above (not just impl<T> Trait for T {} and not the type of default impl used for specialization). If an auto-trait is defined as exactly "a trait which is implemented by default, with opt-out", then auto trait Trait {} suffices.

Intuitively, to check whether a trait Foo that contains a default impl is implemented for some type T, we first check for explicit (positive) impls that apply to T. If any are found, then T implements Foo. Otherwise, we check for negative impls. If any are found, then T does not implement Foo. If neither positive nor negative impls were found, we proceed to check the component types of T (i.e., the types of a struct's fields) to determine whether all of them implement Foo. If so, then Foo is considered implemented by T.

Aha, the (informal) definition of an auto trait.

Modeling Send and Share using default traits

The RFC details how to define Send and Share as auto traits (the current Sync is slightly different, but still an auto trait).

(The private trait Freeze is another auto trait with some explicit opt-in and opt-out impls.)

(auto trait Unpin is another public auto trait with some explicit opt-ins. As a work-around for not having stable negative impls, Unpin has a negative impl for public type PhantomPinned.)

The Copy and Sized traits

Copy is just an ordinary (opt-in) marker trait.

Sized looks like an ordinary trait but the compiler provides all impls (no opt-in or opt-out possible). (The same is true for trait Unsize<T: ?Sized>.)

My thoughts follow.

"auto trait" in conjunction with negative impls are a niche solution to moving a few trait definitions from the language to the standard library:

"auto trait" does not allow all "automatically defined traits" to be offloaded from the compiler: e.g. Sized, Unsize<T>, Drop.

The RFC mentions a few other uses of auto traits:

The only other possible usage I can think of:

Hence it remains unclear whether there is sufficient utility for "auto traits".

The other angle for auto-traits is as an opt-out marker trait (i.e. negative impls #68318). These do not require any rules pertaining to component types, thus, for simplicity, should not use "auto traits" as defined by RFC 19.

My hesitant recommendation

Do not make "auto traits" a public feature and close this issue. They have recognised utility within the standard library but likely very little utility otherwise. (Perhaps in the future a more general form of blanket impls able to emulate the logic of auto traits will be possible. In the mean-time, I simply don't think auto traits have sufficient utility to become a fully fledged language feature.)

Possibly rename the existing "auto traits" to allow the term to be used for a simple "opt-out" trait for use with negative implementations (but this would be the topic of negative impls or a whole new RFC).

madsmtm commented 1 year ago

To me, it mostly has value for checking whether the types involved in a closure satisfy a specific property. A perfect example of this, outside the usual Send/Sync, is UnwindSafe (however badly named that is).

Concretely, I'm using it (behind a feature flag) to make autorelease pools in Objective-C sound, just as a data point for how it's useful.

dhardy commented 1 year ago

Thanks for the examples, adding motivation for this feature.

The aspects of this feature I'm least comfortable with are:

scottmcm commented 1 year ago

My best recollection of current lang consensus is that S-tracking-perma-unstable is correct here. This is essential for the standard library, but the ecosystem effects of "you need to know to opt out of dozens of random auto traits created by various crates" seems unworkable for a crate ecosystem, so without some solution to that, these likely will remain a standard library exclusive privilege.

fogti commented 1 year ago

how about coupling the auto traits so that they only get auto-used when the crate defining them is a direct dependency of the current crate (so no automatic spill of auto traits happens)? (making it crate-level opt-in, trait-level opt-out instead of pure opt-out)

GoldsteinE commented 1 year ago

I think pyo3 usecase wasn’t mentioned here, so I’ll bring it as a data point.

pyo3 needs to track which types can only be accessed when Python’s GIL is held. Currently it uses Send as a proxy for this property, but it’s neither neccessary nor sufficient requirement and only works as a rough heuristic. It seems like there’s no good way to do it properly without auto traits (+ negative impls). Similar usecase was mentioned above with ObjC, so it seems like this feature is neccessary for sound FFI at least in some cases.

I think that this is an evidence that auto traits currently have usecases outside of standard library. Maybe these usecases could be covered by another feature, maybe they’re not enough to sway the decision, but I think it should be recognized that auto traits have utility outside of standard library, and making them perma-unstable will probably make some usecases perma-unsound, unless some other solution is found. I feel like if this issue is to be closed, this tradeoff (“we’re OK with these cases having no known sound solution”) should be explicitly made in the final decision.

https://docs.rs/pyo3/latest/pyo3/marker/trait.Ungil.html — trait in pyo3 that should be auto https://github.com/PyO3/pyo3/issues/2141 — issue in the pyo3 repo

GoldsteinE commented 1 year ago

I’d also like to address this point specifically wrt Ungil:

you need to know to opt out of dozens of random auto traits created by various crates

Opting out of Ungil would only be sensible when you’re doing something with Python and are already using pyo3. In my intuition, auto traits are used to represent some properties related to a certain system (e.g. Send and Sync represent properties related to threads). You opt out of them when your type has some special properties in this system (e.g. is safe to access from multiple threads). Opting out of an auto trait is thus only sensible if you’re actually using this system, so you can promise to uphold some invariants. Opting out of auto traits related to systems you’re not actively integrating with feels like a wrong thing to do in almost any situation.

madsmtm commented 1 year ago

Opting out of auto traits related to systems you’re not actively integrating with feels like a wrong thing to do in almost any situation.

In my case too, you'd only need to opt out of AutoreleaseSafe when you're doing stuff with Objective-C (or perhaps Swift, but they're close enough) autorelease pools.

I'll add that if a library wanted to make sure to opt out of all auto traits (e.g. if it implemented some sort of dynamic type), even for generic structs, they could do:

trait HelperTrait {}

struct MyTypeThatOptsOutOfEverything<T> {
    inner: T,
    p: PhantomData<dyn HelperTrait>,
}

Of course this is yet another SemVer footgun to be aware of, but fortunately we have tools for that now.

GoldsteinE commented 1 year ago

Oops, I mixed up “opting out” and “opting in” in my original message. Either way, the point stands: I don’t see a reason to opt out of arbitrary auto traits unless you’re doing something that can affect this particular system. Author of the auto trait guarantees that it’s safe to impl it if all the fields impl it.

steffahn commented 10 months ago

For Ungil, I’ve found the issue

which is that an auto-trait is unable to restrict access to certain types even if those types always feature a non-'static lifetime as provided by to a higher-kinded for<'a> FnOnce(SomeType<'a>)-style callback.

I’m relatively certain that the AutoreleaseSafe example suffers from the same issue.

Of course one approach would be to declare scoped-tls unsound on the (arguably quite thin) grounds that the standard library only sets precedent that 'static data is allowed to be shared via thread-local storage. Assuming we don’t declare scoped-tls unsound, an auto traits feature seems entirely unable to be a solution for sound API in either of the API cases for AutoreleaseSafe nor Ungil.

GoldsteinE commented 10 months ago

Oh, that’s really sad. If scoped-tls is valid (and I don’t see a reason for it not to be), then auto trait Ungil can only make pyo3 less-obviously-unsound instead of sound. IMHO that’s still a valid reason for it to be an auto trait, but it’s really unfortunate that it doesn’t just make it clean and sound.

madsmtm commented 10 months ago

Hmm, I don't think that's a problem for objc2's AutoreleaseSafe, since the only type it's used on is AutoreleasePool<'pool>, whose only purpose is to carry a lifetime (it's a ZST that doesn't actually carry any data), which it seems like scoped-tls-hkt manages to properly bind to the execution of the closure.

So even though we can access the pool, we can't actually do anything bad with it, since it's only the lifetime that we care about, and that is still correctly bounded. See the following example for details.

```cargo
[dependencies]
# requires `auto_traits` feature
objc2 = { version = "0.5", features = ["unstable-autoreleasesafe"] }
scoped-tls-hkt = "0.1"

use objc2::rc::{autoreleasepool, AutoreleasePool, Id}; use objc2::runtime::NSObject; use scoped_tls_hkt::scoped_thread_local;

fn main() { autoreleasepool(|pool1| { scoped_thread_local!(static POOL: for<'pool> AutoreleasePool<'pool>);

    POOL.set(pool1, || {
        let obj = autoreleasepool(|_pool2| {
            POOL.with(|pool1| {
                Id::autorelease(NSObject::new(), pool1)
            })
        });

        // If we manage to get here, then that's unsound, since the object
        // would have been released in the second autorelease pool.

        println!("{obj:?}");
    });
});

}



If you manage to produce a similar example that does compile, then I'd love to know!
steffahn commented 10 months ago

@madsmtm Here we go:

use std::cell::Cell;

use objc2::rc::{autoreleasepool, AutoreleasePool, Id};
use objc2::runtime::NSObject;
use scoped_tls_hkt::scoped_thread_local;

struct PoolHolder<'p> {
    pool1: AutoreleasePool<'p>,
    obj_cell: &'p Cell<Option<&'p NSObject>>,
}
trait IPoolHolder {
    fn autorelease(&self, id: Id<NSObject>);
}
impl<'pool> IPoolHolder for PoolHolder<'pool> {
    fn autorelease(&self, id: Id<NSObject>) {
        self.obj_cell.set(Some(Id::autorelease(id, self.pool1)));
    }
}

fn main() {
    autoreleasepool(|pool1| {
        scoped_thread_local!(static POOL_HOLDER: for<'p> &'p dyn IPoolHolder);

        let obj_cell = &Cell::new(None);

        POOL_HOLDER.set(&PoolHolder { pool1, obj_cell }, || {
            autoreleasepool(|_pool2| {
                POOL_HOLDER.with(|pool1_holder| pool1_holder.autorelease(NSObject::new()))
            });

            let obj: &NSObject = obj_cell.get().unwrap();

            // If we manage to get here, then that's unsound, since the object
            // would have been released in the second autorelease pool.

            println!("{obj:?}");
        });
    });
}