serde icon indicating copy to clipboard operation
serde copied to clipboard

Lint for `deny_unknown_fields`

Open Mirko-von-Leipzig opened this issue 2 years ago • 2 comments

I suspect the answer is a custom macro or smart tests, but I'm hoping someone has a smarter idea.

We have a large set of serde types which should in 99% of cases implement deny_unknown_fields. We've run into several "bugs" where the type, or one of its fields did not have this derive implemented, often due to refactoring or adding a new field which doesn't have the derive.

I have two questions:

  1. How can I enforce that these types should have this? Ideal would be a crate wide lint which one could opt out for the 1% of times it isn't required. And field types should also have this, unless explicitly opt-out.
  2. How does one implement the equivalent without using derive? i.e. when manually implementing deserialize..

Current thoughts for 1

Test it

Have all such types implement Default and then create tests which serialize, add another unknown field, and assert that deserialization fails.

Issues:

  • type must be Default which isn't always easy
  • type must now be Serialize which is also not always easy
  • test is opt-in instead of opt-out

Macro

Create a macro which wraps does the derive and auto-includes #serde(deny_unknown_fields).

Issues:

  • opt-in instead of opt-out (but probably easier to review)
  • looks weird?

New trait

A new trait or wrapper which essentially wraps Deserialize. Just a vague thought so far, but maybe its possible to have a trait or type which somehow magically enforces this at compile time. Not sure if that's possible.

Thoughts for 2

I tried to understand the macro expansion for serde but I found it hard to follow. I imagine one can achieve something similar by removing all the fields, and then asserting that the remainder is empty?

Mirko-von-Leipzig avatar Sep 06 '23 15:09 Mirko-von-Leipzig

  1. There is deserialize_ignored_any

It can be used to skip some data ( https://serde.rs/ignored-any.html ). And if want to be sure that there is no data left -- you can return errors in all visitor methods (maybe except visit_unit(), but it depends).

  1. You can try to wrap Deserializer implementation which you are using and proxy calls to all methods to the original implementation except deserialize_ignored_any, where you can extract value by deserialize_ignored_any() of original impl and your own visitor, than return error about value you had received. Then when you will use this wrapped version of deserializer -- it will return error any time when will see some extra data which is not covered by your derives.

Rulexec avatar Sep 09 '23 20:09 Rulexec

clippy already has serde-specific lints, maybe it could be a new restriction lint?

oli-obk avatar Sep 18 '23 09:09 oli-obk