Skip to content

derive Debug incorrect assumption #52560

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

Open
axos88 opened this issue Jul 20, 2018 · 6 comments
Open

derive Debug incorrect assumption #52560

axos88 opened this issue Jul 20, 2018 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@axos88
Copy link

axos88 commented Jul 20, 2018

Hello,

I'm guessing there "might" be something wrong how the Debug trait is derived in case a type is owned but not actually needed for formatting the object. The following code fails because B is not necessarily Debug which is correct, however since Foo doesn't actually have an instance of B as a field, that shouldn't be needed, and B::Item is restricted to Debug:

#[derive(Debug)]
struct Foo<B: Bar>(B::Item);


trait Bar {
    type Item: Debug;
}

fn foo<B: Bar>(f: Foo<B>) {
    println!("{:?}", f);
}


struct ABC();

impl Bar for ABC {
    type Item = String;
}


fn main() {
    foo(Foo::<ABC>("a".into()));
}

error[E0277]: `B` doesn't implement `std::fmt::Debug`
  --> src/main.rs:65:22
   |
65 |     println!("{:?}", f);
   |                      ^ `B` cannot be formatted using `:?` because it doesn't implement `std::fmt::Debug`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `B`
   = help: consider adding a `where B: std::fmt::Debug` bound
   = note: required because of the requirements on the impl of `std::fmt::Debug` for `middlewares::authentication::Foo<B>`
   = note: required by `std::fmt::Debug::fmt
@ExpHP
Copy link
Contributor

ExpHP commented Jul 20, 2018

This is simply how derive works, as annoying as it may seem. derive-ing is performed by what are essentially macros; it is a process that happens fully at a syntactical level, using only the definition of the struct. (which is convenient for making custom derives possible)


The problem is, given an arbitrary struct definition,

struct Foo<A, B, C> {
    stuff: Pair<A, B>,
    thing: Builder<B>,
    marker: PhantomData<C>,
}

the derive macro for Debug wants to generate a definition that looks like this:

impl<A, B, C> Debug for Foo<A, B, C>
where ?????
{
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_struct("Foo")
            .field("stuff", &self.stuff)
            .field("thing", &self.thing)
            .field("marker", &self.marker)
            .finish()
    }
}

but the question is, what should be used for the where bounds?

At this point we don't know anything else about the types defined here; we don't know what Pair<A, B> looks like, or what the bounds on the Debug impl for PhantomData look like. So there's basically two options here: A technically-correct way that exposes internal implementation details (and possibly puts private types in a public signature):

where // require each field to impl Debug
    Pair<A, B>: Debug,
    Builder<B>: Debug,
    PhantomData<C>: Debug,

or a conservative guess that is correct in 90% of use-cases:

where // require each type param to impl Debug
    A: Debug, B: Debug, C: Debug

rust goes with the second option.


No doubt, it is an annoying limitation for many people when they first run into it, and there have been discussions about possible ways to improve the situation, such as in this RFC.

@estebank
Copy link
Contributor

estebank commented Jul 20, 2018

error[E0277]: `B` doesn't implement `std::fmt::Debug`
  --> src/main.rs:65:22
   |
65 |     println!("{:?}", f);
   |                      ^ `B` cannot be formatted using `:?` because it doesn't implement `std::fmt::Debug`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `B`
   = help: consider adding a `where B: std::fmt::Debug` bound
   = note: required because of the requirements on the impl of `std::fmt::Debug` for `middlewares::authentication::Foo<B>`
   = note: required by `std::fmt::Debug::fmt

Should at least be

error[E0277]: `B` doesn't implement `std::fmt::Debug`
  --> src/main.rs:65:22
   |
64 | fn foo<B: Bar>(f: Foo<B>) {
   |        ------ help: add a `Debug` bound: `B: Bar + std::fmt::Debug`
65 |     println!("{:?}", f);
   |                      ^ `B` cannot be formatted using `:?` because it doesn't implement `std::fmt::Debug`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `B`
   = note: required because of the requirements on the impl of `std::fmt::Debug` for `middlewares::authentication::Foo<B>`
   = note: required by `std::fmt::Debug::fmt

Edit: this is handled in #65192:

error[E0277]: `B` doesn't implement `std::fmt::Debug`
  --> file9.rs:12:22
   |
11 | fn foo<B: Bar>(f: Foo<B>) {
   |        -- help: consider further restricting this bound: `B: std::fmt::Debug +`
12 |     println!("{:?}", f);
   |                      ^ `B` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `B`
   = note: required because of the requirements on the impl of `std::fmt::Debug` for `Foo<B>`
   = note: required by `std::fmt::Debug::fmt`

@estebank estebank added the A-diagnostics Area: Messages for errors, warnings, and lints label Jul 20, 2018
@axos88
Copy link
Author

axos88 commented Jul 22, 2018

@ExpHP I see your point, however in this case there is no field that would even have B as the type parameter, we only have a field with type B::Item

@ExpHP
Copy link
Contributor

ExpHP commented Jul 22, 2018

Indeed, for your specific struct, it does seem to be possible to generate an impl without any knowledge of the type system, and which is guaranteed to be correct without the risk of possibly exposing a private type in a public signature:

use std::fmt;
impl<B: Bar<Item=Bi>, Bi: fmt::Debug> fmt::Debug for Foo<B> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_tuple("Foo").field(&self.0).finish()
    }
}

This impl is possible to generate based on the facts that:

  • There is only one trait bound on B (Bar); therefore, the associated type Item must belong to that trait.
  • An associated item like Bar::Item must have the same visibility as Bar, which already appears in the type's signature.

Support for generating impls like this is a change that could go through the RFC process.

Personally speaking, I would not be too enthusiastic about it due to rather general issues (I see it providing little value to offset the base cost that comes with any feature like this, which is that the presence of a supported use case constrains future design decisions)

@ExpHP
Copy link
Contributor

ExpHP commented Jul 22, 2018

As an aside, I think that it is generally better when possible to write the following

// use as Foo<B::Item>
struct Foo<B>(B);

The advantage of writing it this way is variance; the above form of Foo is covariant in B, meaning it can be "coerced" to have a shorter lifetime parameter. The way you have written Foo is invariant in B due to the appearance of an associated type, meaning that lifetime parameters are fixed.

Of course, this advice is not one-size-fits-all; in some cases it is desirable to have the Foos from different Bars to be considered as different types, in which case the way you wrote it is perfectly reasonable. Your definition is also reasonable if you only expect to support the Item: 'static use-case.

@axos88
Copy link
Author

axos88 commented Jul 24, 2018

In my case what the B::Item is is an implementation detail of B, and the user of Foo<B> only has to know what B[ackend] it wants to use with his Foo, doesn't really care what the item is until later after processing and the Foo<B> being been passed down to another layer.

But yeah, when possible, I agree.

@estebank estebank added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 25, 2019
@crlf0710 crlf0710 added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants