Handles part 2 #1826
Closed
Handles part 2 #1826
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is part of mozilla#1791, which was merged just after the `0.25.0` release was made. We wanted to give that code a bit of time in main before releasing it. However, this part is useful for traits implemented in Rust.
* Kotlin: fixed external types with async functions (mozilla#1798) * Swift: fixed compile error when multiple crates define async functions
A single global callback won't work if there are multiple foreign modules. In `main` we handle this by inputting a callback in `poll()`. In `release-v0.25.x` we can't change `UNIFFI_CONTRACT_VERSION`, so instead we keep the `rust_future_continuation_callback_set` scaffolding function, then have the scaffolding code pass the callback to `rustfutures.rs`. Added some tests for this in `fixtures/ext-types/proc-macro-lib`. Things worked on Swift and Python for me because that code was based on leaking a pointer and it happened to work if the pointer came from another module. It just halted on Kotlin though and I think the same would happen if I wasn't on CPython and the alternate PointerMangager was used.
bendk
requested review from
jhugman and
mhammond
and removed request for
a team
bendk
force-pushed
the
handles-part2
branch
2 times, most recently
from
10731d1 to
f614359
Compare
bendk
force-pushed
the
handles-part2
branch
3 times, most recently
from
0a30016 to
e4fd71e
Compare
Merge 0.25.1 to main
We noticed a regression in `0.25.x` where dictionaries containing flat errors caused code not to be compiled. We never officially supported compound types that stored errors, but it seems better to allow the code to compile in this case. The reason this isn't supported is that we can't lift flat errors, so therefore we can't lift the dictionary. A better fix for this would be to not implement `Lift` on the dictionary in this case, which would cause a compile error rather than a runtime panic. However, that's actually not so simple as noted in the comment so I went for an easier way. This brings this very close to how they worked in `0.24.x` where we would generate an `FfiConverter` implementation for flat errors, but the lift half of it would just be panics.
Implement `Lift` for flat errors
We noticed a regression in `0.25.x` where dictionaries containing flat errors caused code not to be compiled. We never officially supported compound types that stored errors, but it seems better to allow the code to compile in this case. The reason this isn't supported is that we can't lift flat errors, so therefore we can't lift the dictionary. A better fix for this would be to not implement `Lift` on the dictionary in this case, which would cause a compile error rather than a runtime panic. However, that's actually not so simple as noted in the comment so I went for an easier way. This brings this very close to how they worked in `0.24.x` where we would generate an `FfiConverter` implementation for flat errors, but the lift half of it would just be panics.
Some tweaks to Kotlin were necessary and apart from discovering mozilla#1854 things tended to work fine.
That module was getting pretty long and I want to add some more functionality to it. Let's split it up into smaller files. This commit just moves things around and renames `ContinuationDataCell` to `Scheduler`.
bendk
force-pushed
the
handles-part2
branch
3 times, most recently
from
40df809 to
7874bbd
Compare
Docstrings are declared in .udl file by prefixing a line with three slashes ///. Docstrings can be placed basically anywhere - functions, objects, methods, constructors, callbacks, etc.. A docstring placed in a wrong place will generate UDL parser error.
`Handle` is a u64 newtype that I hope to use to represent objects that get passed across the FFI. `HandleAlloc` is an ffi trait that converts between `Arc<>` and `Handle`.
Consolidated the various handle maps into a single implementation for each language. This handle map works basically the same as all the others, but it's API is based on the `HandleAlloc` trait. Handles have a couple properties: * All foreign handles are odd, which allows us to distinguish between Rust and foreign handles. * For handles store a map ID that can detect when a handle is used with the wrong map. Made all languages always use the handle maps for passing objects. No more trying to leak pointers from to foreign objects. Started updating the ForeignExecutor code to use handles, but this is still a WIP while the ForeignExecutor type is in it's limbo state.
I want to fix mozilla#1797 by cloning the object handle in lower(). However, this presents an issue in the dynamic languages because they perform various failable checks in lower(): type checks, bounds checks, etc. If the check fails, then the cloned handle will be leaked. Prevent this by adding a `check()` method. We do the check first then once we get to `lower()`, nothing can fail.
Currently, `lower()` always returns a borrow of the object handle. This is fine for function arguments, since you know the object is still alive on the stack while the function is being called. However, for function returns this is not correct. To fix this: clone the handle in `lower()`. Added a test for this -- it was surprisingly easy to cause a segfault with the current behavior.
Check if the trait interface handle originates from the other side of the FFI. If it does, clone the handle, rather than returning a handle to the wrapped object. Before, each time the trait object was passed across the FFI, we wrapped it another time On the foreign side, this can be accomplished by a type check. On the Rust side, this requires an extra, `#[doc(hidden)]` method on the trait. This means that UDL-defined trait interfaces need to be wrapped with an attribute macro that inserts that method. Reworked the changelog so that breaking changes for external bindings is its own section. Removed the reexport-scaffolding-macro fixture which often requires changes when the FFI changes. I don't think it's giving us enough value at this point to justify continuing to update it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds on #1823, let's not merge it until that one's approved. This PR:
As mentioned in #1797, I initially wanted to only clone the handles when we returned them, not when passing them as arguments. However, that seemed too complicated so I went with the simpler approach of always cloning them in
lower().