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

Fix ?Sized type parameters in Debug #289

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Fix ?Sized type parameters in Debug #289

merged 5 commits into from
Aug 14, 2023

Conversation

tyranron
Copy link
Collaborator

Synopsis

At the moment, derive_more::Debug fails to work with ?Sized generics:

#[derive(derive_more::Debug)]
struct UnnamedGenericStructUnsized<T: ?Sized>(T);

And generates the following error:

error[E0277]: the size for values of type `T` cannot be known at compilation time
   --> tests/debug.rs:845:14
    |
845 |     #[derive(Debug)]
    |              ^^^^^ doesn't have a size known at compile-time
846 |     struct UnnamedGenericStructUnsized<T: ?Sized>(T);
    |                                        - this type parameter needs to be `std::marker::Sized`
    |
    = note: required for the cast from `&T` to `&dyn Debug`
    = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider borrowing the value, since `&&T` can be coerced into `&dyn Debug`
    |
845 |     #[derive(&Debug)]
    |              +
help: consider removing the `?Sized` bound to make the type parameter `Sized`
    |
846 -     struct UnnamedGenericStructUnsized<T: ?Sized>(T);
846 +     struct UnnamedGenericStructUnsized<T>(T);
    |

At the same moment, std::Debug works OK:

#[derive(std::Debug)]
struct UnnamedGenericStructUnsized<T: ?Sized>(T);

If we look at its expansion:

#[automatically_derived]
impl<T: ::core::fmt::Debug + ?Sized> ::core::fmt::Debug for
    UnnamedGenericStructUnsized<T> {
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        ::core::fmt::Formatter::debug_tuple_field1_finish(f,
            "UnnamedGenericStructUnsized", &&self.0)
    }
}

We can see that std::Debug always uses fields as &&self.0, while we in our derive_more::Debug expansion use &self.0 only.

Solution

Simply use double-ref in the expansion, as the error suggests, and std::Debug does.

Checklist

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

@tyranron tyranron added this to the 1.0.0 milestone Aug 10, 2023
@tyranron tyranron self-assigned this Aug 10, 2023
@tyranron tyranron marked this pull request as draft August 10, 2023 11:14
@tyranron tyranron marked this pull request as ready for review August 10, 2023 11:31
@tyranron tyranron requested a review from JelteF August 10, 2023 11:31
@JelteF
Copy link
Owner

JelteF commented Aug 10, 2023

Does the same issue exist for other derives? Especially Display seems likely to have the same bug

@tyranron
Copy link
Collaborator Author

tyranron commented Aug 10, 2023

@JelteF seems no. The bug goes from the &dyn Debug usage in the debug building API. Display-like and other traits derives don't use such API atm.

I can add similar tests for other traits as a separate PR. Just to be sure.

@tyranron tyranron mentioned this pull request Aug 10, 2023
@tyranron
Copy link
Collaborator Author

@JelteF ping.

@tyranron tyranron merged commit 4aefdbb into master Aug 14, 2023
@tyranron tyranron deleted the fix-debug-unsized branch August 14, 2023 09:54
tyranron pushed a commit that referenced this pull request Jul 4, 2024
## Synopsis

While looking into #328 I realized the current situation around Pointer
derives
and references was even weirder because we store a double-reference to
the
fields in the local variables for Debug, but not for Display. The reason
we
were doing this was because of #289.


## Solution

This stops storing a double-reference, and only adds the additional
reference
in the places where its needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants