-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
- Loading branch information
Showing
11 changed files
with
424 additions
and
208 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,228 @@ | ||
| 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}; | ||
| 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<'_>) { | ||
| if_chain! { | ||
| let reference = deref_expr(arg.value); | ||
| if path_to_local_id(reference, self_hir_id); | ||
| then { | ||
| span_lint( | ||
| cx, | ||
| RECURSIVE_DISPLAY_IMPL, | ||
| expr.span, | ||
| "using reference to `self` in `fmt::Display` implementation might lead to infinite recursion",); | ||
| } | ||
| else { | ||
| if_chain! { | ||
| let arg_ty = cx.typeck_results().expr_ty_adjusted(arg.value); | ||
| let self_ty = cx.typeck_results().node_type(self_hir_id); | ||
| if self_ty == arg_ty; | ||
| then { | ||
| 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 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 | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.