Consider silencing struct_field_names for structs with derive(Serialize/Deserialize)

[jyn514]

Summary

Clippy silences struct_field_names for public structs. This is good and makes sense; the field is part of the public API and renaming it would be a breaking change. However, it still emits the lint for structs with derive(Deserialize). I think these should also be silenced, because Serialize and Deserialize are "super public" - not just available to other crates, but to other programs as well.

Lint Name

struct_field_names

Reproducer

I tried this code:

#![warn(clippy::pedantic)]
use serde::{Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub(crate) struct Resource {
    pub(crate) id: String,
    pub(crate) name: String,
    pub(crate) resource_group: String,
}

I saw this happen:

warning: field name starts with the struct's name
 --> src/lib.rs:9:5
  |
9 |     pub(crate) resource_group: String,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#struct_field_names
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![warn(clippy::pedantic)]
  |         ^^^^^^^^^^^^^^^^
  = note: `#[warn(clippy::struct_field_names)]` implied by `#[warn(clippy::pedantic)]`

I expected to see this happen: No lint is emitted.

As an aside, the lint stops firing if i remove id or name, which seems quite odd.

Version

clippy 0.1.75 (82e1608d 2023-12-21)
rustc 1.75.0 (82e1608df 2023-12-21)

Additional Labels

No response

[m-rph]

@rustbot label +T-macros

[alexis-langlet]

I'll try looking into it.
@rustbot claim

[alexis-langlet]

I've done some test and it looks like the struct_field_names is silenced for public structs (as @jyn514 was saying) but not for struct that are public only inside the crate scope: pub(crate).

It looks logical to me but is it really the wanted behavior?

On the other hand, it seems that struct_field_names is never triggered when there is 2 or less fields in the struct.
That's why the lint was not triggered when removing id or name but maybe this should be the subject of another issue.

[alexis-langlet]

As @y21 taught me in #12089, the number of fields required to trigger the lint is actually a feature and not a bug.

So, if I understand everything correctly, the behavior of the lints on the code sample is perfectly normal.

[m-rph]

The issue is that this is triggered for structs that derive Serialize and Deserialize, which are likely part of a public API.

So, regardless of the number of fields; if these traits are impl'd or derived, then the lint shouldn't trigger on the basis of those structures representing data across programs.

I think that is reasonable, but maybe it is not desirable? What does everyone think?

@rustbot label +S-needs-discussion

[alexis-langlet]

Sorry, I didn't make myself really clear...
The fact is that this will trigger

#![warn(clippy::pedantic)]
use serde::{Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub(crate) struct Resource {
    pub(crate) id: String,
    pub(crate) name: String,
    pub(crate) resource_group: String,
}

But this will not

#![warn(clippy::pedantic)]
use serde::{Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Resource {
    pub id: String,
    pub name: String,
    pub resource_group: String,
}

The fact is that it's not silenced because it's only public in the crate scope, so it will never be used in other crates or programs

[m-rph]

Visibility shouldn't matter because the struct can be private and at the same time be used to deserialize data sent from across the network.

[alexis-langlet]

Ho ok ! I see what you mean
That makes a lot more sense for me 😅

[y21]

It might be fine short-term to allow specifically #[derive(Serialize, Deserialize)], but IMO a better solution would be an attribute that serde or any other crate can apply to (traits) impls in its expansion where exact field names are significant (like, #[clippy::significant_field_names] or something, similar to #[clippy::has_significant_drop]). The lint could then check if any impl has that attribute and avoid linting if so.

That way we wouldn't need to check for specific derives, because this FP doesn't affect just Serialize and Deserialize. For example, the dlopen crate has a #[derive(WrapperApi)] macro that uses the struct field names to load the specified symbols from a dylib. The lint is not very useful here either.

Worth noting (haven't seen anyone point this out) that this also affects the enum_variant_names lint

[m-rph]

I think a tag is a better approach and makes it agnostic to libraries.

[alexis-langlet]

Looks good to me.
Just to be sure that I understand correctly, here is what I'll try to implement, please correct me if I'm wrong:

We want a #[clippy::significant_field_names] tag that silences struct_field_names and enum_variant_names lints.
So something like that

#![warn(clippy::pedantic)]

#[clippy::significant_field_names]
struct A {
    a_id: String,
    a_name: String,
    a_label: String,
}

or something like that

#![warn(clippy::pedantic)]

#[clippy::significant_field_names]
enum Ab {
    IdAb,
    NameAb,
    LabelAb,
}

Should not trigger any warning.

This #[clippy::significant_field_names] can be used as is by end users, or included in other crates macro's code expansions (such as #[derive(Serialize, Deserialize)] or #[derive(WrapperApi)])

[y21]

I don't think derive macros can return attributes or generally modify the annotated item. If that was possible, macro authors could already place #[allow(clippy::struct_field_names)] on the struct. The idea I had with the attribute was to have it be on an impl as an alternate place for macro authors since they can only return new tokens (e.g. impls, which can then be annotated like #[some_attribute] impl MyType {}).

Now that I think about it, maybe we don't need a new attribute and just #[allow(clippy::struct_field_names)] can work on the impl, if using clippy_utils::is_lint_allowed with the HirId of the impl node

You could also ask on zulip to get more opinions if you want to be sure before committing to it (or if you have any questions you can also ask there), it's likely more people will see it there

[Manishearth]

I'm not sure I agree with the core rationale that Serialize/Deserialize are always "public", it depends on what kind of format is expected to be used with them. But I agree in general it doesn't hurt to allow impls to turn off this lint.

[flip1995]

In the latest meeting we decided, that the responsibility of adding this allow attribute should not be on the serde/dlopen authors, but rather on the users of those crates. The main reason for this is, that the lint is pedantic, so you have to opt-in before even seeing this warning. pedantic lints are designed in a way, that adding some allows for them in your code is expected. This is also the case here: The user can just put an allow attribute on the struct that triggers the lint: Playground