Skip to content

Commit 0c615e6

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 0c615e6

17 files changed

+676
-267
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/format_args.rs

+3-23
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2-
use clippy_utils::macros::{FormatArgsArg, FormatArgsExpn};
2+
use clippy_utils::is_diag_trait_item;
3+
use clippy_utils::macros::{is_format_macro, FormatArgsArg, FormatArgsExpn};
34
use clippy_utils::source::snippet_opt;
45
use clippy_utils::ty::implements_trait;
5-
use clippy_utils::{is_diag_trait_item, match_def_path, paths};
66
use if_chain::if_chain;
77
use rustc_errors::Applicability;
88
use rustc_hir::{Expr, ExprKind};
@@ -65,34 +65,14 @@ declare_clippy_lint! {
6565

6666
declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
6767

68-
const FORMAT_MACRO_PATHS: &[&[&str]] = &[
69-
&paths::FORMAT_ARGS_MACRO,
70-
&paths::ASSERT_EQ_MACRO,
71-
&paths::ASSERT_MACRO,
72-
&paths::ASSERT_NE_MACRO,
73-
&paths::EPRINT_MACRO,
74-
&paths::EPRINTLN_MACRO,
75-
&paths::PRINT_MACRO,
76-
&paths::PRINTLN_MACRO,
77-
&paths::WRITE_MACRO,
78-
&paths::WRITELN_MACRO,
79-
];
80-
81-
const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[sym::format_macro, sym::std_panic_macro];
82-
8368
impl<'tcx> LateLintPass<'tcx> for FormatArgs {
8469
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
8570
if_chain! {
8671
if let Some(format_args) = FormatArgsExpn::parse(cx, expr);
8772
let expr_expn_data = expr.span.ctxt().outer_expn_data();
8873
let outermost_expn_data = outermost_expn_data(expr_expn_data);
8974
if let Some(macro_def_id) = outermost_expn_data.macro_def_id;
90-
if FORMAT_MACRO_PATHS
91-
.iter()
92-
.any(|path| match_def_path(cx, macro_def_id, path))
93-
|| FORMAT_MACRO_DIAG_ITEMS
94-
.iter()
95-
.any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, macro_def_id));
75+
if is_format_macro(cx, macro_def_id);
9676
if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
9777
if let Some(args) = format_args.args();
9878
then {

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");
+190
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
use clippy_utils::diagnostics::span_lint;
2+
use clippy_utils::macros::{is_format_macro, root_macro_call_first_node, FormatArgsArg, FormatArgsExpn};
3+
use clippy_utils::{is_diag_trait_item, path_to_local, peel_ref_operators};
4+
use if_chain::if_chain;
5+
use rustc_hir::{Expr, ExprKind, Impl, Item, ItemKind, QPath};
6+
use rustc_lint::{LateContext, LateLintPass};
7+
use rustc_session::{declare_tool_lint, impl_lint_pass};
8+
use rustc_span::{sym, symbol::kw, Symbol};
9+
10+
#[derive(Clone, Copy)]
11+
enum FormatTrait {
12+
Debug,
13+
Display,
14+
}
15+
16+
impl FormatTrait {
17+
fn name(self) -> Symbol {
18+
match self {
19+
FormatTrait::Debug => sym::Debug,
20+
FormatTrait::Display => sym::Display,
21+
}
22+
}
23+
}
24+
25+
declare_clippy_lint! {
26+
/// ### What it does
27+
/// Checks for format trait implementations (e.g. `Display`) with a recursive call to itself
28+
/// which uses `self` as a parameter.
29+
/// This is typically done indirectly with the `write!` macro or with `to_string()`.
30+
///
31+
/// ### Why is this bad?
32+
/// This will lead to infinite recursion and a stack overflow.
33+
///
34+
/// ### Example
35+
///
36+
/// ```rust
37+
/// use std::fmt;
38+
///
39+
/// struct Structure(i32);
40+
/// impl fmt::Display for Structure {
41+
/// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
42+
/// write!(f, "{}", self.to_string())
43+
/// }
44+
/// }
45+
///
46+
/// ```
47+
/// Use instead:
48+
/// ```rust
49+
/// use std::fmt;
50+
///
51+
/// struct Structure(i32);
52+
/// impl fmt::Display for Structure {
53+
/// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
54+
/// write!(f, "{}", self.0)
55+
/// }
56+
/// }
57+
/// ```
58+
#[clippy::version = "1.48.0"]
59+
pub RECURSIVE_FORMAT_IMPL,
60+
correctness,
61+
"Format trait method called while implementing the same Format trait"
62+
}
63+
64+
#[derive(Default)]
65+
pub struct RecursiveFormatImpl {
66+
// Whether we are inside Display or Debug trait impl - None for neither
67+
format_trait_impl: Option<FormatTrait>,
68+
}
69+
70+
impl RecursiveFormatImpl {
71+
pub fn new() -> Self {
72+
Self {
73+
format_trait_impl: None,
74+
}
75+
}
76+
}
77+
78+
impl_lint_pass!(RecursiveFormatImpl => [RECURSIVE_FORMAT_IMPL]);
79+
80+
impl<'tcx> LateLintPass<'tcx> for RecursiveFormatImpl {
81+
fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
82+
if let Some(format_trait_impl) = is_format_trait_impl(cx, item) {
83+
self.format_trait_impl = Some(format_trait_impl);
84+
}
85+
}
86+
87+
fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
88+
// Assume no nested Impl of Debug and Display within eachother
89+
if is_format_trait_impl(cx, item).is_some() {
90+
self.format_trait_impl = None;
91+
}
92+
}
93+
94+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
95+
match self.format_trait_impl {
96+
Some(FormatTrait::Display) => {
97+
check_to_string_in_display(cx, expr);
98+
check_self_in_format_args(cx, expr, FormatTrait::Display);
99+
},
100+
Some(FormatTrait::Debug) => {
101+
check_self_in_format_args(cx, expr, FormatTrait::Debug);
102+
},
103+
None => {},
104+
}
105+
}
106+
}
107+
108+
fn check_to_string_in_display(cx: &LateContext<'_>, expr: &Expr<'_>) {
109+
if_chain! {
110+
// Get the hir_id of the object we are calling the method on
111+
if let ExprKind::MethodCall(path, _, [ref self_arg, ..], _) = expr.kind;
112+
// Is the method to_string() ?
113+
if path.ident.name == sym!(to_string);
114+
// Is the method a part of the ToString trait? (i.e. not to_string() implemented
115+
// separately)
116+
if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
117+
if is_diag_trait_item(cx, expr_def_id, sym::ToString);
118+
// Is the method is called on self
119+
if let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind;
120+
if let [segment] = path.segments;
121+
if segment.ident.name == 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<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx 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(outer_macro) = root_macro_call_first_node(cx, expr);
137+
if let macro_def_id = outer_macro.def_id;
138+
if let Some(format_args) = FormatArgsExpn::find_nested(cx, expr, outer_macro.expn);
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 = peel_ref_operators(cx, 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 is_format_trait_impl(cx: &LateContext<'_>, item: &Item<'_>) -> Option<FormatTrait> {
174+
if_chain! {
175+
// Are we at an Impl?
176+
if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), .. }) = &item.kind;
177+
if let Some(did) = trait_ref.trait_def_id();
178+
if let Some(name) = cx.tcx.get_diagnostic_name(did);
179+
then {
180+
// Is Impl for Debug or Display?
181+
match name {
182+
sym::Debug => Some(FormatTrait::Debug),
183+
sym::Display => Some(FormatTrait::Display),
184+
_ => None,
185+
}
186+
} else {
187+
None
188+
}
189+
}
190+
}

0 commit comments

Comments
 (0)