Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No to_string in display #5574

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1579,6 +1579,7 @@ Released 2018-09-13
[`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
[`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr
[`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
[`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display
[`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
[`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
[`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines
Expand Down
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ mod swap;
mod tabs_in_doc_comments;
mod temporary_assignment;
mod to_digit_is_some;
mod to_string_in_display;
mod trait_bounds;
mod transmute;
mod transmuting_null;
Expand Down Expand Up @@ -797,6 +798,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&tabs_in_doc_comments::TABS_IN_DOC_COMMENTS,
&temporary_assignment::TEMPORARY_ASSIGNMENT,
&to_digit_is_some::TO_DIGIT_IS_SOME,
&to_string_in_display::TO_STRING_IN_DISPLAY,
&trait_bounds::TYPE_REPETITION_IN_BOUNDS,
&transmute::CROSSPOINTER_TRANSMUTE,
&transmute::TRANSMUTE_BYTES_TO_STR,
Expand Down Expand Up @@ -1017,6 +1019,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_early_pass(|| box reference::DerefAddrOf);
store.register_early_pass(|| box reference::RefInDeref);
store.register_early_pass(|| box double_parens::DoubleParens);
store.register_late_pass(|| box to_string_in_display::ToStringInDisplay);
store.register_early_pass(|| box unsafe_removed_from_name::UnsafeNameRemoval);
store.register_early_pass(|| box if_not_else::IfNotElse);
store.register_early_pass(|| box else_if_without_else::ElseIfWithoutElse);
Expand Down Expand Up @@ -1402,6 +1405,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&tabs_in_doc_comments::TABS_IN_DOC_COMMENTS),
LintId::of(&temporary_assignment::TEMPORARY_ASSIGNMENT),
LintId::of(&to_digit_is_some::TO_DIGIT_IS_SOME),
LintId::of(&to_string_in_display::TO_STRING_IN_DISPLAY),
LintId::of(&transmute::CROSSPOINTER_TRANSMUTE),
LintId::of(&transmute::TRANSMUTE_BYTES_TO_STR),
LintId::of(&transmute::TRANSMUTE_FLOAT_TO_INT),
Expand Down Expand Up @@ -1674,6 +1678,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
LintId::of(&swap::ALMOST_SWAPPED),
LintId::of(&to_string_in_display::TO_STRING_IN_DISPLAY),
LintId::of(&transmute::UNSOUND_COLLECTION_TRANSMUTE),
LintId::of(&transmute::WRONG_TRANSMUTE),
LintId::of(&transmuting_null::TRANSMUTING_NULL),
Expand Down
77 changes: 77 additions & 0 deletions clippy_lints/src/to_string_in_display.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
use crate::utils::{match_def_path, paths, span_lint};
use if_chain::if_chain;
use rustc_hir::{def, Expr, ExprKind, FnDecl, FnSig, ImplItem, ImplItemKind, MutTy, Node, Path, QPath, Ty, TyKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};

declare_clippy_lint! {
/// **What it does:** Checks for uses of `to_string()` when implementing
/// `Display` traits.
///
/// **Why is this bad?** Usually `to_string` is implemented indirectly
/// via `Display`. Hence using it while implementing `Display` would
/// lead to infinite recursion.
///
/// **Known problems:** None.
///
/// **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)
/// }
/// }
/// ```
pub TO_STRING_IN_DISPLAY,
correctness,
"to_string method used while implementing Display trait"
}

declare_lint_pass!(ToStringInDisplay => [TO_STRING_IN_DISPLAY]);

impl LateLintPass<'_, '_> for ToStringInDisplay {
fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
let parent_id = cx.tcx.hir().get_parent_item(expr.hir_id);
let parent_node = cx.tcx.hir().find(parent_id);

if_chain! {
if let ExprKind::MethodCall(ref path, _, _) = expr.kind;
if path.ident.as_str() == "to_string";
if let Some(Node::ImplItem(ImplItem {ident, kind, ..})) = parent_node;
if ident.as_str() == "fmt";
if let ImplItemKind::Fn(FnSig {decl, ..}, _) = kind;
if let FnDecl{inputs, ..} = decl;
if let Ty {kind, ..} = &inputs[0];
if let TyKind::Rptr(_, MutTy {ty, ..}) = kind;
if let Ty {kind, ..} = ty;
if let TyKind::Path(QPath::Resolved(_, path)) = kind;
if let Path{res, ..} = path;
if let def::Res::SelfTy(Some(did), _) = res;
if match_def_path(cx, *did, &paths::DISPLAY_TRAIT);
then {
span_lint(
cx,
TO_STRING_IN_DISPLAY,
expr.span,
"Using to_string in fmt::Display implementation might lead to infinite recursion",
);
}
}
}
}
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2138,6 +2138,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "to_digit_is_some",
},
Lint {
name: "to_string_in_display",
group: "correctness",
desc: "to_string method used while implementing Display trait",
deprecation: None,
module: "to_string_in_display",
},
Lint {
name: "todo",
group: "restriction",
Expand Down
37 changes: 37 additions & 0 deletions tests/ui/to_string_in_display.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#![warn(clippy::to_string_in_display)]

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();
}

fn main() {
let a = A;
a.to_string();
a.fmt();
fmt(a);
}
10 changes: 10 additions & 0 deletions tests/ui/to_string_in_display.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: Using to_string in fmt::Display implementation might lead to infinite recursion
--> $DIR/to_string_in_display.rs:24:25
|
LL | write!(f, "{}", self.to_string())
| ^^^^^^^^^^^^^^^^
|
= note: `-D clippy::to-string-in-display` implied by `-D warnings`

error: aborting due to previous error