Add skip attribute to various derives

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.

2 Likes

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

It seems relevant and has not been mentioned yet, the crate derivative does a good job of implementing some standard traits automatically with ignoring some fields.

I wish Rust's attributes were namespaced! Fortunately serde self-namespaces in a way, it's #[serde(skip_serializing)] so it's less likely to conflict with some other macro or custom derive, but I wish we had more mechanisms to ensure that attribute macros will compose

I mean, I can see #[skip] possibly conflicting with something else, as well as the already stabilized #[default]

Rust should at least have a way to make an attribute visible only to a specific macro, something like #[macro_attr(myattrmacro, skip)] which makes #[skip] visible to #[myattrmacro], like this:

#[derive(Ord)]
#[myattrmacro]
pub struct A {
     #[macro_attr(myattrmacro, skip)]
     something: B,
}

So that #[myattrmacro] sees

pub struct A {
     #[skip]
     something: B,
}

And #[derive(Ord)] sees

pub struct A {
     something: B,
}
2 Likes

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.