Add callback to declare functions/statics safe

[arctic-alpaca]

Since Rust 1.82, extern items can be marked as safe. This PR adds the ability to mark functions and statics in the generated bindings as safe via ParseCallbacks.
To fit into Rust's typical safety comment conventions, the callback returns a string that will be prepended to the generated item as safety comment.
Becauseprettyplease does not support non-doc comments, I added an additional test using Formatter::Rustfmt.

[ojeda]

Are these comments meant to describe only the reason why they are marked as safe, i.e. rather than the full signature being correct? Related: rust-lang/rust-clippy#13560.

[arctic-alpaca]

The specific comments are just there to test things are working, they could be replaced by dummy reasoning for test.

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 bindgen to generate correct bindings.

[loftyinclination]

Hoping to use this soon. Is there anything that's holding progress up on merging this?

[emilio]

This seems like it could be simpler. First of all, why does it depend on the unsafe blocks stuff?

I guess it's just a proxy for the target rust version? If so, we should rename it.

I'm a bit skeptic about the value of generating non-doc comments. We should either make them doc comments or not generate them at all. That also allows to simplify the test?

But also, it feels not the most useful feature, because as soon as you have a pointer input around you can't claim that the function is safe... But anyways as long as it's opt in I guess it's ok.

[emilio]

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.

[emilio]

I don't get this. Why do we need this to depend on unsafe_extern_blocks at all? This is for individual functions.

[loftyinclination]

Isn't it only possible to use the safe keyword in an unsafe_extern_block? Otherwise there's no lack of safety to comment on

[arctic-alpaca]

As @loftyinclination said, unsafe_extern_block is a prerequesite to mark an extern function or static safe (source).

[emilio]

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.

[arctic-alpaca]

I haven't seen doc comments used to explain why a method is safe. The only Safety doc comments I encountered are describing how to safely call an unsafe function. That's why I used non-doc comments here.

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]) }
}

_Source

Of course, this is only semi-applicable to the case here. I'm not aware of any official guidelines or established precedent for safe FFI functions.

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 declare_safe is what I would consider the actual SAFETY comment as to why something is safe to call, as that's where the method is declared safe. That's also what I would expect people to read/write.

I'm happy to make it a doc-comment or remove it outright if you prefer.

[loftyinclination]

I would probably prefer to keep the current implementation, as the example above matches my expectations for how this feature would be used.

[loftyinclination]

But also, it feels not the most useful feature, because as soon as you have a pointer input around you can't claim that the function is safe.

I think that this could be quite useful when marking functions whose purpose is to allocate structs helpful -- any reduction in the number of possible unsafe bindings is helpful when dealing with large code bases.

Having said that, I agree with your point about pointers -- currently, given the information accessibly through ItemInfo, it isn't possible to determine whether or not a function takes any pointers. I imagine that changing that would be outside of the scope of this work though.

[loftyinclination]

Not solely a result of these changes, but the documentation on ItemInfo only mentions ParseCallbacks::generated_name_override, even though it is now used by multiple different methods.