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

clippy_lints/src/casts/primitive_method_to_numeric_cast.rs

clippy_lints/src/casts/mod.rs

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

I'll add a test for floats then.

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, ); }, ); }

tests/ui/confusing_method_to_numeric_cast.stderr

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.

Good point, clarified it.

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

GuillaumeGomez · 2025-05-07T19:43:44Z

Finally took time to update this PR and to apply suggestions. I still don't think we should generalize this check between any methods and any existing associated constant so I didn't implement it but I suppose it can always be revisited at a later time.

llogiq · 2025-05-12T15:27:30Z

Thank you. Sorry this was waiting on me for so long.

theemathas · 2025-05-12T15:39:19Z

clippy_lints/src/casts/mod.rs

+    ///
+    /// ### Why restrict this?
+    /// Casting a function pointer to an integer can have surprising results and can occur
+    /// accidentally if parentheses are omitted from a function call. If you aren't doing anything


Is this comment incorrect? I don't think omitted parentheses are in the cases caught by this lint.

Yes that's correct; the expression, i32::max:

omits parenthesis

may be cast as an FP's (abstractly) location to, an i64, for example.

where the most likely intended effect would have been to obtain 2_147_483_647i32

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

clippy_lints/src/casts/primitive_method_to_numeric_cast.rs Outdated Show resolved Hide resolved

alex-semenyuk reviewed Jan 10, 2025

View reviewed changes

clippy_lints/src/casts/primitive_method_to_numeric_cast.rs Outdated Show resolved Hide resolved

samueltardieu reviewed Jan 10, 2025

View reviewed changes

clippy_lints/src/casts/mod.rs Outdated Show resolved Hide resolved

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

tests/ui/confusing_method_to_numeric_cast.stderr Outdated Show resolved Hide resolved

theemathas reviewed Jan 11, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

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

GuillaumeGomez added 2 commits May 7, 2025 18:18

Add primitive_method_to_numeric_cast lint

be40534

Add UI test for primitive_method_to_numeric_cast

c680419

GuillaumeGomez force-pushed the primitive_method_to_numeric_cast branch from 042556c to e276af4 Compare May 7, 2025 16:27

Rename lint into confusing_method_to_numeric_cast

06fa045

GuillaumeGomez force-pushed the primitive_method_to_numeric_cast branch from e276af4 to 06fa045 Compare May 7, 2025 19:39

Clarify docs of CONFUSING_METHOD_TO_NUMERIC_CAST

a94abae

llogiq added this pull request to the merge queue May 12, 2025

Merged via the queue into rust-lang:master with commit b363499 May 12, 2025
11 checks passed

theemathas reviewed May 12, 2025

View reviewed changes

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

GuillaumeGomez commented Jan 10, 2025

samueltardieu Jan 10, 2025 •

edited

Loading

samueltardieu Jan 11, 2025

samueltardieu Jan 10, 2025

theemathas Jan 11, 2025

GuillaumeGomez May 7, 2025

samueltardieu left a comment

samueltardieu Jan 11, 2025

theemathas commented Jan 11, 2025

theemathas commented Jan 11, 2025

theemathas Jan 11, 2025

GuillaumeGomez May 7, 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

This comment has been minimized.

GuillaumeGomez commented May 7, 2025

llogiq commented May 12, 2025

theemathas May 12, 2025

meltyness May 12, 2025 •

edited

Loading

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

Add new confusing_method_to_numeric_cast lint #13979

Add new confusing_method_to_numeric_cast lint #13979

Conversation

GuillaumeGomez commented Jan 10, 2025 • edited by llogiq Loading

rustbot commented Jan 10, 2025

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

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

theemathas commented Jan 11, 2025

theemathas commented Jan 11, 2025

Choose a reason for hiding this comment

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

This comment has been minimized.

GuillaumeGomez commented May 7, 2025

llogiq commented May 12, 2025

Choose a reason for hiding this comment

meltyness May 12, 2025 • edited Loading

Choose a reason for hiding this comment

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

samueltardieu Jan 10, 2025 •

edited

Loading

meltyness May 12, 2025 •

edited

Loading