Auto merge of #11669 - y21:issue11577, r=Jarcho

new lint: `unnecessary_fallible_conversions`

Closes #11577

A new lint that looks for calls such as `i64::try_from(1i32)` and suggests `i64::from(1i32)`. See lint description (and linked issue) for more details for why.

There's a tiny bit of overlap with the `useless_conversion` lint, in that the other one warns `T::try_from(T)` (i.e., fallibly converting to the same type), so this lint ignores cases like `i32::try_from(1i32)` to avoid emitting two warnings for the same expression.

Also, funnily enough, with this one exception, this lint would warn on exactly every case in the `useless_conversion_try` ui test that `useless_conversion` didn't cover (but never two warnings at the same time), which is neat. I did add an `#![allow]` though since we don't want interleaved warnings from multiple lints in the same uitest.

changelog: new lint: `unnecessary_fallible_conversions`

Author: bors

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

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,

clippy_lints/src/implicit_saturating_add.rs

@@ -82,18 +82,18 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitSaturatingAdd {
 
 fn get_int_max(ty: Ty<'_>) -> Option<u128> {
     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,
     }
 }

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<i64, _> = 1i32.try_into();
+    /// let _: Result<i64, _> = <_>::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);

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<U>` and `T: From<U>` 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 `<T as TryFrom<U>>::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:
+/// - `<i64 as TryFrom<_>>::try_from(0i32)`
+/// - `<_ as TryInto<i64>>::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,
+        );
+    }
+}

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.

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.

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 _ = <String>::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()`

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();
+}

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");
+}

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
+

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<i32> for Foo {
+    type Error = ();
+    fn try_from(_: i32) -> Result<Self, Self::Error> {
+        Ok(Foo)
+    }
+}
+impl From<i64> for Foo {
+    fn from(_: i64) -> Self {
+        Foo
+    }
+}
+
+fn main() {
+    // `Foo` only implements `TryFrom<i32>` and not `From<i32>`, so don't lint
+    let _: Result<Foo, _> = 0i32.try_into();
+    let _: Result<Foo, _> = i32::try_into(0i32);
+    let _: Result<Foo, _> = Foo::try_from(0i32);
+
+    // ... it does impl From<i64> however
+    let _: Result<Foo, _> = 0i64.try_into();
+    //~^ ERROR: use of a fallible conversion when an infallible one could be used
+    let _: Result<Foo, _> = i64::try_into(0i64);
+    //~^ ERROR: use of a fallible conversion when an infallible one could be used
+    let _: Result<Foo, _> = Foo::try_from(0i64);
+    //~^ ERROR: use of a fallible conversion when an infallible one could be used
+
+    let _: Result<i64, _> = 0i32.try_into();
+    //~^ ERROR: use of a fallible conversion when an infallible one could be used
+    let _: Result<i64, _> = i32::try_into(0i32);
+    //~^ ERROR: use of a fallible conversion when an infallible one could be used
+    let _: Result<i64, _> = <_>::try_from(0i32);
+    //~^ ERROR: use of a fallible conversion when an infallible one could be used
+
+    // From a macro
+    let _: Result<i64, _> = proc_macros::external!(0i32).try_into();
+}