Add new `confusing_method_to_numeric_cast` lint #13979

GuillaumeGomez · 2025-01-10T16:46:16Z

I don't think we can make fn_to_numeric_cast_any to be emitted in some special cases. Its category cannot be changed at runtime.

I think in this case, the best might be a specialized new lint so we can target exactly what we want.

changelog: Add new confusing_method_to_numeric_cast lint

rustbot · 2025-01-10T16:46:22Z

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

alex-semenyuk · 2025-01-10T17:32:39Z

clippy_lints/src/casts/primitive_method_to_numeric_cast.rs

+        // We get the type on which the `min`/`max` method of the `Ord` trait is implemented.
+        && let [ty] = generics.as_slice()
+        && let Some(ty) = ty.as_type()
+        // We get its name in case it's a primitive with an associated MIN/MAx constant.


typo

Suggested change

// We get its name in case it's a primitive with an associated MIN/MAx constant.

// We get its name in case it's a primitive with an associated MIN/MAX constant.

alex-semenyuk · 2025-01-10T17:36:05Z

clippy_lints/src/casts/primitive_method_to_numeric_cast.rs

+use rustc_lint::LateContext;
+use rustc_middle::ty::{self, Ty};
+
+use super::PRIMITIVE_METHOD_TO_NUMERIC_CAST;


Are there primitive methods only max, min
If so might make sense to specify them at lint name

Or confusing_method_to_numeric_cast?

I'm very bad at naming so I'll take your suggestion. :3

samueltardieu · 2025-01-10T23:25:52Z

clippy_lints/src/casts/mod.rs

+    /// ```no_run
+    /// let _ = u16::MAX as usize;
+    /// ```
+    #[clippy::version = "1.85.0"]


Nit: 1.86.0

GuillaumeGomez · 2025-01-10T23:45:03Z

Renamed and fixed the typos/nits.

samueltardieu · 2025-01-10T23:47:01Z

clippy_lints/src/casts/confusing_method_to_numeric_cast.rs

+        && let Some(ty) = ty.as_type()
+        // We get its name in case it's a primitive with an associated MIN/MAX constant.
+        && let Some(ty_name) = get_primitive_ty_name(ty)
+        && match_def_path(cx, *def_id, &["core", "cmp", "Ord", method_name])


I think you can use the diag item here.

Suggested change

&& match_def_path(cx, *def_id, &["core", "cmp", "Ord", method_name])

&& is_diag_trait_item(cx, *def_id, sym::Ord)

In fact, you can even go simpler and test against the right method directly, both Ord::min and Ord::max are diagnostic items. This way you can use .item_name() directly as you know the method has a name.

if let ty::FnDef(def_id, generics) = cast_from.kind() && cx.tcx.get_diagnostic_name(*def_id).is_some_and(|diag| diag == sym::cmp_ord_min || diag == sym::cmp_ord_max) // We get the type on which the `min`/`max` method of the `Ord` trait is implemented. && let [ty] = generics.as_slice() && let Some(ty) = ty.as_type() // We get its name in case it's a primitive with an associated MIN/MAX constant. && matches!(ty.kind(), ty::Char | ty::Int(_) | ty::Uint(_)) { let mut applicability = Applicability::MaybeIncorrect; let from_snippet = snippet_with_applicability(cx, cast_expr.span, "..", &mut applicability); span_lint_and_then( cx, PRIMITIVE_METHOD_TO_NUMERIC_CAST, expr.span, format!("casting function pointer `{from_snippet}` to `{cast_to}`"), |diag| { diag.span_suggestion_verbose( expr.span, "did you mean to use the associated constant?", format!( "{ty}::{} as {cast_to}", cx.tcx.item_name(*def_id).as_str().to_ascii_uppercase() ), applicability, ); }, ); }

samueltardieu · 2025-01-10T23:57:36Z

clippy_lints/src/casts/confusing_method_to_numeric_cast.rs

+        && let Some(ty) = ty.as_type()
+        // We get its name in case it's a primitive with an associated MIN/MAX constant.
+        && let Some(ty_name) = get_primitive_ty_name(ty)


Suggested change

&& let Some(ty) = ty.as_type()

// We get its name in case it's a primitive with an associated MIN/MAX constant.

&& let Some(ty_name) = get_primitive_ty_name(ty)

&& matches!(ty.kind(), ty::Char | ty::Int(_) | ty::Uint(_))

(floating point types don't implement ::MIN/::MAX)

and then you can use directly ty in your diagnostic, it will print as the type name, no need for the get_primitive_ty_name() function.

Floating point types do have MIN/MAX https://doc.rust-lang.org/std/primitive.f32.html#associatedconstant.MAX

samueltardieu

I think this would deserve more tests, with the various types, as well as some other types implementing Ord not eligible to this lint.

samueltardieu · 2025-01-11T00:11:25Z

clippy_lints/src/casts/confusing_method_to_numeric_cast.rs

+        && let Some(ty) = ty.as_type()
+        // We get its name in case it's a primitive with an associated MIN/MAX constant.
+        && let Some(ty_name) = get_primitive_ty_name(ty)
+        && match_def_path(cx, *def_id, &["core", "cmp", "Ord", method_name])


In fact, you can even go simpler and test against the right method directly, both Ord::min and Ord::max are diagnostic items. This way you can use .item_name() directly as you know the method has a name.

if let ty::FnDef(def_id, generics) = cast_from.kind() && cx.tcx.get_diagnostic_name(*def_id).is_some_and(|diag| diag == sym::cmp_ord_min || diag == sym::cmp_ord_max) // We get the type on which the `min`/`max` method of the `Ord` trait is implemented. && let [ty] = generics.as_slice() && let Some(ty) = ty.as_type() // We get its name in case it's a primitive with an associated MIN/MAX constant. && matches!(ty.kind(), ty::Char | ty::Int(_) | ty::Uint(_)) { let mut applicability = Applicability::MaybeIncorrect; let from_snippet = snippet_with_applicability(cx, cast_expr.span, "..", &mut applicability); span_lint_and_then( cx, PRIMITIVE_METHOD_TO_NUMERIC_CAST, expr.span, format!("casting function pointer `{from_snippet}` to `{cast_to}`"), |diag| { diag.span_suggestion_verbose( expr.span, "did you mean to use the associated constant?", format!( "{ty}::{} as {cast_to}", cx.tcx.item_name(*def_id).as_str().to_ascii_uppercase() ), applicability, ); }, ); }

theemathas · 2025-01-11T06:52:53Z

tests/ui/confusing_method_to_numeric_cast.stderr

@@ -0,0 +1,24 @@
+error: unknown lint: `clippy::primitive_method_to_numeric_cast`


Is this supposed to happen?

theemathas · 2025-01-11T06:54:18Z

Could this be extended to also cover conversions using from/into/try_from/try_into?

theemathas · 2025-01-11T06:58:30Z

Floating point numbers also have inherent min/max methods even though they don't implement Ord https://doc.rust-lang.org/std/primitive.f32.html#method.max

theemathas · 2025-01-11T07:04:00Z

clippy_lints/src/casts/mod.rs

@@ -754,6 +755,32 @@ declare_clippy_lint! {
    "detects `as *mut _` and `as *const _` conversion"
 }

+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for casts of a primitive method pointer to any integer type.


This communicates that casting any method on any integer type will cause this lint to fire, even though it checks for only min and max.

samueltardieu · 2025-01-11T07:57:36Z

Floating point numbers also have inherent min/max methods even though they don't implement Ord https://doc.rust-lang.org/std/primitive.f32.html#method.max

Good point. I wonder whether:

the lint must cover them (probably)
the lint should cover any situation where we have a min/max method as well as MIN/MAX associated constants
the lint should cover any situation where we have a <x> method and an associated X constant (whatever x is) – I think this would be overkill because I couldn't find existing cases other than min and max on primitive types

llogiq · 2025-01-26T11:48:55Z

the lint should cover any situation where we have a min/max method as well as MIN/MAX associated constants

I don't think we need this. People can name their functions and constants however they like, so we should not guess intent based on names here. If we cover the standard library, we'll be fine. If we really want, we can add a configuration entry for people to add their own items to the lint.

GuillaumeGomez · 2025-01-29T19:00:35Z

Someone commented on the issue that min_value and max_value are also confusing and I agree so I plan to add them when I'll update this PR.

meltyness · 2025-01-29T21:20:58Z

Someone commented on the issue that min_value and max_value are also confusing and I agree so I plan to add them when I'll update this PR.

I took a whack at it, it seems like generics available to discern the base type available for the trait methods is not available for these deprecated methods, I made a workaround. I'm not sure it's the best way, but it seems to work with some cursory testing.

 else if let ty::FnDef(def_id, _generics) = cast_from.kind()
        && let Some(method_name) = cx.tcx.opt_item_name(*def_id)
        && let method_name = method_name.as_str()
        && (method_name == "min_value" || method_name == "max_value")
        && ["core", "num"].iter().map(|x| Symbol::intern(x)).zip(cx.get_def_path(*def_id).iter().copied()).all(|(a,b)| a == b)
    {
        let mut applicability = Applicability::MaybeIncorrect;
        let from_snippet = snippet_with_applicability(cx, cast_expr.span, "..", &mut applicability);

        let method_name = if method_name == "min_value" { "min" } else { "max" };
        let inferred_type = cx.get_def_path(*def_id).iter().copied().skip(2).next().unwrap();
        let inferred_type = inferred_type.as_str().split_whitespace().skip(1).next().unwrap();
        let inferred_type = inferred_type.chars().filter(|arg0: &char| char::is_alphanumeric(*arg0)).collect::<String>();
        span_lint_and_then(
            cx,
            CONFUSING_METHOD_TO_NUMERIC_CAST,
            expr.span,
            format!("casting function pointer `{from_snippet}` to `{cast_to}`"),
            |diag| {
                diag.span_suggestion_verbose(
                    expr.span,
                    "did you mean to use the associated constant?",
                    format!("{inferred_type:#?}::{} as {cast_to}", method_name.to_ascii_uppercase()),
                    applicability,
                );
            },
        );
    }

meltyness · 2025-03-12T17:02:35Z

There's also f32::minimum and f32::maximum i've discovered

rustbot · 2025-03-31T23:43:03Z

☔ The latest upstream changes (possibly d28d234) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned llogiq Jan 10, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 10, 2025

alex-semenyuk reviewed Jan 10, 2025

View reviewed changes

samueltardieu reviewed Jan 10, 2025

View reviewed changes

GuillaumeGomez added 3 commits January 11, 2025 00:44

Add primitive_method_to_numeric_cast lint

ed1edf3

Add UI test for primitive_method_to_numeric_cast

d93fba7

Rename lint into confusing_method_to_numeric_cast

042556c

GuillaumeGomez force-pushed the primitive_method_to_numeric_cast branch from 359e815 to 042556c Compare January 10, 2025 23:44

GuillaumeGomez changed the title ~~Add new primitive_method_to_numeric_cast lint~~ Add new confusing_method_to_numeric_cast lint Jan 10, 2025

samueltardieu reviewed Jan 10, 2025

View reviewed changes

samueltardieu reviewed Jan 11, 2025

View reviewed changes

theemathas reviewed Jan 11, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `confusing_method_to_numeric_cast` lint #13979

Add new `confusing_method_to_numeric_cast` lint #13979

GuillaumeGomez commented Jan 10, 2025 •

edited by llogiq

Loading

rustbot commented Jan 10, 2025

alex-semenyuk Jan 10, 2025

alex-semenyuk Jan 10, 2025 •

edited

Loading

samueltardieu Jan 10, 2025

GuillaumeGomez Jan 10, 2025

samueltardieu Jan 10, 2025

GuillaumeGomez commented Jan 10, 2025

samueltardieu Jan 10, 2025 •

edited

Loading

samueltardieu Jan 11, 2025

samueltardieu Jan 10, 2025

theemathas Jan 11, 2025

samueltardieu left a comment

samueltardieu Jan 11, 2025

theemathas Jan 11, 2025

theemathas commented Jan 11, 2025

theemathas commented Jan 11, 2025

theemathas Jan 11, 2025

samueltardieu commented Jan 11, 2025

llogiq commented Jan 26, 2025

GuillaumeGomez commented Jan 29, 2025

meltyness commented Jan 29, 2025

meltyness commented Mar 12, 2025

rustbot commented Mar 31, 2025

	// We get its name in case it's a primitive with an associated MIN/MAx constant.
	// We get its name in case it's a primitive with an associated MIN/MAX constant.

	&& match_def_path(cx, *def_id, &["core", "cmp", "Ord", method_name])
	&& is_diag_trait_item(cx, *def_id, sym::Ord)

		@@ -0,0 +1,24 @@
		error: unknown lint: `clippy::primitive_method_to_numeric_cast`

Add new confusing_method_to_numeric_cast lint #13979

Are you sure you want to change the base?

Add new confusing_method_to_numeric_cast lint #13979

Conversation

GuillaumeGomez commented Jan 10, 2025 • edited by llogiq Loading

rustbot commented Jan 10, 2025

Choose a reason for hiding this comment

alex-semenyuk Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 10, 2025

samueltardieu Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueltardieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theemathas commented Jan 11, 2025

theemathas commented Jan 11, 2025

Choose a reason for hiding this comment

samueltardieu commented Jan 11, 2025

llogiq commented Jan 26, 2025

GuillaumeGomez commented Jan 29, 2025

meltyness commented Jan 29, 2025

meltyness commented Mar 12, 2025

rustbot commented Mar 31, 2025

Add new `confusing_method_to_numeric_cast` lint #13979

Add new `confusing_method_to_numeric_cast` lint #13979

GuillaumeGomez commented Jan 10, 2025 •

edited by llogiq

Loading

alex-semenyuk Jan 10, 2025 •

edited

Loading

samueltardieu Jan 10, 2025 •

edited

Loading