rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.24k stars 12.57k forks source link

#[derive] sometimes uses incorrect bounds #26925

Open comex opened 9 years ago

comex commented 9 years ago

In the following code:

#[derive(Copy, Clone)]
struct Y<T>(&'static fn(T));

both derives expand to impls that require the corresponding trait to be implemented on the type parameter, e.g.:

#[automatically_derived]
impl <T: ::std::marker::Copy> ::std::marker::Copy for Y<T> where
 T: ::std::marker::Copy {
}

However, this isn't actually necessary, as Y<T> will still be eligible for Copy regardless of whether T is.

This may be hard to fix, given the compilation phase at which #[derive] works...

simonbuchan commented 4 years ago

I think I would be a lot happier with the current situation with "just" adding more information to the compiler diagnostic.

At the moment, you get something like:

error[E0599]: no method named `clone` found for type `Ptr<Foo>` in the current scope
 --> src/main.rs:9:15
  |
2 | struct Ptr<T>(*mut T);
  | ---------------------- method `clone` not found for this
...
9 |     let b = a.clone();
  |               ^^^^^ method not found in `Ptr<Foo>`
  |
  = note: the method `clone` exists but the following trait bounds were not satisfied:
          `Ptr<Foo> : std::clone::Clone`
  = help: items from traits can only be used if the trait is implemented and in scope
  = note: the following trait defines an item `clone`, perhaps you need to implement it:
          candidate #1: `std::clone::Clone`

or

error[E0382]: use of moved value: `a`
  --> src/main.rs:10:15
   |
8  |     let a = Ptr(&mut foo);
   |         - move occurs because `a` has type `Ptr<Foo>`, which does not implement the `Copy` trait
9  |     let b = a;
   |             - value moved here
10 |     let ptr = a.0;
   |               ^^^ value used here after move

Neither of these tell you that the reason Ptr doesn't implement Copy or Clone, despite #[derive(Copy, Clone)] being right there. I think some logic to add another sentence for this would get rid of the case where derive doesn't implement when it should, which I think newer users (like me!) would be more likely to hit.

jesperdj commented 4 years ago

Hello, I am a new user of Rust.

I noticed that when I #[derive(Copy, ...)] on a struct with a type parameter, it does not place a bound on the type parameter implicitly. Instead, if I use the struct with a concrete type that is non-copy, my wrapper will be non-copy as well, despite the #[derive(Copy, ...)]:

#[derive(Copy, Clone, Debug)]
struct Wrapper<T>(T);

#[derive(Debug)]
struct NonCopy();

fn main() {
    // Works fine, type T is not restricted
    let x = Wrapper(NonCopy());

    // Actually a move, not a copy
    let y = x;

    // Trouble here, x was moved instead of copied
    println!("{:?}, {:?}", x, y);
}

Fine, I understand that it works like this.

My point is about the documentation. The docs of std::marker::Copy say this, which is looks wrong / out-of-date:

There is a small difference between the two: the derive strategy will also place a Copy bound on type parameters, which isn't always desired.

This does not seem to be the case (anymore).

tyranron commented 4 years ago

@jesperdj it was about impl blocks, not the type definition.

#[derive(Copy)]
struct Wrapper<T>(T);

expands to

struct Wrapper<T>(T);

impl<T: Copy> Copy for Wrapper<T> { ... }

... but sometimes (for example, when using PhantomData) you need just:

impl<T> Copy for Wrapper<T> { ... }

In such cases you need to provide a manual impl.

A1-Triard commented 4 years ago

In such cases you need to provide a manual impl.

When using PhantomData derivative crate works pretty well.

Boscop commented 4 years ago

When using PhantomData derivative crate works pretty well.

Another tip: A common situation is that you have a member of type Option<T> or Vec<T> and you derive Default on the struct, and you want to struct to be Default even when T isn't. Then you just need to switch to derive(SmartDefault).

WaffleLapkin commented 4 years ago

I've found a problem that is somehow different from most of the mentioned issues, though still related.

For a generic struct:

struct Wrap<T> { /* ... */ }

You can implement Partial{Eq,Ord} in 2 ways.

First (1):

impl<T: PartialEq> PartialEq<Wrap<T>> for Wrap<T> { /* ... */ }

Second (2):

impl<T: PartialEq<U>, U> PartialEq<Wrap<U>> for Wrap<T> { /* ... */ }

The (1) allows type inference based on Partial{Eq,Ord} (e.g.: assert_eq!(s.parse(), Ok(X))), while (2) allows to compare different types (e.g.: Wrap<&str> and Wrap<String>).

The #[derive] currently produces (1)-impl, so in case you want (2) you need either custom derive (like derivative crate) or implement the trait(s) by hand.

But there is a catch: if you implement PartialEq (not Ord though) by hand, then you lose the StructuralEq StructuralPartialEq implementations, meaning you can't match on consts:

const W: Wrap<i32> = Wrap(1);

match Wrap(1) {
    W => {},
    // error: to use a constant of type `Wrap` in a pattern, `Wrap` must be annotated with `#[derive(PartialEq, Eq)]`
    //   --> src/main.rs:18:9
    //    |
    // 18 |         W => {},
    //    |         ^
    _ => {},
}

(playground)

So a real solution (some markers those bounds to loose?) may only be in std/compiler.

ishitatsuyuki commented 4 years ago

I don't know what rustc used to be but it seems that today rustc will happily accept bounds like where String: PartialEq<T>? If so, what's blocking this from being implemented?

Edit: It seems that we can't expose private types in bounds and therefore just writing bounds like above won't work. https://github.com/rust-lang/rust/issues/26925#issuecomment-129698062

cuviper commented 4 years ago

Even if they're public types, it's questionable to expose the types of private fields in the constraints.

lukaslueg commented 3 years ago

This came up in #80567. Is there a policy regarding manual implementations in stdlib due to this problem with derive?

Krantz-XRF commented 3 years ago

In Haskell, GHC has an extension called StandaloneDeriving, which allows manually specifying "trait bounds" (instance constraints in Haskell) for an automatically derived instance:

-- FlatOrdered ignores the existing order on a certain type
data FlatOrdered a = FlatOrdered a deriving Show
instance Eq (FlatOrdered a) where
  _ == _ = True
instance Ord (FlatOrdered a) where
  _ <= _ = False

-- Then we use it to ignore ordering on a certain field
data T a = T
  { ordered :: Int
  , unordered :: FlatOrdered a
  } deriving Show            -- automatically derived: instance Show a => Show (T a)
deriving instance Eq (T a)   -- constraint removed, instead of: Eq a => Eq (T a)
deriving instance Ord (T a)  -- constraint removed, instead of: Ord a => Ord (T a)

Then we have

> T 2 (FlatOrdered 3) == T 2 (FlatOrdered 4)
True
> T 2 (FlatOrdered 3) <= T 2 (FlatOrdered 4)
True
> T 2 (FlatOrdered 3) >= T 2 (FlatOrdered 4)
True

The point here is, rustc can do exactly the same thing: allow the user to customize the trait bounds for a certain derived trait impl. When the compiler sees such a customized derive, it mechanically applies the trait bound (to the automatically generated impl block), but does not alter the implementation code for the methods. Whether or not the specified trait bounds are legal can be checked in a later compilation phase.

I know what I demonstrated above can be done with derivative. But it is just a rough illustration. The above approach (1) achieves the goal, (2) avoids procedural macros, and (3) should be rather easy to implement in the compiler.

josephg commented 3 years ago

Just to pile on, I've been trying to use traits for configuration of a set of behaviour, and it breaks. Worse, it breaks silently:

use std::fmt::Debug;

trait SomeTrait {
    type Foo: Debug;
}

// This should always be Debug because S::Foo is Debug.
#[derive(Debug)]
struct SomeStruct<S: SomeTrait>(S::Foo);

fn foo<S: SomeTrait>(s: SomeStruct<S>) {
    println!("{:?}", s); // Error: `S` cannot be formatted using `{:?}` because it doesn't implement `Debug`
}

Playground link

Logarithmus commented 3 years ago

@comex what is the current blocker for fixing this issue? I haven't find any PR's trying to implement it.

agausmann commented 3 years ago

@Logarithmus As far as I can tell, the blocker is that this is still in "design" stage, it's still an open problem waiting for a good solution. I have a rough idea for a solution, but there's a few things I still need to work out before I'd consider it solid.

let4be commented 3 years ago

Is there any 3rd party crate that helps to Derive proper clone implementations in the mean time?...

I'd be happy to anything that simply calls .clone() on each and every field in struct(except fields with implemented Copy)... My code heavily uses generics and I would like to avoid imposing Clone restrictions on all my generic params(they don't need it and it would confuse users of my library, all of those types are already either under Arc or Rc)

eggyal commented 3 years ago

@let4be see https://github.com/rust-lang/rust/issues/26925#issuecomment-299709166 and https://github.com/rust-lang/rust/issues/26925#issuecomment-313892553 and https://github.com/rust-lang/rust/issues/26925#issuecomment-394190748 and https://github.com/rust-lang/rust/issues/26925#issuecomment-500650321

Kixunil commented 1 year ago

Does seven years of this being open convince the Rust team that maybe adding the option to define custom bounds is less bad than waiting for a miracle Chalk while forcing people to manually implement a bunch of traits on large structs in security-critical projects (so additional dependencies are highly undesirable)?

Perhaps at least minimal attribute #[expose] that would use T::HowThisIsUsedInTheField: Trait which knowingly, intentionally leaks the implementation of the struct (user acknowledges this when writing the attribute).

If yes, I'm willing to take a look into implementing it.

nikomatsakis commented 1 year ago

I'm personally convinced that we should add some form of annotation -- there have been a number of RFCs on the topic -- I think the challenge is finding one that folks are happy with. I seem to recall @cramertj working with an RFC last year on this topic?

(That said, I think we know how to do 'perfect derive' in a technical sense now, but there are interesting semver-related questions on whether we should do so...I still think we should, but maybe only over an edition.)

Kixunil commented 1 year ago

@nikomatsakis thanks for explaining! Given that there are already bunch of semver hazards wouldn't it be best to have a tool that can detect SemVer breakages and encourage lib authors to run it in CI? I think I've seen such tool somewhere already so if there's a way to make it more available that might resolve the problem.

But perfect_derive attribute isn't too bad either. I know it's useful because I have some code where it would already work exactly as I need.

fmease commented 1 year ago

I'd wager an item-specific #[perfect_derive] attribute would be quite useful & ergonomic in combination with restrictions (RFC 3323) irrespective of whether we add a field-specific #[expose] attribute or not. Imagine perfectly deriving a crate-local trait implementation without much ado (which obviously doesn't entail any SemVer hazards). Not that RFC 3323 mentions anything about deriving with visibility restriction (like the syntax) but it is certainly a future possibility.

dhardy commented 1 year ago

I'm personally convinced that we should add some form of annotation

I'm very happy with the syntax I went with in impl-tools:

#[autoimpl(Clone, Default where A: trait, B: trait)]
#[autoimpl(Debug ignore self.c where A: Debug)]
struct X<A, B: Debug, C> {
    a: A,
    b: B,
    c: PhantomData<C>,
}

This uses the special "keyword" trait and does not apply any bounds on generics by default, thus unfortunately it's incompatible with the current derive syntax.

micolous commented 1 year ago

It's not clear to me which PR changed this, but it looks like rustc >= 1.66.0 has made it so deriving Default for enums with a type parameter (https://github.com/rust-lang/rust/issues/99463 [^1]) no longer requires T: Default when T is not used in the #[default] value.

As a simplified example, Foo<T> re-implements Option<T>, and derives Default rather than explicitly implementing it as Option does:

#[derive(Debug, Default)]
enum Foo<T> {
    #[default]
    None,
    Some(T),
}

#[derive(Debug)]
struct Bar {
    v: u8,
}

fn main() {
    let a: Foo<Bar> = Default::default();
    let b = Foo::Some(Bar { v: 1 });

    println!("{a:?}, {b:?}");
}

On rustc 1.65.0 that fails with a compile error:

error[E0277]: the trait bound `Bar: Default` is not satisfied
  --> ./defaults.rs:14:23
   |
14 |     let a: Foo<Bar> = Default::default();
   |                       ^^^^^^^^^^^^^^^^ the trait `Default` is not implemented for `Bar`
   |
   = help: the trait `Default` is implemented for `Foo<T>`
note: required for `Foo<Bar>` to implement `Default`
  --> ./defaults.rs:1:17
   |
1  | #[derive(Debug, Default)]
   |                 ^^^^^^^
   = note: this error originates in the derive macro `Default` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `Bar` with `#[derive(Default)]`
   |
9  | #[derive(Default)]
   |

[^1]: correction: my example is actually subtly different

jhpratt commented 1 year ago

@micolous That is intentional. The previous behavior that included the Default bound was a bug, as the RFC clearly stated the bound would not be present.

micolous commented 1 year ago

@micolous That is intentional. The previous behavior that included the Default bound was a bug, as the RFC clearly stated the bound would not be present.

Thanks for confirming. I agree the old behaviour was unexpected; it just seemed like was related to this issue (based on https://github.com/rust-lang/rust/issues/99463 duping against this one) and wasn't clear what exactly changed here (as this is still open) that this should now "suddenly work". 😅

I had derived Default because of Clippy's derivable_impls lint, but then due to some (unrelated) CI shenanigans, it looks like my CI setup wasn't actually testing against rustc < 1.66.0.

Digging into it some more now, it looks like my example is subtly different to https://github.com/rust-lang/rust/issues/99463, so deriving Default for a struct with a type parameter and an Option<T> (or Foo<T> with my example above) field still requires T: Default, even on rustc 1.68.2 (stable) and 1.70.0-nightly (4087deacc 2023-04-12)... so that explains why this is still open. 😄

jhpratt commented 1 year ago

It is related to the issue in a way, just not in any manner that can be generalized. The only thing that changed was #[derive(Default)] on enums, as it was explicitly accepted.

For what it's worth, the disagreement on bounds is the sole reason that #[default] can't be placed on an enum variant with data.

ggutoski commented 1 year ago

Yet another example from associated types: Playground