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

prevent specialized negative impls #74648

Closed
wants to merge 3 commits into from
Closed

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jul 22, 2020

closes #74629

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2020
@lcnr lcnr changed the title prevent specialization of auto traits prevent specialized negative impls Jul 22, 2020
@WaffleLapkin
Copy link
Member

Does this completely forbid "ad-hoc" specialization that was possible before?

#![feature(negative_impls)]
#![feature(optin_builtin_traits)]
pub trait Trait {}
pub struct Special;

impl<T> Trait for T where private::Is<T>: private::NotSpecial {}
impl Trait for Special {}

mod private {
    pub auto trait NotSpecial {}
    pub struct Is<T>(T);
    impl !NotSpecial for Is<super::Special> {}   
}

(playground)
It seemed to work just fine in simple examples like this...

@lcnr
Copy link
Contributor Author

lcnr commented Jul 22, 2020

It does, see the discussion in #74525 (comment) for more details. There are subtle problems rn where negative impls don't work as expected.

If you really need this kind of specialization, I would recommend you to use the approach taken in #74254 which shouldn't have the problems caused by partial negative impls. That one does depend on [feature(marker_trait_attr, min_specialization)] though. It will probably take a while before specialization is possible on stable.

@WaffleLapkin
Copy link
Member

@lcnr sadly, my trait actually has an associated type so current implementation of specialization won't work for me :(

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks about right, I didn't read it 100% yet, but some of the errors seemed a bit surprising..?

src/test/ui/issues/issue-29516.stderr Outdated Show resolved Hide resolved
src/test/ui/issues/issue-41974.rs Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
error: auto traits must not contain where bounds
error: negative impls are not allowed for tuples
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we disallow negative impls for all tuples? (I think that'd be ok, actually, though I think we could also permit them for tuples if they are only restricted based on arity, but I don't see the point.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think allowing them for tuples will prevent us from ever adding variadic tuples, so it should be easier to just disallow them for now

@@ -6,6 +6,7 @@ trait MyTrait {
type Foo;
}

default impl !MyTrait for u32 {} //~ ERROR auto traits must not contain where bounds
default impl !MyTrait for u32 {}
//~^ ERROR negative impls on primitive types must not contain where bounds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the "where bound" here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like it's more about the default keyword, perhaps?


trait MyTrait {}

impl<T> !MyTrait for T {} //~ ERROR auto traits must not contain where bounds
impl<T> !MyTrait for T {}
//~^ ERROR negative impls on type parameters must not contain where bounds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any way to highlight the T: Sized bound? e.g., for drop we print something like

error[E0367]: Drop impl requires T: std::marker::Sized but the struct it is implemented for does not

@lcnr
Copy link
Contributor Author

lcnr commented Jul 29, 2020

This is still wip and I think I should reuse the drop code even if the self type is not an adt.

Will ping you on zulip once it makes sense to review this again 😅

@Muirrum Muirrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020
@bors
Copy link
Contributor

bors commented Aug 15, 2020

☔ The latest upstream changes (presumably #75549) made this pull request unmergeable. Please resolve the merge conflicts.

@lcnr
Copy link
Contributor Author

lcnr commented Sep 18, 2020

let's do this at another time before stabilizing negative impls. This is not really important rn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

negative_impls and auto_traits experimental features allow trait impls to overlap
6 participants