-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Implement lint for black_boxing ZSTs #150037
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: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,13 @@ lint_builtin_allow_internal_unsafe = | |
| lint_builtin_anonymous_params = anonymous parameters are deprecated and will be removed in the next edition | ||
| .suggestion = try naming the parameter or explicitly ignoring it | ||
|
|
||
| lint_builtin_black_box_zst_call = `black_box` on zero-sized callable `{$ty}` has no effect on call opacity | ||
| .label = zero-sized callable passed here | ||
|
|
||
| lint_builtin_black_box_zst_help = coerce to a function pointer and call `black_box` on that pointer instead | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be cool if we were actually able to suggest a code fix here! But, I don't think that's necessary for this PR, just a nice future improvement.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like? any example or idea?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. guess the error message already says to coerce the object to a pointer, but we could also show a suggested code snippet. I think that would also let rust-analyzer make the change automatically. Given an error message like: The suggestion would be something like: We have some other errors and warnings that make suggestions like this, so I'd try to look at what they do and see if you can reuse that infrastructure. |
||
|
|
||
| lint_builtin_black_box_zst_note = zero-sized callable values have no runtime representation, so the call still targets the original function directly | ||
|
|
||
| lint_builtin_clashing_extern_diff_name = `{$this}` redeclares `{$orig}` with a different signature | ||
| .previous_decl_label = `{$orig}` previously declared here | ||
| .mismatch_label = this signature doesn't match the previous declaration | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #![deny(black_box_zst_calls)] | ||
|
|
||
| use std::hint::black_box; | ||
|
|
||
| fn add(a: u32, b: u32) -> u32 { | ||
| a + b | ||
| } | ||
|
|
||
| fn main() { | ||
| let add_bb = black_box(add); | ||
| //~^ ERROR `black_box` on zero-sized callable | ||
| let _ = add_bb(1, 2); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| error: `black_box` on zero-sized callable `fn(u32, u32) -> u32 {add}` has no effect on call opacity | ||
| --> $DIR/lint-black-box-zst-call.rs:10:18 | ||
| | | ||
| LL | let add_bb = black_box(add); | ||
| | ^^^^^^^^^^---^ | ||
| | | | ||
| | zero-sized callable passed here | ||
| | | ||
| = note: zero-sized callable values have no runtime representation, so the call still targets the original function directly | ||
| = help: coerce to a function pointer and call `black_box` on that pointer instead | ||
| note: the lint level is defined here | ||
| --> $DIR/lint-black-box-zst-call.rs:1:9 | ||
| | | ||
| LL | #![deny(black_box_zst_calls)] | ||
| | ^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| error: aborting due to 1 previous error | ||
|
|
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 don't think the terminology "zero-sized callable" appears anywhere else. Should we be more specific and say "function item" or "closure" here?
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.
There is another footgun potential here. For the specific case of closures, using
black_boxis probably wrong even if the closure's hidden type is has non-zero size.After all, such use of
black_boxwill apply the black box effect to the values of captured variables, but like here, it has no effect on call opacity.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 think if I used "function item" in the message it would look confusing if a user triggered it with black_box(()) as my implementation checks layout.is_zst(). If you like I can update the message to "zero-sized type" to be more readable and accurate.
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.
And for the footgun I agree but this PR is scoped to ZSTs so I think to address it in a followup PR.
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.
If it's not limited to callables, then yeah this should be reworded.
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've updated the error message to zero-sized type making it more readable and accurate. Thanks!