Skip to content

Commit b34cd79

Browse files
committed
Auto merge of #8188 - jamesmcm:recursive_display_impl, r=camsteffen
new lint: `recursive_format_impl` 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 This is my first contribution so please check it for better / more idiomatic checks + error messages. Note the format macro paths are shared with the `format_in_format_args` lint - maybe those could be moved to clippy utils too. This relates to issues #2691 and #7830 ------ changelog: Renamed `to_string_in_display` lint to [`recursive_format_impl`] with new check for any use of self as Display or Debug inside the same format trait impl.
2 parents 4931cab + b162b11 commit b34cd79

17 files changed

+685
-276
lines changed

CHANGELOG.md

+3-3
Original file line numberDiff line numberDiff line change
@@ -1438,7 +1438,7 @@ Released 2020-11-19
14381438
* [`manual_strip`] [#6038](https://github.com/rust-lang/rust-clippy/pull/6038)
14391439
* [`map_err_ignore`] [#5998](https://github.com/rust-lang/rust-clippy/pull/5998)
14401440
* [`rc_buffer`] [#6044](https://github.com/rust-lang/rust-clippy/pull/6044)
1441-
* [`to_string_in_display`] [#5831](https://github.com/rust-lang/rust-clippy/pull/5831)
1441+
* `to_string_in_display` [#5831](https://github.com/rust-lang/rust-clippy/pull/5831)
14421442
* `single_char_push_str` [#5881](https://github.com/rust-lang/rust-clippy/pull/5881)
14431443

14441444
### Moves and Deprecations
@@ -1481,7 +1481,7 @@ Released 2020-11-19
14811481
[#5949](https://github.com/rust-lang/rust-clippy/pull/5949)
14821482
* [`doc_markdown`]: allow using "GraphQL" without backticks
14831483
[#5996](https://github.com/rust-lang/rust-clippy/pull/5996)
1484-
* [`to_string_in_display`]: avoid linting when calling `to_string()` on anything that is not `self`
1484+
* `to_string_in_display`: avoid linting when calling `to_string()` on anything that is not `self`
14851485
[#5971](https://github.com/rust-lang/rust-clippy/pull/5971)
14861486
* [`indexing_slicing`] and [`out_of_bounds_indexing`] treat references to arrays as arrays
14871487
[#6034](https://github.com/rust-lang/rust-clippy/pull/6034)
@@ -3385,6 +3385,7 @@ Released 2018-09-13
33853385
[`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len
33863386
[`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
33873387
[`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex
3388+
[`recursive_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#recursive_format_impl
33883389
[`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation
33893390
[`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
33903391
[`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
@@ -3459,7 +3460,6 @@ Released 2018-09-13
34593460
[`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments
34603461
[`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
34613462
[`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
3462-
[`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display
34633463
[`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
34643464
[`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
34653465
[`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::TRANSMUTE_UNDEFINED_REPR),
6262
LintId::of(transmute::UNSOUND_COLLECTION_TRANSMUTE),
6363
LintId::of(transmute::WRONG_TRANSMUTE),

clippy_lints/src/lib.register_lints.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,7 @@ store.register_lints(&[
416416
ranges::RANGE_PLUS_ONE,
417417
ranges::RANGE_ZIP_WITH_LEN,
418418
ranges::REVERSED_EMPTY_RANGES,
419+
recursive_format_impl::RECURSIVE_FORMAT_IMPL,
419420
redundant_clone::REDUNDANT_CLONE,
420421
redundant_closure_call::REDUNDANT_CLOSURE_CALL,
421422
redundant_else::REDUNDANT_ELSE,
@@ -460,7 +461,6 @@ store.register_lints(&[
460461
tabs_in_doc_comments::TABS_IN_DOC_COMMENTS,
461462
temporary_assignment::TEMPORARY_ASSIGNMENT,
462463
to_digit_is_some::TO_DIGIT_IS_SOME,
463-
to_string_in_display::TO_STRING_IN_DISPLAY,
464464
trailing_empty_array::TRAILING_EMPTY_ARRAY,
465465
trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS,
466466
trait_bounds::TYPE_REPETITION_IN_BOUNDS,

clippy_lints/src/lib.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,7 @@ mod ptr_eq;
332332
mod ptr_offset_with_cast;
333333
mod question_mark;
334334
mod ranges;
335+
mod recursive_format_impl;
335336
mod redundant_clone;
336337
mod redundant_closure_call;
337338
mod redundant_else;
@@ -364,7 +365,6 @@ mod swap;
364365
mod tabs_in_doc_comments;
365366
mod temporary_assignment;
366367
mod to_digit_is_some;
367-
mod to_string_in_display;
368368
mod trailing_empty_array;
369369
mod trait_bounds;
370370
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));
@@ -938,6 +938,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
938938
ls.register_renamed("clippy::disallowed_type", "clippy::disallowed_types");
939939
ls.register_renamed("clippy::disallowed_method", "clippy::disallowed_methods");
940940
ls.register_renamed("clippy::ref_in_deref", "clippy::needless_borrow");
941+
ls.register_renamed("clippy::to_string_in_display", "clippy::recursive_format_impl");
941942

942943
// uplifted lints
943944
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)