Restriction lint for function pointer casts

[michaelsproul]

The existing lints for function pointer casts cover the cases where a cast is non-portable or would result in truncation, however there's currently no way to forbid numeric function pointer casts entirely.

I've added a new lint fn_to_numeric_cast_any, which allows one to ban all numeric function pointer casts, including to usize. This is useful if you're writing high-level Rust and want to opt-out of potentially surprising behaviour, avoiding silent bugs from forgotten parentheses, e.g.

fn foo() -> u32 {
    10
}

fn main() {
    let _ = foo as usize; // oops, forgot to call foo and got a random address instead!
}

I'm open to suggestions for the naming of the lint, because fn_to_numeric_cast_any is a bit clunky. Ideally I'd call this lint fn_to_numeric_cast, but that name is already taken for the more specific lint. We've stuck with fn_to_numeric_cast_any to avoid renaming the existing lint, or choosing a different name that's too generic (like fn_cast).

I'm also open to changing the suggestion behaviour, as adding parentheses is only one of many possible ways to fix the lint.

changelog: add [`fn_to_numeric_cast_any`] restriction lint

[rust-highfive]

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

[camsteffen]

Could we generalize the lint even more, ban a cast to any type, and name it fn_cast? That makes the lint name situation a little better.

[michaelsproul]

Thanks for the feedback @camsteffen! I've addressed all but one of your suggestions and I'm liking the new fn_cast name a lot!

[bors]

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

[camsteffen]

Sorry, I'm second guessing what to do for the lint name. I didn't realize that this lint still allows as fn() (function item to function pointer), so I don't think fn_cast really works. Like you said earlier, fn_to_numeric_cast would be good but is already taken.

I feel that we need to rename fn_to_numeric_cast because its existence alongside this lint would be very confusing. So I suggest renaming it to nonstandard_fn_cast. This more clearly communicates the intent - "don't cast to a non-usize integer" - and that it's just a style thing.

For this new lint, I suggest fn_cast_to_integer. Importantly, this is a new name so it does not change the meaning of an existing lint. I'm using "integer" instead of "numeric" because Rust doesn't allow func as f32.

That leaves fn_to_numeric_cast_with_truncation, which would no longer have a neat similarity to any other lint names, but I think it's okay as is.

In sum:

• Rename fn_to_numeric_cast to nonstandard_fn_cast
• New lint: fn_cast_to_integer
• fn_to_numeric_cast_with_truncation - leave it

@rust-lang/clippy thoughts?

[michaelsproul]

Alternatively we could keep the fn_cast name and forbid fn item to fn pointer casts, although it would forbid code like this (from quickcheck's readme):

fn prop(xs: Vec<isize>) -> TestResult {
    if xs.len() != 1 {
        return TestResult::discard()
    }
    TestResult::from_bool(xs == reverse(&xs))
}
quickcheck(prop as fn(Vec<isize>) -> TestResult);

Maybe this pattern is rare enough that hitting it with a restriction lint is considered acceptable. I'm not familiar with the process for removing a lint entirely... is it painful?

[flip1995]

* Rename `fn_to_numeric_cast` to `nonstandard_fn_cast`

I wouldn't rename it, because I think it makes sense to keep the name close to the fn_to_numeric_cast_with_truncation because those two lints are related.

I don't see a problem with keeping the old names and introduce a new fn_cast_to_integer lint. Especially since it is a restriction lint and the common Clippy user won't encounter this similarity in names, as long as they don't actively look for it. And at this point they probably read the documentation either way.

[camsteffen]

I wouldn't rename it

Yeah I agree...

Then it makes less sense to use "integer" for the new lint because it sounds more specific when it is actually less specific. So now I've come full circle and think the new lint should be fn_to_numeric_cast_any. 😂

[camsteffen]

Maybe this pattern is rare enough that hitting it with a restriction lint is considered acceptable.

We should decide what behavior we want for the lint, and name it accordingly. Not the other way around ☺️. I don't think linting function item to
function pointer would make sense because it is very low risk.

[flip1995]

I've come full circle and think the new lint should be fn_to_numeric_cast_any

By now you're my go-to person for lint names 😄 So I'd be fine with that.

Agreed on not linting fn -> fn_ptr.

[michaelsproul]

Ok, updated back to fn_to_numeric_cast_any and ready for review (and merge? 🤞 )

[camsteffen]

One minor thing and then ready to merge!

[camsteffen]

I squashed the commits. Thanks!

@bors r+

[bors]

📌 Commit fbd0fb9 has been approved by camsteffen

[bors]

⌛ Testing commit fbd0fb9 with merge 8aff5dd...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 8aff5dd to master...