'overflow evaluating the requirement' when deriving debug for item that contains itself

[sornas]

trafficstars

the following code fails to compile

#[derive(derive_more::Debug)]
struct Message {
    inner: Box<Message>,
}

fn main() {}

error[E0275]: overflow evaluating the requirement `Message: Debug`
 --> src/main.rs:1:10
  |
1 | #[derive(derive_more::Debug)]
  |          ^^^^^^^^^^^^^^^^^^
  |
  = note: required for `Box<Message>` to implement `Debug`
  = help: see issue #48214
  = note: this error originates in the derive macro `derive_more::Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

the same error is reported with stuff like Vec<Message> and with enum instead of struct.

$ cargo --version --verbose
cargo 1.78.0 (54d8815d0 2024-03-26)
release: 1.78.0
commit-hash: 54d8815d04fa3816edc207bbc4dd36bf18014dbc
commit-date: 2024-03-26
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Arch Linux Rolling Release [64-bit]
$ cat Cargo.toml
[package]
name = "derive-more-debug"
version = "0.1.0"
edition = "2021"

[dependencies]
derive_more = { version = "1.0.0-beta.6", features = ["debug"] }

[sornas]

expanding the derive with cargo expand shows

struct Message {
    inner: Box<Message>,
}
#[automatically_derived]
impl ::core::fmt::Debug for Message
where
    Box<Message>: ::core::fmt::Debug,
{
    fn fmt(
        &self,
        __derive_more_f: &mut ::core::fmt::Formatter<'_>,
    ) -> ::core::fmt::Result {
        let inner = &&self.inner;
        ::core::fmt::DebugStruct::finish(
            ::core::fmt::DebugStruct::field(
                &mut ::core::fmt::Formatter::debug_struct(__derive_more_f, "Message"),
                "inner",
                inner,
            ),
        )
    }
}
fn main() {}

error[E0275]: overflow evaluating the requirement `Message: Debug`
 --> src/main.rs:7:5
  |
7 |     Box<Message>: ::core::fmt::Debug,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: required for `Box<Message>` to implement `Debug`
  = help: see issue #48214

i don't think derive_more can figure out blanket impls such as impl<T> Debug for Box<T>. for std they can of course be special cased, but not in general.

[sornas]

the linked rust issue is https://github.com/rust-lang/rust/issues/48214. interestingly, with the feature trivial_bounds (and using nightly), the cargo expanded code compiles, but not the derive(derive_more::Debug). but i'd like to stay on stable anyway :-)

[tyranron]

@sornas doesn't this code introduce an unbounded recursion?

[JelteF]

I think @sornas oversimplified the example a bit too much, because indeed you wouldn't be able to ever construct such a struct afaict. But something like this is completely valid as a kind of linked list and has the same issue where derive_more::Debug fails, but std::Debug succeeds just fine.

#[derive(Debug)]
pub struct Item {
    pub next: Option<Box<Item>>,
}

fn main() {
    println!("{:?}", Item{next: None})
}

So this seems quite a problematic about the bounds the we add with our current implementation of the Debug derive.

A few options on how to address this:

1. Wait for https://github.com/rust-lang/rust/issues/48214 to be resolved
2. Detect when we add self-referential bounds and don't add those.

I think 1 is not really an option, given how little attention that issue has gotten.

2 seems fairly easy to do, but then the question becomes what do we do instead. A few options for those again:

1. Don't do anything and hope for the best.
2. Replace the self-reference with dyn Debug
3. Replace the self-reference with Box<dyn Debug>

I'm not sure which of these is best.

I kinda like 2, but I worry there are some cases that won't work with this.

3 seems a little safer, because it will also be Sized. But it seems annoying to have to rely on Box for that.

1 at least won't regress with regards to std, but we might get similar errors to std when generating code because it doesn't add correct bounds. Still that seems a lot better than what we have now, because now we throw errors for things that work fine doesn't throw errors for.

For reference this is the code I was playing around with when trying what bounds do work correctly:

use std::fmt;
use std::fmt::Debug;

pub struct Item {
    pub next: Option<Box<Item>>,
}

impl fmt::Debug for Item
where Option<Box<dyn Debug>>: Debug{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("Item")
         .field("next", &self.next)
         .finish()
    }
}

fn main() {
    println!("{:?}", Item{next: Some(Box::new(Item {next: None}))})
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5cf8f07f03511bc3084f139a7a3e5978

[tyranron]

@JelteF

I kinda like 2, but I worry there are some cases that won't work with this.

Yes, we cannot reliably detect such cases by the type name only, because there are aliases and import renames:

#[derive(Debug)]
pub struct Item {
    // No way we can detect this is the same type while expanding a macro.
    pub next: Option<Box<Another>>,
}

pub type Another = Item;

However, instead of messing with types, maybe it's worth to add user the ability to overwrite the generated bounds. Recalling the context of the question, we made #[debug(bound(...)] attribute to work in an additive way, by adding additional ones to the ones generated by the macro already. This has a lot of sense, given the rationale described behind the link. However, there are still could be cases where we do want to get rid of auto-generated bounds, and this issue is only one example:

#[derive(Debug)]
#[debug(replace(bound()))] // hypotetical syntax
pub struct Item {
    pub next: Option<Box<Item>>,
}

expands to

impl fmt::Debug for Item { // all the bounds replaced with nothing
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("Item")
         .field("next", &self.next)
         .finish()
    }
}

Another possible application of replace(bound(...)) is when private types are leaked:

#[derive(Debug)]
struct Private<T>(T);

#[derive(Debug)]
pub struct Public<T>(Private<T>);

At the moment, the expansion looks like:

impl<T> fmt::Debug for  Public<T> 
where Private<T>: fmt::Debug // oops private type is leaked into public impl, it may not compile!
{}

But having an ability to replace the bounds we could just use:

#[derive(Debug)]
#[debug(replace(bound(T: Debug)))]
pub struct Public<T>(Private<T>);

and the expansion will work:

impl<T> fmt::Debug for  Public<T> 
where T: Debug 
{}

[JelteF]

I kinda like 2, but I worry there are some cases that won't work with this.

Yes, we cannot reliably detect such cases by the type name only, because there are aliases and import renames:

I mainly meant that dyn Debug might not work for some cases. I didn't think of aliases, but I honestly don't think that would be super common problem. Now I think, we should automatically remove non-aliased self-referential bounds. Because those bounds will always cause compilation failures, so there's no point in adding them.

I think removing is good enough, because indeed users can add any necessary bounds manually, if a bound is still needed for successful compilation. I think the only bounds we should add by default are ones that we know are necessary.

However, instead of messing with types, maybe it's worth to add user the ability to overwrite the generated bounds.

I totally agree that this makes sense as a feature. But I wouldn't consider it a 1.0.0, so let's make that a separate issue.

[tyranron]

@JelteF

I think removing is good enough

Maybe we could do even better. I think we can omit generating trait bounds at all, unless some type parameter participates in the field type. And, we can do it fairly well, considering the fact that, unlike an arbitrary type, a type parameter cannot be aliased or renamed anyhow.

[JelteF]

Yeah I think that would be even better

[JelteF]

Re-opening this as we still need to apply the same fix from #371 to the Display derive (#371 only touched the Debug derive).