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

forbid conditional, negative impls #79098

Open
nikomatsakis opened this issue Nov 16, 2020 · 8 comments
Open

forbid conditional, negative impls #79098

nikomatsakis opened this issue Nov 16, 2020 · 8 comments
Assignees
Labels
F-negative_impls #![feature(negative_impls)] T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 16, 2020

We currently permit negative impls (feature-gated, #68318) for both auto- and regular traits. However, the semantics of negative impls are somewhat unresolved. The current trait checker's implementation in particular does not work well with "conditional" negative impls -- in other words, negative impls that do not apply to all instances of a type. Further, the semantics of such impls when combined with auto traits are not fully agreed upon.

Consider:

auto trait Foo { }
struct Bar<T> { }
impl<T: Copy> !Foo for Bar<T> { }

The question is, does Bar<Box<T>> implement Foo? There is some disagreement about what would be expected here.

As a temporary step, the plan is to forbid impls of this kind using similar logic to what we use for Drop impls. There was a PR in this direction #74648.

Another similar step would be to forbid negative impls for traits that have multiple generic parameters:

trait Foo<A> { }
impl<A, B> !Foo<A> for B { } // error

There is no particular reason that we can't support multiple parameters, but I suspect that the drop logic is not designed to handle cases like this.

Related issues:

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. F-negative_impls #![feature(negative_impls)] labels Nov 16, 2020
@codehag

This comment has been minimized.

@nikomatsakis
Copy link
Contributor Author

Example of us enforcing a similar rule for drop check:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fd4485ceb5a5145a1c77e336d778aecb

Note that the Drop impl is allowed with T: Debug, because that bound appears on the struct.

@nikomatsakis
Copy link
Contributor Author

Relevant code in drop-check is here:

pub fn check_drop_impl(tcx: TyCtxt<'_>, drop_impl_did: DefId) -> Result<(), ErrorReported> {
let dtor_self_type = tcx.type_of(drop_impl_did);

@nikomatsakis
Copy link
Contributor Author

@spastorino do you plan to look into this? (If so, I can assign it to you.)

As we said in our call, the obvious next step is to refactor the check_drop_impl code to be generic over the trait and not specific to Drop.

We will want to extend it, also, because I don't want you to be able to do things like

impl<A, B> !Send for (A, B) where A: Copy { }

In other words, we can kind of define what the "type parameters and predicates" are for other types like tuple types, &T types, etc, and ensure that the where-clauses/predicates used by the negative impl are limited to those. But the first step would just be to make it work for structs.

@nikomatsakis
Copy link
Contributor Author

This is blocking stabilization of negative impls, although we could opt to just stabilize negative impls for non-auto-traits.

@lcnr
Copy link
Contributor

lcnr commented Apr 1, 2022

i am currently looking into reworking dropck, cc #95309, but i also want to generalize the predicate part to get rid of SimpleEqRelation

while I am at it I could just write #74648 take 2

@pitaj
Copy link
Contributor

pitaj commented Feb 25, 2023

Negative conditional impls enables the following pattern as well. Figured I should put this here

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=752114f2762321fa148318468b7b4352

@dhardy
Copy link
Contributor

dhardy commented Jun 30, 2023

Consider:

auto trait Foo { }
struct Bar<T> { }
impl<T: Copy> !Foo for Bar<T> { }

The question is, does Bar<Box<T>> implement Foo? There is some disagreement about what would be expected here.

What's the disagreement? According to the RFC:

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.

  • "first check for explicit (positive) impls": none are found; however if Foo and Bar are public then a downstream crate could provide an impl for a local type Local
  • "Otherwise, we check for negative impls": Box<T> is not Copy so none are found, though again a downstream crate might provide one for Local
  • we proceed to check the component types: there are none, which presumably means that Foo is considered implemented by Bar<Box<T>>

Aside: does Bar<Local>: Foo? If Local: Copy we have an explicit !Foo impl; in this case any impl of Foo or !Foo for Bar<Local> in the downstream crate would be a conflict. (This is no different than positive impls.)


As I mentioned in #13231, auto trait as used for e.g. Send and Unpin is more complex than required for general opt-out marker traits, hence should probably not be used. (Would removal of the rules concerning component types and opt-in solve the apparent issue here?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-negative_impls #![feature(negative_impls)] T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants