-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
The to_string_in_display lint is renamed to recursive_display_impl A check is added for the use of self formatted with Display inside any format string in the Display impl The to_string_in_display check is kept as is - like in the format_in_format_args lint
Showing
11 changed files
with
477 additions
and
210 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,222 @@ | ||
| use clippy_utils::diagnostics::span_lint; | ||
| use clippy_utils::higher::{FormatArgsArg, FormatArgsExpn}; | ||
| use clippy_utils::{is_diag_trait_item, match_def_path, path_to_local_id, paths}; | ||
| use if_chain::if_chain; | ||
| use rustc_hir::{Expr, ExprKind, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, UnOp}; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_session::{declare_tool_lint, impl_lint_pass}; | ||
| use rustc_span::{sym, ExpnData, ExpnKind, Symbol}; | ||
|
|
||
| const FORMAT_MACRO_PATHS: &[&[&str]] = &[ | ||
| &paths::FORMAT_ARGS_MACRO, | ||
| &paths::ASSERT_EQ_MACRO, | ||
| &paths::ASSERT_MACRO, | ||
| &paths::ASSERT_NE_MACRO, | ||
| &paths::EPRINT_MACRO, | ||
| &paths::EPRINTLN_MACRO, | ||
| &paths::PRINT_MACRO, | ||
| &paths::PRINTLN_MACRO, | ||
| &paths::WRITE_MACRO, | ||
| &paths::WRITELN_MACRO, | ||
| ]; | ||
|
|
||
| const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[sym::format_macro, sym::std_panic_macro]; | ||
|
|
||
| fn outermost_expn_data(expn_data: ExpnData) -> ExpnData { | ||
| if expn_data.call_site.from_expansion() { | ||
| outermost_expn_data(expn_data.call_site.ctxt().outer_expn_data()) | ||
| } else { | ||
| expn_data | ||
| } | ||
| } | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// Checks for recursive use of `Display` trait inside its implementation. | ||
| /// | ||
| /// ### Why is this bad? | ||
| /// This is unconditional recursion and so will lead to infinite | ||
| /// recursion and a stack overflow. | ||
| /// | ||
| /// ### Example | ||
| /// | ||
| /// ```rust | ||
| /// use std::fmt; | ||
| /// | ||
| /// struct Structure(i32); | ||
| /// impl fmt::Display for Structure { | ||
| /// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| /// write!(f, "{}", self.to_string()) | ||
| /// } | ||
| /// } | ||
| /// | ||
| /// ``` | ||
| /// Use instead: | ||
| /// ```rust | ||
| /// use std::fmt; | ||
| /// | ||
| /// struct Structure(i32); | ||
| /// impl fmt::Display for Structure { | ||
| /// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| /// write!(f, "{}", self.0) | ||
| /// } | ||
| /// } | ||
| /// ``` | ||
| #[clippy::version = "1.48.0"] | ||
| pub RECURSIVE_DISPLAY_IMPL, | ||
| correctness, | ||
| "`Display` trait method called while implementing `Display` trait" | ||
| } | ||
|
|
||
| #[derive(Default)] | ||
| pub struct RecursiveDisplayImpl { | ||
| in_display_impl: bool, | ||
| // hir_id of self parameter of method inside Display Impl - i.e. fmt(&self) | ||
| self_hir_id: Option<HirId>, | ||
| } | ||
|
|
||
| impl RecursiveDisplayImpl { | ||
| pub fn new() -> Self { | ||
| Self { | ||
| in_display_impl: false, | ||
| self_hir_id: None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl_lint_pass!(RecursiveDisplayImpl => [RECURSIVE_DISPLAY_IMPL]); | ||
|
|
||
| impl LateLintPass<'_> for RecursiveDisplayImpl { | ||
| fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { | ||
| if is_display_impl(cx, item) { | ||
| self.in_display_impl = true; | ||
| } | ||
| } | ||
|
|
||
| fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { | ||
| if is_display_impl(cx, item) { | ||
| self.in_display_impl = false; | ||
| self.self_hir_id = None; | ||
| } | ||
| } | ||
|
|
||
| fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { | ||
| if_chain! { | ||
| // If we are in Display impl, then get hir_id for self in method impl - i.e. fmt(&self) | ||
| if self.in_display_impl; | ||
| if let ImplItemKind::Fn(.., body_id) = &impl_item.kind; | ||
| let body = cx.tcx.hir().body(*body_id); | ||
| if !body.params.is_empty(); | ||
| then { | ||
| let self_param = &body.params[0]; | ||
| self.self_hir_id = Some(self_param.pat.hir_id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { | ||
| if_chain! { | ||
| if self.in_display_impl; | ||
| if let Some(self_hir_id) = self.self_hir_id; | ||
| then { | ||
| check_to_string_in_display(cx, expr, self_hir_id); | ||
| check_self_in_format_args(cx, expr, self_hir_id); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_to_string_in_display(cx: &LateContext<'_>, expr: &Expr<'_>, self_hir_id: HirId) { | ||
| if_chain! { | ||
| // Get the hir_id of the object we are calling the method on | ||
| if let ExprKind::MethodCall(path, _, [ref self_arg, ..], _) = expr.kind; | ||
| // Is the method to_string() ? | ||
| if path.ident.name == sym!(to_string); | ||
| // Is the method a part of the ToString trait? (i.e. not to_string() implemented | ||
| // separately) | ||
| if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); | ||
| if is_diag_trait_item(cx, expr_def_id, sym::ToString); | ||
| // Is the method is called on self | ||
| if path_to_local_id(self_arg, self_hir_id); | ||
| then { | ||
| span_lint( | ||
| cx, | ||
| RECURSIVE_DISPLAY_IMPL, | ||
| expr.span, | ||
| "using `to_string` in `fmt::Display` implementation might lead to infinite recursion", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_self_in_format_args(cx: &LateContext<'_>, expr: &Expr<'_>, self_hir_id: HirId) { | ||
| // Check each arg in format calls - do we ever use Display on self (directly or via deref)? | ||
| if_chain! { | ||
| if let Some(format_args) = FormatArgsExpn::parse(expr); | ||
| let expr_expn_data = expr.span.ctxt().outer_expn_data(); | ||
| let outermost_expn_data = outermost_expn_data(expr_expn_data); | ||
| if let Some(macro_def_id) = outermost_expn_data.macro_def_id; | ||
| if FORMAT_MACRO_PATHS | ||
| .iter() | ||
| .any(|path| match_def_path(cx, macro_def_id, path)) | ||
| || FORMAT_MACRO_DIAG_ITEMS | ||
| .iter() | ||
| .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, macro_def_id)); | ||
| if let ExpnKind::Macro(_, _name) = outermost_expn_data.kind; | ||
| if let Some(args) = format_args.args(); | ||
| then { | ||
| for (_i, arg) in args.iter().enumerate() { | ||
| // We only care about Display, okay to use Debug here | ||
| if !arg.is_display() { | ||
| continue; | ||
| } | ||
| check_format_arg_self(cx, expr, self_hir_id, arg); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_format_arg_self(cx: &LateContext<'_>, expr: &Expr<'_>, self_hir_id: HirId, arg: &FormatArgsArg<'_>) { | ||
| // Handle multiple dereferencing of references e.g. &&self | ||
| // Handle single dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl) | ||
| // Since the argument to fmt is itself a reference: &self | ||
| let reference = single_deref(deref_expr(arg.value)); | ||
| if path_to_local_id(reference, self_hir_id) { | ||
| span_lint( | ||
| cx, | ||
| RECURSIVE_DISPLAY_IMPL, | ||
| expr.span, | ||
| "using `self` in `fmt::Display` implementation might lead to infinite recursion", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| fn deref_expr<'a, 'b>(expr: &'a Expr<'b>) -> &'a Expr<'b> { | ||
| if let ExprKind::AddrOf(_, _, reference) = expr.kind { | ||
| deref_expr(reference) | ||
| } else { | ||
| expr | ||
| } | ||
| } | ||
|
|
||
| fn single_deref<'a, 'b>(expr: &'a Expr<'b>) -> &'a Expr<'b> { | ||
| if let ExprKind::Unary(UnOp::Deref, ex) = expr.kind { | ||
| ex | ||
| } else { | ||
| expr | ||
| } | ||
| } | ||
|
|
||
| fn is_display_impl(cx: &LateContext<'_>, item: &'hir Item<'_>) -> bool { | ||
| if_chain! { | ||
| // Are we at an Impl? | ||
| if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), .. }) = &item.kind; | ||
| if let Some(did) = trait_ref.trait_def_id(); | ||
| then { | ||
| // Is it for Display trait? | ||
| match_def_path(cx, did, &paths::DISPLAY_TRAIT) | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| } |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,197 @@ | ||
| #![warn(clippy::recursive_display_impl)] | ||
| #![allow(clippy::inherent_to_string_shadow_display, clippy::to_string_in_format_args)] | ||
|
|
||
| use std::fmt; | ||
|
|
||
| struct A; | ||
| impl A { | ||
| fn fmt(&self) { | ||
| self.to_string(); | ||
| } | ||
| } | ||
|
|
||
| trait B { | ||
| fn fmt(&self) {} | ||
| } | ||
|
|
||
| impl B for A { | ||
| fn fmt(&self) { | ||
| self.to_string(); | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for A { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "{}", self.to_string()) | ||
| } | ||
| } | ||
|
|
||
| fn fmt(a: A) { | ||
| a.to_string(); | ||
| } | ||
|
|
||
| struct C; | ||
|
|
||
| impl C { | ||
| // Doesn't trigger if to_string defined separately | ||
| // i.e. not using ToString trait (from Display) | ||
| fn to_string(&self) -> String { | ||
| String::from("I am C") | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for C { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "{}", self.to_string()) | ||
| } | ||
| } | ||
|
|
||
| enum D { | ||
| E(String), | ||
| F, | ||
| } | ||
|
|
||
| impl std::fmt::Display for D { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match &self { | ||
| Self::E(string) => write!(f, "E {}", string.to_string()), | ||
| Self::F => write!(f, "F"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check for use of self as Display, in Display impl | ||
| // Triggers on direct use of self | ||
| struct G {} | ||
|
|
||
| impl std::fmt::Display for G { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "{}", self) | ||
| } | ||
| } | ||
|
|
||
| // Triggers on reference to self | ||
| struct H {} | ||
|
|
||
| impl std::fmt::Display for H { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "{}", &self) | ||
| } | ||
| } | ||
|
|
||
| // Triggers on multiple reference to self | ||
| struct H2 {} | ||
|
|
||
| impl std::fmt::Display for H2 { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "{}", &&&self) | ||
| } | ||
| } | ||
|
|
||
| // Doesn't trigger on correct deref | ||
| struct I {} | ||
|
|
||
| impl std::ops::Deref for I { | ||
| type Target = str; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| "test" | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for I { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| write!(f, "{}", &**self) | ||
| } | ||
| } | ||
|
|
||
| // Does trigger when deref resolves to self | ||
| struct J {} | ||
|
|
||
| impl std::ops::Deref for J { | ||
| type Target = str; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| "test" | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for J { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| write!(f, "{}", &*self) | ||
| } | ||
| } | ||
|
|
||
| struct J2 {} | ||
|
|
||
| impl std::ops::Deref for J2 { | ||
| type Target = str; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| "test" | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for J2 { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| write!(f, "{}", *self) | ||
| } | ||
| } | ||
|
|
||
| // Doesn't trigger on Debug | ||
| struct K {} | ||
|
|
||
| impl std::fmt::Debug for K { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| write!(f, "test") | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for K { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| write!(f, "{:?}", self) | ||
| } | ||
| } | ||
|
|
||
| // Doesn't trigger on struct fields | ||
| struct L { | ||
| field1: u32, | ||
| field2: i32, | ||
| } | ||
|
|
||
| impl std::fmt::Display for L { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
| write!(f, "{},{}", self.field1, self.field2) | ||
| } | ||
| } | ||
|
|
||
| // Doesn't trigger on nested enum matching | ||
| enum Tree { | ||
| Leaf, | ||
| Node(Vec<Tree>), | ||
| } | ||
|
|
||
| impl std::fmt::Display for Tree { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| match self { | ||
| Tree::Leaf => write!(f, "*"), | ||
| Tree::Node(children) => { | ||
| write!(f, "(")?; | ||
| for child in children.iter() { | ||
| write!(f, "{},", child)?; | ||
| } | ||
| write!(f, ")") | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| let a = A; | ||
| a.to_string(); | ||
| a.fmt(); | ||
| fmt(a); | ||
|
|
||
| let c = C; | ||
| c.to_string(); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| error: using `to_string` in `fmt::Display` implementation might lead to infinite recursion | ||
| --> $DIR/recursive_display_impl.rs:25:25 | ||
| | | ||
| LL | write!(f, "{}", self.to_string()) | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: `-D clippy::recursive-display-impl` implied by `-D warnings` | ||
|
|
||
| error: using `self` in `fmt::Display` implementation might lead to infinite recursion | ||
| --> $DIR/recursive_display_impl.rs:69:9 | ||
| | | ||
| LL | write!(f, "{}", self) | ||
| | ^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error: using `self` in `fmt::Display` implementation might lead to infinite recursion | ||
| --> $DIR/recursive_display_impl.rs:78:9 | ||
| | | ||
| LL | write!(f, "{}", &self) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error: using `self` in `fmt::Display` implementation might lead to infinite recursion | ||
| --> $DIR/recursive_display_impl.rs:87:9 | ||
| | | ||
| LL | write!(f, "{}", &&&self) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error: using `self` in `fmt::Display` implementation might lead to infinite recursion | ||
| --> $DIR/recursive_display_impl.rs:121:9 | ||
| | | ||
| LL | write!(f, "{}", &*self) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error: using `self` in `fmt::Display` implementation might lead to infinite recursion | ||
| --> $DIR/recursive_display_impl.rs:137:9 | ||
| | | ||
| LL | write!(f, "{}", *self) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error: aborting due to 6 previous errors | ||
|
|
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.