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 sound builtin auto trait impls #93367

Closed
lcnr opened this issue Jan 27, 2022 · 24 comments · Fixed by #120716
Closed

Tracking Issue for sound builtin auto trait impls #93367

lcnr opened this issue Jan 27, 2022 · 24 comments · Fixed by #120716
Labels
A-auto-traits Area: auto traits (e.g., `auto trait Send {}`) A-traits Area: Trait system C-future-incompatibility Category: Future-incompatibility lints T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jan 27, 2022

This is the summary issue for the SUSPICIOUS_AUTO_TRAIT_IMPLS future-compatibility warning. The goal of this page is to describe why this lint was added and how you can fix code that is affected by it. It also provides a place to ask questions
or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What is the warning for?

Rust has some special traits, called either "auto traits" or "traits with a default impl", which are automatically implemented for your types under certain conditions. The most notable examples for this are the traits Send and Sync. A type implements these trait automatically if all its fields also implement this trait. This means that types like struct MyData { data: Vec<String> } automatically implement Send without the user having to think about it.

There are some cases where the builtin impls are not enough. For traits like Send, Sync and Unpin these builtin impls can even be unsound when implementing abstractions using unsafe code. Because of this, it is allowed to also manually implement these traits, disabling the builtin impl for your type in the process.

The way we currently disable builtin impls has some quite subtle bugs, causing unsoundness: #84857. This unsoundness is luckily quite difficult to exploit, but can be triggered under the following conditions:

  • There exists an explicit specialized impl of an auto trait which has a stronger where-clauses than the type definition. We consider an impl to be "specialized" here if the generic arguments of the Self type of the impl are not simply unique generic parameters. impl Trait for X is specialized if X is Vec<i32>, HashMap<String, V> or even HashMap<T, T> and not specialized for Vec<T> or HashMap<K, V>.
  • The builtin impl for the self type of that explicit impl applies for at least one concrete type rejected by the where-clauses of the explicit impl

EXAMPLE

To fix this unsoundness, we intend to simplify the rules for builtin auto trait implementations. Right now, we only discard the builtin implementation if an explicit impl applies to the given type while ignoring where-clauses of that explicit impl. This will be changed to just always discard the builtin impl once at least one explicit impl for that type exists.

As this can cause types to stop implementing auto traits if a specialized impl for that type exists, we now warn again specialized impls if builtin impls could apply for your type. If the builtin impl would definitely not apply for your type, this change will not affect you, and we therefore try to not emit a warning in these cases.

When will this change take effect?

After a crater run detected that our intended change to make builtin auto trait impls sound causes some breakage in the wider ecosystem.

We are currently searching for ways to lessen the impact by modifying the implementation to be more similar to the current situation, while still being sound. If that is not possible, we will introduce this change after the future compatibility lint has been on stable for long enough to reduce the impact of landing this change and if there are existing workarounds for breakage caused by the new rules.

Some of the cases which broke need either marker traits (#29864) or negative impls (#68318) affecting coherence to get back the old behavior. Both of these are currently not too far away from stabilization, so this will probably remain as a future compatibility warning until one of these ends up getting stabilized.

How to fix this?

This depends on why the explicit impl has been added in the first place. If you do not want the builtin impl to apply, you may add a field to your type which definitely does not implement the given auto trait. You may also be able to remove the explicit impl entirely, if that case is already currently covered by the builtin impl. For anything else, please report your case here so that we know about the issue and are able to extend this section.

If your code triggered this lint, please tell us about it in this issue so we may take it into consideration when deciding on how to proceed.

@lcnr lcnr added A-traits Area: Trait system T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-future-incompatibility Category: Future-incompatibility lints WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 A-auto-traits Area: auto traits (e.g., `auto trait Send {}`) labels Jan 27, 2022
jethrogb pushed a commit to fortanix/rust-mbedtls that referenced this issue Feb 4, 2022
@jethrogb
Copy link
Contributor

jethrogb commented Feb 4, 2022

I had implemented my own generic collection type containing a NonNull field. I then had implemented Send and Sync for collections of specific types. I'm not sure why this would ever be unsound.

@lcnr
Copy link
Contributor Author

lcnr commented Feb 4, 2022

@jethrogb your case is fairly interesting and would not actually be impacted by this change. The future compat lint should ideally only triggers in cases where:

  • you manually implement Send for a specialized type, List<Certificate> in this case
  • and our intended change to the rules governing auto traits will cause your impl to disable a currently used builtin impl for Send for your type.

The lint is not perfect however, so we currently lint all specialized impls if the builtin impl may currently apply to your type in some way. As in your case, the builtin impl only applied for T = Certificate anyways as List contains a Box<T> which also has an explicit impl for Box<Certificate>, it is completely covered by your explicit impl and your crate would have therefore not been impacted by this change.

Improving the lint to detect specialized auto trait impls which fully cover the builtin impl for a type isn't trivial. I would much rather lint too much (causing the cases to be mentioned here) than not lint something which later changes behavior, accidentally breaking some code.

Note that the lint exists to notify people of a future change which might change the behavior of their code even if it is currently sound. Only changing the behavior for impls which can cause unsoundness seems would afaict be fragile and difficult to understand for users.

bors bot added a commit to fortanix/rust-mbedtls that referenced this issue Feb 5, 2022
180: Rearrange Send/Sync impls for MbedtlsList/MbedtlsBox r=AdrianCX a=jethrogb

See rust-lang/rust#93367

Co-authored-by: Jethro Beekman <jethro@fortanix.com>
@madsmtm
Copy link
Contributor

madsmtm commented Feb 22, 2022

Hey, I hit this warning on a struct that uses PhantomData to state that it does not necessarily always implement some marker traits (some FFI stuff, for specifics see here, here and here).

I don't quite grep the specifics of this issue, but would it make sense to extend the lint / your analysis to "look inside" PhantomData as well?

Playground link.

@lcnr
Copy link
Contributor Author

lcnr commented Feb 23, 2022

@madsmtm yeah, that's an issue with our implementation of the lint, should be able to fix that one soon

Manishearth added a commit to Manishearth/rust that referenced this issue Feb 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 24, 2022
ehuss pushed a commit to ehuss/rust that referenced this issue Mar 3, 2022
@Diggsey
Copy link
Contributor

Diggsey commented Mar 10, 2022

I haven't really looked into it yet, but one of my crates is flagging up this warning: Diggsey/scoped-tls-hkt#2

@lcnr
Copy link
Contributor Author

lcnr commented Mar 14, 2022

your code shouldn't change when we land the fix for #84857, so this shouldn't necessarily warn. I think #94925 will fix that.

Thanks 👍

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 15, 2022
relax `suspicious_auto_trait_impls` lint wrt lifetimes

fixes the warning for rust-lang#93367 (comment).
@bluss
Copy link
Member

bluss commented May 1, 2022

I had this case in matrixmultiply that triggers the warning:

/// A Send + Sync raw pointer wrapper          
#[derive(Copy, Clone)]
#[repr(transparent)]
pub(crate) struct Ptr<T> { ptr: T }
unsafe impl<T> Sync for Ptr<*const T> { }
unsafe impl<T> Sync for Ptr<*mut T> { }
unsafe impl<T> Send for Ptr<*const T> { }
unsafe impl<T> Send for Ptr<*mut T> { }

I believe the code was problem-free because all usage of Ptr<T> were in the form of either Ptr<*const X> or Ptr<*mut X>. I can avoid the warning by duplicating some definitions and going with something like Ptr<T> { *const T }, PtrMut<T> { *mut T } instead.

Would it be ok to assume that this code doesn't change meaning in the future? Or does it need updating? The lint kind of implies that it can be ignored for some cases.

@Escapingbug
Copy link

I run into this issue while trying to exclude a type when implementing a trait.

Roughly my code does is something like:

auto trait NotEq {}
struct TypeCmp<U, V>(U, V);
impl<T> !NotEq for TypeComp<T, T> {}

impl<T> SomeTrait<T> for MyStruct
where
    T: Default,
    TypeCmp<T, u8>: NotEq,
    TypeCmp<T, u16>: NotEq,
    // More types that I want to exclude
{
    // ... trait impl
}

For excluded types such as u8 and u16 in this example, I have a seperate implementation.
Without such exclusion, a conflicting implementation error would show up.
I don't know if I understand this correctly, in future release the NotEq auto trait would be dropped for all types as I have already given an explicit implementation !NotEq ?
I'm not sure how do I workaround this.

@lcnr
Copy link
Contributor Author

lcnr commented Mar 16, 2023

NotEq would start getting dropped for TypeComp<T, U> where T and U are unrelated.

I'm not sure how do I workaround this.

ignoring the fact that auto traits will probably never get stabilized, you could add a bound T: AllowedInImpl where AllowedInImpl has negative impls for u8, u16 and so on

For excluded types such as u8 and u16 in this example, I have a seperate implementation.

that sounds like you may want specialization, not negative impls though

@Escapingbug
Copy link

ignoring the fact that auto traits will probably never get stabilized, you could add a bound T: AllowedInImpl where AllowedInImpl has negative impls for u8, u16 and so on

But in my case, I think, I still need the AllowedInImpl automatically implemented for all types?
The situation I encoutered is that, I want this SomeTrait implementation automatically derived for all types that with Default (or other traits possibly added, Default is just an example here). That is, whenever the downstream crate imported my implementation and its own type does implemented Default, it gets a SomeTrait for free.

For excluded types such as u8 and u16 in this example, I have a seperate implementation.
that sounds like you may want specialization, not negative impls though

Yes I knew that all along, but I don't know if the specialization works in this case currently.
It might be my fault, but I keep getting conflicting implementations error complaining upstream crates may add a new Default implementation for u8, etc.

@Escapingbug

This comment was marked as off-topic.

@Rexagon
Copy link

Rexagon commented Apr 14, 2023

Sorry for the possible off-topic, but I ran into this issue in the context of another problem.

I wanted some trait Item<G: Group> to be Send depending on G.

trait Group {
    type Container: AsRef<dyn Item<Self>> + Clone;
}

trait Item<G: Group> {
    fn make_owned(&self) -> G::Container;
}

struct View<'a, G> {
    item: &'a dyn Item<G>,
}

// Rc
struct RcGroup;
impl Group for RcGroup {
    type Container = std::rc::Rc<dyn Item<Self>>;
}

// Arc
struct ArcGroup;
impl Group for ArcGroup {
    type Container = std::sync::Arc<dyn Item<Self>>;
}

unsafe impl<'a> Send for (dyn Item<ArcGroup> + 'a) {}   // <---

fn test_send<T: Send>() {}
fn should_compile() {
    test_send::<View<ArcGroup>>();
}

P.S. Wrapping dyn trait into a newtype also compiles with suspicious_auto_trait_impls warning

struct Wrapper<'a, G>(dyn Item<G> + 'a);
unsafe impl<'a> Send for Wrapper<'a, ArcGroup> {}

Taowyoo pushed a commit to fortanix/rust-mbedtls that referenced this issue Apr 19, 2023
180: Rearrange Send/Sync impls for MbedtlsList/MbedtlsBox r=AdrianCX a=jethrogb

See rust-lang/rust#93367

Co-authored-by: Jethro Beekman <jethro@fortanix.com>
Taowyoo pushed a commit to fortanix/rust-mbedtls that referenced this issue Apr 24, 2023
180: Rearrange Send/Sync impls for MbedtlsList/MbedtlsBox r=AdrianCX a=jethrogb

See rust-lang/rust#93367

Co-authored-by: Jethro Beekman <jethro@fortanix.com>
@Escapingbug
Copy link

To make my usecase even more clear, I almost copied the idea here. If I'm not wrong, this will also get invalidated in the future.
In my usecase, I truly need this ability, or else I will have to write a bunch of workaround procedure-macro to cope with this problem.

@pitaj
Copy link
Contributor

pitaj commented Dec 1, 2023

Is this lint supposed to trigger on specialized negative impls like the following?

#![feature(negative_impls)]

struct A;
struct B;
struct C;

struct Helper<T>(T);

impl !Send for Helper<A> {}
impl !Send for Helper<B> {}

Playground

If this lint is supposed to cover negative impls like this, the lint message and the description here are kinda confusing. (Though I understand negative impls are unstable so it's not a huge deal)

@lcnr
Copy link
Contributor Author

lcnr commented Dec 1, 2023

If this lint is supposed to cover negative impls like this, the lint message and the description here are kinda confusing.

yes (though the behavior change has already landed. This lint should be updated)

the issue is that impl !Send for Helper<A> {} also disables the builtin auto trait impl for impl<T: Send> Send for Helper<T> {}, which is probably not the expected behavior.

@raindust
Copy link

raindust commented Dec 17, 2023

pub trait Irrelative {
	type Type;
}

impl<T> Irrelative for T
where
	T: ?Sized,
{
	type Type = !;
}

pub struct Equality<X, Y>(<X as Irrelative>::Type, <Y as Irrelative>::Type);
#[rustc_unsafe_specialization_marker]
pub auto trait NotEqual {}
impl<X> !NotEqual for Equality<X, X> {}

Given the code above, how can I update the negative impl related logic about Equality?

@jplatte
Copy link
Contributor

jplatte commented Dec 17, 2023

Tracking issues are not the right place to ask for help. Please try on Discourse (users.rust-lang.org) or Discord.

@bors bors closed this as completed in 5b8b435 Feb 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 20, 2024
Rollup merge of rust-lang#120716 - spastorino:change-some-lint-msgs, r=lcnr

Change leak check and suspicious auto trait lint warning messages

The leak check lint message "this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!" is misleading as some cases may not be phased out and could end being accepted. This is under discussion still.

The suspicious auto trait lint the change in behavior already happened, so the new message is probably more accurate.

r? `@lcnr`

Closes rust-lang#93367
@lcnr
Copy link
Contributor Author

lcnr commented Feb 20, 2024

The behavior of auto trait has changed in #113312, so the behavior of auto trait impls will now stay as is for the foreseeable future:

If there's any impl whose self type is Adt<DoesNotMatter>, then no structural builtin auto trait implementation is generated. I.e. once you implement an auto trait for your type, even if you use concrete generic arguments, the builtin implementation is disabled for all instances of your type

ogoffart added a commit to slint-ui/slint that referenced this issue Feb 21, 2024
```
error: lint `suspicious_auto_trait_impls` has been removed: no longer needed, see #93367 <rust-lang/rust#93367> for more information
```
ogoffart added a commit to slint-ui/slint that referenced this issue Feb 21, 2024
```
error: lint `suspicious_auto_trait_impls` has been removed: no longer needed, see #93367 <rust-lang/rust#93367> for more information
```
ogoffart added a commit to slint-ui/slint that referenced this issue Feb 21, 2024
```
error: lint `suspicious_auto_trait_impls` has been removed: no longer needed, see #93367 <rust-lang/rust#93367> for more information
```
@wdanilo
Copy link

wdanilo commented Aug 14, 2024

Similarly to @Escapingbug, I feel that inability to write now the following code is extremely limiting:

auto trait NotEq {}
struct TypeCmp<U, V>(U, V);
impl<T> !NotEq for TypeCmp<T, T> {}

#[derive(Debug, Clone, Copy)]
struct HCons<H, T> {
    head: H,
    tail: T,
}

#[derive(Debug, Clone, Copy)]
struct HNil;

#[derive(Debug, Clone, Copy)]
struct Zero;

#[derive(Debug, Clone, Copy)]
struct Succ<N> {
    _phantom: PhantomData<N>,
}

// ===============
// === IndexOf ===
// ===============

type IndexOf<T, X> = <T as HasIndexOf<X>>::Index;

trait HasIndexOf<T> {
    type Index;
}

impl<H, T> HasIndexOf<H> for HCons<H, T> {
    type Index = Zero;
}

impl<H, T, X> HasIndexOf<X> for HCons<H, T>
where
    T: HasIndexOf<X>,
    TypeCmp<X, H>: NotEq,
{
    type Index = <T as HasIndexOf<X>>::Index;
}

How can this be expressed (computing indexed of a H-List using associated types)?

Edit: Even the Rust Book tells about this use case here: https://doc.rust-lang.org/stable/unstable-book/language-features/negative-impls.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-auto-traits Area: auto traits (e.g., `auto trait Send {}`) A-traits Area: Trait system C-future-incompatibility Category: Future-incompatibility lints T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.