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..1e7e5f53cc2 --- /dev/null +++ b/clippy_lints/src/from_over_into.rs @@ -0,0 +1,83 @@ +use crate::utils::paths::INTO; +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, 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. + /// + /// **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<..>`" +} + +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; + 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 to implement `From` instead", + ); + } + } + } + + extract_msrv_attr!(LateContext); +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 02ba422a2f5..35b057d7b6a 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, @@ -1014,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)); @@ -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..18f56f85432 --- /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 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 3848bca3207..0f47f1cbc40 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 34805263104..e3e3b335cbe 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, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 `-> ()`