-
Notifications
You must be signed in to change notification settings - Fork 777
Add callback to declare functions/statics safe #3058
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 all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| // bindgen-parse-callbacks: declare-safe | ||
|
|
||
| const int my_safe_var[3] = {1,2,3}; | ||
|
|
||
| int my_safe_func(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| use bindgen::{clang_version, Builder}; | ||
| use bindgen::callbacks::{ItemInfo, ItemKind, ParseCallbacks}; | ||
| use bindgen::{clang_version, Builder, Formatter}; | ||
| use owo_colors::{OwoColorize, Style}; | ||
| use similar::{ChangeTag, TextDiff}; | ||
| use std::env; | ||
|
|
@@ -427,6 +428,58 @@ fn test_header_contents() { | |
| assert_eq!(expected, actual); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_declare_safe() { | ||
| // prettyplease does not retain non-doc comments: https://github.com/dtolnay/prettyplease/issues/50 | ||
|
|
||
| #[derive(Debug)] | ||
| struct DeclareSafe; | ||
|
|
||
| impl ParseCallbacks for DeclareSafe { | ||
| fn declare_safe(&self, item_info: ItemInfo<'_>) -> Option<String> { | ||
| match item_info.kind { | ||
| ItemKind::Function => { | ||
| if item_info.name == "my_safe_func" { | ||
| return Some("safe to call".to_owned()); | ||
| } | ||
| } | ||
| ItemKind::Var => { | ||
| if item_info.name == "my_safe_var" { | ||
| return Some("safe to access".to_owned()); | ||
| } | ||
| } | ||
| _ => todo!(), | ||
| } | ||
| None | ||
| } | ||
| } | ||
|
|
||
| let actual = builder() | ||
| .disable_header_comment() | ||
| .header_contents( | ||
| "safe.h", | ||
| "const int my_safe_var[3] = {1,2,3};\ | ||
| int my_safe_func();", | ||
| ) | ||
| .formatter(Formatter::Rustfmt) | ||
| .parse_callbacks(Box::new(DeclareSafe)) | ||
| .generate() | ||
| .unwrap() | ||
| .to_string(); | ||
|
|
||
| let expected = r#"unsafe extern "C" { | ||
| /* Safety : "safe to access" */ | ||
| pub safe static my_safe_var: [::std::os::raw::c_int; 3usize]; | ||
| } | ||
| unsafe extern "C" { | ||
| /* Safety : "safe to call" */ | ||
| pub safe fn my_safe_func() -> ::std::os::raw::c_int; | ||
| } | ||
|
Comment on lines
+471
to
+477
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. Are these comments meant to describe only the reason why they are marked as
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. The specific comments are just there to test things are working, they could be replaced by The comments can be used to express any kind of safety reasoning. Generally I'd expect people to only reason about why a function is safe to call or a const is safe to access and to rely on |
||
| "#; | ||
|
|
||
| assert_eq!(expected, actual); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_multiple_header_calls_in_builder() { | ||
| let actual = builder() | ||
|
|
||
|
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. Not solely a result of these changes, but the documentation on |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,6 +134,17 @@ pub trait ParseCallbacks: fmt::Debug { | |
| vec![] | ||
| } | ||
|
|
||
| /// Allows declaring items as `safe`. | ||
| /// | ||
| /// The returned string will be prepended to the item as `Safety: ...` comment. | ||
| /// | ||
| /// When using [`Formatter::Prettyplease`][crate::Formatter::Prettyplease] to format code, non-doc comments are removed ([issue][doc_removal]). | ||
|
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. I don't understand why this would be a non-doc comment to begin with. I would expect this to be part of the doc comment, just like on unsafe functions? Or otherwise maybe just don't do the comment dance otherwise? Most people are never going to read the non-doc comments on a generated file.
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. I haven't seen doc comments used to explain why a method is safe. The only This example shows what I mean: /// Converts a mutable string slice to a mutable byte slice.
///
/// # Safety
///
/// The caller must ensure that the content of the slice is valid UTF-8
/// before the borrow ends and the underlying `str` is used.
///
/// Use of a `str` whose contents are not valid UTF-8 is undefined behavior.
///
/// ...
pub unsafe fn as_bytes_mut(&mut self) -> &mut [u8] {
// SAFETY: the cast from `&str` to `&[u8]` is safe since `str`
// has the same layout as `&[u8]` (only libstd can make this guarantee).
// The pointer dereference is safe since it comes from a mutable reference which
// is guaranteed to be valid for writes.
unsafe { &mut *(self as *mut str as *mut [u8]) }
}Of course, this is only semi-applicable to the case here. I'm not aware of any official guidelines or established precedent for While it could just be omitted, I think it would be nice to be able to see the safety reasoning in the generated file and it could (potentially) satisfy lints. The reasoning in I'm happy to make it a doc-comment or remove it outright if you prefer.
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. I would probably prefer to keep the current implementation, as the example above matches my expectations for how this feature would be used. |
||
| /// | ||
| /// [doc_removal]: https://github.com/dtolnay/prettyplease/issues/50 | ||
| fn declare_safe(&self, _item_info: ItemInfo<'_>) -> Option<String> { | ||
| None | ||
| } | ||
|
|
||
| /// Provide a list of custom attributes. | ||
| /// | ||
| /// If no additional attributes are wanted, this function should return an | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -856,10 +856,17 @@ impl CodeGenerator for Var { | |
| .unsafe_extern_blocks | ||
| .then(|| quote!(unsafe)); | ||
|
|
||
| let (safety_comment, var_safety) = utils::declare_safe( | ||
| &canonical_ident, | ||
| crate::callbacks::ItemKind::Var, | ||
| ctx, | ||
| ); | ||
|
|
||
| let tokens = quote!( | ||
| #safety extern "C" { | ||
| #(#attrs)* | ||
| pub static #maybe_mut #canonical_ident: #ty; | ||
| #safety_comment | ||
| pub #var_safety static #maybe_mut #canonical_ident: #ty; | ||
| } | ||
| ); | ||
|
|
||
|
|
@@ -4870,11 +4877,18 @@ impl CodeGenerator for Function { | |
| .unsafe_extern_blocks | ||
| .then(|| quote!(unsafe)); | ||
|
|
||
| let (safety_comment, fn_safety) = utils::declare_safe( | ||
| &ident, | ||
| crate::callbacks::ItemKind::Function, | ||
| ctx, | ||
| ); | ||
|
|
||
| let tokens = quote! { | ||
| #block_attributes | ||
| #safety extern #abi { | ||
| #(#attributes)* | ||
| pub fn #ident ( #( #args ),* ) #ret; | ||
| #safety_comment | ||
| pub #fn_safety fn #ident ( #( #args ),* ) #ret; | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -5346,6 +5360,44 @@ pub(crate) mod utils { | |
| use std::path::PathBuf; | ||
| use std::str::FromStr; | ||
|
|
||
| pub(super) fn declare_safe( | ||
| item_ident: &proc_macro2::Ident, | ||
| item_kind: crate::callbacks::ItemKind, | ||
| context: &BindgenContext, | ||
| ) -> ( | ||
| Option<proc_macro2::TokenStream>, | ||
| Option<proc_macro2::TokenStream>, | ||
| ) { | ||
| let safety_comment = context | ||
| .options() | ||
| .rust_features | ||
| .unsafe_extern_blocks | ||
|
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. I don't get this. Why do we need this to depend on
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. Isn't it only possible to use the
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. As @loftyinclination said, |
||
| .then( || { | ||
| context.options().last_callback(|cb| { | ||
| cb.declare_safe(crate::callbacks::ItemInfo { | ||
| name: &item_ident.to_string(), | ||
| kind: item_kind, | ||
| }) | ||
| }) | ||
| }) | ||
| .flatten() | ||
| .map(|safety_comment| { | ||
| let comment = | ||
| proc_macro2::Punct::new('/', proc_macro2::Spacing::Joint); | ||
| let comment2 = | ||
| proc_macro2::Punct::new('*', proc_macro2::Spacing::Alone); | ||
| let comment3 = | ||
| proc_macro2::Punct::new('*', proc_macro2::Spacing::Joint); | ||
| let comment4 = | ||
| proc_macro2::Punct::new('/', proc_macro2::Spacing::Alone); | ||
|
|
||
| quote!(#comment #comment2 Safety: #safety_comment #comment3 #comment4) | ||
| }); | ||
|
|
||
| let item_safety = safety_comment.is_some().then_some(quote!(safe)); | ||
| (safety_comment, item_safety) | ||
| } | ||
|
|
||
| pub(super) fn serialize_items( | ||
| result: &CodegenResult, | ||
| context: &BindgenContext, | ||
|
|
||
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.
So this extra test is only to test the comment generation? Why not making it a doc comment to begin with? I'd rather remove this.