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

Add internal lint derive_deserialize_allowing_unknown #14360

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Mar 6, 2025

Adds an internal lint to check for #[derive(serde::Deserialize)] without #[serde(deny_unknown_fields)].

Today, if you run Clippy with the following clippy.toml, Clippy will produce a warning, but there will be no accompanying note:

# In the following configuration, "recommendation" should be "reason" or "replacement".
disallowed-macros = [
    { path = "std::panic", recommendation = "return a `std::result::Result::Error` instead" },
]
$ cargo clippy
    Checking a v0.1.0 (/home/smoelius/tmp/a)
warning: use of a disallowed macro `std::panic`
 --> src/lib.rs:2:5
  |
2 |     panic!();
  |     ^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros
  = note: `#[warn(clippy::disallowed_macros)]` on by default

The underlying problem is: the enum that derives serde::Deserialize (DisallowedPathEnum) does not have the attribute #[serde(deny_unknown_fields)].

This lint identifies such problems by checking trait impls. An alternative I considered was to walk clippy_config::conf::Conf directly. However, that would not catch the DisallowedPathEnum case because it is not used in Conf directly.

Just to be clear, no one asked for this. So I hope the maintainers do not mind.

changelog: none

smoelius added 2 commits March 6, 2025 06:31
`cargo dev dogfood` fails at this commit. The next commit addresses the
errors that are produced.
@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 6, 2025
@llogiq
Copy link
Contributor

llogiq commented Mar 7, 2025

Why should that only be an internal lint? I could see it as a restriction lint, with some documentation that anything deserialized from user data should have that attribute.

@smoelius
Copy link
Contributor Author

smoelius commented Mar 7, 2025

@llogiq Thank you very much for your comments.

Why should that only be an internal lint? I could see it as a restriction lint,

I had the impression that Clippy preferred to not lint third-party APIs. For example, I recall this comment from 2022: #8434 (review)

Granted, the comment called serde an exception, but the sentiment seemed to be that more such lints were unwanted.

with some documentation that anything deserialized from user data should have that attribute.

I think some might disagree (hence, the reason for the optional attribute). But if the lint is allow by default, that might not matter.

Please tell me if I should proceed with changing the lint's category.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants