neon-bindings/rfcs

Pre-RFC: Borrowing refactor

Opened this issue · 1 comments

Borrow

Problem

The current borrowing API was designed to support borrowing both binary JavaScript data and the internal Rust data of classes. It is intended to prevent aliased borrows.

However, it has multiple soundness holes (JavaScript handles might alias the same object, multiple Lock can be created). It also has an API that includes types and helpers from before NLL that are awkward and unnecessary today.

Lastly, classes were removed in the N-API backend, leaving a generic API that only applies to borrowing binary data.

Proposal

Module

  • Remove neon::borrow
  • Create neon::binary
  • Move binary types (e.g., JsArrayBuffer) to neon::binary
  • Move borrowing into neon::binary

Removal

Buffer is a legacy type from before JavaScript included ArrayBuffer. It is now a sub-class of Uint8Array. Neon does not need any type level distinction for JsBuffer because it can be treated as an array view.

  • Remove JsBuffer
  • Only reference to buffer is a constructor on ArrayBuffer for creating a Buffer. The return type is indistinguishable from a Uint8Array in Neon.

Addition

Neon does not currently understand views. We will need to add this. We most likely do not need to know the view type and can have a simple JsArrayView. If we want to include the type, it could be done with a generic phantom data element. JsArrayView<u8>.

Borrowing

In an initial implementation, we will only support borrowing ArrayBuffer. This way we can avoid complicated set math for determining if any of our borrows overlap. We can simply keep a HashSet of starting pointers.

Lock

The Lock type will be removed from the public API and moved to Context.

Views

Borrowing views will mostly be a convenience API. We will still borrow the entire ArrayBuffer. This will behave very similar to how borrowing a &[u8] from a Vec<u8> borrows the entire Vec.

This is necessary to remove complexity from users. If we don't provide this API, users cannot borrow buffers without first either copying data to an ArrayBuffer or passing the underlying buffer along with a Range.

Great proposal, thanks for writing it up!

A couple possible tweaks:

  • Maybe JsArrayView should be called JsArrayBufferView to match the standard name?
  • I'd suggest the buffer constructor should be on the view type instead of JsArrayBuffer, since Buffer <: Uint8Array <: ArrayBufferView (i.e. a buffer is-a ArrayBufferView and has-a ArrayBuffer).