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.

[GuillaumeGomez]

That seems related but separated from the current goal of this PR, no? More like a second step. First we warn for this as cast, second we can provide a suggestion saying that if they actually want the address, they can use this newly added method on functions to cast the function address into a usize. What do you think?

[joshtriplett]

@GuillaumeGomez I was proposing it in part because it might make it easier for us to agree that we have a better alternative to the as-cast. And if we know that's where we want to get to, then we might not want the churn of driving people towards as something as usize and then towards .fn_addr().

Definitely not looking to make the perfect the enemy of the good, here. Rather, trying to make sure we have a sufficient good that people feel motivated to warn about as usize.

[GuillaumeGomez]

Yeah, thinking some more about it today, I agree with you. If the libs team is ok with the addition of this new method on fn types, then I can send a PR. My only issue is that we'll need for this new method to be stabilized, and in the meantime, the current issue will remain. I suppose we suggest the new method on nightly and the longer version until then to reduce this delay?

[joshtriplett]

We have an accepted ACP for an API that would work for this: rust-lang/libs-team#589 (comment)

We'd like to see a lint based on this, and attempt to ship and stabilize that API in a timely fashion.

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.

[GuillaumeGomez]

Then I can send a PR to implement this new API as a first step if it's ok with you and we'll see the next step once merged?

[apiraino]

hello, trying to get current status here: is here currently waiting on @GuillaumeGomez to implement the lint described in this comment?

What about the needs-fcp label? Is an FCP needed to land this at a later point?

Thanks for clarifying a bit the next steps 🙂

[traviscross]

What about the needs-fcp label? Is an FCP needed to land this at a later point?

New lints and significant lint expansions must be approved by the lang team, so in whatever form this ends up coming back, this will need a lang FCP.

[Amanieu]

I had a long discussion with @GuillaumeGomez about this in person. The current situation is that the lack of this lint results in real code in the wild being subtly incorrect, potentially resulting in soundness issues (#81686, #146364). While it would be nice to wait for as_ptr or addr methods to be added to function types and stabilized, I don't think that this should block adding this lint: the suggestion can always be adjusted in the future once (and if) the new methods are stabilized. As such, I would like @rust-lang/lang to re-considering adding this lint in its current state.

[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.

[GuillaumeGomez]

Fixed merge conflicts.

[rust-log-analyzer]

The job x86_64-gnu-miri failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] cargo_metadata test:false 11.085
error: direct cast of function item into an integer
   --> src/tools/miri/src/shims/native_lib/trace/parent.rs:504:31
    |
504 |     new_regs.set_ip(mempr_off as usize);
    |                               ^^^^^^^^
    |
    = note: `-D function-casts-as-integer` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(function_casts_as_integer)]`
help: first cast to a function pointer `unsafe extern "C" fn()`
    |
504 |     new_regs.set_ip(mempr_off as unsafe extern "C" fn() as usize);
    |                               +++++++++++++++++++++++++

error: direct cast of function item into an integer
   --> src/tools/miri/src/shims/native_lib/trace/parent.rs:545:30
    |
545 |     new_regs.set_ip(mempr_on as usize);
    |                              ^^^^^^^^
    |
help: first cast to a function pointer `unsafe extern "C" fn()`
    |
545 |     new_regs.set_ip(mempr_on as unsafe extern "C" fn() as usize);
    |                              +++++++++++++++++++++++++

[RUSTC-TIMING] miri test:false 18.035
error: could not compile `miri` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

[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?