Complete Dart slice borrowing #437
Conversation
Manishearth
changed the title
Manishearth
force-pushed
the
dart-struct-appendlists
branch
from
0199fdc to
da768cb
Compare
| //! appropriately: [`StructBorrowInfo::compute_for_struct_field()`] is super helpful for getting a list edges something should get passed down to. | ||
| //! | ||
| //! Finally, when converting Rust-to-host, the relevant lifetime edges should be proxied down to the final host type constructors who can stash them wherever needed. | ||
| //! This is one again a matter of filtering fields by the lifetimes they use, and for nested structs you can use [`StructBorrowInfo::compute_for_struct_field()`]. |
There was a problem hiding this comment.
praise: nice explanation
There was a problem hiding this comment.
I really felt that the existing code was confusing partly because there was nothing telling you where to start. I might document the borowing_fields approach some day.
| @@ -26,17 +26,45 @@ final class BorrowedFields { | |||
| c = Utf8Decoder().convert(underlying.c._pointer.asTypedList(underlying.c._length)); | |||
|
|
|||
| // ignore: unused_element | |||
| _BorrowedFieldsFfi _pointer(ffi.Allocator temp) { | |||
| // If this struct contains any slices, their lifetime-edge-relevant objects (typically _FinalizedArenas) will only | |||
There was a problem hiding this comment.
nit: I think explanatory comments like this should live in the generating code, rather than the generated code
There was a problem hiding this comment.
I kiiinda want people to be able to understand what the code does when they poke at it but in this case I get it
There was a problem hiding this comment.
I changed my mind: I think it's useful to have these in the generated code to track things down, especially since which code handles which edges is nontrivial
| ParamBorrowInfo::TemporarySlice => false, | ||
| ParamBorrowInfo::BorrowedSlice => true, | ||
| _ => unreachable!( | ||
| "Slices must produce slice ParamBorrowInfo, found {param_borrow_kind:?}" |
There was a problem hiding this comment.
Not a big fan of this error case existing. We already have the type, why does ParamBorrowInfo include the type again?
There was a problem hiding this comment.
because the different types contain different kinds of borrowing info. I'm not a fan of this case either but it seemed like the cleanest way to get info across, no matter what we do there will be some such assert somewhere
There was a problem hiding this comment.
Then should we get the borrow info from the hir::Slice?
There was a problem hiding this comment.
No because this function computes it all
There was a problem hiding this comment.
Basically, either we have this error case, or we return ParamBorrowInfo as a more structlike thing with is_borrowed and struct_borrow_info fields, with an assertion that struct_borrow_info is none for non-structs
| _FinalizedArena.withLifetime(core.List<core.List<Object>> lifetimeAppendArray) : this.arena = ffi2.Arena() { | ||
| _finalizer.attach(this, this.arena); |
There was a problem hiding this comment.
you can slightly simplify this by invoking the main constructor
| _FinalizedArena.withLifetime(core.List<core.List<Object>> lifetimeAppendArray) : this.arena = ffi2.Arena() { | |
| _finalizer.attach(this, this.arena); | |
| _FinalizedArena.withLifetime(core.List<core.List<Object>> lifetimeAppendArray) : this() { |
There was a problem hiding this comment.
aha! Was wondering if there was syntax for that
There was a problem hiding this comment.
Nope:
lib/src/lib.g.dart:348:91: Error: Redirecting constructors can't have a body.
Try removing the body, or not making this a redirecting constructor.
_FinalizedArena.withLifetime(core.List<core.List<Object>> lifetimeAppendArray) : this() {
a0264e7 to
ff91676
Compare
This completes the Dart slice borrowing work fixing #421. It adds appendlists for edges that can be passed down for slices to be appended to.
Also fixes #437: I decided that by removing implicit lifetimes from the equation; we can simplify a bunch of code that had to previously worry about lifetime dependencies. Yay!
I believe this technique has sufficient helpers that it will be easy to implement in JS when needed.
This code does not handle nested structs completely correctly. It's a minor thing and I know how to fix it, but at this point this PR is already complicated enough, I'll work on that soon.
Part of #439