Conversation
lionel-
force-pushed
the
task/clippy-unsafe
branch
2 times, most recently
from
6bcf156 to
f075190
Compare
lionel-
force-pushed
the
task/clippy-unsafe
branch
from
f075190 to
a6714bc
Compare
crates/libr/src/types.rs
Outdated
| /// across FFI boundaries. | ||
| #[repr(transparent)] | ||
| #[derive(Copy, Clone, PartialEq, Eq, Hash)] | ||
| pub struct SEXP(pub *mut SEXPREC); |
There was a problem hiding this comment.
I'd advocate for not making pub *mut SEXPREC pub
It seems like it compiles fine for me without this
There was a problem hiding this comment.
That would make it harder to convert a *void to a SEXP, which can be needed e.g. for callbacks. We could add a From method for this case though.
I'm not sure we need to prevent access on the raw pointer, which is as (un)safe as our wrapper.
I'll make it private for now though.
crates/harp/src/weak_ref.rs
Outdated
| Some(RObject::new(unsafe { | ||
| libr::R_WeakRefValue(self.weak_ref.sexp) | ||
| })) | ||
| Some(RObject::new({ libr::R_WeakRefValue(self.weak_ref.sexp) })) |
There was a problem hiding this comment.
When I compile locally I get a note about extra braces
crates/libr/src/constant_globals.rs
Outdated
| pub unsafe fn $name() -> bool { | ||
| [<$name _has>] | ||
| pub fn $name() -> bool { | ||
| unsafe { [<$name _has>] } |
There was a problem hiding this comment.
I do not agree with making everything exposed from libr safe.
These are by definition unsafe. I mean, it's as unsafe as you can get. You're calling the R API directly.
I'd like to talk about this if you feel strongly that we should do this, it feels very wrong to me and like a very large change. We have typically been "re-exposing" the unsafe R API with safer Rust wrappers.
There was a problem hiding this comment.
We can talk about it, but many of our "safe" wrappers are strictly just as safe as the libr entry points (not very).
With clippy you now get lints about dereference of other raw pointer types such as * char. So we still have this safety net, which is more targeted.
Unless you wrap the R API with fully vetted Rust types and methods, the harp wrappers are still unsafe by nature, you have to know what you're doing to use them safely. I think making the libr entry points safe or unsafe is mostly inconsequential from this point of view.
I'll try and go back to unsafe to see what this actually buys us though, might be easy to keep it unsafe.
There was a problem hiding this comment.
yep this was just about missing Safety doc! No real need to make it safe, it was just for consistency with our harp wrappers. I agree it also makes sense for this lower level crate to provide unsafe entry points.
I've added an opt out for this lint in libr.
2 participants
Commits 1 and 3: Remove
unsafefrom more harp functions. That fixes the "missing safety doc" lint.Commit 2 (best reviewed separately): Wrap
SEXPin a type instead of a type alias, to avoid clippy requiring#[allow(clippy::not_unsafe_ptr_arg_deref)]on every function taking and dereferencing aSEXP. The type is tagged with#[repr(transparent)]to ensure FFI compatibility.Wrapping a raw pointer in this way with transparent representation is the standard approach for type safety, see e.g.
PinandNonNull. https://doc.rust-lang.org/reference/type-layout.html#the-transparent-representation