-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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.
4f843ad
to
1645fd5
Compare
This comment has been minimized.
This comment has been minimized.
1645fd5
to
bc71088
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
bc71088
to
1adb4d1
Compare
CI finally passed. \o/ |
/// to prevent confusion with (associated) constants. | ||
pub FUNCTION_CASTS_AS_INTEGER, | ||
Warn, | ||
"Casting a function into an integer", |
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.
It's by convention always lowercase in the compiler.
"Casting a function into an integer", | |
"casting a function into an integer", |
/// The `function_casts_as_integer` lint detects cases where users cast a function into an | ||
/// integer. |
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.
No need to mention the "users", might be auto-generated code.
/// The `function_casts_as_integer` lint detects cases where users cast a function into an | |
/// integer. | |
/// The `function_casts_as_integer` lint detects cases where a function item is casted | |
/// into an integer. |
/// 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. |
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.
This doesn't really explain what is the problem this lint is checking for, ie. why is it bad. What about saying that by casting a function item to an integer it's implicitly creating a function pointer and it's that one that is casted to an integer, and so by making it explicit it improves re-ability of the code and prevents bugs (in particular with enum variants).
#[derive(LintDiagnostic)] | ||
#[diag(lint_function_casts_as_integer)] | ||
struct FunctionCastsAsIntegerMsg<'tcx> { | ||
#[subdiagnostic] | ||
sugg: FunctionCastsAsIntegerSugg<'tcx>, | ||
} | ||
|
||
#[derive(Subdiagnostic)] | ||
#[suggestion( | ||
lint_cast_as_fn, | ||
code = " as {cast_from_ty}", | ||
applicability = "machine-applicable", | ||
style = "verbose" | ||
)] | ||
struct FunctionCastsAsIntegerSugg<'tcx> { | ||
#[primary_span] | ||
pub suggestion: Span, | ||
pub cast_from_ty: Ty<'tcx>, | ||
pub cast_to_ty: Ty<'tcx>, | ||
} |
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.
Should be moved to src/lints.rs
where all the other #[derive(LintDiagnostic)]
struct lives.
|
||
#[derive(LintDiagnostic)] | ||
#[diag(lint_function_casts_as_integer)] | ||
struct FunctionCastsAsIntegerMsg<'tcx> { |
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.
By convention we name these with the Diag
suffix.
struct FunctionCastsAsIntegerMsg<'tcx> { | |
struct FunctionCastsAsIntegerDiag<'tcx> { |
@@ -1,4 +1,3 @@ | |||
|
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.
unrelated changes?
use rustc_span::Span; | ||
use rustc_span::Symbol; | ||
use rustc_span::{Span, Symbol}; |
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.
same here, unrelated changes?
|
||
#![deny(function_casts_as_integer)] |
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.
Better not changing the lint level in it's main test.
#![deny(function_casts_as_integer)] | |
//@ check-pass | |
|
||
fn foo() {} | ||
|
||
fn main() { |
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.
Could you also add tests for enum variants.
enum MyEnum {
Variant,
}
let _a = MyEnum::Variant as usize; //~ WARN: function_casts_as_integer
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