jerel / membrane

Membrane is an opinionated crate that generates a Dart package from a Rust library. Extremely fast performance with strict typing and zero copy returns over the FFI boundary via bincode.
Apache License 2.0
89 stars 12 forks source link

Update ffigen #17

Closed kturney closed 2 years ago

kturney commented 2 years ago
Help me Jerel, you're my only hope. ![Save me Jerel](https://c.tenor.com/94c-LWTF2b0AAAAC/leia-youre-my-only-hope.gif)

I've gone down a bit of a rabbit hole trying to get M1 support without resorting to hacks like symlinking the Homebrew llvm. Latest ffigen works great on the M1, but we've got some membrane type issues to iron out.

Tests are allllllmost passing, as seen in https://github.com/jerel/membrane/commit/35b8da1e8982805a1ed91c487a96f9e896617e80

Without the test in that commit skipped, the first error we get is (https://github.com/jerel/membrane/commit/1ea6cdd9b03ee9f7602adb95c9ab0eb3463251f7)

00:00 +6 -1: can call a function with optional args with none of the args or all of the args [E]
  type 'Pointer<Int64>' is not a subtype of type 'Pointer<Long>' of 'two'
  package:dart_example/accounts.dart 1928:31  AccountsApi.optionsDemo
  test/main_test.dart 79:24                   main.<fn>

Okay. Easy enough, right? Change Int64 to Long (https://github.com/jerel/membrane/commit/3219c7f9d5e8f4e81f0953a2818d39242897a205).

Now the fun error happens.

00:00 +0 -1: loading test/main_test.dart [E]
  Failed to load "test/main_test.dart":
  lib/accounts.dart:1887:11: Error: The method 'asTypedList' isn't defined for the class 'Pointer<Long>'.
   - 'Pointer' is from 'dart:ffi'.
   - 'Long' is from 'dart:ffi'.
  Try correcting the name to the name of an existing method, or defining a method named 'asTypedList'.
        ptr.asTypedList(1).setAll(0, [two]);
            ^^^^^^^^^^^

Digging in, it looks like there is no extension LongPointer on Pointer<Long> like there is for extension Int64Pointer on Pointer<Int64> and most of the other types. We can see where they recently added the extension BoolPointer during some native type work.

Do we need a feature request to the Dart SDK? And then we will wait for another language release?

I'm not entirely sure why we need the asTypedList part. Is there a way we can work around the lack of it?

Potential other solution is finding some way to get the bindings to generate with Int64 instead of Long for Rust i64.

jerel commented 2 years ago

Thanks for the excellent detailed bug report. :grin: It does seem odd that dart:ffi doesn't yet provide an extension for Long... as far as I can tell we would need it to convert any ffi.Long value into a Uint8List via the asTypedList method.

However from the first error this is looking like a change in how ffigen interprets C types between version 4.* and 5.0. In our membrane_types/src/c.rs module we generate the C types that are used to write the .h files that ffigen ingests. We've been using signed long and up until now ffigen created a Pointer<ffi.Int64> from that, in 5.0.0 it begins generating a more literal Pointer<ffi.Long> and all the code downstream then begins failing with type errors. If we make the membrane generated C types more explicit by changing c.rs from signed long to an int64_t that may fix the compatibility issue.

kturney commented 2 years ago

I guess minimum rust version for tests to pass is 1.54 due to iterable arrays

a weird error I got with 1.53 ```bash test tests/ui/single.rs ... mismatch EXPECTED: ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ error: #[async_dart] expects a `namespace` attribute --> tests/ui/single.rs:24:1 | 24 | #[async_dart] | ^^^^^^^^^^^^^ | = note: this error originates in the attribute macro `async_dart` (in Nightly builds, run with -Z macro-backtrace for more info) error: only `namespace=""`, `disable_logging=true`, `os_thread=true`, and `timeout=1000` are valid options --> tests/ui/single.rs:27:1 | 27 | #[async_dart(namespace = "a", foo = true)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the attribute macro `async_dart` (in Nightly builds, run with -Z macro-backtrace for more info) error: `os_thread=true` is not a valid option for `sync_dart` --> tests/ui/single.rs:30:1 | 30 | #[sync_dart(namespace = "a", os_thread = true)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the attribute macro `sync_dart` (in Nightly builds, run with -Z macro-backtrace for more info) error: expected enum `Result` --> tests/ui/single.rs:36:29 | 36 | pub async fn no_result() -> i32 {} | ^^^ error: A tuple may not be returned from an `async_dart` function. If a tuple is needed return a struct containing the tuple. --> tests/ui/single.rs:39:37 | 39 | pub async fn bare_tuple() -> Result<(i32, i32), String> {} | ^^^^^^^^^^ error: expected enum `Result` --> tests/ui/single.rs:42:36 | 42 | pub async fn top_level_option() -> Option {} | ^^^^^^ error: expected a struct, vec, or scalar type but found `dyn Fn()` --> tests/ui/single.rs:45:36 | 45 | pub async fn return_fn() -> Result {} | ^^^ error: #[sync_dart] expected a return type of `Result` found an emitter --> tests/ui/single.rs:53:8 | 53 | pub fn emitter_in_sync_return() -> impl membrane::emitter::Emitter> { | ^^^^^^^^^^^^^^^^^^^^^^ error: not a supported argument type for Dart interop --> tests/ui/single.rs:60:26 | 60 | pub async fn failing_arg(self) -> Result<(), String> { | ^^^^ error: not a supported argument type for Dart interop, please use i64 instead. --> tests/ui/single.rs:65:32 | 65 | pub async fn bad_arg_type(one: i32) -> Result {} | ^^^ error: not a supported argument type for Dart interop --> tests/ui/single.rs:68:35 | 68 | pub async fn failing_arg_two(foo: &[i8]) -> Result<(), String> { | ^^^^^ ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ ACTUAL OUTPUT: ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ error: #[async_dart] expects a `namespace` attribute --> tests/ui/single.rs:24:1 | 24 | #[async_dart] | ^^^^^^^^^^^^^ | = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info) error: only `namespace=""`, `disable_logging=true`, `os_thread=true`, and `timeout=1000` are valid options --> tests/ui/single.rs:27:1 | 27 | #[async_dart(namespace = "a", foo = true)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info) error: `os_thread=true` is not a valid option for `sync_dart` --> tests/ui/single.rs:30:1 | 30 | #[sync_dart(namespace = "a", os_thread = true)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info) error: expected enum `Result` --> tests/ui/single.rs:36:29 | 36 | pub async fn no_result() -> i32 {} | ^^^ error: A tuple may not be returned from an `async_dart` function. If a tuple is needed return a struct containing the tuple. --> tests/ui/single.rs:39:37 | 39 | pub async fn bare_tuple() -> Result<(i32, i32), String> {} | ^^^^^^^^^^ error: expected enum `Result` --> tests/ui/single.rs:42:36 | 42 | pub async fn top_level_option() -> Option {} | ^^^^^^ error: expected a struct, vec, or scalar type but found `dyn Fn()` --> tests/ui/single.rs:45:36 | 45 | pub async fn return_fn() -> Result {} | ^^^ error: #[sync_dart] expected a return type of `Result` found an emitter --> tests/ui/single.rs:53:8 | 53 | pub fn emitter_in_sync_return() -> impl membrane::emitter::Emitter> { | ^^^^^^^^^^^^^^^^^^^^^^ error: not a supported argument type for Dart interop --> tests/ui/single.rs:60:26 | 60 | pub async fn failing_arg(self) -> Result<(), String> { | ^^^^ error: not a supported argument type for Dart interop, please use i64 instead. --> tests/ui/single.rs:65:32 | 65 | pub async fn bad_arg_type(one: i32) -> Result {} | ^^^ error: not a supported argument type for Dart interop --> tests/ui/single.rs:68:35 | 68 | pub async fn failing_arg_two(foo: &[i8]) -> Result<(), String> { | ^^^^^ ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ note: If the actual output is the correct output you can bless it by rerunning your test with the environment variable TRYBUILD=overwrite test tests/ui/stream.rs ... ok test compiler_ui ... FAILED failures: ---- compiler_ui stdout ---- thread 'compiler_ui' panicked at '1 of 2 tests failed', /Users/kyle.turney/.cargo/registry/src/github.com-1ecc6299db9ec823/trybuild-1.0.63/src/run.rs:101:13 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: compiler_ui test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 16.36s ```
jerel commented 2 years ago

Thanks for this contribution Kyle, great work as always!

On your last comment above that error is coming from https://github.com/dtolnay/trybuild which I use to guard against compiler errors changing from something that's helpful into something misleading / unhelpful. On older Rust versions the compiler emits very slightly different messages (usually just different whitespace or spanning) and so that test diff fails despite the Membrane code still being correct.

One very tiny change I made was to switch chain([]) to chain(vec![]) so we could keep Rust 1.49 compat (we only build against 1.49 and test against stable). I don't know if anyone is using this project with older versions but for now I guess I'll keep it since it's low effort. :man_shrugging: