From a451b2af30c173b081f5d6150e558a65062a1dc8 Mon Sep 17 00:00:00 2001 From: Bastian Kersting Date: Sat, 19 Dec 2020 15:49:42 +0100 Subject: [PATCH 1/4] 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 af3b1c1db2ae..de8da99cdee1 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 000000000000..fe7120b0f9a2 --- /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 02ba422a2f5b..58587481922c 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 000000000000..292d0924fb17 --- /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 000000000000..17f30fa837ec --- /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 7afc53613563..a192ebde3ebf 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 96cef1ed5a5f..96041a7dd850 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 c45634c2b6df..02038b5fb6b5 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 `-> ()` From dd005c17e72a48f8579cd26a7165c960b04f6aa3 Mon Sep 17 00:00:00 2001 From: Bastian Kersting Date: Sun, 20 Dec 2020 13:00:17 +0100 Subject: [PATCH 2/4] Added MSRV and fixed typo --- clippy_lints/src/from_over_into.rs | 30 ++++++++++++++++++++++----- clippy_lints/src/lib.rs | 2 +- tests/ui/from_over_into.stderr | 2 +- tests/ui/min_rust_version_attr.rs | 8 +++++++ tests/ui/min_rust_version_attr.stderr | 8 +++---- 5 files changed, 39 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs index fe7120b0f9a2..c7988d6f01fa 100644 --- a/clippy_lints/src/from_over_into.rs +++ b/clippy_lints/src/from_over_into.rs @@ -1,9 +1,12 @@ use crate::utils::paths::INTO; -use crate::utils::{match_def_path, span_lint_and_help}; +use crate::utils::{match_def_path, meets_msrv, 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}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_semver::RustcVersion; +use rustc_session::{declare_tool_lint, impl_lint_pass}; + +const FROM_OVER_INTO_MSRV: RustcVersion = RustcVersion::new(1, 41, 0); declare_clippy_lint! { /// **What it does:** Searches for implementations of the `Into<..>` trait and suggests to implement `From<..>` instead. @@ -38,10 +41,25 @@ declare_clippy_lint! { "Warns on implementations of `Into<..>` to use `From<..>`" } -declare_lint_pass!(FromOverInto => [FROM_OVER_INTO]); +pub struct FromOverInto { + msrv: Option, +} + +impl FromOverInto { + #[must_use] + pub fn new(msrv: Option) -> Self { + FromOverInto { msrv } + } +} + +impl_lint_pass!(FromOverInto => [FROM_OVER_INTO]); impl LateLintPass<'_> for FromOverInto { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { + if !meets_msrv(self.msrv.as_ref(), &FROM_OVER_INTO_MSRV) { + return; + } + let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id); if_chain! { if let hir::ItemKind::Impl{ .. } = &item.kind; @@ -55,9 +73,11 @@ impl LateLintPass<'_> for FromOverInto { 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", + "consider to implement From instead", ); } } } + + extract_msrv_attr!(LateContext); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 58587481922c..35b057d7b6a4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1016,6 +1016,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box checked_conversions::CheckedConversions::new(msrv)); store.register_late_pass(move || box mem_replace::MemReplace::new(msrv)); store.register_late_pass(move || box ranges::Ranges::new(msrv)); + store.register_late_pass(move || box from_over_into::FromOverInto::new(msrv)); store.register_late_pass(move || box use_self::UseSelf::new(msrv)); store.register_late_pass(move || box missing_const_for_fn::MissingConstForFn::new(msrv)); @@ -1205,7 +1206,6 @@ 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)); diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr index 17f30fa837ec..c9c8e7c4b538 100644 --- a/tests/ui/from_over_into.stderr +++ b/tests/ui/from_over_into.stderr @@ -9,7 +9,7 @@ LL | | } | |_^ | = note: `-D clippy::from-over-into` implied by `-D warnings` - = help: consider implement From instead + = help: consider to implement From instead error: aborting due to previous error diff --git a/tests/ui/min_rust_version_attr.rs b/tests/ui/min_rust_version_attr.rs index 3848bca32075..0f47f1cbc402 100644 --- a/tests/ui/min_rust_version_attr.rs +++ b/tests/ui/min_rust_version_attr.rs @@ -57,6 +57,14 @@ pub fn checked_conversion() { let _ = value <= (u32::MAX as i64) && value >= 0; } +pub struct FromOverInto(String); + +impl Into for String { + fn into(self) -> FromOverInto { + FromOverInto(self) + } +} + pub fn filter_map_next() { let a = ["1", "lol", "3", "NaN", "5"]; diff --git a/tests/ui/min_rust_version_attr.stderr b/tests/ui/min_rust_version_attr.stderr index 348052631049..e3e3b335cbe1 100644 --- a/tests/ui/min_rust_version_attr.stderr +++ b/tests/ui/min_rust_version_attr.stderr @@ -1,12 +1,12 @@ error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:142:24 + --> $DIR/min_rust_version_attr.rs:150:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::manual-strip` implied by `-D warnings` note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:141:9 + --> $DIR/min_rust_version_attr.rs:149:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -17,13 +17,13 @@ LL | assert_eq!(.to_uppercase(), "WORLD!"); | error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:154:24 + --> $DIR/min_rust_version_attr.rs:162:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:153:9 + --> $DIR/min_rust_version_attr.rs:161:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 7e641c8be774bba047d7d9ee57455957fd4e2a3b Mon Sep 17 00:00:00 2001 From: Bastian Kersting Date: Sun, 20 Dec 2020 20:10:00 +0100 Subject: [PATCH 3/4] Fixed error messages --- clippy_lints/src/from_over_into.rs | 4 ++-- tests/ui/from_over_into.stderr | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs index c7988d6f01fa..083101ab1c64 100644 --- a/clippy_lints/src/from_over_into.rs +++ b/clippy_lints/src/from_over_into.rs @@ -71,9 +71,9 @@ impl LateLintPass<'_> for FromOverInto { 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.", + "An implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true.", None, - "consider to implement From instead", + "consider to implement `From` instead", ); } } diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr index c9c8e7c4b538..0825d47add24 100644 --- a/tests/ui/from_over_into.stderr +++ b/tests/ui/from_over_into.stderr @@ -1,4 +1,4 @@ -error: An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true. +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 { @@ -9,7 +9,7 @@ LL | | } | |_^ | = note: `-D clippy::from-over-into` implied by `-D warnings` - = help: consider to implement From instead + = help: consider to implement `From` instead error: aborting due to previous error From 0d6c128bfd41b61f246f603cfcd9ee4fc97ca3a0 Mon Sep 17 00:00:00 2001 From: llogiq Date: Mon, 21 Dec 2020 09:18:30 +0100 Subject: [PATCH 4/4] Update clippy_lints/src/from_over_into.rs Co-authored-by: Takayuki Nakata --- clippy_lints/src/from_over_into.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs index 083101ab1c64..1e7e5f53cc2a 100644 --- a/clippy_lints/src/from_over_into.rs +++ b/clippy_lints/src/from_over_into.rs @@ -71,7 +71,7 @@ impl LateLintPass<'_> for FromOverInto { 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.", + "an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true", None, "consider to implement `From` instead", );