-
Notifications
You must be signed in to change notification settings - Fork 1.6k
RFC: Safety Tags #3842
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?
RFC: Safety Tags #3842
Conversation
error: expected identifier, found keyword `use` --> src/main.rs:2:20 | 2 | #![clippy::safety::use(invariant::ValidPtr)] // 💡 | ^^^ expected identifier, found keyword
Co-authored-by: kennytm <kennytm@gmail.com>
Also * distinguish safety::requires and safety::checked * replace safety::r#use with safety::import * only allow uninhabited enum as tag item
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 really like the direction this is moving with requires
and checked
since the initial version.
Co-authored-by: Mads Marquart <mads@marquart.dk>
rust-lang/rust-clippy#11600 was a related effort on the clippy side that stalled on inactivity, though not specific to |
Thanks for the link. I’ve expanded the “Alternatives” section with a side-by-side comparison. |
I've left some comments in rust-for-linux channel on zulip:
src: #rust-for-linux > Provide the meaning of "Valid" to Understand. @ 💬 |
|
||
#[safety::checked( | ||
aligned = "alignment of ptr has be adjusted", | ||
valid_ptr, initialized = "delegated to the caller" |
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.
(As an enhanced example from my comment in #3458 (comment) of RFC unsafe fields )
I'm wondering that there should be a specific #[safety::delegated]
attribute the same as #[safety::checked]
, but more handy and concise to use:
#[safety::requires(
// bad comments by mentioning things in release build, but that's what the current implementation is in libstd
no_more_than_cap = "Caller must ensure the new_len <= cap (in release mode)",
valid_T_all_in_length_range = "Please also make sure all T in 0..=new_len are valid, since this call can't ensure this!"
)]
unsafe fn set_len(&mut self, new_len: usize) {
debug_assert!(new_len <= self.capacity());
#[safety::delegated(
no_more_than_cap, valid_T_all_in_length_range
)]
unsafe { self.len = new_len }
}
Tags in delegated
are merged into checked
, so both can coexist and work well.
What I mean handy is that we can omit definition texts in such context, because safety requirements are first introduced on unsafe field declaration or unsafe function being called, so it's verbose to duplicate them when without extra context attached:
struct Vec<T> {
#[safety::requires(
no_more_than_cap = "0 <= len <= cap",
valid_T_all_in_length_range = "all elements in the Vec<T> between 0 and len are valid T"
)]
unsafe len: usize,
...
}
// The same safety requirements as on `unsafe len` field are rendered here
// because safety tags need support from rustdoc.
#[safety::requires(no_more_than_cap, valid_T_all_in_length_range)]
unsafe fn set_len(&mut self, new_len: usize) {
debug_assert!(new_len <= self.capacity());
#[safety::delegated(no_more_than_cap, valid_T_all_in_length_range)]
unsafe { self.len = new_len }
}
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.
Another reason to have #[safety::delegated]
is that #[safety::requires]
must incorporate all tags delegated from the fn body. Must means that a diagnostic happens if such one tag is missing.
checked
won't have such delegation checks when listing delegated tags in requires
.
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.
Paste a totally semantic-unsafety-oriented example given in my comment on RFC unsafe fields:
struct Vec<T> {
#[safety::requires(any {
good_on_read,
hazard_on_mut(valid_T_all_in_length_range, no_more_than_cap)
})]
unsafe len: usize,
}
pub fn len(&self) -> usize {
#[safety::checked(good_on_read)]
unsafe { self.len }
}
#[safety::requires(hazard_on_mut)]
unsafe fn set_len(&mut self, new_len: usize) {
#[safety::delegated(hazard_on_mut)]
unsafe { self.len = new_len }
}
This is an improvement to #3842 (comment) as well.
(I don't really suggest supporting safety tags beyond visual unsafe operations too far though.)
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.
you should at least add the unsafe
back to the len
field, I don't think it is supposed that #[safety::requires]
can be applicable on things declared safe.
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.
The snippet is more to clippy PR danger_not_accepted
.
Safety tags for unsafe fields are already stated to be supported.
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 mean your declaration of len
is not an unsafe
field at all, not whether about the attribute supports unsafe
field or not.
struct Vec<T> {
#[safety::requires(any {
good_on_read,
hazard_on_mut(valid_T_all_in_length_range, no_more_than_cap)
})]
len: usize,
//^ NO UNSAFE IN SIGHT
}
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.
danger_not_accepted doesn't require unsafe.
But to avoid further unnecessary misinterpretation, I've just modified it the way you asked.
Summary
This RFC introduces a concise safety-comment convention for unsafe code in standard libraries:
tag every public unsafe function with
#[safety::requires]
and call with#[safety::checked]
.Safety tags refine today’s safety-comment habits: a featherweight syntax that condenses every
requirement into a single, check-off reminder.
The following snippet compiles today if we enable enough nightly features, but we expect Clippy
and Rust-Analyzer to enforce tag checks and provide first-class IDE support.
Rendered