Skip to content

Commit ff32bcb

Browse files
committed
Add internal lint derive_deserialize_allowing_unknown
`cargo dev dogfood` fails at this commit. The next commit addresses the errors that are produced.
1 parent aeb6ac9 commit ff32bcb

File tree

8 files changed

+271
-0
lines changed

8 files changed

+271
-0
lines changed

clippy_config/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
rustc::diagnostic_outside_of_impl,
1313
rustc::untranslatable_diagnostic
1414
)]
15+
#![deny(clippy::derive_deserialize_allowing_unknown)]
1516

1617
extern crate rustc_data_structures;
1718
extern crate rustc_errors;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
use clippy_utils::diagnostics::span_lint;
2+
use clippy_utils::{def_path_res, paths};
3+
use rustc_ast::tokenstream::{TokenStream, TokenTree};
4+
use rustc_ast::{AttrStyle, DelimArgs};
5+
use rustc_hir::def::Res;
6+
use rustc_hir::def_id::LocalDefId;
7+
use rustc_hir::{
8+
AttrArgs, AttrItem, AttrPath, Attribute, HirId, Impl, Item, ItemKind, Path, QPath, TraitRef, Ty, TyKind,
9+
};
10+
use rustc_lint::{LateContext, LateLintPass};
11+
use rustc_lint_defs::declare_tool_lint;
12+
use rustc_middle::ty::TyCtxt;
13+
use rustc_session::declare_lint_pass;
14+
use rustc_span::sym;
15+
use std::sync::OnceLock;
16+
17+
declare_tool_lint! {
18+
/// ### What it does
19+
/// Checks for structs or enums that derive `serde::Deserialize` and that
20+
/// do not have a `#[serde(deny_unknown_fields)]` attribute.
21+
///
22+
/// ### Why is this bad?
23+
/// If the struct or enum is used in [`clippy_config::conf::Conf`] and a
24+
/// user inserts an unknown field by mistake, the user's error will be
25+
/// silently ignored.
26+
///
27+
/// ### Example
28+
/// ```rust
29+
/// #[derive(Deserialize)]
30+
/// pub struct DisallowedPath {
31+
/// path: String,
32+
/// reason: Option<String>,
33+
/// replacement: Option<String>,
34+
/// }
35+
/// ```
36+
///
37+
/// Use instead:
38+
/// ```rust
39+
/// #[derive(Deserialize)]
40+
/// #[serde(deny_unknown_fields)]
41+
/// pub struct DisallowedPath {
42+
/// path: String,
43+
/// reason: Option<String>,
44+
/// replacement: Option<String>,
45+
/// }
46+
/// ```
47+
pub clippy::DERIVE_DESERIALIZE_ALLOWING_UNKNOWN,
48+
Allow,
49+
"`#[derive(serde::Deserialize)]` without `#[serde(deny_unknown_fields)]`",
50+
report_in_external_macro: true
51+
}
52+
53+
declare_lint_pass!(DeriveDeserializeAllowingUnknown => [DERIVE_DESERIALIZE_ALLOWING_UNKNOWN]);
54+
55+
impl<'tcx> LateLintPass<'tcx> for DeriveDeserializeAllowingUnknown {
56+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
57+
// Is this an `impl` (of a certain form)?
58+
let ItemKind::Impl(Impl {
59+
of_trait: Some(TraitRef {
60+
path: Path { res, .. }, ..
61+
}),
62+
self_ty:
63+
Ty {
64+
kind:
65+
TyKind::Path(QPath::Resolved(
66+
None,
67+
Path {
68+
res: Res::Def(_, self_ty_def_id),
69+
..
70+
},
71+
)),
72+
..
73+
},
74+
..
75+
}) = item.kind
76+
else {
77+
return;
78+
};
79+
80+
// Is it an `impl` of the trait `serde::Deserialize`?
81+
if !is_serde_deserialize_res(cx.tcx, res) {
82+
return;
83+
}
84+
85+
// Is it derived?
86+
if !cx.tcx.has_attr(item.owner_id, sym::automatically_derived) {
87+
return;
88+
}
89+
90+
// Is `self_ty` local?
91+
let Some(local_def_id) = self_ty_def_id.as_local() else {
92+
return;
93+
};
94+
95+
// Does `self_ty` have a variant with named fields?
96+
if !has_variant_with_named_fields(cx.tcx, local_def_id) {
97+
return;
98+
}
99+
100+
let hir_id = cx.tcx.local_def_id_to_hir_id(local_def_id);
101+
102+
// Does `self_ty` have `#[serde(deny_unknown_fields)]`?
103+
if let Some(tokens) = find_serde_attr_item(cx.tcx, hir_id)
104+
&& tokens.iter().any(is_deny_unknown_fields_token)
105+
{
106+
return;
107+
}
108+
109+
span_lint(
110+
cx,
111+
DERIVE_DESERIALIZE_ALLOWING_UNKNOWN,
112+
item.span,
113+
"`#[derive(serde::Deserialize)]` without `#[serde(deny_unknown_fields)]`",
114+
);
115+
}
116+
}
117+
118+
fn is_serde_deserialize_res(tcx: TyCtxt<'_>, res: &Res) -> bool {
119+
static SERDE_DESERIALIZE_RESES: OnceLock<Vec<Res>> = OnceLock::new();
120+
121+
let serde_deserialize_reses = SERDE_DESERIALIZE_RESES.get_or_init(|| def_path_res(tcx, &paths::SERDE_DESERIALIZE));
122+
123+
serde_deserialize_reses.contains(res)
124+
}
125+
126+
// Determines whether `def_id` corresponds to an ADT with at least one variant with named fields. A
127+
// variant has named fields if its `ctor` field is `None`.
128+
fn has_variant_with_named_fields(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
129+
let ty = tcx.type_of(def_id).skip_binder();
130+
131+
let rustc_middle::ty::Adt(adt_def, _) = ty.kind() else {
132+
return false;
133+
};
134+
135+
adt_def.variants().iter().any(|variant_def| variant_def.ctor.is_none())
136+
}
137+
138+
fn find_serde_attr_item(tcx: TyCtxt<'_>, hir_id: HirId) -> Option<&TokenStream> {
139+
tcx.hir_attrs(hir_id).iter().find_map(|attribute| {
140+
if let Attribute::Unparsed(attr_item) = attribute
141+
&& let AttrItem {
142+
path: AttrPath { segments, .. },
143+
args: AttrArgs::Delimited(DelimArgs { tokens, .. }),
144+
style: AttrStyle::Outer,
145+
..
146+
} = &**attr_item
147+
&& segments.len() == 1
148+
&& segments[0].as_str() == "serde"
149+
{
150+
Some(tokens)
151+
} else {
152+
None
153+
}
154+
})
155+
}
156+
157+
fn is_deny_unknown_fields_token(tt: &TokenTree) -> bool {
158+
if let TokenTree::Token(token, _) = tt
159+
&& token
160+
.ident()
161+
.is_some_and(|(token, _)| token.as_str() == "deny_unknown_fields")
162+
{
163+
true
164+
} else {
165+
false
166+
}
167+
}

clippy_lints_internal/src/lib.rs

+3
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ extern crate rustc_span;
3131

3232
mod almost_standard_lint_formulation;
3333
mod collapsible_calls;
34+
mod derive_deserialize_allowing_unknown;
3435
mod interning_literals;
3536
mod invalid_paths;
3637
mod lint_without_lint_pass;
@@ -45,6 +46,7 @@ use rustc_lint::{Lint, LintStore};
4546
static LINTS: &[&Lint] = &[
4647
almost_standard_lint_formulation::ALMOST_STANDARD_LINT_FORMULATION,
4748
collapsible_calls::COLLAPSIBLE_SPAN_LINT_CALLS,
49+
derive_deserialize_allowing_unknown::DERIVE_DESERIALIZE_ALLOWING_UNKNOWN,
4850
interning_literals::INTERNING_LITERALS,
4951
invalid_paths::INVALID_PATHS,
5052
lint_without_lint_pass::DEFAULT_LINT,
@@ -64,6 +66,7 @@ pub fn register_lints(store: &mut LintStore) {
6466
store.register_early_pass(|| Box::new(unsorted_clippy_utils_paths::UnsortedClippyUtilsPaths));
6567
store.register_early_pass(|| Box::new(produce_ice::ProduceIce));
6668
store.register_late_pass(|_| Box::new(collapsible_calls::CollapsibleCalls));
69+
store.register_late_pass(|_| Box::new(derive_deserialize_allowing_unknown::DeriveDeserializeAllowingUnknown));
6770
store.register_late_pass(|_| Box::new(invalid_paths::InvalidPaths));
6871
store.register_late_pass(|_| Box::<interning_literals::InterningDefinedSymbol>::default());
6972
store.register_late_pass(|_| Box::<lint_without_lint_pass::LintWithoutLintPass>::default());
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
#![deny(clippy::derive_deserialize_allowing_unknown)]
2+
3+
use serde::{Deserialize, Deserializer};
4+
5+
#[derive(Deserialize)] //~ derive_deserialize_allowing_unknown
6+
struct Struct {
7+
flag: bool,
8+
limit: u64,
9+
}
10+
11+
#[derive(Deserialize)] //~ derive_deserialize_allowing_unknown
12+
enum Enum {
13+
A(bool),
14+
B { limit: u64 },
15+
}
16+
17+
// negative tests
18+
19+
#[derive(Deserialize)]
20+
#[serde(deny_unknown_fields)]
21+
struct StructWithDenyUnknownFields {
22+
flag: bool,
23+
limit: u64,
24+
}
25+
26+
#[derive(Deserialize)]
27+
#[serde(deny_unknown_fields)]
28+
enum EnumWithDenyUnknownFields {
29+
A(bool),
30+
B { limit: u64 },
31+
}
32+
33+
#[derive(Deserialize)]
34+
#[serde(untagged, deny_unknown_fields)]
35+
enum MultipleSerdeAttributes {
36+
A(bool),
37+
B { limit: u64 },
38+
}
39+
40+
#[derive(Deserialize)]
41+
struct TupleStruct(u64, bool);
42+
43+
#[derive(Deserialize)]
44+
#[serde(deny_unknown_fields)]
45+
enum EnumWithOnlyTupleVariants {
46+
A(bool),
47+
B(u64),
48+
}
49+
50+
struct ManualSerdeImplementation;
51+
52+
impl<'de> Deserialize<'de> for ManualSerdeImplementation {
53+
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
54+
where
55+
D: Deserializer<'de>,
56+
{
57+
let () = <() as Deserialize>::deserialize(deserializer)?;
58+
Ok(ManualSerdeImplementation)
59+
}
60+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
error: `#[derive(serde::Deserialize)]` without `#[serde(deny_unknown_fields)]`
2+
--> tests/ui-internal/derive_deserialize_allowing_unknown.rs:5:10
3+
|
4+
LL | #[derive(Deserialize)]
5+
| ^^^^^^^^^^^
6+
|
7+
note: the lint level is defined here
8+
--> tests/ui-internal/derive_deserialize_allowing_unknown.rs:1:9
9+
|
10+
LL | #![deny(clippy::derive_deserialize_allowing_unknown)]
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12+
= note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)
13+
14+
error: `#[derive(serde::Deserialize)]` without `#[serde(deny_unknown_fields)]`
15+
--> tests/ui-internal/derive_deserialize_allowing_unknown.rs:11:10
16+
|
17+
LL | #[derive(Deserialize)]
18+
| ^^^^^^^^^^^
19+
|
20+
= note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)
21+
22+
error: aborting due to 2 previous errors
23+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
# In the following configuration, "recommendation" should be "reason" or "replacement".
2+
disallowed-macros = [
3+
{ path = "std::panic", recommendation = "return a `std::result::Result::Error` instead" },
4+
]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#[rustfmt::skip]
2+
//@error-in-other-file: error reading Clippy's configuration file: data did not match any variant of untagged enum DisallowedPathEnum
3+
fn main() {
4+
panic!();
5+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
error: error reading Clippy's configuration file: data did not match any variant of untagged enum DisallowedPathEnum
2+
--> $DIR/tests/ui-toml/toml_unknown_config_struct_field/clippy.toml:3:5
3+
|
4+
LL | { path = "std::panic", recommendation = "return a `std::result::Result::Error` instead" },
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
7+
error: aborting due to 1 previous error
8+

0 commit comments

Comments
 (0)