From a451b2af30c173b081f5d6150e558a65062a1dc8 Mon Sep 17 00:00:00 2001 From: Bastian Kersting Date: Sat, 19 Dec 2020 15:49:42 +0100 Subject: [PATCH] Added from_over_into lint --- CHANGELOG.md | 1 + clippy_lints/src/from_over_into.rs | 63 ++++++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 5 +++ tests/ui/from_over_into.rs | 21 ++++++++++ tests/ui/from_over_into.stderr | 15 +++++++ tests/ui/unused_unit.fixed | 1 + tests/ui/unused_unit.rs | 1 + tests/ui/unused_unit.stderr | 38 +++++++++--------- 8 files changed, 126 insertions(+), 19 deletions(-) create mode 100644 clippy_lints/src/from_over_into.rs create mode 100644 tests/ui/from_over_into.rs create mode 100644 tests/ui/from_over_into.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index af3b1c1db2a..de8da99cdee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1841,6 +1841,7 @@ Released 2018-09-13 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref [`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect +[`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into [`future_not_send`]: https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs new file mode 100644 index 00000000000..fe7120b0f9a --- /dev/null +++ b/clippy_lints/src/from_over_into.rs @@ -0,0 +1,63 @@ +use crate::utils::paths::INTO; +use crate::utils::{match_def_path, span_lint_and_help}; +use if_chain::if_chain; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Searches for implementations of the `Into<..>` trait and suggests to implement `From<..>` instead. + /// + /// **Why is this bad?** According the std docs implementing `From<..>` is preferred since it gives you `Into<..>` for free where the reverse isn't true. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// struct StringWrapper(String); + /// + /// impl Into for String { + /// fn into(self) -> StringWrapper { + /// StringWrapper(self) + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// struct StringWrapper(String); + /// + /// impl From for StringWrapper { + /// fn from(s: String) -> StringWrapper { + /// StringWrapper(s) + /// } + /// } + /// ``` + pub FROM_OVER_INTO, + style, + "Warns on implementations of `Into<..>` to use `From<..>`" +} + +declare_lint_pass!(FromOverInto => [FROM_OVER_INTO]); + +impl LateLintPass<'_> for FromOverInto { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { + let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id); + if_chain! { + if let hir::ItemKind::Impl{ .. } = &item.kind; + if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(impl_def_id); + if match_def_path(cx, impl_trait_ref.def_id, &INTO); + + then { + span_lint_and_help( + cx, + FROM_OVER_INTO, + item.span, + "An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true.", + None, + "consider implement From instead", + ); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 02ba422a2f5..58587481922 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -207,6 +207,7 @@ mod float_literal; mod floating_point_arithmetic; mod format; mod formatting; +mod from_over_into; mod functions; mod future_not_send; mod get_last_with_len; @@ -614,6 +615,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING, &formatting::SUSPICIOUS_ELSE_FORMATTING, &formatting::SUSPICIOUS_UNARY_OP_FORMATTING, + &from_over_into::FROM_OVER_INTO, &functions::DOUBLE_MUST_USE, &functions::MUST_USE_CANDIDATE, &functions::MUST_USE_UNIT, @@ -1203,6 +1205,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box manual_unwrap_or::ManualUnwrapOr); store.register_late_pass(|| box manual_ok_or::ManualOkOr); store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs); + store.register_late_pass(|| box from_over_into::FromOverInto); store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync); let disallowed_methods = conf.disallowed_methods.iter().cloned().collect::>(); store.register_late_pass(move || box disallowed_method::DisallowedMethod::new(&disallowed_methods)); @@ -1417,6 +1420,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING), LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING), LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING), + LintId::of(&from_over_into::FROM_OVER_INTO), LintId::of(&functions::DOUBLE_MUST_USE), LintId::of(&functions::MUST_USE_UNIT), LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF), @@ -1663,6 +1667,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING), LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING), LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING), + LintId::of(&from_over_into::FROM_OVER_INTO), LintId::of(&functions::DOUBLE_MUST_USE), LintId::of(&functions::MUST_USE_UNIT), LintId::of(&functions::RESULT_UNIT_ERR), diff --git a/tests/ui/from_over_into.rs b/tests/ui/from_over_into.rs new file mode 100644 index 00000000000..292d0924fb1 --- /dev/null +++ b/tests/ui/from_over_into.rs @@ -0,0 +1,21 @@ +#![warn(clippy::from_over_into)] + +// this should throw an error +struct StringWrapper(String); + +impl Into for String { + fn into(self) -> StringWrapper { + StringWrapper(self) + } +} + +// this is fine +struct A(String); + +impl From for A { + fn from(s: String) -> A { + A(s) + } +} + +fn main() {} diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr new file mode 100644 index 00000000000..17f30fa837e --- /dev/null +++ b/tests/ui/from_over_into.stderr @@ -0,0 +1,15 @@ +error: An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true. + --> $DIR/from_over_into.rs:6:1 + | +LL | / impl Into for String { +LL | | fn into(self) -> StringWrapper { +LL | | StringWrapper(self) +LL | | } +LL | | } + | |_^ + | + = note: `-D clippy::from-over-into` implied by `-D warnings` + = help: consider implement From instead + +error: aborting due to previous error + diff --git a/tests/ui/unused_unit.fixed b/tests/ui/unused_unit.fixed index 7afc5361356..a192ebde3eb 100644 --- a/tests/ui/unused_unit.fixed +++ b/tests/ui/unused_unit.fixed @@ -11,6 +11,7 @@ #![deny(clippy::unused_unit)] #![allow(dead_code)] +#![allow(clippy::from_over_into)] struct Unitter; impl Unitter { diff --git a/tests/ui/unused_unit.rs b/tests/ui/unused_unit.rs index 96cef1ed5a5..96041a7dd85 100644 --- a/tests/ui/unused_unit.rs +++ b/tests/ui/unused_unit.rs @@ -11,6 +11,7 @@ #![deny(clippy::unused_unit)] #![allow(dead_code)] +#![allow(clippy::from_over_into)] struct Unitter; impl Unitter { diff --git a/tests/ui/unused_unit.stderr b/tests/ui/unused_unit.stderr index c45634c2b6d..02038b5fb6b 100644 --- a/tests/ui/unused_unit.stderr +++ b/tests/ui/unused_unit.stderr @@ -1,5 +1,5 @@ error: unneeded unit return type - --> $DIR/unused_unit.rs:18:28 + --> $DIR/unused_unit.rs:19:28 | LL | pub fn get_unit (), G>(&self, f: F, _g: G) -> () | ^^^^^^ help: remove the `-> ()` @@ -11,109 +11,109 @@ LL | #![deny(clippy::unused_unit)] | ^^^^^^^^^^^^^^^^^^^ error: unneeded unit return type - --> $DIR/unused_unit.rs:19:18 + --> $DIR/unused_unit.rs:20:18 | LL | where G: Fn() -> () { | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:18:58 + --> $DIR/unused_unit.rs:19:58 | LL | pub fn get_unit (), G>(&self, f: F, _g: G) -> () | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:20:26 + --> $DIR/unused_unit.rs:21:26 | LL | let _y: &dyn Fn() -> () = &f; | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:27:18 + --> $DIR/unused_unit.rs:28:18 | LL | fn into(self) -> () { | ^^^^^^ help: remove the `-> ()` error: unneeded unit expression - --> $DIR/unused_unit.rs:28:9 + --> $DIR/unused_unit.rs:29:9 | LL | () | ^^ help: remove the final `()` error: unneeded unit return type - --> $DIR/unused_unit.rs:33:29 + --> $DIR/unused_unit.rs:34:29 | LL | fn redundant (), G, H>(&self, _f: F, _g: G, _h: H) | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:35:19 + --> $DIR/unused_unit.rs:36:19 | LL | G: FnMut() -> (), | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:36:16 + --> $DIR/unused_unit.rs:37:16 | LL | H: Fn() -> (); | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:40:29 + --> $DIR/unused_unit.rs:41:29 | LL | fn redundant (), G, H>(&self, _f: F, _g: G, _h: H) | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:42:19 + --> $DIR/unused_unit.rs:43:19 | LL | G: FnMut() -> (), | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:43:16 + --> $DIR/unused_unit.rs:44:16 | LL | H: Fn() -> () {} | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:46:17 + --> $DIR/unused_unit.rs:47:17 | LL | fn return_unit() -> () { () } | ^^^^^^ help: remove the `-> ()` error: unneeded unit expression - --> $DIR/unused_unit.rs:46:26 + --> $DIR/unused_unit.rs:47:26 | LL | fn return_unit() -> () { () } | ^^ help: remove the final `()` error: unneeded `()` - --> $DIR/unused_unit.rs:56:14 + --> $DIR/unused_unit.rs:57:14 | LL | break(); | ^^ help: remove the `()` error: unneeded `()` - --> $DIR/unused_unit.rs:58:11 + --> $DIR/unused_unit.rs:59:11 | LL | return(); | ^^ help: remove the `()` error: unneeded unit return type - --> $DIR/unused_unit.rs:75:10 + --> $DIR/unused_unit.rs:76:10 | LL | fn test()->(){} | ^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:78:11 + --> $DIR/unused_unit.rs:79:11 | LL | fn test2() ->(){} | ^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:81:11 + --> $DIR/unused_unit.rs:82:11 | LL | fn test3()-> (){} | ^^^^^ help: remove the `-> ()`