rust-lang / libs-team

The home of the library team
Apache License 2.0
110 stars 18 forks source link

Customizing `#[derive(Debug)]` #334

Open clubby789 opened 5 months ago

clubby789 commented 5 months ago

Proposal

Problem statement

The current behaviour of the #[derive(Debug)] macro can be overly broad and not work for some use cases. Users with types that do not fit the derive are forced to either write verbose custom implementations (which they must remember to update when the type is updated), or a 3rd-party derive. These approaches both mean missing out on upstream optimizations to the macro.

Motivating examples or use cases

#[derive(Debug)]
struct FunctionBuilder<'cx> {
  cx: &'cx SomeVeryLargeContextType,  // Required for the struct, but unecessarily bloats debug output
  name: String,
  instructions: Vec<Instruction>,
}
#[derive(Debug)]
struct MyListForTraitImpls<T>(Vec<T>); // Should be displayed as a list of elements like `Vec`, but includes the `MyListForTraitImpls` type name
#[derive(Debug)]
struct CallbackRunner {
    f: Box<dyn Fn() -> usize>, // We don't want this displayed, but it will fail to compile as `dyn Fn() -> usize` is not `Debug`
    name: &'static str,
}

Solution sketch

Introducing a new #[debug(..)] attribute which can be applied to fields or types to customize the behavior of the derive.

skip

#[debug(skip)] on a field will cause it to not be emitted, and not require the type of that field to implement Debug.

Ideally,

#[derive(Debug)]
struct Xxx<T> {
    #[debug(skip)]
    something: T,
    /* other fields that don't use T */
}

should not place a : Debug bound on T.

transparent

Similarly to repr(transparent), #[debug(transparent)] may be placed on any type containing a single field to use the debug implementation of that field directly. The macro should expand to something like

/* ... */
Debug::fmt(self.0)
/* ... */

Other ideas

Other attributes that I am less commited to, but could be of use:

Alternatives

The main alternative is using a 3rd-party crate such as derivative. The reasoning against this is explained in the problem statement.

Links and related work

Derivative - One of the top 350 most downloaded crates on crates.io.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

Second, if there's a concrete solution:

Internals thread

joshtriplett commented 5 months ago

Copying a suggestion from the internals thread:

If skip is added, it should probably be spelled #[skip(Debug)] and be applicable to Eq, Ord and other "field-wise derivable" traits as well.

clubby789 commented 5 months ago

I went with #[debug] because of the precedent with #[default], but I think there's definitely a good case for that ordering if skip is extended to other derives.

jhpratt commented 5 months ago

Procedurally, is an ACP sufficient? This feels like a relatively large change.

Veykril commented 5 months ago

Copying a suggestion from the internals thread:

If skip is added, it should probably be spelled #[skip(Debug)] and be applicable to Eq, Ord and other "field-wise derivable" traits as well.

Note that the choice here has certain implications for the ecosystem. Going with a skip derive helper would imply that third-party derives should likely adopt this form as well over them using their own#[my_derive(skip)] helper.

clubby789 commented 5 months ago

Procedurally, is an ACP sufficient? This feels like a relatively large change.

I've added a link to the Pre-RFC I made on IRLO; Josh suggested filing an ACP so it could be nominated for discussion.

joshtriplett commented 5 months ago

@jhpratt I think an ACP is sufficient; this seems like a small change, which would be a relatively small PR, and the primary challenge is deciding "should we do this", which is what we have ACPs for.

scottmcm commented 5 months ago

Copying from IRLO:

jdahlstrom: If skip is added, it should probably be spelled #[skip(Debug)]

TBH, I prefer this phrasing. It makes the proposal "add a common skip attribute" instead of "add a bunch of customization knobs".

Then #[skip(Hash)] works for fields not worth hashing, and the compiler can do things like deny if you derive(PartialEq, Hash) but have skip(PartialEq), since that means that the Hash implementation is just wrong.


But that direction would implicitly be a "also, ecosystem, maybe you should support skip like this too", at which point an RFC becomes more useful.

dvdsk commented 5 months ago

I like the idea of a common skip attribute.

I am not sure about transparent for Debug. I like the Debug fmt to show the 'ugly' truth. In the mentioned use case it saves two lines. It might also look cleaner, but for a clean look shouldn't we rely on Display?

A similar concern around skipping. That is easily resolved by still showing the name of the field.

joshtriplett commented 5 months ago

We discussed this in today's @rust-lang/libs-api meeting.

We'd like to go ahead and approve #[skip(...)] to skip a field for field-wise derives.

We also felt that there should be some syntax to skip a field for all field-wise derives, without having to repeat the whole list. #[skip] could do that.

Something, probably rustc, needs to lint against un-handled arguments for skip, such as skip(Degub).

And it would probably also make sense to have a lint for the cases scottmcm mentioned: skipping a field for a subset of derives that seems unlikely to make sense, such as deriving more than one of Hash/PartialOrd/PartialEq` but skipping a field for some-but-not-all of those.

cuviper commented 5 months ago

How do you know it's unhandled if a 3rd party derive might use it? Or is the assertion that #[skip(_)] is only going to be allowed for builtins?

joshtriplett commented 5 months ago

@cuviper If skip is going to be shared across derives, one potential way would be to assume that the only arguments skip takes are the names of traits, so if it has an argument that's not the name of a trait being derived, lint.

Making it a lint and not an error also makes it easy enough to disable.

fmease commented 5 months ago

Technically speaking this is a breaking change though isn't it? If a pre-existing crate exposes a derive macro with a helper attribute skip and errors on #[skip(Debug)] for example, existing #[derive(Debug, Custom)] would suddenly break. Not sure if this would be the fault of the 3rd-party library?

I guess the same could've been said about Default's #[default].

Edit: Ah, furthermore, the feature gate error alone can also break existing packages.

clubby789 commented 5 months ago

I'm currently implementing the unsupported skip argument as a lint so it can be disabled if needed (although right now there seem to be issues with that). As for feature gating, not sure how to handle that 😕 A crater run might be a good idea. Seems like there was some discussion about this on a previous similar proposal https://internals.rust-lang.org/t/add-skip-attribute-to-various-derives/17411/

fmease commented 4 months ago

Revisiting this I think we should be more careful with introducing more and more unnamespaced helper attributes. If we continue this trend (#[default], #[skip], etc.) we end up digging our own grave imo.

As the last comment from the linked IRLO thread mentions, ideally we would have better language support for this: Qualified helper attributes or better yet fully hygienic helper attributes (*). This comes close to a pre-RFC and blocking #[skip] on this would probably delay it close to indefinitely.

() If an (attr or derive) proc macro declares a helper attribute helper, no other proc macros will be able to see it inside the token stream they receive. If multiple proc macros declare the same helper attribute helper and the user tries to use it we will reject it as ambiguous* and force them to qualify it. E.g., as #[a::helper] / #[a::helper] in the case of attr proc macros and as #[derive::A::helper] / #[derive::B::helper] in the case of derive proc macros, modulo syntax bikeshedding. Skipping further details.

Maybe we should nominate this for T-lang discussion.

I still plan on reviewing the implementation of #[skip] (rust-lang/rust#121053) given I'm assigned to it but personally speaking I'd rather we follow a more principled approach. Not that I necessarily have a say in this, I'm but a member of T-compiler-contributors.

fmease commented 4 months ago

cc @petrochenkov (you might be interested in this topic)

kennytm commented 4 months ago

#[skip] is very different from #[default] though.

#[skip] is basically agnostic from the trait being derived (you write #[skip(Debug)] not #[debug(skip)]), while #[default] can only make sense in the Default trait. The "helper attribute" concern should block #[default] and #[debug] and #[partial_ord] etc. but not #[skip].

Or that #[skip] should be entirely removed in favor of the trait-specific #[default(skip)] and #[debug(skip)] and #[partial_ord(skip)] etc.

dead-claudia commented 4 months ago

Curious question: how should third party libraries integrate with that #[skip(...)]? It's reasonable for a library/framework like Serde to also want to do things like #[skip(Foo)] for some arbitrary derive trait Foo - that library itself already uses #[serde(skip)], #[serde(skip_serializing)], and #[serde(skip_deserializing)] for what's essentially #[skip(Serialize, Deserialize)], #[skip(Serialize)], and #[skip(Deserialize)].

kennytm commented 4 months ago

I think the field that is #[skip]'ed should be entirely deleted from the TokenStream sent to the proc macro. However currently no similar treatment applied to proc macro: all attributes, even helpers from other macros, are visible as-is. To be consistent with the current behavior I'm afraid the custom proc-macro needs to handle the #[skip] themselves.

Ideally,

  1. helper attributes that are known to belong to other derive macros should be hidden away from the proc_macro_derive
  2. skipped fields should be removed, and if the field survives the #[skip] attribute itself should be hidden away from the proc_macro_derive (similar to #[cfg] and #[cfg_attr])

but it may be too late to change (1) now.

Proof of concept ```rust extern crate proc_macro; use proc_macro::TokenStream; #[proc_macro_derive(First, attributes(first_a, first_b, common))] pub fn derive_first(item: TokenStream) -> TokenStream { println!("derive(First): {item}"); TokenStream::new() } #[proc_macro_derive(Second, attributes(second_a, second_b, common))] pub fn derive_second(item: TokenStream) -> TokenStream { println!("derive(Second): {item}"); TokenStream::new() } ``` ```rust #[derive(Default, First, Second)] pub struct F { /// documentation #[rustfmt::skip] #[first_a] pub a1: u8, #[allow(unknown_lints)] #[second_a] pub a2: u8, #[cfg_attr(any(), first_a)] #[cfg_attr(all(), second_a)] pub a3: u8, #[first_b] #[second_b] pub b: u8, #[common] pub c: u8, #[cfg(any())] pub d: u8, #[skip(First)] #[first_a] pub skip_first: u8, #[skip(Second)] #[first_a] pub skip_second: u8, #[skip] #[first_a] pub skip_all: u8, } ``` Current output is something like: ``` derive(First): pub struct F { #[doc = "documentation"] #[rustfmt :: skip] #[first_a] pub a1 : u8, #[allow(unknown_lints)] #[first_a] pub a2 : u8, #[second_a] pub a3 : u8, #[first_b] #[second_b] pub b : u8, #[common] pub c : u8, #[skip(First)] #[first_a] pub skip_first : u8, #[skip(Second)] #[first_a] pub skip_second : u8, #[skip] #[first_a] pub skip_all : u8, } derive(Second): pub struct F { #[doc = "documentation"] #[rustfmt :: skip] #[first_a] pub a1 : u8, #[allow(unknown_lints)] #[second_a] pub a2 : u8, #[second_a] pub a3 : u8, #[first_b] #[second_b] pub b : u8, #[common] pub c : u8, #[skip(First)] #[first_a] pub skip_first : u8, #[skip(Second)] #[first_a] pub skip_second : u8, #[skip] #[first_a] pub skip_all : u8, } ``` Ideal output: ``` derive(First): pub struct F { #[doc = "documentation"] #[rustfmt :: skip] #[first_a] pub a1 : u8, #[allow(unknown_lints)] pub a2 : u8, pub a3 : u8, #[first_b] pub b : u8, #[common] pub c : u8, #[first_a] pub skip_second : u8, } derive(Second): pub struct F { #[doc = "documentation"] #[rustfmt :: skip] pub a1 : u8, #[allow(unknown_lints)] #[second_a] pub a2 : u8, #[second_a] pub a3 : u8, #[second_b] pub b : u8, #[common] pub c : u8, pub skip_first : u8, } ```

This is also the problem that for trait that is going to construct an object like Default, if you #[skip] a field to make it invisible from the proc macro, it becomes impossible to implement that trait.

#[derive(Default)]
struct G {
    #[skip(Default)]
    pub a: u8,
}
// This will generate:
impl Default for G {
    fn default() -> Self {
        Self {
        } // which is a compilation error because the constructor must fill in all fields including `a`!
    }
}
fmease commented 3 weeks ago

https://github.com/rust-lang/libs-team/issues/334#issuecomment-1942951274:

Ah, furthermore, the feature gate error alone can also break existing packages.

To give an update (at long last), the crater report did confirm my suspicion. We have 4 confirmed root regressions: [1], [2], [3], [4], [4.1], [4.2].

As such and for the other reasons I gave above, I deem the feature as currently designed unacceptable. I'm willing to work with the author @clubby789 on alternative designs if they agree to it like ones based on "hygienic" helper attributes. In any case, I'm going to submit a T-lang meeting proposal at the end of July — latest — hopefully containing an actionable solution sketch.