diff --git a/CHANGELOG.md b/CHANGELOG.md index b28075d8e..993406b69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5528,6 +5528,7 @@ Released 2018-09-13 [`unknown_clippy_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints [`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast +[`unnecessary_fallible_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map [`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map [`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 8a3415133..d9c97a8cc 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -430,6 +430,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::TYPE_ID_ON_BOX_INFO, crate::methods::UNINIT_ASSUMED_INIT_INFO, crate::methods::UNIT_HASH_INFO, + crate::methods::UNNECESSARY_FALLIBLE_CONVERSIONS_INFO, crate::methods::UNNECESSARY_FILTER_MAP_INFO, crate::methods::UNNECESSARY_FIND_MAP_INFO, crate::methods::UNNECESSARY_FOLD_INFO, diff --git a/clippy_lints/src/implicit_saturating_add.rs b/clippy_lints/src/implicit_saturating_add.rs index 453797f18..24f62490f 100644 --- a/clippy_lints/src/implicit_saturating_add.rs +++ b/clippy_lints/src/implicit_saturating_add.rs @@ -82,18 +82,18 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingAdd { fn get_int_max(ty: Ty<'_>) -> Option { match ty.peel_refs().kind() { - Int(IntTy::I8) => i8::max_value().try_into().ok(), - Int(IntTy::I16) => i16::max_value().try_into().ok(), - Int(IntTy::I32) => i32::max_value().try_into().ok(), - Int(IntTy::I64) => i64::max_value().try_into().ok(), - Int(IntTy::I128) => i128::max_value().try_into().ok(), - Int(IntTy::Isize) => isize::max_value().try_into().ok(), - Uint(UintTy::U8) => u8::max_value().try_into().ok(), - Uint(UintTy::U16) => u16::max_value().try_into().ok(), - Uint(UintTy::U32) => u32::max_value().try_into().ok(), - Uint(UintTy::U64) => u64::max_value().try_into().ok(), - Uint(UintTy::U128) => Some(u128::max_value()), - Uint(UintTy::Usize) => usize::max_value().try_into().ok(), + Int(IntTy::I8) => i8::MAX.try_into().ok(), + Int(IntTy::I16) => i16::MAX.try_into().ok(), + Int(IntTy::I32) => i32::MAX.try_into().ok(), + Int(IntTy::I64) => i64::MAX.try_into().ok(), + Int(IntTy::I128) => i128::MAX.try_into().ok(), + Int(IntTy::Isize) => isize::MAX.try_into().ok(), + Uint(UintTy::U8) => Some(u8::MAX.into()), + Uint(UintTy::U16) => Some(u16::MAX.into()), + Uint(UintTy::U32) => Some(u32::MAX.into()), + Uint(UintTy::U64) => Some(u64::MAX.into()), + Uint(UintTy::U128) => Some(u128::MAX), + Uint(UintTy::Usize) => usize::MAX.try_into().ok(), _ => None, } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index e1a14e293..90f6bc5e3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -99,6 +99,7 @@ mod suspicious_to_owned; mod type_id_on_box; mod uninit_assumed_init; mod unit_hash; +mod unnecessary_fallible_conversions; mod unnecessary_filter_map; mod unnecessary_fold; mod unnecessary_iter_cloned; @@ -3655,6 +3656,33 @@ declare_clippy_lint! { "cloning a `Waker` only to wake it" } +declare_clippy_lint! { + /// ### What it does + /// Checks for calls to `TryInto::try_into` and `TryFrom::try_from` when their infallible counterparts + /// could be used. + /// + /// ### Why is this bad? + /// In those cases, the `TryInto` and `TryFrom` trait implementation is a blanket impl that forwards + /// to `Into` or `From`, which always succeeds. + /// The returned `Result<_, Infallible>` requires error handling to get the contained value + /// even though the conversion can never fail. + /// + /// ### Example + /// ```rust + /// let _: Result = 1i32.try_into(); + /// let _: Result = <_>::try_from(1i32); + /// ``` + /// Use `from`/`into` instead: + /// ```rust + /// let _: i64 = 1i32.into(); + /// let _: i64 = <_>::from(1i32); + /// ``` + #[clippy::version = "1.75.0"] + pub UNNECESSARY_FALLIBLE_CONVERSIONS, + style, + "calling the `try_from` and `try_into` trait methods when `From`/`Into` is implemented" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3801,6 +3829,7 @@ impl_lint_pass!(Methods => [ PATH_ENDS_WITH_EXT, REDUNDANT_AS_STR, WAKER_CLONE_WAKE, + UNNECESSARY_FALLIBLE_CONVERSIONS, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -3827,6 +3856,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods { match expr.kind { hir::ExprKind::Call(func, args) => { from_iter_instead_of_collect::check(cx, expr, args, func); + unnecessary_fallible_conversions::check_function(cx, expr, func); }, hir::ExprKind::MethodCall(method_call, receiver, args, _) => { let method_span = method_call.ident.span; @@ -4316,6 +4346,9 @@ impl Methods { } unnecessary_lazy_eval::check(cx, expr, recv, arg, "then_some"); }, + ("try_into", []) if is_trait_method(cx, expr, sym::TryInto) => { + unnecessary_fallible_conversions::check_method(cx, expr); + } ("to_owned", []) => { if !suspicious_to_owned::check(cx, expr, recv) { implicit_clone::check(cx, name, expr, recv); diff --git a/clippy_lints/src/methods/unnecessary_fallible_conversions.rs b/clippy_lints/src/methods/unnecessary_fallible_conversions.rs new file mode 100644 index 000000000..6660ad215 --- /dev/null +++ b/clippy_lints/src/methods/unnecessary_fallible_conversions.rs @@ -0,0 +1,119 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::get_parent_expr; +use clippy_utils::ty::implements_trait; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::{sym, Span}; + +use super::UNNECESSARY_FALLIBLE_CONVERSIONS; + +/// What function is being called and whether that call is written as a method call or a function +/// call +#[derive(Copy, Clone)] +#[expect(clippy::enum_variant_names)] +enum FunctionKind { + /// `T::try_from(U)` + TryFromFunction, + /// `t.try_into()` + TryIntoMethod, + /// `U::try_into(t)` + TryIntoFunction, +} + +fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &Expr<'_>, + node_args: ty::GenericArgsRef<'tcx>, + kind: FunctionKind, + primary_span: Span, +) { + if let &[self_ty, other_ty] = node_args.as_slice() + // useless_conversion already warns `T::try_from(T)`, so ignore it here + && self_ty != other_ty + && let Some(self_ty) = self_ty.as_type() + && let Some(from_into_trait) = cx.tcx.get_diagnostic_item(match kind { + FunctionKind::TryFromFunction => sym::From, + FunctionKind::TryIntoMethod | FunctionKind::TryIntoFunction => sym::Into, + }) + // If `T: TryFrom` and `T: From` both exist, then that means that the `TryFrom` + // _must_ be from the blanket impl and cannot have been manually implemented + // (else there would be conflicting impls, even with #![feature(spec)]), so we don't even need to check + // what `>::Error` is: it's always `Infallible` + && implements_trait(cx, self_ty, from_into_trait, &[other_ty]) + { + let parent_unwrap_call = get_parent_expr(cx, expr) + .and_then(|parent| { + if let ExprKind::MethodCall(path, .., span) = parent.kind + && let sym::unwrap | sym::expect = path.ident.name + { + Some(span) + } else { + None + } + }); + + let (sugg, span, applicability) = match kind { + FunctionKind::TryIntoMethod if let Some(unwrap_span) = parent_unwrap_call => { + // Extend the span to include the unwrap/expect call: + // `foo.try_into().expect("..")` + // ^^^^^^^^^^^^^^^^^^^^^^^ + // + // `try_into().unwrap()` specifically can be trivially replaced with just `into()`, + // so that can be machine-applicable + + ("into()", primary_span.with_hi(unwrap_span.hi()), Applicability::MachineApplicable) + } + FunctionKind::TryFromFunction => ("From::from", primary_span, Applicability::Unspecified), + FunctionKind::TryIntoFunction => ("Into::into", primary_span, Applicability::Unspecified), + FunctionKind::TryIntoMethod => ("into", primary_span, Applicability::Unspecified), + }; + + span_lint_and_sugg( + cx, + UNNECESSARY_FALLIBLE_CONVERSIONS, + span, + "use of a fallible conversion when an infallible one could be used", + "use", + sugg.into(), + applicability + ); + } +} + +/// Checks method call exprs: +/// - `0i32.try_into()` +pub(super) fn check_method(cx: &LateContext<'_>, expr: &Expr<'_>) { + if let ExprKind::MethodCall(path, ..) = expr.kind { + check( + cx, + expr, + cx.typeck_results().node_args(expr.hir_id), + FunctionKind::TryIntoMethod, + path.ident.span, + ); + } +} + +/// Checks function call exprs: +/// - `>::try_from(0i32)` +/// - `<_ as TryInto>::try_into(0i32)` +pub(super) fn check_function(cx: &LateContext<'_>, expr: &Expr<'_>, callee: &Expr<'_>) { + if let ExprKind::Path(ref qpath) = callee.kind + && let Some(item_def_id) = cx.qpath_res(qpath, callee.hir_id).opt_def_id() + && let Some(trait_def_id) = cx.tcx.trait_of_item(item_def_id) + { + check( + cx, + expr, + cx.typeck_results().node_args(callee.hir_id), + match cx.tcx.get_diagnostic_name(trait_def_id) { + Some(sym::TryFrom) => FunctionKind::TryFromFunction, + Some(sym::TryInto) => FunctionKind::TryIntoFunction, + _ => return, + }, + callee.span, + ); + } +} diff --git a/tests/ui/manual_string_new.fixed b/tests/ui/manual_string_new.fixed index 273be4e0f..2d4c5a029 100644 --- a/tests/ui/manual_string_new.fixed +++ b/tests/ui/manual_string_new.fixed @@ -1,4 +1,5 @@ #![warn(clippy::manual_string_new)] +#![allow(clippy::unnecessary_fallible_conversions)] macro_rules! create_strings_from_macro { // When inside a macro, nothing should warn to prevent false positives. diff --git a/tests/ui/manual_string_new.rs b/tests/ui/manual_string_new.rs index 0d5514fc8..20f0be6aa 100644 --- a/tests/ui/manual_string_new.rs +++ b/tests/ui/manual_string_new.rs @@ -1,4 +1,5 @@ #![warn(clippy::manual_string_new)] +#![allow(clippy::unnecessary_fallible_conversions)] macro_rules! create_strings_from_macro { // When inside a macro, nothing should warn to prevent false positives. diff --git a/tests/ui/manual_string_new.stderr b/tests/ui/manual_string_new.stderr index 399652d3f..cb2d78c39 100644 --- a/tests/ui/manual_string_new.stderr +++ b/tests/ui/manual_string_new.stderr @@ -1,5 +1,5 @@ error: empty String is being created manually - --> $DIR/manual_string_new.rs:13:13 + --> $DIR/manual_string_new.rs:14:13 | LL | let _ = "".to_string(); | ^^^^^^^^^^^^^^ help: consider using: `String::new()` @@ -8,49 +8,49 @@ LL | let _ = "".to_string(); = help: to override `-D warnings` add `#[allow(clippy::manual_string_new)]` error: empty String is being created manually - --> $DIR/manual_string_new.rs:16:13 + --> $DIR/manual_string_new.rs:17:13 | LL | let _ = "".to_owned(); | ^^^^^^^^^^^^^ help: consider using: `String::new()` error: empty String is being created manually - --> $DIR/manual_string_new.rs:19:21 + --> $DIR/manual_string_new.rs:20:21 | LL | let _: String = "".into(); | ^^^^^^^^^ help: consider using: `String::new()` error: empty String is being created manually - --> $DIR/manual_string_new.rs:26:13 + --> $DIR/manual_string_new.rs:27:13 | LL | let _ = String::from(""); | ^^^^^^^^^^^^^^^^ help: consider using: `String::new()` error: empty String is being created manually - --> $DIR/manual_string_new.rs:27:13 + --> $DIR/manual_string_new.rs:28:13 | LL | let _ = ::from(""); | ^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` error: empty String is being created manually - --> $DIR/manual_string_new.rs:32:13 + --> $DIR/manual_string_new.rs:33:13 | LL | let _ = String::try_from("").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` error: empty String is being created manually - --> $DIR/manual_string_new.rs:38:21 + --> $DIR/manual_string_new.rs:39:21 | LL | let _: String = From::from(""); | ^^^^^^^^^^^^^^ help: consider using: `String::new()` error: empty String is being created manually - --> $DIR/manual_string_new.rs:43:21 + --> $DIR/manual_string_new.rs:44:21 | LL | let _: String = TryFrom::try_from("").unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` error: empty String is being created manually - --> $DIR/manual_string_new.rs:46:21 + --> $DIR/manual_string_new.rs:47:21 | LL | let _: String = TryFrom::try_from("").expect("this should warn"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `String::new()` diff --git a/tests/ui/unnecessary_fallible_conversions.fixed b/tests/ui/unnecessary_fallible_conversions.fixed new file mode 100644 index 000000000..9668a6b99 --- /dev/null +++ b/tests/ui/unnecessary_fallible_conversions.fixed @@ -0,0 +1,6 @@ +#![warn(clippy::unnecessary_fallible_conversions)] + +fn main() { + let _: i64 = 0i32.into(); + let _: i64 = 0i32.into(); +} diff --git a/tests/ui/unnecessary_fallible_conversions.rs b/tests/ui/unnecessary_fallible_conversions.rs new file mode 100644 index 000000000..9fa6c08b1 --- /dev/null +++ b/tests/ui/unnecessary_fallible_conversions.rs @@ -0,0 +1,6 @@ +#![warn(clippy::unnecessary_fallible_conversions)] + +fn main() { + let _: i64 = 0i32.try_into().unwrap(); + let _: i64 = 0i32.try_into().expect("can't happen"); +} diff --git a/tests/ui/unnecessary_fallible_conversions.stderr b/tests/ui/unnecessary_fallible_conversions.stderr new file mode 100644 index 000000000..b918fdf77 --- /dev/null +++ b/tests/ui/unnecessary_fallible_conversions.stderr @@ -0,0 +1,17 @@ +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions.rs:4:23 + | +LL | let _: i64 = 0i32.try_into().unwrap(); + | ^^^^^^^^^^^^^^^^^^^ help: use: `into()` + | + = note: `-D clippy::unnecessary-fallible-conversions` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_fallible_conversions)]` + +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions.rs:5:23 + | +LL | let _: i64 = 0i32.try_into().expect("can't happen"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `into()` + +error: aborting due to 2 previous errors + diff --git a/tests/ui/unnecessary_fallible_conversions_unfixable.rs b/tests/ui/unnecessary_fallible_conversions_unfixable.rs new file mode 100644 index 000000000..68e617cc0 --- /dev/null +++ b/tests/ui/unnecessary_fallible_conversions_unfixable.rs @@ -0,0 +1,43 @@ +//@aux-build:proc_macros.rs +//@no-rustfix +#![warn(clippy::unnecessary_fallible_conversions)] + +extern crate proc_macros; + +struct Foo; +impl TryFrom for Foo { + type Error = (); + fn try_from(_: i32) -> Result { + Ok(Foo) + } +} +impl From for Foo { + fn from(_: i64) -> Self { + Foo + } +} + +fn main() { + // `Foo` only implements `TryFrom` and not `From`, so don't lint + let _: Result = 0i32.try_into(); + let _: Result = i32::try_into(0i32); + let _: Result = Foo::try_from(0i32); + + // ... it does impl From however + let _: Result = 0i64.try_into(); + //~^ ERROR: use of a fallible conversion when an infallible one could be used + let _: Result = i64::try_into(0i64); + //~^ ERROR: use of a fallible conversion when an infallible one could be used + let _: Result = Foo::try_from(0i64); + //~^ ERROR: use of a fallible conversion when an infallible one could be used + + let _: Result = 0i32.try_into(); + //~^ ERROR: use of a fallible conversion when an infallible one could be used + let _: Result = i32::try_into(0i32); + //~^ ERROR: use of a fallible conversion when an infallible one could be used + let _: Result = <_>::try_from(0i32); + //~^ ERROR: use of a fallible conversion when an infallible one could be used + + // From a macro + let _: Result = proc_macros::external!(0i32).try_into(); +} diff --git a/tests/ui/unnecessary_fallible_conversions_unfixable.stderr b/tests/ui/unnecessary_fallible_conversions_unfixable.stderr new file mode 100644 index 000000000..286decf8f --- /dev/null +++ b/tests/ui/unnecessary_fallible_conversions_unfixable.stderr @@ -0,0 +1,41 @@ +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions_unfixable.rs:27:34 + | +LL | let _: Result = 0i64.try_into(); + | ^^^^^^^^ help: use: `into` + | + = note: `-D clippy::unnecessary-fallible-conversions` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_fallible_conversions)]` + +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions_unfixable.rs:29:29 + | +LL | let _: Result = i64::try_into(0i64); + | ^^^^^^^^^^^^^ help: use: `Into::into` + +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions_unfixable.rs:31:29 + | +LL | let _: Result = Foo::try_from(0i64); + | ^^^^^^^^^^^^^ help: use: `From::from` + +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions_unfixable.rs:34:34 + | +LL | let _: Result = 0i32.try_into(); + | ^^^^^^^^ help: use: `into` + +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions_unfixable.rs:36:29 + | +LL | let _: Result = i32::try_into(0i32); + | ^^^^^^^^^^^^^ help: use: `Into::into` + +error: use of a fallible conversion when an infallible one could be used + --> $DIR/unnecessary_fallible_conversions_unfixable.rs:38:29 + | +LL | let _: Result = <_>::try_from(0i32); + | ^^^^^^^^^^^^^ help: use: `From::from` + +error: aborting due to 6 previous errors + diff --git a/tests/ui/useless_conversion_try.rs b/tests/ui/useless_conversion_try.rs index a5feefbe0..803d3b39f 100644 --- a/tests/ui/useless_conversion_try.rs +++ b/tests/ui/useless_conversion_try.rs @@ -1,5 +1,5 @@ #![deny(clippy::useless_conversion)] -#![allow(clippy::needless_if)] +#![allow(clippy::needless_if, clippy::unnecessary_fallible_conversions)] fn test_generic(val: T) -> T { let _ = T::try_from(val).unwrap();