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
) toneon::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 onArrayBuffer
for creating aBuffer
. The return type is indistinguishable from aUint8Array
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 calledJsArrayBufferView
to match the standard name? - I'd suggest the
buffer
constructor should be on the view type instead ofJsArrayBuffer
, sinceBuffer
<:Uint8Array
<:ArrayBufferView
(i.e. a buffer is-aArrayBufferView
and has-aArrayBuffer
).