Skip to content
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 negative impls #68318

Open
2 tasks
nikomatsakis opened this issue Jan 17, 2020 · 37 comments
Open
2 tasks

Tracking issue for negative impls #68318

nikomatsakis opened this issue Jan 17, 2020 · 37 comments
Labels
A-trait-system Area: Trait system B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-negative_impls #![feature(negative_impls)] I-lang-nominated Nominated for discussion during a lang team meeting. S-tracking-impl-incomplete Status: The implementation is incomplete. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 17, 2020

Generalized negative impls were introduced in #68004. They were split out from "opt-in builtin traits" (#13231).

Work in progress

This issue was added in advance of #68004 landed so that I could reference it from within the code. It will be closed if we opt not to go this direction. A writeup of the general feature is available in this hackmd, but it will need to be turned into a proper RFC before this can truly advance.

Current plans

Unresolved questions to be addressed through design process

  • What should the WF requirements be? (Context)
  • Should we permit combining default + negative impls like default impl !Trait for Type { }? (Context)
@nikomatsakis nikomatsakis added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 17, 2020
@jonas-schievink jonas-schievink added B-unstable Blocker: Implemented in the nightly compiler and unstable. A-trait-system Area: Trait system labels Mar 29, 2020
@kennytm kennytm added the F-negative_impls #![feature(negative_impls)] label Apr 30, 2020
@pirocks
Copy link

pirocks commented Nov 21, 2021

Odd possibly off topic question about this. The following compiles:

#![feature(negative_impls)]

pub struct Test{

}

impl !Drop for Test {}

fn foo(){
    drop(Test{})
}

Should it?

@Allen-Webb
Copy link

Odd possibly off topic question about this. The following compiles:

#![feature(negative_impls)]

pub struct Test{

}

impl !Drop for Test {}

fn foo(){
    drop(Test{})
}

Should it?

I would say no, because if you wanted to shift the error to runtime you could implement Drop as:

pub struct Test{

}

impl Drop for Test {
    fn drop() {
        panic!("Do not drop Test.")
    }
}

I generally like to catch anything at compile time that can be caught at compile time.

@gThorondorsen
Copy link

According to the documentation

This function is not magic; it is literally defined as

pub fn drop<T>(_x: T) { }

As a trait bound, T: Drop means that the type has defined a custom Drop::drop method, nothing more (see the warn-by-default drop_bounds lint). It does not mean that T has non-trivial drop glue (e.g. String: Drop does not hold). Conversely, T: !Drop only says that writing impl Drop for T { … } in the future is a breaking change, it does not imply anything about T's drop glue and definitely does not mean that values of type T cannot be dropped (plenty of people have tried to design such a feature for Rust with no success so far).

(If you did not already know, drop glue is the term for all the code that std::mem::drop(…) expands to, recursively gathering all Drop::drop methods of the type, its fields, the fields of those fields, and so on.)

Yes, the Drop trait is very counter-intuitive. I suggest extending drop_bounds to cover !Drop as well. Maybe even mentioning !Drop at all should be a hard error for now (neither impls nor bounds make sense IMO). Can @nikomatsakis or anyone else add this concern to the unresolved questions section in the description?

@rMazeiks
Copy link

Not sure if this is the right place to report a bug with the current nightly implementation, but a negative implementation and its converse seem to "cancel each other out".

For example:

trait A {}
trait B {}

// logically equivalent negative impls
impl<T: A> !B for T {}
impl<T: B> !A for T {}

// this should not be possible, but compiles:
impl A for () {}
impl B for () {}

The above compiles without errors on nightly, though it shouldn't. Removing one of the negative impls fixes the issue and results in an error as expected.

@JohnScience
Copy link
Contributor

JohnScience commented Dec 30, 2021

Allowing negative trait bounds would make my code much better by allowing to express di- and poly-chotomy.

In Rust 1.57.0 the following doesn't compile:

#![feature(negative_impls)]

trait IntSubset {}

impl <T> IntSubset for T where T: FixedSizeIntSubset + !ArbitrarySizeIntSubset {}
impl <T> IntSubset for T where T: !FixedSizeIntSubset + ArbitrarySizeIntSubset {}

trait FixedSizeIntSubset {}

impl<T: FixedSizeIntSubset> !ArbitrarySizeIntSubset for T {}

trait ArbitrarySizeIntSubset {}

impl<T: ArbitrarySizeIntSubset> !FixedSizeIntSubset for T {}

@mwerezak
Copy link

mwerezak commented Mar 13, 2022

Coming here from a compiler error, how do I "use marker types" to indicate a struct is not Send on stable Rust? I don't see any "PhantomUnsend" or similar anywhere in std.

error[E0658]: negative trait bounds are not yet fully implemented; use marker types for now

@rMazeiks
Copy link

@mwerezak I think I've seen PhantomData<*mut ()>. If I understand correctly, *mut () is not Send, so neither is the enclosing PhantomData nor your struct. (Did not test)

PhantomUnsend might be a good alias/newtype for better readability – I had no idea what PhantomData<*mut ()> meant at first :D

@mwerezak
Copy link

@rMazeiks Thanks, I wouldn't have known really what type would be the appropriate choice to use with PhantomData for this.

A PhantomUnsend sounds like a good idea.

@nikomatsakis
Copy link
Contributor Author

I think we should just rule out !Drop -- drop is a very special trait.

@dhardy
Copy link
Contributor

dhardy commented Apr 19, 2022

Can we add "negative super traits" to the unwritten RFC? That is,

pub trait Foo {}

pub trait Bar: !Foo {}

// We now know that the two traits are exclusive, thus can write:
trait X {}
impl<T: Foo> X for T {}
impl<T: Bar> X for T {}

Quote from the Hackmd:

This implies you cannot add a negative impl for types defined in upstream crates and so forth.

Negative super traits should get around this: the above snippet should work even if Foo is imported from another crate.

The potential issue here is that adding implementations to any trait exported by a library is technically a breaking change (though I believe it is already in some circumstances due to method resolution, and also is with any other aspect of negative trait bounds).

@nikomatsakis
Copy link
Contributor Author

I would prefer to leave that for future work, @dhardy -- I'd rather not open the door on negative where clauses just now, but also I think that it'd be interesting to discuss the best way to model mutually exclusive traits (e.g., maybe we want something that looks more like enums).

@JohnScience
Copy link
Contributor

JohnScience commented Nov 28, 2023

For those who's wondering what OIBITs are,

The acronym “OIBIT”, while quite fun to say, is quite the anachronism. It stand for “opt-in builtin trait”.

Source: https://internals.rust-lang.org/t/pre-rfc-renaming-oibits-and-changing-their-declaration-syntax/3086

Also see https://github.com/rust-lang/rfcs/blob/master/text/0019-opt-in-builtin-traits.md.

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting. We had a consensus in favor of stabilizing enough of negative impls here.

The subset we'd be in favor of stabilizing would be to require that the negative impl be "always applicable". Effectively, the impls can't have any bounds other than those required for an instance of the type. So, if struct Foo<T> requires T: Debug then you can impl<T> !Send for Foo<T> where T: Debug, but if struct Foo<T> doesn't require T: Debug then you can only impl<T> !Send for Foo<T> with no bounds on T.

One further consideration: a negative impl impl !Send for Foo isn't just "this is not Send", it's a promise that it won't become Send in the future. If a user wants "this is not Send but I'm making no promises that it won't be in the future", we would need something like impl ?Send for Foo. The lang team was generally in favor of adding this form as well.

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Nov 29, 2023

To add to the @joshtriplett's comment:

  1. The "always applicable" rule is the same as we currently have for Drop
  2. The rule that is easy to remember with negative impls is: it's always a breaking change to remove an implementation, both positive and negative ones
    • This rule would be somewhat more complex with "questionable implementations" if we end up adding them, since then it would be okay to remove those

I've also volunteered to work on stabilization of this feature and related work here.

@ChayimFriedman2
Copy link
Contributor

Are you proposing to also stabilize negative bounds (as a semver promise) for non-auto-traits? If not, how can removing a negative impl of an auto trait be a breaking change with any auto trait we have?

@scottmcm
Copy link
Member

scottmcm commented Nov 29, 2023

More on "questionable" impls:

  • We've wanted these in the past for other reasons, and indeed have #[rustc_reservation_impl] (https://github.com/rust-lang/rust/pull/62661/files#diff-3f4f29f2f5d7a54d28195beb05a24a0736a64ffe35eb30d8e7b2227c6a2fd615R564) to make this possible in certain cases.

  • I would love to have them in rustdocs for various things. If we can't yet agree on f32: !Ord, having a reserved impl Ord for f32; as a place to put "Look, it's intentional that this doesn't exist and here's what you should do instead".

  • Similarly, that could replace a bunch of the on_implemented use cases. reserved impl FromResidual<Option<_>> for Result<_, _>; would be nicer than doing that with text, for example, and could hopefully let it use the real trait solver instead of a side system with its own quirks.

  • They would also be great for new standard library traits, since they could start by always reserving impls for all our fundamental types. That would avoid problems like how we couldn't impl Extend for &mut impl Extend because of reference being fundamental, even though that would be useful to have.

  • With the tweaked coherence rules, IIRC you might sometimes want in a library to reserve some things (particularly blanket impls) to be able to have the option of adding them later, since otherwise downstream crates might add conflicting impls on their own types.

@WaffleLapkin
Copy link
Member

@ChayimFriedman2 no, negative bounds are a separate feature and there is no suggestions to stabilize that.

The reason removing a negative impl is a breaking change is that typechecker is allowed to assume that it is. It currently doesn't use this AFAIK, but in the feature we want to allow code like this:

struct NeverSend;
impl !Send for NeverSend {}

trait X {}

impl<T: Send> X for T {}
impl X for NeverSend {} // typechecked is allowed to assume those impl don't overlap

@spastorino
Copy link
Member

@WaffleLapkin we were working on this with @lcnr and @nikomatsakis and at some point but the work was deferred. I'm happy if you can move this forward and I'd be interested in following along. I think I even have a couple of PRs about this half-baked that I'm not sure if are relevant anymore :). Feel free to reach out if I can help with things.

There's also the RFC draft that we were working in meetings with Niko https://hackmd.io/ZmpF0ITPRWKx6jYxgCWS7g

@joshtriplett
Copy link
Member

@spastorino How easily could we add impl ?Send for SomeType?

@lcnr
Copy link
Contributor

lcnr commented Nov 30, 2023

from a t-types pov it's pretty straightforward to implement with the following design constraints

  • only allowed for auto traits
  • required to be fully applicable, same as Drop

if t-lang decides that we should have that feature, it could be stabilized in 1-2 releases (assuming someone has the capacity to work)

@spastorino
Copy link
Member

spastorino commented Nov 30, 2023

@lcnr 👍, my understanding was also that this is "easy" to do. I'd be up for working on this impl ?Send for SomeType if t-lang decides to have the feature.

@Jules-Bertholet
Copy link
Contributor

  • Should we permit combining default + negative impls like default impl !Trait for Type { }? (Context)

One possibility would be to allow such impls to be specialized by a more specific positive impl defined in the same crate. This would provide a way of expressing that a positive impl is guaranteed not to be extended or generalized in the future.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this, as mentioned above, and the team was in favor of doing it. The next step would be someone putting together a concrete proposal, probably in the form of a stabilization PR with a suitably detailed stabilization report.

Until then, there's probably not much more to discuss, so we'll remove the nomination. As soon as the stabilization report / PR is posted, please nominate for T-lang.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 6, 2023
Encephala added a commit to Encephala/rusty-db that referenced this issue Feb 29, 2024
Not sure if I like this syntax, but okay.
It'd be nice if there was a way to have a negative trait bound, i.e. "P
is a Parser but not a Combinator", but that's part of Rust unstable so
far, rust-lang/rust#68318.
@clarfonthey
Copy link
Contributor

clarfonthey commented Jan 28, 2025

Since I found myself running into this, I wanted to poke around and see what's left of this feature. A few things that I wanted to clarify about the current status quo:

  1. Is there a genuine difference between impl !Trait and impl ?Trait besides just API guarantees? Essentially wondering whether more would need to be done than syntax— I'm assuming so, but it's not exactly clear.

    Yes, it affects coherence per Tracking issue for negative impls #68318 (comment).

  2. Does the interaction with specialization have to be left as an unresolved issue here, or can it be moved into the tracking issue(s) for specialization instead? It feels silly to block stabilising this on how it interacts with an unstable feature, unless there's serious concern that stabilising it now could break that in the future. (I don't think it would, but worth checking.)

    Discussed here: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/negative.20impls.3A.20specialization/near/496281282

  3. The implication here is that the lang team is okay with just a stabilisation report and an FCP for this, but since there was also a linked draft RFC, it's worth checking whether the request is to make a stabilisation report or an actual RFC for this.

    Currently waiting on T-lang decision.

  4. Is there any consensus on how negative impls should interact with unsafe traits? For example, if you have unsafe trait Trait, would you have to do unsafe impl !Trait or just impl !Trait? My assumption is that the negative impl would not have to be unsafe, but this is something that wasn't mentioned at all from what I read.

    Discussed here: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/negative.20impls.3A.20unsafe.20traits/near/496281455

  5. When it comes to stabilising this feature, should conditional negative impls be put under a separate feature gate, or just removed entirely for now? Not sure what the consensus is here.Since it's been over a year since folks have posted here, I'm assuming that nobody is currently busy with this, and I would like to help at least work on cleaning up the details here, but I also am fine not doing so if someone else is still working on it.

    Currently waiting on T-lang decision.


Some additional questions to answer, added after the fact:

  1. Is impl ?Trait considered a hard requirement of stabilisation, or is it something that's simply desired as a future extension?

    Currently waiting on T-lang decision.

  2. Is !Drop a valid impl? How does this interact with Destruct?

    Discussed here: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/negative.20impls.3A.20.60!Drop.60

@traviscross
Copy link
Contributor

traviscross commented Jan 28, 2025

  1. Is there a genuine difference between impl !Trait and impl ?Trait besides just API guarantees? Essentially wondering whether more would need to be done than syntax— I'm assuming so, but it's not exactly clear.

Yes. impl !Unpin can be relied upon in coherence checking, but ?Unpin cannot.

For the rest of these good questions, we should probably discuss:

@rustbot labels +I-lang-nominated

Also:

cc @rust-lang/types

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 28, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Jan 28, 2025

2. Does the interaction with specialization have to be left as an unresolved issue here, or can it be moved into the tracking issue(s) for specialization instead? It feels silly to block stabilising this on how it interacts with an unstable feature, unless there's serious concern that stabilising it now could break that in the future.

async discussion thread: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/negative.20impls.3A.20specialization/near/496281282

4. Is there any consensus on how negative impls should interact with unsafe traits? For example, if you have unsafe trait Trait, would you have to do unsafe impl !Trait or just impl !Trait? My assumption is that the negative impl would not have to be unsafe, but this is something that wasn't mentioned at all from what I read.

async discussion thread: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/negative.20impls.3A.20unsafe.20traits/near/496281455

@dhardy
Copy link
Contributor

dhardy commented Jan 28, 2025

I think we should just rule out !Drop -- drop is a very special trait.

I found a use case for this while working on async-local; for types that don't impl Drop, the thread_local macro won't register destructor functions, and the lifetime of these types can be extended by using Condvar making it possible to hold references to thread locals owned by runtime worker threads in an async context or on any runtime managed thread so long as worker threads rendezvous while destroying thread local data. For types that do impl Drop, they will immediately deallocate regardless of whether the owning thread blocks and so synchronized shutdowns cannot mitigate the possibility of pointers being invalidated, making the safety of this dependent on types not implementing Drop.

@Bajix This sounds like a hack around lifetimes. Is there any guarantee that accessing a !Drop value held by a thread is valid simply because the thread hasn't exited yet?

I'm asking because, once specialization is available, it may make sense to let every type implement Drop. Semantically this is simpler and theoretically it should be no different (note that Rust does not guarantee that drop will be called on thread exit).

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Jan 28, 2025

Note that “does not implement Drop” does not mean “has a trivial destructor”, if the type has a Drop-implementing field.

It may make sense to let every type implement Drop.

I agree, but I don't think specialization is necessarily required. I propose this at rust-lang/rfcs#3762 (comment)

@oli-obk
Copy link
Contributor

oli-obk commented Jan 28, 2025

Please start a thread for !Drop, it seems to require some discussion and tracking issues are not very well suited for that

@clarfonthey
Copy link
Contributor

clarfonthey commented Jan 29, 2025

Please start a thread for !Drop, it seems to require some discussion and tracking issues are not very well suited for that

https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/negative.20impls.3A.20.60!Drop.60/near/496452885

Also, FWIW, I'm treating my earlier comment as a list of current questions, so, any team members are free to edit it to update the list if necessary: #68318 (comment)

Or just move it to the issue description!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-negative_impls #![feature(negative_impls)] I-lang-nominated Nominated for discussion during a lang team meeting. S-tracking-impl-incomplete Status: The implementation is incomplete. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests