Skip to content

Commit 7de473e

Browse files
committed
Add recursive_format_impl lint
The to_string_in_display lint is renamed to recursive_format_impl A check is added for the use of self formatted with Display or Debug inside any format string in the same impl The to_string_in_display check is kept as is - like in the format_in_format_args lint For now only Display and Debug are checked This could also be extended to other Format traits (Binary, etc.)
1 parent 788a8bc commit 7de473e

17 files changed

+639
-244
lines changed

CHANGELOG.md

+3-3
Original file line numberDiff line numberDiff line change
@@ -1267,7 +1267,7 @@ Released 2020-11-19
12671267
* [`manual_strip`] [#6038](https://github.com/rust-lang/rust-clippy/pull/6038)
12681268
* [`map_err_ignore`] [#5998](https://github.com/rust-lang/rust-clippy/pull/5998)
12691269
* [`rc_buffer`] [#6044](https://github.com/rust-lang/rust-clippy/pull/6044)
1270-
* [`to_string_in_display`] [#5831](https://github.com/rust-lang/rust-clippy/pull/5831)
1270+
* `to_string_in_display` [#5831](https://github.com/rust-lang/rust-clippy/pull/5831)
12711271
* `single_char_push_str` [#5881](https://github.com/rust-lang/rust-clippy/pull/5881)
12721272

12731273
### Moves and Deprecations
@@ -1310,7 +1310,7 @@ Released 2020-11-19
13101310
[#5949](https://github.com/rust-lang/rust-clippy/pull/5949)
13111311
* [`doc_markdown`]: allow using "GraphQL" without backticks
13121312
[#5996](https://github.com/rust-lang/rust-clippy/pull/5996)
1313-
* [`to_string_in_display`]: avoid linting when calling `to_string()` on anything that is not `self`
1313+
* `to_string_in_display`: avoid linting when calling `to_string()` on anything that is not `self`
13141314
[#5971](https://github.com/rust-lang/rust-clippy/pull/5971)
13151315
* [`indexing_slicing`] and [`out_of_bounds_indexing`] treat references to arrays as arrays
13161316
[#6034](https://github.com/rust-lang/rust-clippy/pull/6034)
@@ -3213,6 +3213,7 @@ Released 2018-09-13
32133213
[`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len
32143214
[`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
32153215
[`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex
3216+
[`recursive_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#recursive_format_impl
32163217
[`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation
32173218
[`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
32183219
[`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
@@ -3287,7 +3288,6 @@ Released 2018-09-13
32873288
[`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments
32883289
[`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
32893290
[`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
3290-
[`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display
32913291
[`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
32923292
[`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
32933293
[`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments

clippy_lints/src/lib.register_all.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
241241
LintId::of(ranges::MANUAL_RANGE_CONTAINS),
242242
LintId::of(ranges::RANGE_ZIP_WITH_LEN),
243243
LintId::of(ranges::REVERSED_EMPTY_RANGES),
244+
LintId::of(recursive_format_impl::RECURSIVE_FORMAT_IMPL),
244245
LintId::of(redundant_clone::REDUNDANT_CLONE),
245246
LintId::of(redundant_closure_call::REDUNDANT_CLOSURE_CALL),
246247
LintId::of(redundant_field_names::REDUNDANT_FIELD_NAMES),
@@ -267,7 +268,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
267268
LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS),
268269
LintId::of(temporary_assignment::TEMPORARY_ASSIGNMENT),
269270
LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME),
270-
LintId::of(to_string_in_display::TO_STRING_IN_DISPLAY),
271271
LintId::of(transmute::CROSSPOINTER_TRANSMUTE),
272272
LintId::of(transmute::TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS),
273273
LintId::of(transmute::TRANSMUTE_BYTES_TO_STR),

clippy_lints/src/lib.register_correctness.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,12 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
5252
LintId::of(ptr::INVALID_NULL_PTR_USAGE),
5353
LintId::of(ptr::MUT_FROM_REF),
5454
LintId::of(ranges::REVERSED_EMPTY_RANGES),
55+
LintId::of(recursive_format_impl::RECURSIVE_FORMAT_IMPL),
5556
LintId::of(regex::INVALID_REGEX),
5657
LintId::of(self_assignment::SELF_ASSIGNMENT),
5758
LintId::of(serde_api::SERDE_API_MISUSE),
5859
LintId::of(size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT),
5960
LintId::of(swap::ALMOST_SWAPPED),
60-
LintId::of(to_string_in_display::TO_STRING_IN_DISPLAY),
6161
LintId::of(transmute::UNSOUND_COLLECTION_TRANSMUTE),
6262
LintId::of(transmute::WRONG_TRANSMUTE),
6363
LintId::of(transmuting_null::TRANSMUTING_NULL),

clippy_lints/src/lib.register_lints.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,7 @@ store.register_lints(&[
414414
ranges::RANGE_PLUS_ONE,
415415
ranges::RANGE_ZIP_WITH_LEN,
416416
ranges::REVERSED_EMPTY_RANGES,
417+
recursive_format_impl::RECURSIVE_FORMAT_IMPL,
417418
redundant_clone::REDUNDANT_CLONE,
418419
redundant_closure_call::REDUNDANT_CLOSURE_CALL,
419420
redundant_else::REDUNDANT_ELSE,
@@ -458,7 +459,6 @@ store.register_lints(&[
458459
tabs_in_doc_comments::TABS_IN_DOC_COMMENTS,
459460
temporary_assignment::TEMPORARY_ASSIGNMENT,
460461
to_digit_is_some::TO_DIGIT_IS_SOME,
461-
to_string_in_display::TO_STRING_IN_DISPLAY,
462462
trailing_empty_array::TRAILING_EMPTY_ARRAY,
463463
trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS,
464464
trait_bounds::TYPE_REPETITION_IN_BOUNDS,

clippy_lints/src/lib.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,7 @@ mod ptr_eq;
330330
mod ptr_offset_with_cast;
331331
mod question_mark;
332332
mod ranges;
333+
mod recursive_format_impl;
333334
mod redundant_clone;
334335
mod redundant_closure_call;
335336
mod redundant_else;
@@ -362,7 +363,6 @@ mod swap;
362363
mod tabs_in_doc_comments;
363364
mod temporary_assignment;
364365
mod to_digit_is_some;
365-
mod to_string_in_display;
366366
mod trailing_empty_array;
367367
mod trait_bounds;
368368
mod transmute;
@@ -704,7 +704,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
704704
store.register_late_pass(|| Box::new(modulo_arithmetic::ModuloArithmetic));
705705
store.register_early_pass(|| Box::new(reference::DerefAddrOf));
706706
store.register_early_pass(|| Box::new(double_parens::DoubleParens));
707-
store.register_late_pass(|| Box::new(to_string_in_display::ToStringInDisplay::new()));
707+
store.register_late_pass(|| Box::new(recursive_format_impl::RecursiveFormatImpl::new()));
708708
store.register_early_pass(|| Box::new(unsafe_removed_from_name::UnsafeNameRemoval));
709709
store.register_early_pass(|| Box::new(else_if_without_else::ElseIfWithoutElse));
710710
store.register_early_pass(|| Box::new(int_plus_one::IntPlusOne));
@@ -935,6 +935,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
935935
ls.register_renamed("clippy::disallowed_type", "clippy::disallowed_types");
936936
ls.register_renamed("clippy::disallowed_method", "clippy::disallowed_methods");
937937
ls.register_renamed("clippy::ref_in_deref", "clippy::needless_borrow");
938+
ls.register_renamed("clippy::to_string_in_display", "clippy::recursive_format_impl");
938939

939940
// uplifted lints
940941
ls.register_renamed("clippy::invalid_ref", "invalid_value");
+201
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
use clippy_utils::diagnostics::span_lint;
2+
use clippy_utils::format_macros::is_format_macro;
3+
use clippy_utils::macros::{root_macro_call, FormatArgsArg, FormatArgsExpn};
4+
use clippy_utils::{is_diag_trait_item, path_to_local};
5+
use if_chain::if_chain;
6+
use rustc_hir::{Expr, ExprKind, Impl, Item, ItemKind, UnOp};
7+
use rustc_lint::{LateContext, LateLintPass};
8+
use rustc_session::{declare_tool_lint, impl_lint_pass};
9+
use rustc_span::{sym, symbol::kw, Symbol};
10+
11+
#[derive(Clone, Copy)]
12+
enum FormatTrait {
13+
Debug,
14+
Display,
15+
}
16+
17+
impl FormatTrait {
18+
fn name(self) -> Symbol {
19+
match self {
20+
FormatTrait::Debug => sym::Debug,
21+
FormatTrait::Display => sym::Display,
22+
}
23+
}
24+
}
25+
26+
declare_clippy_lint! {
27+
/// ### What it does
28+
/// Checks for format trait implementations (e.g. `Display`) with a recursive call to itself
29+
/// which uses `self` as a parameter.
30+
/// This is typically done indirectly with the `write!` macro or with `to_string()`.
31+
///
32+
/// ### Why is this bad?
33+
/// This will lead to infinite recursion and a stack overflow.
34+
///
35+
/// ### Example
36+
///
37+
/// ```rust
38+
/// use std::fmt;
39+
///
40+
/// struct Structure(i32);
41+
/// impl fmt::Display for Structure {
42+
/// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
43+
/// write!(f, "{}", self.to_string())
44+
/// }
45+
/// }
46+
///
47+
/// ```
48+
/// Use instead:
49+
/// ```rust
50+
/// use std::fmt;
51+
///
52+
/// struct Structure(i32);
53+
/// impl fmt::Display for Structure {
54+
/// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
55+
/// write!(f, "{}", self.0)
56+
/// }
57+
/// }
58+
/// ```
59+
#[clippy::version = "1.48.0"]
60+
pub RECURSIVE_FORMAT_IMPL,
61+
correctness,
62+
"Format trait method called while implementing the same Format trait"
63+
}
64+
65+
#[derive(Default)]
66+
pub struct RecursiveFormatImpl {
67+
// Whether we are inside Display or Debug trait impl - None for neither
68+
format_trait_impl: Option<FormatTrait>,
69+
}
70+
71+
impl RecursiveFormatImpl {
72+
pub fn new() -> Self {
73+
Self {
74+
format_trait_impl: None,
75+
}
76+
}
77+
}
78+
79+
impl_lint_pass!(RecursiveFormatImpl => [RECURSIVE_FORMAT_IMPL]);
80+
81+
impl LateLintPass<'_> for RecursiveFormatImpl {
82+
fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
83+
if let Some(format_trait_impl) = is_format_trait_impl(cx, item) {
84+
self.format_trait_impl = Some(format_trait_impl);
85+
}
86+
}
87+
88+
fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
89+
// Assume no nested Impl of Debug and Display within eachother
90+
if is_format_trait_impl(cx, item).is_some() {
91+
self.format_trait_impl = None;
92+
}
93+
}
94+
95+
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
96+
match self.format_trait_impl {
97+
Some(FormatTrait::Display) => {
98+
check_to_string_in_display(cx, expr);
99+
check_self_in_format_args(cx, expr, FormatTrait::Display);
100+
},
101+
Some(FormatTrait::Debug) => {
102+
check_self_in_format_args(cx, expr, FormatTrait::Debug);
103+
},
104+
None => {},
105+
}
106+
}
107+
}
108+
109+
fn check_to_string_in_display(cx: &LateContext<'_>, expr: &Expr<'_>) {
110+
let map = cx.tcx.hir();
111+
if_chain! {
112+
// Get the hir_id of the object we are calling the method on
113+
if let ExprKind::MethodCall(path, _, [ref self_arg, ..], _) = expr.kind;
114+
// Is the method to_string() ?
115+
if path.ident.name == sym!(to_string);
116+
// Is the method a part of the ToString trait? (i.e. not to_string() implemented
117+
// separately)
118+
if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
119+
if is_diag_trait_item(cx, expr_def_id, sym::ToString);
120+
// Is the method is called on self
121+
if path_to_local(self_arg).map(|x| map.name(x)) == Some(kw::SelfLower);
122+
then {
123+
span_lint(
124+
cx,
125+
RECURSIVE_FORMAT_IMPL,
126+
expr.span,
127+
"using `self.to_string` in `fmt::Display` implementation will cause infinite recursion",
128+
);
129+
}
130+
}
131+
}
132+
133+
fn check_self_in_format_args(cx: &LateContext<'_>, expr: &Expr<'_>, impl_trait: FormatTrait) {
134+
// Check each arg in format calls - do we ever use Display on self (directly or via deref)?
135+
if_chain! {
136+
if let Some(macro_call) = root_macro_call(expr.span);
137+
if let Some(format_args) = FormatArgsExpn::parse(cx, expr);
138+
if let macro_def_id = macro_call.def_id;
139+
if is_format_macro(cx, macro_def_id);
140+
if let Some(args) = format_args.args();
141+
then {
142+
for arg in args {
143+
if arg.format_trait != impl_trait.name() {
144+
continue;
145+
}
146+
check_format_arg_self(cx, expr, &arg, impl_trait);
147+
}
148+
}
149+
}
150+
}
151+
152+
fn check_format_arg_self(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &FormatArgsArg<'_>, impl_trait: FormatTrait) {
153+
// Handle multiple dereferencing of references e.g. &&self
154+
// Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl)
155+
// Since the argument to fmt is itself a reference: &self
156+
let reference = deref_expr(arg.value);
157+
let map = cx.tcx.hir();
158+
// Is the reference self?
159+
let symbol_ident = impl_trait.name().to_ident_string();
160+
if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) {
161+
span_lint(
162+
cx,
163+
RECURSIVE_FORMAT_IMPL,
164+
expr.span,
165+
&format!(
166+
"using `self` as `{}` in `impl {}` will cause infinite recursion",
167+
&symbol_ident, &symbol_ident
168+
),
169+
);
170+
}
171+
}
172+
173+
fn deref_expr<'a, 'b>(expr: &'a Expr<'b>) -> &'a Expr<'b> {
174+
match expr.kind {
175+
ExprKind::Unary(UnOp::Deref, ex) => match ex.kind {
176+
ExprKind::Unary(..) => expr,
177+
_ => deref_expr(ex),
178+
},
179+
ExprKind::AddrOf(_, _, reference) => deref_expr(reference),
180+
_ => expr,
181+
}
182+
}
183+
184+
fn is_format_trait_impl(cx: &LateContext<'_>, item: &Item<'_>) -> Option<FormatTrait> {
185+
if_chain! {
186+
// Are we at an Impl?
187+
if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), .. }) = &item.kind;
188+
if let Some(did) = trait_ref.trait_def_id();
189+
if let Some(name) = cx.tcx.get_diagnostic_name(did);
190+
then {
191+
// Is Impl for Debug or Display?
192+
match name {
193+
sym::Debug => Some(FormatTrait::Debug),
194+
sym::Display => Some(FormatTrait::Display),
195+
_ => None,
196+
}
197+
} else {
198+
None
199+
}
200+
}
201+
}

0 commit comments

Comments
 (0)