-
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.
Add recursive_format_trait_impl lint
The to_string_in_display lint is renamed to recursive_format_trait_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.)
- Loading branch information
Showing
13 changed files
with
893 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,269 @@ | ||
| 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, | ||
| ]; | ||
|
|
||
| #[derive(Clone, Copy)] | ||
| enum ImplTrait { | ||
| Debug, | ||
| Display, | ||
| } | ||
|
|
||
| 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` or `Debug` traits inside their 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_FORMAT_TRAIT_IMPL, | ||
| correctness, | ||
| "Format trait method called while implementing the same Format trait" | ||
| } | ||
|
|
||
| #[derive(Default)] | ||
| pub struct RecursiveFormatTraitImpl { | ||
| // Whether we are inside Display or Debug trait impl - None for neither | ||
| format_trait_impl: Option<ImplTrait>, | ||
| // hir_id of self parameter of method inside Display Impl - i.e. fmt(&self) | ||
| self_hir_id: Option<HirId>, | ||
| } | ||
|
|
||
| impl RecursiveFormatTraitImpl { | ||
| pub fn new() -> Self { | ||
| Self { | ||
| format_trait_impl: None, | ||
| self_hir_id: None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl_lint_pass!(RecursiveFormatTraitImpl => [RECURSIVE_FORMAT_TRAIT_IMPL]); | ||
|
|
||
| impl LateLintPass<'_> for RecursiveFormatTraitImpl { | ||
| fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { | ||
| if let Some(format_trait_impl) = is_format_trait_impl(cx, item) { | ||
| self.format_trait_impl = Some(format_trait_impl); | ||
| } | ||
| } | ||
|
|
||
| fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { | ||
| // Assume no nested Impl of Debug and Display within eachother | ||
| if is_format_trait_impl(cx, item).is_some() { | ||
| self.format_trait_impl = None; | ||
| self.self_hir_id = None; | ||
| } | ||
| } | ||
|
|
||
| fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { | ||
| if_chain! { | ||
| // If we are in Display or Debug impl, then get hir_id for self in method impl - i.e. fmt(&self) | ||
| if self.format_trait_impl.is_some(); | ||
| 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 let Some(self_hir_id) = self.self_hir_id { | ||
| match self.format_trait_impl { | ||
| Some(ImplTrait::Display) => { | ||
| check_to_string_in_display(cx, expr, self_hir_id); | ||
| check_self_in_format_args(cx, expr, self_hir_id, ImplTrait::Display); | ||
| }, | ||
| Some(ImplTrait::Debug) => { | ||
| check_self_in_format_args(cx, expr, self_hir_id, ImplTrait::Debug); | ||
| }, | ||
| None => {}, | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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_FORMAT_TRAIT_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, impl_trait: ImplTrait) { | ||
| // 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() { | ||
| match impl_trait { | ||
| // In Display, we only care about Display (it is okay to use Debug) | ||
| ImplTrait::Display => { | ||
| if !arg.is_display() { | ||
| continue; | ||
| }}, | ||
| // In Debug, we only care about Debug (it is okay to use Display and ToString) | ||
| ImplTrait::Debug => { | ||
| if !arg.is_debug() { | ||
| continue; | ||
| }}, | ||
|
|
||
| }; | ||
| check_format_arg_self(cx, expr, self_hir_id, arg, impl_trait); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_format_arg_self( | ||
| cx: &LateContext<'_>, | ||
| expr: &Expr<'_>, | ||
| self_hir_id: HirId, | ||
| arg: &FormatArgsArg<'_>, | ||
| impl_trait: ImplTrait, | ||
| ) { | ||
| // 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) { | ||
| match impl_trait { | ||
| ImplTrait::Display => { | ||
| span_lint( | ||
| cx, | ||
| RECURSIVE_FORMAT_TRAIT_IMPL, | ||
| expr.span, | ||
| "using `self` as Display in `fmt::Display` implementation might lead to infinite recursion", | ||
| ); | ||
| }, | ||
| ImplTrait::Debug => { | ||
| span_lint( | ||
| cx, | ||
| RECURSIVE_FORMAT_TRAIT_IMPL, | ||
| expr.span, | ||
| "using `self` as Debug in `fmt::Debug` 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_format_trait_impl(cx: &LateContext<'_>, item: &'hir Item<'_>) -> Option<ImplTrait> { | ||
| 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? | ||
| if match_def_path(cx, did, &paths::DISPLAY_TRAIT) { | ||
| Some(ImplTrait::Display) | ||
| } | ||
| // Is it for Debug trait? | ||
| else if match_def_path(cx, did, &paths::DEBUG_TRAIT) { | ||
| Some(ImplTrait::Debug) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.