A more principled approach to managing C handles

[lnicola]

All the C getters (c_dataset and friends) are unsafe, but this is totally unidiomatic, cf. https://doc.rust-lang.org/std/os/fd/trait.IntoRawFd.html#tymethod.into_raw_fd or pointer arithmetic. Rust considers it safe to manipulate pointers and other handles, while dereferencing or turning them back into something unsable is unsafe.

I think we should follow the same pattern.

Update

by @metasim

The scope of this topic has expanded beyond just the C handle getters into how we might be more principled, consistent and idiomatic.

Related

• Borrowed vs. owned geometries #4
• RFC: handle getters (c_dataset and friends) should be safe #168
• Debug-printing geometries segfaults #365
• Provide an API to create a Geometry object from an owned OGRGeometryH #306

[metasim]

@lnicola If we make this change we should probably work out some conventions ahead of time:

1. Naming: Right now we have methods like c_dataset, to_c_hsrs, c_options, to_c_hct, c_geometry, etc.
   a. Should it have a preposition prefix (to_, into_, as_, etc.), implying a conversion or consumption, or just be a noun (e.g. c_dataset, c_geometry)? I think the prefixes imply more than it being a getter, and therefore should be a noun only.
   b. Should they be type specific (e.g. c_dataset) or generic/consistent (c_handle, or gdal_ptr)? I prefer the latter, but that would be breaking.
2. In the case of into_raw_fd, the receiver is consumed. I don't think that's what we should do, because it's usually used internally to call into the C API and we want to use the Rust wrapper. But worth considering.
3. Visibility: should the pointers/handles to all C-GDAL objects be public? Or case-by-case? Or none? I prefer none, but that would require us to have all the GDAL API implemented.
4. Would returning something like Borrow<HandleType> be safer (keep unintended aliasing from happening)? Currently (at least in the Geometry API) I think you can create two Rust instances pointing to the same C memory. (?)
5. Should all wrapper types have new_from_handle methods, whereby users can instantiate any of the Rust types with handles fetched from who-knows-where? I personally think things like Geometry::owned have code smell and suspect there must be a better way of managing pointer provenance.
   • Corollary: Should this be even allowed? What's the use case?

[metasim]

To help inform our approach, I experimented with converting SpatialRef, CoordTransform and CoordTransformOptions to use the foreign_types crate, which seems quite principled in how it manages then handling of pointers to C structs as both owned and borrowed values.

master...metasim:gdal:prototype/foreign_types

So far I'm really liking the consistency and the better handling of returning borrowed types (e.g. a SpatialRef that's owned by, say, a Layer. I really think it shows promise.

Example

This:

foreign_type! {
    /// A OpenGIS Spatial Reference System definition.
    #[derive(Debug)]
    pub unsafe type SpatialRef {
        type CType = libc::c_void;
        fn drop = gdal_sys::OSRRelease;
        fn clone = gdal_sys::OSRClone;
    }
}

Expands to this (we don't have to use the macro, but I think it's worth it):

#[doc = " A OpenGIS Spatial Reference System definition."]
#[derive(Debug)]
#[repr(transparent)]
pub struct SpatialRef(::foreign_types::export::NonNull<libc::c_void>);
#[doc = "A borrowed reference to a [`SpatialRef`](struct.SpatialRef.html)."]
pub struct SpatialRefRef(::foreign_types::Opaque);
unsafe impl ::foreign_types::ForeignType for SpatialRef {
    type CType = libc::c_void;
    type Ref = SpatialRefRef;
    #[inline]
    unsafe fn from_ptr(ptr: *mut libc::c_void) -> SpatialRef {
        SpatialRef(<::foreign_types::export::NonNull<_>>::new_unchecked(ptr))
    }
    #[inline]
    fn as_ptr(&self) -> *mut libc::c_void { <::foreign_types::export::NonNull<_>>::as_ptr(self.0) }
}
unsafe impl ::foreign_types::ForeignTypeRef for SpatialRefRef { type CType = libc::c_void; }
impl ::foreign_types::export::Drop for SpatialRef {
    #[inline]
    fn drop(&mut self) { unsafe { (gdal_sys::OSRRelease)(::foreign_types::ForeignType::as_ptr(self)); } }
}
impl ::foreign_types::export::Deref for SpatialRef {
    type Target = SpatialRefRef;
    #[inline]
    fn deref(&self) -> &SpatialRefRef { unsafe { ::foreign_types::ForeignTypeRef::from_ptr(::foreign_types::ForeignType::as_ptr(self)) } }
}
impl ::foreign_types::export::DerefMut for SpatialRef {
    #[inline]
    fn deref_mut(&mut self) -> &mut SpatialRefRef { unsafe { ::foreign_types::ForeignTypeRef::from_ptr_mut(::foreign_types::ForeignType::as_ptr(self)) } }
}
impl ::foreign_types::export::Borrow<SpatialRefRef> for SpatialRef {
    #[inline]
    fn borrow(&self) -> &SpatialRefRef { &**self }
}
impl ::foreign_types::export::BorrowMut<SpatialRefRef> for SpatialRef {
    #[inline]
    fn borrow_mut(&mut self) -> &mut SpatialRefRef { &mut **self }
}
impl ::foreign_types::export::AsRef<SpatialRefRef> for SpatialRef {
    #[inline]
    fn as_ref(&self) -> &SpatialRefRef { &**self }
}
impl ::foreign_types::export::AsMut<SpatialRefRef> for SpatialRef {
    #[inline]
    fn as_mut(&mut self) -> &mut SpatialRefRef { &mut **self }
}
impl ::foreign_types::export::Clone for SpatialRef {
    #[inline]
    fn clone(&self) -> SpatialRef {
        unsafe {
            let ptr = (gdal_sys::OSRClone)(::foreign_types::ForeignType::as_ptr(self));
            ::foreign_types::ForeignType::from_ptr(ptr)
        }
    }
}
impl ::foreign_types::export::ToOwned for SpatialRefRef {
    type Owned = SpatialRef;
    #[inline]
    fn to_owned(&self) -> SpatialRef {
        unsafe {
            let ptr = (gdal_sys::OSRClone)(::foreign_types::ForeignTypeRef::as_ptr(self));
            ::foreign_types::ForeignType::from_ptr(ptr)
        }
    }
}

[metasim]

I've now moved on to migrating several other types. Here's the current list, updated as I go:

• Defn
• Feature
• Geometry
• SpatialRef
• CoordTransform
• CoordTransformOptions
• RasterBand

Periodically testing under cargo-careful here.

[metasim]

Notes on foreign_types.

• Documentation and examples are somewhat sparse, but maintainer is responsive to how-to questions. If you dig deep enough (including the very large projects that use the crate), you get good examples.
• At first I didn't love the idea of using a macro, but after implementing ForeignType and ForeignTypeRef manually, it's definitely worth it.
• Can handle both generics as well as lifetime parameters, once you understand the macro.
• ForeignType::from_ptr, which is unsafe, is how you create instances.
• ForeignType::as_ptr, which is "safe" (as we desire here), lets you get the pointer.
• ForeignType::into_ptr consumes/moves ownership, helpful for C functions that take over ownership.
• Most type impl methods should be implemented on the borrowed XYZRef type, because the owned XYZ type auto de-refs to it, enabling methods to work on both types.
• If Clone is supported, then ToOwned is implemented by the macro, which is nice. This is how you go from XYZRef to XYZ (via XYZRef::to_owned).
• (I think) The value of type CType is implicitly expected to be Sized. Therefore (and I could be missing something), the type alias in bindgen can't be explicitly used. For example bindgen gives us type OGRSpatialReferenceH = *mut libc::c_void, but in ForeignType implementation, type CType = OGRSpatialReferenceH will cause errors. Instead it ends up needing to be type CType = libc::c_void, which seems unfortunate.

[metasim]

Notes on Migration

• There are several cases–such as Layer–where the code is simplified and made more principled through foreign_types.
  • e.g.: Layer was holding onto a &Defn solely for the lifetime parameter tracking, which wasn't technically needed originally, and easier to eliminate with foreign_types.
  • OwnedLayer can go away.
  • We can eliminate owned: bool fields that exist in some types because this is explicit in the type; e.g. Geometry.
• In cases where the type is always owned by the C library and never managed directly in Rust, the implementation is quite simple if you use Opaque and implement ForeignTypeRef. e.g. RasterBand.
• Haven't yet figured out if pointers to pointers to C structures (such as CslStringList) can be effectively managed by this library.
• The mutability semantics around RasterBand are decoupled from Dataset. Even though Dataset owns every RasterBand and defines its lifetime, you can mutate a RasterBand without having a mutable reference to Dataset. Not sure if this is customary, an oddity, or a non-issue.
  • I think this is right: Under foreign_types, Dataset::rasterband(&self, band_index: isize) would return Result<&mut Rasterband>. Or maybe there's a new method Dataset::rasterband_mut(&mut self, band_index: isize).

[lnicola]

Under foreign_types, Dataset::rasterband(&self, band_index: isize) would return Result<&mut Rasterband>. Or maybe there's a new method Dataset::rasterband_mut(&mut self, band_index: isize).

Shouldn't it return Result<&RasterBandRef> or Result<&mut RasterBandRef>?

The mutability semantics around RasterBand are decoupled from Dataset.

I think we can do fn rasterband and Dataset::rasterband_mut, and implement the latter only for mutable Datasets.

[metasim]

Shouldn't it return Result<&RasterBandRef> or Result<&mut RasterBandRef>?

Yes.

This highlights an aspect of foreign_types I don't like (but kinda understand why). The library almost normalizes owned vs borrowed values across the C/Rust FFI barrier, except it introduces the {T}Ref type for borrowed values instead of relying wholly on the Rust references (e.g. 'a & {T}). I surmise (🤷‍♂️) it has something to do with working round lifetime parameter constraints and aliasing a pointer with a reference, but I think the ergonomics would be better if there was just a single RasterBand type, and signatures were like:

• Dataset::rasterband(&self, band_index: isize) -> Result<&RasterBand>
• Dataset::rasterband_mut(&self, band_index: isize) -> Result<&mut RasterBand>

[lnicola]

Posting as top-level because I didn't look in close detail to your changes and other questions, but I think:

• we absolutely want to distinguish between owned and borrowed values
• we can already do this today, with better or worse success
• I still have no idea what that lazy_feature_geometry thing is 😅
• we don't need foreign-types -- if you look at the code, it's tiny and pretty much trivial except Opaque and the macro
• I don't particularly like the as_ptr / into_ptr naming convention
• I strongly dislike proc macros, for various reasons
• however, being consistent here would be pretty good, and future code would do well to reuse the same patterns

So, while I don't particularly love foreign-types, I think it would be great if we could use it. Maybe we can try this for the next release.

[metasim]

I strongly dislike proc macros, for various reasons

Do you include derive macros under the category of proc macros?

[metasim]

we don't need foreign-types -- if you look at the code, it's tiny and pretty much trivial except Opaque and the macro

From where I sit, it provides two benefits.

1. Enforces consistency across what is otherwise a pretty sprawling API (e.g. someone working on vector ends up doing things differently from someone working on raster).
2. Significantly reduces boilerplate.

That said, I think we could do something simpler that's more closely linked to our use cases and naming conventions with traits and derive macros.

[metasim]

I don't particularly like the as_ptr / into_ptr naming convention

I like the as_ and into_ parts, because it distinguishes (in the foreign_types context) the difference between borrowing a pointer vs taking ownership of the pointer. This is important in gdal because some pointer returning functions indicate only through documentation that the receiver does or does not have responsibility for freeing the memory.

As for the _ptr part, would you prefer something like _c_gdal; as in as_c_gdal and into_c_gdal.

[lnicola]

Do you include derive macros under the category of proc macros?

Yes 😬.

As for the _ptr part, would you prefer something like _c_gdal, as in as_c_gdal and into_c_gdal.

Yeah, that's what I meant. as_ and into_ are fine, ptr feels a bit overloaded here. curl has plain raw, openssl uses foreign-types, I'm not sure what convention other crates are using.

[metasim]

For the record, I think maintaining consistency and safety will require a lot more work on our and our contributors' part if we shun the use of proc/derive macros. While I think macros can be easily abused, I do think they have worthwhile benefits, particularly derive macros. I'm not sure what the right next step is here.

[metasim]

A PR has been submitted to provide a more concrete foundation to collaboratively further work this issue.