Add skip attribute to various derives

An idea I had was to borrow #[skip] from serde for Hash, Ord, Eq, etc. Should be an easy add for these derives? Specific syntax I'm imagining:

#[derive(Ord, Eq, Hash)]
pub struct MyKey {
    part_a: &'static str,
    part_b: &'static str,
    #[skip(Ord, Eq, Hash)]
    last_modified_at: SystemTime
}
18 Likes

In the past I’ve contributed the Rust-Analyzer assist to expand Rust’s built in derives to their actual trait impls. The assist works well enough to quickly customize the impls, but it has the notable downside of making code harder to read and maintain in the common case where a single field can’t be included.

This design looks exactly right to me, and I can think of numerous occasions where I would’ve loved to have this. It’s easy to write, and importantly: easy to read and keep up to date.

It also doesn’t seem like it would conflict with serde’s skip attributes and could potentially even be made to compose with cfg_attr.

I really like this!

6 Likes

On the note of compat, it's also forward compatible with others adding their own derives, eg #[skip(Hash, my_crate::MyDerive)].

3 Likes

Along the same lines can be something like:

#[derive(Clone)]
struct WithCache {
    #[clone_as(Default::default())]
    expiring_cache: HashMap<Input, (Instant, Output)>,
}

which drops the cache on Clone::clone().

I do like the idea, however I think that would be a distinct proposal from this one.

3 Likes

On the other hand, explicit impls are relatively easy to read, easy to skip, and don't complicate the type definition. Everyone want to dump attributes on type definition. Skip, serde, default, derive_more, it's easy to turn the concise definition into a soup of attributes which merge, overlap and interact in confusing ways.

It would be great if there was a better story to derives than "just add more attributes". For example, what if derive invocations were separate items, something like

struct Foo {
    bar: Bar,
    baz: Baz,
    quux: Vec<Quux>,
    moo: Arc<Mutex<Moo>>,
    tump: Vec<Tump>,
}

derive!(Debug for Foo; bar, #[uppercase] baz, tump);
derive!(Hash for Foo; #[skip] baz);

This way each derive would be fully self-contained and independent, and wouldn't complicate the type definition.

Alternatively, we could write it as

#[derive(Debug(bar, #[uppercase] baz, tump))]
#[derive(Hash(#[skip] baz))]
struct Foo {
    bar: Bar,
    baz: Baz,
    quux: Vec<Quux>,
    moo: Arc<Mutex<Moo>>,
    tump: Vec<Tump>,
}

That would at least have the benefit of reducing the number of attributes on fields to the minimum.

5 Likes

I have considered syntax almost identical to this in the past, just using "ignore" instead of "skip". So :+1: from me in principle.

Given that

    #[skip(Ord, Hash)]
    last_modified_at: SystemTime

is just wrong, is there anything that could be done to make that mistake harder?


I wonder if maybe it'd be better to offer a "by key" form? PartialEq and PartialOrd can easily be done by delegation to a tuple, so some way to say "I want to derive this using (&self.part_a, &self.part_b)" might be a reasonable alternative.

(Or, if you want to pick more directly, let Self { part_a, part_b, _last_modified } = self; (part_a, part_b), as self is &Self for these, so you get references out.)

I'm sorry, I'm not exactly sure what you're calling a mistake here? Like, missing a derive skip among a list?

Specifically, skipping something in PartialEq but not in Hash makes the implementation incorrect, as you'll get different hash values for things that eq considers the same.

2 Likes

A couple points:

  1. I'm not wholly convinced that this is something we should worry about addressing as it's not enforced as is; it could be a clippy lint. That said, I see the severity of the concern. Maybe this is something that should be provided as a separate proposal? Especially as an existing open vector of incorrectness. Getting static_assertions natively would be nice, and default assertions like this could be turned on.
  2. We can't enforce those kinds of correctness outside of std, but we could make this a check in derive itself somehow, such as an array of required and prohibited couplets.
1 Like

Re the alternative suggestion, I think the verbosity wouldn't balance with the convenience over writing the impl manually.

Shameless plug: https://crates.io/crates/impl-tools

#[autoimpl(Debug ignore self.animal where T: Debug)]
// Generates: impl<T, A: Animal> std::fmt::Debug for Named<A> where T: Debug { .. }
#[autoimpl(Deref, DerefMut using self.animal)]
// Generates: impl<T, A: Animal> std::ops::Deref for Named<A> { .. }
// Generates: impl<T, A: Animal> std::ops::DerefMut for Named<A> { .. }
struct Named<T, A: Animal> {
    name: T,
    animal: A,
}

There are other third-party macros offering #[ignore] attrs on fields, but I find this style messy: potentially too many attrs (besides the issue of matching which derive is affected by which field attribute).

Note: impl-tools's #[autoimpl] doesn't support very many traits currently. PRs/requests welcome. I'll be releasing version 0.5.0 soon in any case.

4 Likes

I think it makes more sense to use a single attribute for all comparison traits. Previous discussion on this: #[cmp(ignore)] attribute for deriving `*Eq`, `*Ord`, `Hash`

6 Likes

Once this PR lands (required to actually support these traits!), this will be possible:

#[autoimpl(PartialEq, Eq, PartialOrd, Ord, Hash ignore self._f)]
struct MixedComponents {
    i: i32,
    s: &'static str,
    _f: fn() -> i32,
}

Wouldn't adding a new attribute consumed by default derive macros be a breaking change? Attributes are unscoped, and some other derive macro, attribute proc macro or tooling attribute may have been already named skip and used in the struct definition. That would lead to some potentially very confusing and dangerous breakage in those places.

At the very least if a new attribute is added, it must be explicitly scoped in a way which minimizes the potential for breakage.

It seems like a very big footgun of the proc macro system that the attributes consumed by proc macros aren't scoped by default in any way. At least with non-std proc macros the breakage can be managed by an explicit crate version bump, which isn't possible for builtins.

1 Like

In the case that another macro used #[skip] as an annotation and that this other macro is evaluated after #[derive], yes. Considering that before Rust 1.57.0 this wasn't even supported (Tracking issue for macro attributes in `#[derive]` output · Issue #81119 · rust-lang/rust · GitHub), it's not all that likely that this happened in the wild.


Regardless, my opinion is that extensions to #[derive] should follow the style of impl-tools, not Derivative or Educe since the latter are less concise and less easy to read:

#[derive(Derivative)]
#[derivative(PartialEq, Eq)]
struct Foo<S, T: ?Sized> {
    foo: S,
    #[derivative(PartialEq="ignore")]
    bar: u8,
    #[derivative(PartialEq(bound=""), Eq(bound=""))]
    ptr: *const T,
}

#[derive(Educe)]
#[educe(PartialEq(bound = "S: PartialEq"), Eq(bound = "S: Eq"))]
struct Foo<S, T: ?Sized> {
    foo: S,
    #[educe(PartialEq(ignore))]
    bar: u8,
    ptr: *const T,
}

// impl-tools:
#[autoimpl(PartialEq, Eq ignore self.bar where S: trait)]
struct Foo<S, T: ?Sized> {
    foo: S,
    bar: u8,
    ptr: *const T,
}

Note: #[derive] and Derivative add bounds like S: PartialEq, T: PartialEq on generic parameters by default; Educe and impl-tools do not.

1 Like

BTW impl-tools 0.5 is released now. Let me know if there are any issues.

The style of #[skip(Multiple, Trait, Derives)] certainly implies a requirement that it be remitted by the Multiple and Trait macros so that Derives can pick it up.

You've put your finger on what bothers me about attributes in Rust. Attribute soup is a real problem.

There's no direct tie between attributes so it's hard to tell what affects what. How do I know #[skip] belongs to #[derive]? Are other macros going to key off it? How do I know if they do or don't?

They also pollute struct definitions. I work in a serialization-heavy codebase full of #[nom]s and #[serde]s. It's fairly unpleasant to read code where type definitions are half attributes. I'm wary of us leaning on attributes too readily as a quick band aid to problems like this. We might get trapped on a local maximum full of hashtags.

4 Likes