Add new function_casts_as_integer lint

[GuillaumeGomez]

The function_casts_as_integer lint detects cases where users cast a function pointer into an integer.

warn-by-default

Example

fn foo() {} let x = foo as usize;

warning: casting a function into an integer implicitly --> $DIR/function_casts_as_integer.rs:9:17 | LL | let x = foo as usize; | ^^^^^^^^ | help: add `fn() as usize` | LL | let x = foo as fn() as usize; | +++++++ 

Explanation

You should never cast a function directly into an integer but go through a cast as fn first to make it obvious what's going on. It also allows to prevent confusion with (associated) constants.

Related to #81686 and https://stackoverflow.com/questions/68701177/whats-the-meaning-of-casting-a-rust-enum-variant-to-a-numeric-data-type

r? @Urgau

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[flip1995]

Clippy has a few lints for fn to integer casts. But they are all restriction or style lints in Clippy. Adding a warn-by-default lint about this to rustc might be a bit aggressive 🤔

[GuillaumeGomez]

I know, I implemented one myself. 😉 I think it highlights the fact that this is a big issue and that the compiler should warn about it and eventually even forbid this fn to integer cast (you need to cast to an fn pointer first).

But in any case, it's up to the lang team.

[flip1995]

Agreed 👍 Just want to add this information as "prior art" for the lang team to make this decision. Even though it might've sounded like it, I'm not against adding this lint to rustc.

Clippy question: Do you think if this lint gets added to rustc, we can (partially) deprecate Clippy lints?

[GuillaumeGomez]

Hard to say. For example confusing_method_to_numeric_cast provides extra information about what (likely) went wrong. But with the current lint, they likely would already have seen the problem and fixed it. So by default I'd say yes. But we could eventually uplift part of them to add the extra context clippy has that this lint doesn't provide. Would make it much more interesting and even more useful.

[flip1995]

Yeah, a partial uplift might be good then, should this be accepted.

[traviscross]

The last time we discussed this on lang, one part of the outcome is what @joshtriplett said in #141470 (comment):

If that API ends up taking longer than expected, we'd also approve an interim lint catching specific cases like the integer max/min functions.

To what degree might that be helpful?

In prior discussion, one place where the PR in its current form raised questions is that it seems that we'd be asking people to write out full function signatures in order to make these casts (including, then, having to import or otherwise name function argument and return types that may otherwise not be needed there), and there was a feeling that this may seem too onerous.

What options might we have to ameliorate that?

[Amanieu]

I can realistically see the following options:

1. Keep this lint as implemented.
2. Change this lint to use _ placeholders instead of type names in the function signature. This will usually be shorter than the original signature, which helps a bit.
3. Add a method on functions to extract the address as a usize. Unfortunately this runs into the question of whether function pointers have provenance since if this is the case then we will want separate expose_provenance and addr methods. This lint would then suggest expose_provenance since this matches the current semantics of pointer to integer casts.

While option 3 seems superior, it is likely to take a long time to settle the provenance question and stabilize the API. What I am suggesting is that we could implement option 1 or 2 in the meantime.

[GuillaumeGomez]

As the main goal of this lint is to make people realize they're likely doing a function pointer cast (and not an integer cast), I think the current approach is the best.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[traviscross]

The last time we discussed this on lang, we discussed how special-casing the functions where this is a known problem, e.g. min, max, size_of, etc., might be one option. To what degree do you think that would or would not be helpful?

[GuillaumeGomez]

It's what we initially did in clippy before realizing there were cases like minimum/maximum too. The list just grew too big to be maintainable. Hence why I opened the current PR.

In short: if you didn't mean to cast a function, then thanks to this lint you will realize right away. If did you mean to cast a function, then either you allow the lint or you apply the suggestion. In the long term, it seems that we want to forbid casting functions to integers directly, so this lint would be a first step into this direction while fixing a very big already existing unnoticed source of bugs.

[bors]

☔ The latest upstream changes (presumably #147779) made this pull request unmergeable. Please resolve the merge conflicts.

[traviscross]

What do you think about suggesting that people do this cast via a pointer to unit? I.e., f as *const () as usize? If the person is expecting max to be a number rather than a function, there's no reason to be casting through a pointer, so perhaps this would achieve the same effect, in terms of prevent error, while not having to elaborate the function signatures.

[GuillaumeGomez]

But then the readability of the suggested code is pretty bad since you have no tip of why you need this intermediate cast.

[traviscross]

I don't know. I'm not sure that passing it through fn(..) -> .. provides any more of a tip about why the intermediate cast is needed. If anything, the elaborated function signature is going to raise more questions for me as the reader about whether there's maybe something else non-obvious going on. If I see a cast through a void pointer to an integer, though, I'll think, "OK, that makes sense, whatever".

[GuillaumeGomez]

Well, if you think "OK, that makes sense, whatever", then that's an issue. The whole point is to force you to realize what's going on by reading it. Then either it's the intended purpose and you leave things as is (and eventually add a code comment 😄), or it's not and then you clean up.