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

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

Closed
sornas opened this issue May 25, 2024 · 9 comments · Fixed by #371 or #387
Closed
Assignees
Milestone

Comments

@sornas
Copy link
Contributor

sornas commented May 25, 2024

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
Copy link
Contributor Author

sornas commented May 25, 2024

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
Copy link
Contributor Author

sornas commented May 25, 2024

the linked rust issue is rust-lang/rust#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
Copy link
Collaborator

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

@JelteF
Copy link
Owner

JelteF commented Jun 12, 2024

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 Tracking issue for RFC #2056: Allow trivial constraints to appear in where clauses  rust-lang/rust#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

@JelteF JelteF added this to the 1.0.0 milestone Jun 12, 2024
@tyranron
Copy link
Collaborator

@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
Copy link
Owner

JelteF commented Jun 12, 2024

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
Copy link
Collaborator

tyranron commented Jun 12, 2024

@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
Copy link
Owner

JelteF commented Jun 13, 2024

Yeah I think that would be even better

tyranron added a commit that referenced this issue Jul 19, 2024
## Synopsis

The problem, as reported in the issue, is that code like the following

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

expands into something like

```rust
impl std::fmt::Debug for Item where Item: Debug { /* ... */ }
```

which does not compile. This PR changes the Debug derive so it does not
emit those bounds.

## Solution

My understanding of the current code is that we iterate over all fields
of the struct/enum and add either a specific
format bound (e.g. `: fmt::Binary`), a default `: fmt::Debug` bound or
skip it if either it is marked
as `#[debug(skip)]` or the entire container has a format attribute. 

The suggested solution in the issue (if I understood it correctly) was
to only add bounds if the type is a type
variable, since rustc already knows if a concrete type is, say, `:
fmt::Debug`. So, instead of adding the bound for
every type, we first check that the type contains one of the container's
type variables. Since types can be nested, it
is an unfortunately long recursive function handling the different types
of types. This part of Rust syntax is probably
not going to change, so perhaps it is feasible to shorten some of the
branches into `_ => false`.

One drawback of this implementation is that we iterate over the list of
type variables every time we find a "leaf type".
I chose `Vec` over `HashSet` because in my experience there are only a
handful of type variables per container.

Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Kai Ren <tyranron@gmail.com>
@JelteF
Copy link
Owner

JelteF commented Jul 19, 2024

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

@JelteF JelteF reopened this Jul 19, 2024
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 20, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 24, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 24, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
JelteF added a commit that referenced this issue Jul 24, 2024
Resolves #363

Related to #371

Requires #377

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.
tyranron added a commit that referenced this issue Jul 25, 2024
Resolves #363
Requires #377
Follows #371

## Synopsis

The problem is that the `Display` derive adds bounds for all types
that
are used in the format string. But this is not necessary for types that
don't contain a type variable. And adding those bounds can result in
errors like for the following code:

```rust
#[derive(Display, Debug)]
#[display("{inner:?}")]
#[display(bounds(T: Display))]
struct OptionalBox<T> {
    inner: Option<Box<T>>,
}

#[derive(Display, Debug)]
#[display("{next}")]
struct ItemStruct {
    next: OptionalBox<ItemStruct>,
}
```

That code would generate the following error:

```text
error[E0275]: overflow evaluating the requirement `ItemStruct: derive_more::Display`
```

## Solution

This makes sure we don't add unnecessary bounds for Display-like
derives. It does so in the same way as #371 did for the Debug derive: By
only adding bounds when the type contains a type variable.

Co-authored-by: Kai Ren <tyranron@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment