-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Add new function_casts_as_integer
lint
#141470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add new function_casts_as_integer
lint
#141470
Conversation
This comment has been minimized.
This comment has been minimized.
07f2c3c
to
4978962
Compare
This comment has been minimized.
This comment has been minimized.
3db3153
to
d8b1955
Compare
This comment has been minimized.
This comment has been minimized.
d8b1955
to
45984df
Compare
This comment has been minimized.
This comment has been minimized.
45984df
to
a6107b4
Compare
This comment has been minimized.
This comment has been minimized.
a6107b4
to
24d757e
Compare
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
24d757e
to
3529162
Compare
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/// a cast as `fn` first to make it obvious what's going on. It also allows | ||
/// to prevent confusion with (associated) constants. | ||
pub FUNCTION_CASTS_AS_INTEGER, | ||
Warn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, a partial uplift might be good then, should this be accepted.
Sorry but I disagree, you can just copy what the lint tells you. The whole point is to have these annotations appear in the code, in part to prevent confusing associated items with methods. So putting back the nomination. @rustbot labels -I-lang-radar +I-lang-nominated |
OK. Fair enough. We can discuss. What are your thoughts on the mentioned interaction with #140803 though? |
See also:
I'm curious too what your thoughts are about these: fn f() {}
fn main() {
let x: usize = (&raw const *&f) as _;
let x: usize = (&raw const *&f) as *const () as _;
let x: usize = f as *const () as _;
} |
☔ The latest upstream changes (presumably #141700) made this pull request unmergeable. Please resolve the merge conflicts. |
Interesting discussion, in particular the cast from integer to function. Although I think it's a different problem as the compiler currently doesn't allow it (casting from integer to function), unlike the current code which adds a warning for a cast from function to integer. There are multiple clippy lints to check for this issue, which proves that there is a big need for this silent issue. So the goal here is to prevent having an involuntary cast of a function to an integer. Now about the code you provided: fn f() {}
fn main() {
let x: usize = (&raw const *&f) as _;
let x: usize = (&raw const *&f) as *const () as _;
let x: usize = f as *const () as _;
} For me it presents one big issue: we cannot assume from this code that |
It seems, to me, like there are two separate questions here:
The first question seems easy: "yes". The second question is extremely bikesheddable. I'd like to see us separate these questions in our discussion, and decide the first in principle before we start bikeshedding the second. As a reminder, the goal of such a lint is to catch things like |
Sounds good to me. And agreed, second question will likely be the most difficult to agree upon. |
In the spirit of our long-term goals of eliminating |
That seems related but separated from the current goal of this PR, no? More like a second step. First we warn for this |
@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 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 |
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? |
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 |
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? |
The
function_casts_as_integer
lint detects cases where users cast a function pointer into an integer.warn-by-default
Example
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