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

Only put Debug-like bounds on type variables #371

Merged

Conversation

sornas
Copy link
Contributor

@sornas sornas commented Jun 18, 2024

Resolves #363

Well, at least it's a suggestion for a resolution :p

Synopsis

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

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

expands into something like

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.

Status

I should add more tests, probably? I've at least verified the case mentioned above (for tuple structs, data structs and
enums). I must admit I haven't really looked in the documentation, but I should probably edit that too.

Checklist

  • Documentation is updated (not required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (not required)

@sornas sornas force-pushed the debug-bounds-only-participating-type-variables branch 2 times, most recently from 776409d to f13c7d9 Compare June 18, 2024 17:36
@sornas sornas force-pushed the debug-bounds-only-participating-type-variables branch from f13c7d9 to 16713df Compare June 18, 2024 17:41
path.segments.iter().any(|segment| {
self.type_variables
.iter()
.any(|type_var| *type_var == &segment.ident)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is outside my Rust knowledge, but I'm starting to think this is wrong. If the type is a path (like std::vec::Vec), then it cannot contain a type variable, correct? We only care if it is the only segment or itself a type variable (T in std::vec::Vec<T>) at some part of the path.

Copy link
Owner

@JelteF JelteF Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, correct. I think an easy way to test that would be:

#[derive(derive_more::Debug)]
struct Vec {
    next: std::vec::Vec<i32>,
}

syn::Type::Infer(_) => false,
syn::Type::Macro(_) => false,
syn::Type::Never(_) => false,
syn::Type::TraitObject(_) => false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trait objects can contain type parameters:

trait MyTrait<T> {}

struct Test<T>
{
    t: Box<dyn MyTrait<T>>,
}

Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This filtering logic should also apply to the Display derive, not just to the Debug one.

@JelteF
Copy link
Owner

JelteF commented Jul 3, 2024

@sornas I have some time to work on derive_more the next ~2 weeks. Since this is one of the last items that's blocking 1.0, I was wondering if you'll have time to work on this during that time. Otherwise I think I might pick it up to get it over the finish line, if I'm able to get the other 1.0 blockers done.

@sornas
Copy link
Contributor Author

sornas commented Jul 3, 2024

unfortunately i don't think i'll have time, so feel free to take over. but i'll let you know here if it changes.

@JelteF JelteF added this to the 1.0.0 milestone Jul 4, 2024
@JelteF JelteF force-pushed the debug-bounds-only-participating-type-variables branch from 6968c49 to 659dfdb Compare July 5, 2024 21:05
@JelteF JelteF force-pushed the debug-bounds-only-participating-type-variables branch from 659dfdb to 6131444 Compare July 5, 2024 21:16
@JelteF JelteF marked this pull request as ready for review July 19, 2024 01:35
@JelteF JelteF requested a review from tyranron July 19, 2024 01:35
@tyranron tyranron merged commit 162535e into JelteF:master Jul 19, 2024
17 checks passed
@tyranron tyranron mentioned this pull request Jul 19, 2024
JelteF added a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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>
@link2xt
Copy link

link2xt commented Aug 3, 2024

This change resulted in a regression, after updating from 1.0.0-beta.6 to 1.0.0-beta.7 compilation fails in deltachat/deltachat-core-rust#5850

@JelteF
Copy link
Owner

JelteF commented Aug 3, 2024

Hmm, that's not great. Can you make a new issue for this that shows some code that now fails compilation?

@link2xt
Copy link

link2xt commented Aug 3, 2024

Update: seems to be fixed (or worked around?) in n0-computer/iroh#2578

@link2xt
Copy link

link2xt commented Aug 3, 2024

I opened an issue #392

tyranron pushed a commit that referenced this pull request Aug 5, 2024
Resolves #392
Related to #371

## Synopsis

Before #371 this code would compile without any issues, but that would
actually
hide the problem that the resulting `Debug` implementation was never
applicable, because the bounds could not be satisfied.

## Solution

This problem was already solved by #371, but this adds a test case to
ensure
that we don't regress here again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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