magiclen / educe

This crate offers procedural macros designed to facilitate the swift implementation of Rust's built-in traits.
MIT License
131 stars 11 forks source link

Wrong bounds applied to derived impls #17

Closed ijackson closed 5 months ago

ijackson commented 8 months ago

The documentation says:

Generic parameters will be automatically bound to the Clone trait if necessary.

This suggests a precise bound. In fact the bound seems to be ad-hoc and sometimes wrong. For example:

#[derive(Educe)]
#[educe(Clone)]
pub struct Outer<T>(Option<T>);

This expands to:

impl<T> ::core::clone::Clone for Outer<T> {

(ie, without any bound) and obviously the implementation doesn't compile:

3 | #[derive(Educe)]
  |          ^^^^^ the trait `Clone` is not implemented for `T`
  |
  = note: required for `Option<T>` to implement `Clone`
  = note: this error originates in the derive macro `Educe` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider restricting type parameter `T`
  |
5 | pub struct Outer<T: std::clone::Clone>(Option<T>);
  |                   +++++++++++++++++++

I think there are about three reasonable behaviours:

  1. Never add bounds; ask the user to supply bound if one is needed. This is the default behaviour in educe 0.4.x. It has the virtues that the user has precise control, and if the user forgets, it doesn't compile.
  2. Add bounds for every generic parameter. This is the behaviour of std's derives, and of educe 0.4.x with #[educe(Clone(bound))].
  3. Add precise bounds for every field, ie in the above, you'd end up with something like ... where Option<T>: Clone.

I haven't looked at the source code in educe 0.5 but I think it may be adding bounds for a generic parameter T iff the T appears as such in a field, but not if it is inside some other type. That is quite a surprising behaviour.

Where the user wants to supply precise bounds, or none at all, with educe 0.5, the user can write #[educe(Clone(bound()))] or #[educe(Clone(bound(.....)))]. But a call site that previously used #[educe(Clone(bounds))], with educe 0.4, is not straightforward to convert. I think at the very least it would be nice to have a syntax that requested behaviour (2) - adding bounds for every generic parameter.

I thini ideally the default would be precise bounds ((3) in my list).

I've also noticed that the default bounds can be influenced by whether a method is supplied. For example:

#[derive(Educe, Debug)]
#[educe(Clone)]
pub struct Manual<T> {
//  whether Manual<T>: Clone has a T: Clone bound seems to depend on this
//    #[educe(Clone(method = "our_clone"))]
    field: T,
}

fn our_clone<T>(_input: &T) -> T {
    panic!()
}

This seems quite surprising.

If we do suppress the automatic bounds in this case, we should ideally provide a way to supplement the automatic bounds, as otherwise the user must write all the correct automatic bounds out again.

I think that would probably be a breaking change.

ijackson commented 8 months ago

I have discovered that the same machinery which generates the wrong bounds can also generate wrong lists of generics for Debug helper structs:

use educe::Educe;
use std::fmt::{self, Debug};

#[derive(Educe)]
#[educe(Debug(bound = "X: Debug"))]
pub struct Thing<X> {
    #[educe(Debug(method = "no_fmt"))]
    field: Box<X>,
}

fn no_fmt<Q>(_: &Q, _f: &mut fmt::Formatter) -> fmt::Result {
    Ok(())
}

fn main() {
    let a = Thing {
        field: Box::new(42),
    };
    println!("{:?}", &a);
}

Produces:


error[E0401]: can't use generic parameters from outer item
 --> src/main.rs:8:16
  |
4 | #[derive(Educe)]
  |               - help: try introducing a local generic parameter here: `X,`
5 | #[educe(Debug(bound = "X: Debug"))]
6 | pub struct Thing<X> {
  |                  - type parameter from outer item
7 |     #[educe(Debug(method = "no_fmt"))]
8 |     field: Box<X>,
  |                ^ use of generic parameter from outer item

I think the find_idents_in_type function is fundamentally misconceived. I'm not sure it's even possible to reliably do what it is trying to do, in a proc macro. I will send an MR to do things a different way, which I think should be relatively straightforward.

dzmitry-lahoda commented 5 months ago

we also has issues

    fn print_bytes(bytes: &[u8], f: &mut core::fmt::Formatter) -> core::fmt::Result {
        println!("{:?}", bytes);
        Ok(())
    }

    #[derive(Educe)]
    #[educe(Debug)]
    pub struct Bytes<const N: usize>(#[educe(Debug(method(print_bytes)))] pub [u8; N]);

    panic!("{:?}", Bytes([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]));
error[E0401]: can't use generic parameters from outer item
   --> :140:84
    |
138 |     #[derive(Educe)]
    |                   - help: try introducing a local generic parameter here: `N,`
139 |     #[educe(Debug)]
140 |     pub struct Bytes<const N: usize>(#[educe(Debug(method(print_bytes)))] pub [u8; N]);
    |                            - const parameter from outer item                       ^ use of generic parameter from outer item
dzmitry-lahoda commented 5 months ago

@ijackson I saw you did many PRs and merging of these seems not happen. So do you have some branch in fork with all your fixes? Thank you