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

where_clauses_object_safety future compatibility lint #51443

Open
leoyvens opened this issue Jun 8, 2018 · 23 comments
Open

where_clauses_object_safety future compatibility lint #51443

leoyvens opened this issue Jun 8, 2018 · 23 comments
Labels
A-traits Area: Trait system A-typesystem Area: The type system B-unstable Blocker: Implemented in the nightly compiler and unstable. C-future-compatibility Category: Future-compatibility lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@leoyvens
Copy link
Contributor

leoyvens commented Jun 8, 2018

This is the summary issue for the WHERE_CLAUSES_OBJECT_SAFETY future-compatibility warning and other related errors. The goal of this page is describe why this change was made 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 (https://github.com/rust-lang/rfcs/blob/master/text/1122-language-semver.md).

What is the warning for?

As was discovered in #50781 a combination of implementing a trait directly for a dyn type and where clauses involving Self can punch a hole in our dyn-capability rules rules. See the minimization:

trait Trait {}

trait X { fn foo(&self) where Self: Trait; }

impl X for () { fn foo(&self) {} }

impl Trait for dyn X {}

// Segfault at opt-level 0, SIGILL otherwise.
pub fn main() { <dyn X as X>::foo(&()); }

The fix applied in #50966 is to tighten the dyn-capability rules to make X not dyn-capable in this case, in general making any trait that contains Self in where clauses not dyn-capable, much like we disallow Self in arguments or return type. Few root regressions appeared in the crater run and those were fixable, though some crates being unmaintained complicates things.

However that fix is heavy handed and disallows things we actually wish to support, still we went ahead with the warning as a stop gap while we look for a better, more targeted fix. The original issue contains some discussion of alternative fixes. With tight chalk integration, we could do some clever things.

Other alternatives discussed for the short-term:

  • Checking from an impl Foo for dyn Bar + .. whether that impl is causing trouble. This seems to be a dead end because it's hard to tell which dyn types are targeted by a blanket impl such as impl<U: ?Sized + Bounds> Trait for U.
  • Checking the cast sites with a rule like "if dyn Foo: Trait then to cast T into dyn Foo we require that T: Trait must also hold", probably for every Trait such that Self: Trait appears in where clauses in the traits methods. This would alleviate the warning for Self: Trait where clauses. This may be practical to implement now, but seems subtle, we'd need to give it more thought.

When will this warning become a hard error?

Hopefully we will develop a finer-grained rule and this warning will never be an error.

How to fix this?

Either drop the where clause or stop using dyn types with the affected trait. If this is not viable, that's probably ok, it's very unlikely that your code is actually unsound. But please do report your case here so take we may take it into consideration and see how to better support it!

@leoyvens leoyvens added A-typesystem Area: The type system A-traits Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Jun 8, 2018
@jkordish jkordish added the C-future-compatibility Category: Future-compatibility lints label Jun 13, 2018
@nikomatsakis
Copy link
Contributor

To clarify: we are specifically interested in getting feedback on places where this lint causes you issues, since that will help us to judge how well more precise fixes would work.

@cramertj
Copy link
Member

This caused a future-compat warning in the upcoming futures 0.3 crate. Minimized example:

#![feature(arbitrary_self_types, pin)]

use std::marker::Unpin;
use std::mem::PinMut;

trait Future {
    fn poll(self: PinMut<Self>);
}

trait FutureExt: Future {
    fn poll_unpin(&mut self) where Self: Unpin {
        PinMut::new(self).poll()
    }
}

The error:

warning: the trait `FutureExt` cannot be made into an object
  --> foo.rs:11:5
   |
11 | /     fn poll_unpin(&mut self) where Self: Unpin {
12 | |         PinMut::new(self).poll()
13 | |     }
   | |_____^
   |
   = note: #[warn(where_clauses_object_safety)] on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
   = note: method `poll_unpin` references the `Self` type in where clauses

It seems like it should be possible to permit calls to poll_unpin in cases where Self is a dyn Trait type where Trait: Unpin.

@nikomatsakis
Copy link
Contributor

Thanks @cramertj -- that falls pretty squarely into the rules we had in mind! Let's discuss the details of those rules in #50781 I guess, I want to try and "re-sync" (but I guess I'd rather keep this thread for people to report issues).

But just to summarize, the rule we had in mind is roughly this:

  • If a trait Trait has a method method with a where-clause where Self: XXX
    • that is object-safe so long as dyn Trait: XXX cannot be proven
      • although it is a bit weird in the case where trait Trait: XXX -- we could perhaps try to distinguish those cases; in Chalk terms, XXX would be an implied bound of Trait

That applies to your example, I believe, since dyn Future: Unpin does not hold.

This is a kind of generalization of the rule around where Self: Sized -- except that, in that case, we know that dyn Trait: Sized could never be proven, and hence we can suspend the where-clause requirements entirely.

@locka99
Copy link

locka99 commented Jul 13, 2018

For anyone reading this, I got the warning/error on a trait that was like this:

pub trait Config {
    fn save(&self, path: &Path) -> Result<(), ()> where Self: serde::Serialize { ... }
}

In my case I was able to make the problem go away by requiring the thing implementing Config to also implement serde::Serialize:

pub trait Config: serde::Serialize {
    fn save(&self, path: &Path) -> Result<(), ()> { ... }
}

@emilio
Copy link
Contributor

emilio commented Jul 30, 2018

This broke bindgen:

error: the trait `ir::template::TemplateParameters` cannot be made into an object
   --> src/ir/template.rs:134:5
    |
134 | /     fn all_template_params(&self, ctx: &BindgenContext) -> Vec<TypeId>
135 | |     where
136 | |         Self: ItemAncestors,
137 | |     {
...   |
141 | |         }).collect()
142 | |     }
    | |_____^
    |
note: lint level defined here
   --> src/lib.rs:11:9
    |
11  | #![deny(warnings)]
    |         ^^^^^^^^
    = note: #[deny(where_clauses_object_safety)] implied by #[deny(warnings)]
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
    = note: method `all_template_params` references the `Self` type in where clauses

error: the trait `ir::template::TemplateParameters` cannot be made into an object
   --> src/ir/template.rs:147:5
    |
147 | /     fn used_template_params(&self, ctx: &BindgenContext) -> Vec<TypeId>
148 | |     where
149 | |         Self: AsRef<ItemId>,
150 | |     {
...   |
160 | |                     .collect()
161 | |     }
    | |_____^
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
    = note: method `used_template_params` references the `Self` type in where clauses

error: aborting due to 2 previous errors

error: Could not compile `bindgen`.

This is on this bindgen commit fwiw: rust-lang/rust-bindgen@29dad40

emilio added a commit to rust-lang/rust-bindgen that referenced this issue Jul 30, 2018
@Emerentius
Copy link
Contributor

Emerentius commented Aug 1, 2018

I have an extension trait that was never supposed to be made into an object which triggers this. It only exists to add a few methods.
I could limit the implementors to just [T] down from T: AsRef<[T]> which would stop the lint from firing (and also be a better design), but it would also make the dyn capability pointless as there's only one type that could be made into the object.

I like this as a lint but the it will become a hard error in a future release! part of the error message should go. Not all traits require dyn capability.

Edit: In fact, the lint is a false positive. The trait isn't dyn capable already because it contains generic functions.

@alecmocatta
Copy link
Contributor

I bumped into this upcasting boxed trait objects with marker traits. There may be a way to do this safely that doesn't fall foul of this lint?

use std::any::Any;

trait Trait: Any {
	fn into_any(self: Box<Self>) -> Box<Any>;
	fn into_any_send(self: Box<Self>) -> Box<Any+Send> where Self: Send;
	fn into_any_sync(self: Box<Self>) -> Box<Any+Sync> where Self: Sync;
	fn into_any_send_sync(self: Box<Self>) -> Box<Any+Send+Sync> where Self: Send+Sync;
}

impl<T> Trait for T where T: Any {
	fn into_any(self: Box<Self>) -> Box<Any> {
		self
	}
	fn into_any_send(self: Box<Self>) -> Box<Any+Send> where Self: Send {
		self
	}
	fn into_any_sync(self: Box<Self>) -> Box<Any+Sync> where Self: Sync {
		self
	}
	fn into_any_send_sync(self: Box<Self>) -> Box<Any+Send+Sync> where Self: Send+Sync {
		self
	}
}

fn main() {
	let a: usize = 123;
	let b: Box<Trait+Send+Sync> = Box::new(a);
	let c: Box<Any+Send+Sync> = b.into_any_send_sync();
	let _d: usize = *Box::<Any>::downcast(c).unwrap();
}

(Playground)

@passcod
Copy link
Contributor

passcod commented Aug 24, 2018

This warns on AnyMap, but I don't understand enough to really figure out what's going on and how it should be fixed:

warning: the trait `anymap::any::CloneToAny` cannot be made into an object
  --> /home/.cargo/registry/src/github.com-1ecc6299db9ec823/anymap-0.12.1/src/any.rs:21:5
   |
21 |     fn clone_to_any_send_sync(&self) -> Box<CloneAny + Send + Sync> where Self: Send + Sync;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
   = note: method `clone_to_any_send_sync` references the `Self` type in where clauses

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Sep 16, 2018

I have an extension trait that i want to be usable with ?Sized types (including str) where this is causing a problem.

This is the reduced version of the trait:

trait SelfOps{
    fn piped_ref<'a,F, U>(&'a self, f: F) -> U
    where
        F: FnOnce(&'a Self) -> U,
    {
        f(self)
    }

    fn format_debug(&self)->String
    where Self:fmt::Debug
    {
        format!("{:?}",self)
    }
}

impl<This:?Sized> SelfOps for This{}

With the previous 2 methods I get this somewhat confusing pair of warning and error :


warning: the trait `SelfOps` cannot be made into an object
  --> src/main.rs:11:5
   |
11 | /     fn format_debug(&self)->String
12 | |     where Self:fmt::Debug
13 | |     {
14 | |         format!("{:?}",self)
15 | |     }
   | |_____^
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
   = note: method `format_debug` references the `Self` type in where clauses

error[E0038]: the trait `SelfOps` cannot be made into an object
  --> src/main.rs:23:18
   |
23 |     let x=&() as &SelfOps;
   |                  ^^^^^^^^ the trait `SelfOps` cannot be made into an object
   |
   = note: method `piped_ref` has generic type parameters

It seems to me that if it knows that it can't be turned into a trait object because it has a generic method,
then it should not warn because of the Self type in the where clause of format_debug.

@chris-morgan
Copy link
Member

On the AnyMap case: it’s substantially the same as what @alecmocatta is reporting. This lint causes trouble with auto traits.

core::any::Any has the ability to be Any + Send, and Any + Send + Sync, and that’s OK because it doesn’t need any support inside the trait object for code specific to Send and/or Sync functionality—just an impl Any + Send block.

AnyMap has an extension of that, a cloneable Any. That part is fine, but when you get to additional trait bounds, you need a function that will be in the vtable; and that’s where we run into trouble under this new scheme.

Here’s a simplified version of some of the code (covering only the Send bound):

#[doc(hidden)]
pub trait CloneToAny {
    /// Clone `self` into a new `Box<CloneAny>` object.
    fn clone_to_any(&self) -> Box<CloneAny>;

    /// Clone `self` into a new `Box<CloneAny + Send>` object.
    fn clone_to_any_send(&self) -> Box<CloneAny + Send> where Self: Send;
}

impl<T: Any + Clone> CloneToAny for T {
    fn clone_to_any(&self) -> Box<CloneAny> {
        Box::new(self.clone())
    }

    fn clone_to_any_send(&self) -> Box<CloneAny + Send> where Self: Send {
        Box::new(self.clone())
    }
}

pub trait CloneAny: Any + CloneToAny { }

impl<T: Any + Clone> CloneAny for T { }

impl Clone for Box<CloneAny> {
    fn clone(&self) -> Box<CloneAny> {
        (**self).clone_to_any()
    }
}

impl Clone for Box<CloneAny + Send> {
    fn clone(&self) -> Box<CloneAny + Send> {
        (**self).clone_to_any_send()
    }
}

Now as it stands, I know that if I have a Box<CloneAny + Send>, then I know that calling clone_to_any_send is safe.

I would like to retain the same interface: adding trait bounds is a known and sane way of handling these things—the way you’re supposed to handle it. But if necessary, I suppose the proliferation of traits can begin, with CloneAny, CloneSendAny, CloneSendSyncAny, &c.

As it stands, I’m seeing a few options for me as a developer:

  1. Bury my head in the sand and hope the problem goes away without my doing anything (if I’m feeling particularly foolishly bull-headed, which I’m not at present, add #[allow(where_clauses_object_safety)]!);
  2. Drop the clone_to_any_* methods, leaving only clone_to_any, then cross my fingers, make the Clone implementation transmute between Box<CloneAny> and Box<CloneAny + Send>, and hope that the vtables are the same and that nothing will explode;
  3. Appeal to see if it’s reasonable to the lint can be disabled specifically for auto traits (that is, if my existing code can be deemed acceptable after all);
  4. Stop using additional bounds, and proliferate traits.

Expanding on the appeal: I don’t know, but my impression is that auto traits won’t trigger the memory unsafety problem. If so, is it reasonable to check whether the additional bounds on Self are all auto traits, and consider that acceptable code?

And if tweaking the lint is not reasonable, is what I outline in the second item reasonable? Will Rust eat my laundry if I transmute Box<CloneAny> and Box<CloneAny + Send>?

(Quite incidentally: where I’m from “laundry” refers to the room where clothes washing is done, not to the pile of clothes, which is called the “washing”. The whole “may eat your laundry” thing was therefore much more amusing to contemplate than it would have been if it were mere clothes that were being devoured.)

@alecmocatta
Copy link
Contributor

And if tweaking the lint is not reasonable, is what I outline in the second item reasonable? Will Rust eat my laundry if I transmute Box<CloneAny> and Box<CloneAny + Send>?

This is effectively what's done by the stdlib here:

rust/src/liballoc/boxed.rs

Lines 455 to 517 in f1aefb4

impl Box<dyn Any> {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
/// Attempt to downcast the box to a concrete type.
///
/// # Examples
///
/// ```
/// use std::any::Any;
///
/// fn print_if_string(value: Box<Any>) {
/// if let Ok(string) = value.downcast::<String>() {
/// println!("String ({}): {}", string.len(), string);
/// }
/// }
///
/// fn main() {
/// let my_string = "Hello World".to_string();
/// print_if_string(Box::new(my_string));
/// print_if_string(Box::new(0i8));
/// }
/// ```
pub fn downcast<T: Any>(self) -> Result<Box<T>, Box<dyn Any>> {
if self.is::<T>() {
unsafe {
let raw: *mut dyn Any = Box::into_raw(self);
Ok(Box::from_raw(raw as *mut T))
}
} else {
Err(self)
}
}
}
impl Box<dyn Any + Send> {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
/// Attempt to downcast the box to a concrete type.
///
/// # Examples
///
/// ```
/// use std::any::Any;
///
/// fn print_if_string(value: Box<Any + Send>) {
/// if let Ok(string) = value.downcast::<String>() {
/// println!("String ({}): {}", string.len(), string);
/// }
/// }
///
/// fn main() {
/// let my_string = "Hello World".to_string();
/// print_if_string(Box::new(my_string));
/// print_if_string(Box::new(0i8));
/// }
/// ```
pub fn downcast<T: Any>(self) -> Result<Box<T>, Box<dyn Any + Send>> {
<Box<dyn Any>>::downcast(self).map_err(|s| unsafe {
// reapply the Send marker
Box::from_raw(Box::into_raw(s) as *mut (dyn Any + Send))
})
}
}

@leoyvens
Copy link
Contributor Author

my impression is that auto traits won’t trigger the memory unsafety problem. If so, is it reasonable to check whether the additional bounds on Self are all auto traits, and consider that acceptable code?

This is true, this lint can be tweaked to not fire if the bounds are all auto traits.

pierrechevalier83 added a commit to pierrechevalier83/rust-typemap that referenced this issue Dec 27, 2018
Refering to Self in where clause became illegal because:
> As was discovered in #50781 a combination of implementing a trait
> directly for a dyn type and where clauses involving Self can punch a
> hole in our dyn-capability rules.

See this issue for details:
rust-lang/rust#51443
@tbu-
Copy link
Contributor

tbu- commented Mar 5, 2019

How should I deal with that warning?

Does it only trigger if I create a trait object or otherwise interact with dyn Trait? I think I'm doing neither but the warning still triggers.

problame added a commit to 49nord/humblegen-rs that referenced this issue Jun 17, 2020
…ase: auth)

* The handler trait gets an additional method `intercept_handler_pre(&self, &req, ctx)`
* `ctx` is an associated type of the handler trait
* `ctx`'s role is to store data extracted by the interceptor

Check out the newly added example `humblegen/tests/rust/service-authorization-using-interceptor`.

- [ ] Problem: I do not know how I should deal with this warning:

```
WARNINGS:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
warning: the trait `protocol::BlogApi` cannot be made into an object
   --> $DIR/spec.rs:100:14
    |
98  | pub trait BlogApi {
    |           ------- this trait cannot be made into an object...
99  |     type Context: Default + Sized + Send + Sync;
100 |     async fn intercept_handler_pre(
    |              ^^^^^^^^^^^^^^^^^^^^^ ...because method `intercept_handler_pre` references the `Self` type in its `where` clause
    |
    = note: `#[warn(where_clauses_object_safety)]` on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #51443 <rust-lang/rust#51443>
    = help: consider moving `intercept_handler_pre` to another trait
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
```

ISTM that it is related to `async-trait` because it goes away if we remove the `async` in front of the `async fn intercept_handler_pre` decl.
reiner-dolp pushed a commit to 49nord/humblegen-rs that referenced this issue Jun 18, 2020
…ase: auth)

* The handler trait gets an additional method `intercept_handler_pre(&self, &req, ctx)`
* `ctx` is an associated type of the handler trait
* `ctx`'s role is to store data extracted by the interceptor

Check out the newly added example `humblegen/tests/rust/service-authorization-using-interceptor`.

- [ ] Problem: I do not know how I should deal with this warning:

```
WARNINGS:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
warning: the trait `protocol::BlogApi` cannot be made into an object
   --> $DIR/spec.rs:100:14
    |
98  | pub trait BlogApi {
    |           ------- this trait cannot be made into an object...
99  |     type Context: Default + Sized + Send + Sync;
100 |     async fn intercept_handler_pre(
    |              ^^^^^^^^^^^^^^^^^^^^^ ...because method `intercept_handler_pre` references the `Self` type in its `where` clause
    |
    = note: `#[warn(where_clauses_object_safety)]` on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #51443 <rust-lang/rust#51443>
    = help: consider moving `intercept_handler_pre` to another trait
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
```

ISTM that it is related to `async-trait` because it goes away if we remove the `async` in front of the `async fn intercept_handler_pre` decl.
@itowlson
Copy link

itowlson commented Jul 6, 2020

I'm hitting this on a method that doesn't appear to reference the Self type in a where clause. When I try to use a dyn Store elsewhere in the program I get the error on this method:

#[async_trait]
pub trait Store {
    async fn fetch_pod_modules(&self, pod: &Pod) -> anyhow::Result<HashMap<String, Vec<u8>>> {
        // ...
    }
}

// Error:
// the trait `kubelet::store::Store` cannot be made into an object
// the trait cannot be made into an object because method `fetch_pod_modules`
// references the `Self` type in its `where` clause

I think this is to do with the way async_trait expands async methods. The docs give the following example of an expansion (https://docs.rs/async-trait/0.1.36/async_trait/#explanation):

impl Advertisement for AutoplayingVideo {
    fn run<'async>(
        &'async self,
    ) -> Pin<Box<dyn core::future::Future<Output = ()> + Send + 'async>>
    where
        Self: Sync + 'async,  // <--------- AHA?
    {
        // ...
    }
}

Is there a recommended workaround for async_trait members?

@itowlson
Copy link

itowlson commented Jul 6, 2020

Answering my own question: the workaround is to add Sync as a supertrait of Store (documentation: https://docs.rs/async-trait/0.1.36/async_trait/#dyn-traits).

@indygreg
Copy link
Contributor

indygreg commented Dec 4, 2021

I wanted to provide feedback that the warnings emitted by this in 1.57 (which are effectively unchanged from previous comments) when async_trait (and presumably other macros) is in play are confusing. Concretely:

warning: the trait `repository::ReleaseReader` cannot be made into an object
   --> tugger-debian/src/repository/mod.rs:283:14
    |
283 |     async fn resolve_packages(
    |              ^^^^^^^^^^^^^^^^
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> tugger-debian/src/repository/mod.rs:283:14
    |
172 | pub trait ReleaseReader {
    |           ------------- this trait cannot be made into an object...
...
283 |     async fn resolve_packages(
    |              ^^^^^^^^^^^^^^^^ ...because method `resolve_packages` references the `Self` type in its `where` clause
    = help: consider moving `resolve_packages` to another trait

As @itowlson pointed out in previous comments, this is an issue with how async_trait works and can easily be worked around by adding Sync as a supertrait (thank you for the tips!). But, the compiler warning didn't make it obvious that a macro was involved.

In other compiler diagnostics, when an issue involves a macro, the compiler can give me the hint that a macro is in play. This can often short-circuit a lot of debugging. Would it be possible for the compiler to detect when this issue is (possibly) influenced via a macro and provide such a hint?

Furthermore, the existing warning says e.g.

because method `resolve_packages` references the `Self` type in its `where` clause`

However, my trait member is defined as async fn resolve_packages(&self, ...) -> Result<BinaryPackageList<'static>, RepositoryReadError>; and... doesn't have a where clause! I think the macro expansion performed by async_trait does introduce a where clause. But since the compiler isn't printing the expanded output, this left me scratching my head as to what exactly the references the... error message was referring to! (What where clause?!) Again, I think this warning could be much more actionable if there were a hint that a macro were involved and/or a pointer to the exact where clause introducing the constraint.

@lunar-mining
Copy link

lunar-mining commented Jul 2, 2022

I'm encoutering the same issue: https://github.com/lunar-mining/trait_ptr

When I add Sync to the trait it compiles fine providing the trait methods pass &self. But if I change this to an Arc<Self> then it produces the same warning.

This compiles without the warning:

use std::sync::{Arc, Weak};

fn main() {
    println!("Hello, world!");
}

pub type MyTraitPtr = Arc<dyn MyTrait>;

#[async_trait]
pub trait MyTrait: Sync {
    async fn foo(&self) {}
}

pub struct Parent {
    child: MyTraitPtr,
}

This does not:

use async_trait::async_trait;
use std::sync::{Arc, Weak};

fn main() {
    println!("Hello, world!");
}

pub type MyTraitPtr = Arc<dyn MyTrait>;

#[async_trait]
pub trait MyTrait: Sync {
    async fn foo(self: Arc<Self>) {}
}

pub struct Parent {
    child: MyTraitPtr,
}

This is the full warning:

note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
  --> src/main.rs:12:14
   |
11 | pub trait MyTrait: Sync {
   |           ------- this trait cannot be made into an object...
12 |     async fn foo(self: Arc<Self>) {}
   |              ^^^ ...because method `foo` references the `Self` type in its `where` clause
   = help: consider moving `foo` to another trait

@dtolnay
Copy link
Member

dtolnay commented Dec 29, 2022

Did something happen intentionally with this recently? The following code compiles cleanly on nightly-2022-12-28, but triggers a warning with nightly-2022-12-29. Yet I don't see any recent PR linked to this tracking issue.

pub trait Trait {
    fn method(&self) where Self: Sync;
}
warning: the trait `Trait` cannot be made into an object
 --> src/main.rs:2:8
  |
2 |     fn method(&self) where Self: Sync;
  |        ^^^^^^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
 --> src/main.rs:2:8
  |
1 | pub trait Trait {
  |           ----- this trait cannot be made into an object...
2 |     fn method(&self) where Self: Sync;
  |        ^^^^^^ ...because method `method` references the `Self` type in its `where` clause
  = help: consider moving `method` to another trait
  = note: `#[warn(where_clauses_object_safety)]` on by default

@Darksonn
Copy link
Contributor

This has also recently started triggering on tokio::io::AsyncBufReadExt.

@woodruffw
Copy link
Contributor

woodruffw commented Dec 30, 2022

Triggering on der (RustCrypto/formats) as well: https://github.com/RustCrypto/formats/actions/runs/3803406941/jobs/6469791942

The triggering offending code:

/// Encode the value part of a Tag-Length-Value encoded field, sans the [`Tag`]
/// and [`Length`].
pub trait EncodeValue {
    /// Get the [`Header`] used to encode this value.
    fn header(&self) -> Result<Header>
    where
        Self: Tagged,
    {
        Header::new(self.tag(), self.value_len()?)
    }

    /// Compute the length of this value (sans [`Tag`]+[`Length`] header) when
    /// encoded as ASN.1 DER.
    fn value_len(&self) -> Result<Length>;

    /// Encode value (sans [`Tag`]+[`Length`] header) as ASN.1 DER using the
    /// provided [`Writer`].
    fn encode_value(&self, encoder: &mut dyn Writer) -> Result<()>;
}

(Based on my reading of the object safety docs, the problem here is the where Self: Tagged constraint?)

@dtolnay
Copy link
Member

dtolnay commented Jan 8, 2023

we are specifically interested in getting feedback on places where this lint causes you issues, since that will help us to judge how well more precise fixes would work.

Overall my impression is that this lint is too burdensome in the current form.

Async_trait is a casualty if we keep this. For example:

#[async_trait]
pub trait Trait {
    fn synchronous(&self) {}
    async fn asynchronous(&self) {}
}

This needs to expand to something like:

pub trait Trait {
    // want to call this on non-Sync impls
    fn synchronous(&self) {}

    // as long as we're using Send futures (e.g. tokio) this only makes
    // sense to call on Sync impls, hence the where-clause
    fn asynchronous(&self) -> Pin<Box<dyn Future<Output = ()> + Send + '_>>
    where
        Self: Sync,
    {
        Box::pin(async move { ... })
    }
}

Using the following expansion instead is not a sufficient workaround because that prevents calling synchronous on non-Sync impls:

pub trait Trait: Sync {
    fn synchronous(&self) {}
    fn asynchronous(&self) -> Pin<Box<dyn Future<Output = ()> + Send + '_>> {
        Box::pin(async move { ... })
    }
}

From studying #51443 (comment), it's not clear to me that the justification for this lint applies to the above trait in the first place. The motivation for the lint is that someone might write:

unsafe impl Sync for dyn Trait + '_ {}

and then improperly be able to call the asynchronous method on non-Sync receivers. However that's silly for 2 reasons:

  1. Sync is an unsafe trait. Don't do stupid things with it.
  2. They can't write that impl! It does not compile.
error[E0321]: cross-crate traits with a default impl, like `Sync`, can only be implemented for a struct/enum type, not `dyn Trait`
 --> src/main.rs
  |
  | unsafe impl Sync for dyn Trait + '_ {}
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't implement cross-crate trait with a default impl for non-struct/enum type

So I think this needs to be allowed.

@RalfJung
Copy link
Member

RalfJung commented Jan 8, 2023

Well, we need to change something to fix #50781. Have there been other plausible proposals?

@dtolnay
Copy link
Member

dtolnay commented Jan 8, 2023

Yes I tried to imply a counterproposal in the "2 reasons" in my comment.

  1. If a trait is unsafe, let it be used as bound in a where-clause of trait method of a dyn-safe trait.
  2. If a trait is an autotrait, let it be used as a bound in a where-clause of trait methods of a dyn-safe trait.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 3, 2023
Autotrait bounds on dyn-safe trait methods

This PR is a successor to rust-lang#106604 implementing the approach encouraged by rust-lang#106604 (comment).

**I propose making it legal to use autotraits as trait bounds on the `Self` type of trait methods in a trait object.** rust-lang#51443 (comment) justifies why this use case is particularly important in the context of the async-trait crate.

```rust
#![feature(auto_traits)]
#![deny(where_clauses_object_safety)]

auto trait AutoTrait {}

trait MyTrait {
    fn f(&self) where Self: AutoTrait;
}

fn main() {
    let _: &dyn MyTrait;
}
```

Previously this would fail with:

```console
error: the trait `MyTrait` cannot be made into an object
 --> src/main.rs:7:8
  |
7 |     fn f(&self) where Self: AutoTrait;
  |        ^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#51443 <rust-lang#51443>
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
 --> src/main.rs:7:8
  |
6 | trait MyTrait {
  |       ------- this trait cannot be made into an object...
7 |     fn f(&self) where Self: AutoTrait;
  |        ^ ...because method `f` references the `Self` type in its `where` clause
  = help: consider moving `f` to another trait
```

In order for this to be sound without hitting rust-lang#50781, **I further propose that we disallow handwritten autotrait impls that apply to trait objects.** Both of the following were previously allowed (_on nightly_) and no longer allowed in my proposal:

```rust
auto trait AutoTrait {}

trait MyTrait {}
impl AutoTrait for dyn MyTrait {}  // NOT ALLOWED

impl<T: ?Sized> AutoTrait for T {}  // NOT ALLOWED
```

(`impl<T> AutoTrait for T {}` remains allowed.)

After this change, traits with a default impl are implemented for a trait object **if and only if** the autotrait is one of the trait object's trait bounds (or a supertrait of a bound). In other words `dyn Trait + AutoTrait` always implements AutoTrait while `dyn Trait` never implements AutoTrait.

Fixes dtolnay/async-trait#228.

r? `@lcnr`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 3, 2023
Autotrait bounds on dyn-safe trait methods

This PR is a successor to rust-lang#106604 implementing the approach encouraged by rust-lang#106604 (comment).

**I propose making it legal to use autotraits as trait bounds on the `Self` type of trait methods in a trait object.** rust-lang#51443 (comment) justifies why this use case is particularly important in the context of the async-trait crate.

```rust
#![feature(auto_traits)]
#![deny(where_clauses_object_safety)]

auto trait AutoTrait {}

trait MyTrait {
    fn f(&self) where Self: AutoTrait;
}

fn main() {
    let _: &dyn MyTrait;
}
```

Previously this would fail with:

```console
error: the trait `MyTrait` cannot be made into an object
 --> src/main.rs:7:8
  |
7 |     fn f(&self) where Self: AutoTrait;
  |        ^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#51443 <rust-lang#51443>
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
 --> src/main.rs:7:8
  |
6 | trait MyTrait {
  |       ------- this trait cannot be made into an object...
7 |     fn f(&self) where Self: AutoTrait;
  |        ^ ...because method `f` references the `Self` type in its `where` clause
  = help: consider moving `f` to another trait
```

In order for this to be sound without hitting rust-lang#50781, **I further propose that we disallow handwritten autotrait impls that apply to trait objects.** Both of the following were previously allowed (_on nightly_) and no longer allowed in my proposal:

```rust
auto trait AutoTrait {}

trait MyTrait {}
impl AutoTrait for dyn MyTrait {}  // NOT ALLOWED

impl<T: ?Sized> AutoTrait for T {}  // NOT ALLOWED
```

(`impl<T> AutoTrait for T {}` remains allowed.)

After this change, traits with a default impl are implemented for a trait object **if and only if** the autotrait is one of the trait object's trait bounds (or a supertrait of a bound). In other words `dyn Trait + AutoTrait` always implements AutoTrait while `dyn Trait` never implements AutoTrait.

Fixes dtolnay/async-trait#228.

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system A-typesystem Area: The type system B-unstable Blocker: Implemented in the nightly compiler and unstable. C-future-compatibility Category: Future-compatibility lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

No branches or pull requests