diff --git a/CHANGELOG.md b/CHANGELOG.md index d96c74b6e412..573bedfebd65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2091,6 +2091,7 @@ Released 2018-09-13 [`for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles [`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_instead_of_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_instead_of_into [`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 [`from_str_radix_10`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_str_radix_10 diff --git a/clippy_lints/src/from_instead_of_into.rs b/clippy_lints/src/from_instead_of_into.rs new file mode 100644 index 000000000000..d690249ed7e2 --- /dev/null +++ b/clippy_lints/src/from_instead_of_into.rs @@ -0,0 +1,119 @@ +use crate::utils::span_lint_and_then; +use crate::utils::snippet_opt; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{GenericBound, WherePredicate}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::sym; + +declare_clippy_lint! { + /// **What it does:** Checking for using of From or TryFrom trait as a generic bound. + /// + /// **Why is this bad?** Into and TryInto are supersets of From and TryFrom. Due to + /// coherence rules, sometimes From and TryFrom are forbid to implemented but Into and + /// TryInto are not. So Into is a more generic bound than From, We should choose Into or + /// TryInto instead of From or TryFrom. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// fn foo(a: T) where u32: From {} + /// fn bar(a: T) where u32: TryFrom {} + /// ``` + /// Use instead: + /// ```rust + /// fn foo(a: T) where T: Into {} + /// fn bar(a: T) where T: TryInto {} + /// ``` + pub FROM_INSTEAD_OF_INTO, + style, + "Into or TryInto trait is a better choice than From or TryFrom trait as a generic bound" +} + +declare_lint_pass!(FromInsteadOfInto => [FROM_INSTEAD_OF_INTO]); + +impl LateLintPass<'tcx> for FromInsteadOfInto { + fn check_where_predicate(&mut self, cx: &LateContext<'tcx>, wp: &'tcx WherePredicate<'tcx>) { + fn is_target_generic_bound(cx: &LateContext<'tcx>, b: &GenericBound<'_>) -> bool { + if_chain! { + if let Some(r) = b.trait_ref(); + if let Some(def_id) = r.trait_def_id(); + then { + cx.tcx.is_diagnostic_item(sym::from_trait, def_id) || + cx.tcx.is_diagnostic_item(sym::try_from_trait, def_id) + } else { + false + } + } + } + + if let WherePredicate::BoundPredicate(wbp) = wp { + if_chain! { + let bounds = wbp.bounds; + if let Some(position) = bounds.iter().position(|b| is_target_generic_bound(cx, b)); + let target_bound = &bounds[position]; + if let Some(tr_ref) = target_bound.trait_ref(); + if let Some(def_id) = tr_ref.trait_def_id(); + if let Some(last_seg) = tr_ref.path.segments.last(); + if let Some(generic_arg) = last_seg.args().args.first(); + if let Some(bounded_ty) = snippet_opt(cx, wbp.bounded_ty.span); + if let Some(generic_arg_of_from_or_try_from) = snippet_opt(cx, generic_arg.span()); + then { + let replace_trait_name; + let target_trait_name; + if cx.tcx.is_diagnostic_item(sym::from_trait, def_id) { + replace_trait_name = "Into"; + target_trait_name = "From"; + } else if cx.tcx.is_diagnostic_item(sym::try_from_trait, def_id) { + replace_trait_name = "TryInto"; + target_trait_name = "TryFrom"; + } else { + return; + } + let message = format!("{} trait is preferable than {} as a generic bound", replace_trait_name, target_trait_name); + let switched_predicate = format!("{}: {}<{}>", generic_arg_of_from_or_try_from, replace_trait_name, bounded_ty); + + let low; + if position == 0 { + low = wp.span().lo(); + } else { + let previous_bound = &bounds[position -1]; + low = previous_bound.span().hi(); + } + let removed_span = target_bound.span().with_lo(low); + + span_lint_and_then( + cx, + FROM_INSTEAD_OF_INTO, + wp.span(), + &message, + |diag| { + diag.span_suggestion( + removed_span, + &format!("remove {} bound", target_trait_name), + "".to_string(), + Applicability::MaybeIncorrect, + ); + + let sugg; + if bounds.len() == 1 { + sugg = switched_predicate; + } else { + sugg = format!(", {}", switched_predicate); + } + diag.span_suggestion( + wp.span().with_lo(wp.span().hi()), + "Add this bound predicate", + sugg, + Applicability::MaybeIncorrect, + ); + } + ); + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d66e3720be0d..5f04f4a8860a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -12,6 +12,7 @@ #![feature(rustc_private)] #![feature(stmt_expr_attributes)] #![feature(control_flow_enum)] +#![feature(array_map)] #![recursion_limit = "512"] #![cfg_attr(feature = "deny-warnings", deny(warnings))] #![allow(clippy::missing_docs_in_private_items, clippy::must_use_candidate)] @@ -210,6 +211,7 @@ mod float_literal; mod floating_point_arithmetic; mod format; mod formatting; +mod from_instead_of_into; mod from_over_into; mod from_str_radix_10; mod functions; @@ -641,6 +643,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_instead_of_into::FROM_INSTEAD_OF_INTO, &from_over_into::FROM_OVER_INTO, &from_str_radix_10::FROM_STR_RADIX_10, &functions::DOUBLE_MUST_USE, @@ -1268,6 +1271,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box redundant_slicing::RedundantSlicing); store.register_late_pass(|| box from_str_radix_10::FromStrRadix10); store.register_late_pass(|| box manual_map::ManualMap); + store.register_late_pass(|| box from_instead_of_into::FromInsteadOfInto); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1482,6 +1486,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_instead_of_into::FROM_INSTEAD_OF_INTO), LintId::of(&from_over_into::FROM_OVER_INTO), LintId::of(&from_str_radix_10::FROM_STR_RADIX_10), LintId::of(&functions::DOUBLE_MUST_USE), @@ -1739,6 +1744,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_instead_of_into::FROM_INSTEAD_OF_INTO), LintId::of(&from_over_into::FROM_OVER_INTO), LintId::of(&from_str_radix_10::FROM_STR_RADIX_10), LintId::of(&functions::DOUBLE_MUST_USE), diff --git a/tests/ui/from_instead_of_into.fixed b/tests/ui/from_instead_of_into.fixed new file mode 100644 index 000000000000..ff9ddb2d87d9 --- /dev/null +++ b/tests/ui/from_instead_of_into.fixed @@ -0,0 +1,39 @@ +// run-rustfix +#![allow(unused_imports)] +#![allow(unused_variables)] +#![allow(dead_code)] +#![warn(clippy::from_instead_of_into)] +use std::convert::TryFrom; +use std::convert::TryInto; + +fn foo(a: T) +where + T: Into, +{ +} + +fn foo1(a: T) +where + T: Into, u32: Copy + Clone, +{ +} + +fn bar(a: T) +where + T: TryInto, +{ +} + +fn bar1(a: T) +where + T: TryInto, u32: Copy + Clone, +{ +} + +fn bar2(a: T) +where + T: TryInto, u32: Copy + Clone, +{ +} + +fn main() {} diff --git a/tests/ui/from_instead_of_into.rs b/tests/ui/from_instead_of_into.rs new file mode 100644 index 000000000000..b56f4a10c164 --- /dev/null +++ b/tests/ui/from_instead_of_into.rs @@ -0,0 +1,39 @@ +// run-rustfix +#![allow(unused_imports)] +#![allow(unused_variables)] +#![allow(dead_code)] +#![warn(clippy::from_instead_of_into)] +use std::convert::TryFrom; +use std::convert::TryInto; + +fn foo(a: T) +where + u32: From, +{ +} + +fn foo1(a: T) +where + u32: Copy + Clone + From, +{ +} + +fn bar(a: T) +where + u32: TryFrom, +{ +} + +fn bar1(a: T) +where + u32: Copy + TryFrom + Clone, +{ +} + +fn bar2(a: T) +where + u32: TryFrom + Copy + Clone, +{ +} + +fn main() {} diff --git a/tests/ui/from_instead_of_into.stderr b/tests/ui/from_instead_of_into.stderr new file mode 100644 index 000000000000..04a1acc88f35 --- /dev/null +++ b/tests/ui/from_instead_of_into.stderr @@ -0,0 +1,78 @@ +error: Into trait is preferable than From as a generic bound + --> $DIR/from_instead_of_into.rs:11:5 + | +LL | u32: From, + | ^^^^^^^^^^^^ + | + = note: `-D clippy::from-instead-of-into` implied by `-D warnings` +help: remove From bound + | +LL | , + | -- +help: Add this bound predicate + | +LL | u32: FromT: Into, + | ^^^^^^^^^^^^ + +error: Into trait is preferable than From as a generic bound + --> $DIR/from_instead_of_into.rs:17:5 + | +LL | u32: Copy + Clone + From, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove From bound + | +LL | u32: Copy + Clone, + | -- +help: Add this bound predicate + | +LL | u32: Copy + Clone + From, T: Into, + | ^^^^^^^^^^^^^^ + +error: TryInto trait is preferable than TryFrom as a generic bound + --> $DIR/from_instead_of_into.rs:23:5 + | +LL | u32: TryFrom, + | ^^^^^^^^^^^^^^^ + | +help: remove TryFrom bound + | +LL | , + | -- +help: Add this bound predicate + | +LL | u32: TryFromT: TryInto, + | ^^^^^^^^^^^^^^^ + +error: TryInto trait is preferable than TryFrom as a generic bound + --> $DIR/from_instead_of_into.rs:29:5 + | +LL | u32: Copy + TryFrom + Clone, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove TryFrom bound + | +LL | u32: Copy + Clone, + | -- +help: Add this bound predicate + | +LL | u32: Copy + TryFrom + Clone, T: TryInto, + | ^^^^^^^^^^^^^^^^^ + +error: TryInto trait is preferable than TryFrom as a generic bound + --> $DIR/from_instead_of_into.rs:35:5 + | +LL | u32: TryFrom + Copy + Clone, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove TryFrom bound + | +LL | + Copy + Clone, + | -- +help: Add this bound predicate + | +LL | u32: TryFrom + Copy + Clone, T: TryInto, + | ^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors + diff --git a/tests/ui/needless_question_mark.fixed b/tests/ui/needless_question_mark.fixed index 71fb3565224e..0136e2b4cbdf 100644 --- a/tests/ui/needless_question_mark.fixed +++ b/tests/ui/needless_question_mark.fixed @@ -87,6 +87,7 @@ fn also_bad(tr: Result) -> Result { Err(false) } +#[allow(clippy::from_instead_of_into)] fn false_positive_test(x: Result<(), U>) -> Result<(), T> where T: From, diff --git a/tests/ui/needless_question_mark.rs b/tests/ui/needless_question_mark.rs index e31f6f48fa7c..f43705f397f5 100644 --- a/tests/ui/needless_question_mark.rs +++ b/tests/ui/needless_question_mark.rs @@ -87,6 +87,7 @@ fn also_bad(tr: Result) -> Result { Err(false) } +#[allow(clippy::from_instead_of_into)] fn false_positive_test(x: Result<(), U>) -> Result<(), T> where T: From, diff --git a/tests/ui/needless_question_mark.stderr b/tests/ui/needless_question_mark.stderr index 567bc518a3fd..6237d44e3423 100644 --- a/tests/ui/needless_question_mark.stderr +++ b/tests/ui/needless_question_mark.stderr @@ -67,19 +67,19 @@ LL | return Ok(t.magic?); | ^^^^^^^^^^^^ help: try: `t.magic` error: Question mark operator is useless here - --> $DIR/needless_question_mark.rs:138:9 + --> $DIR/needless_question_mark.rs:139:9 | LL | Ok(to.magic?) // should be triggered | ^^^^^^^^^^^^^ help: try: `to.magic` error: Question mark operator is useless here - --> $DIR/needless_question_mark.rs:154:9 + --> $DIR/needless_question_mark.rs:155:9 | LL | Some(to.magic?) // should be triggered | ^^^^^^^^^^^^^^^ help: try: `to.magic` error: Question mark operator is useless here - --> $DIR/needless_question_mark.rs:162:9 + --> $DIR/needless_question_mark.rs:163:9 | LL | Ok(to.magic?) // should be triggered | ^^^^^^^^^^^^^ help: try: `to.magic`