Use DiplomatSlice/etc instead of splitting params into data/len pairs #615

Manishearth · 2024-08-01T08:50:59Z

Fixes #613

Things this does not do that it should, but I might do in a followup:

Potentially get rid of DiplomatStr in favor of the new types. It's not really worth it to allow people to specify &[&DiplomatStr], they should just go all the way IMO.
Add checks so that the shortcuts are only allowed in params, not struct fields
Clean up the tests: Macro tests should also ensure stuff compiles #617
Use a StdlibOrRuntime enum instead of a boolean

Manishearth · 2024-08-01T12:34:08Z

@ambiguousname I'd like some help making the relevant changes here for JS.

The change made here is that basically until now slices in structs and params were handled differently: in a struct a slice becomes a DiplomatFooView C type, whereas in params it gets split into a foo_data and foo_len pair of parameters on the FFI function, which complicates conversion code (since now you need to branch on struct-vs-params, and you need to deal with the fact that some params end up becoming multiple params on the FFI function.

However I'm not super familiar with slice handling in your backend.

It appears to be the case that the Kotlin backend already handles things correctly (previously it was incorrect), where it didn't know about this issue with slice params. Fortunately, now that things are uniform, Kotlin is automagically correct already. cc @jcrist1 just so you're aware

I managed to make it work for Dart.

robertbastian

not an in-depth review

runtime/src/slices.rs

robertbastian · 2024-08-01T12:43:14Z

feature_tests/dart/lib/src/BorrowedFields.g.dart

    final (utf8StrData, utf8StrLength) = utf8Str._utf8AllocIn(utf8StrArena.arena);
+    final utf8Str = ffi.Struct.create<_SliceUtf8>();


I'd push the struct creation into allocIn, but I can follow up

I'd love that yeah.

robertbastian · 2024-08-01T12:44:09Z

feature_tests/dart/lib/src/BorrowedFields.g.dart

@@ -75,6 +81,6 @@ final class BorrowedFields {
 }

 @meta.ResourceIdentifier('BorrowedFields_from_bar_and_strings')
-@ffi.Native<_BorrowedFieldsFfi Function(ffi.Pointer<ffi.Opaque>, ffi.Pointer<ffi.Uint16>, ffi.Size, ffi.Pointer<ffi.Uint8>, ffi.Size)>(isLeaf: true, symbol: 'BorrowedFields_from_bar_and_strings')
+@ffi.Native<_BorrowedFieldsFfi Function(ffi.Pointer<ffi.Opaque>)>(isLeaf: true, symbol: 'BorrowedFields_from_bar_and_strings')


issue: the arguments disappeared

Fixed. The param_types_ffi_cast etc stuff is kinda confusing, we generate a lot of different things in one function. I didn't bother to try and understand it too deeply but we should document this better if we get the chance.

(Also now that this is no longer doing two-parameter stuff we probably can clean things up)

feature_tests/dart/lib/src/BorrowedFields.g.dart

Manishearth · 2024-08-01T13:21:40Z

I'm signing off soon, but @robertbastian please do feel free to polish up the Dart section and push, I did a quick-and-dirty fix which still appears to be broken.

(let me know if you've pushed though, so I don't overwrite)

Manishearth · 2024-08-01T13:30:23Z

I tried to fix some things but Dart's still broken

ambiguousname · 2024-08-01T14:01:20Z

I'll take a look and adjust the JS backend. We have a few open issues with how JS handles slices anyways (I believe to do with cleanup in particular), so maybe I can re-write the JS slices system to account for those issues in addition to your changes. I'll ping you before EOD today when I've got something.

Manishearth · 2024-08-01T14:31:38Z

found a double free and also fixed the dart issues, but won't be able to push for another hour I think

ambiguousname · 2024-08-01T15:31:26Z

@Manishearth I did some review of the updated .wasm output and how JS handles slices; I believe JS handles slices the same whether they're being used as struct fields or function parameters.

WASM output still requires a pointer and size, the new types are ignored: (func $icu4x_Locale_new_mv1 (type 3) (param i32 i32) (result i32)

As for backend logic, JS uses the same DiplomatBuf type for slices regardless of conversion context.

For clarity, struct field conversion is done in js/type_generation/mod.rs, line 165 and parameter conversion is done js/type_generation/mod.rs, line 307. Both use the same gen_js_to_c_for_type function.

The only change I can think of is renaming DiplomatBuf to DiplomatSliceBuf for clarity?

As a caveat, I can't seem to find tests in feature_tests that handle cases where a struct has a DiplomatSlice as a member, so I can't verify JS behavior. JS does pass all tests (including the one slice test that's in its suite) with the new changes.

TL;DR JS already handles slices for structs and function parameters in the same way. If I'm missing something or you'd like additional changes, let me know.

Manishearth · 2024-08-01T15:46:03Z

Oh yeah, that makes sense, the WASM abi hates structs so everything is done splatted out anyway. Nice to not have to worry about this!

Manishearth · 2024-08-01T16:58:35Z

And CI passes!

Manishearth · 2024-08-01T16:59:56Z

That took less time than I expected.

As a bonus, this simplifies the macro code so a lot of the gnarly DiplomatOption handling I wanted to do becomes really straightforward, and that reduces the amount of macro mess that was going to be in #558 ! (I never pushed the macro mess code because I was annoyed with it)

robertbastian

LG overall. Will work on the Dart simplifications so please no more force pushes.

core/src/ast/types.rs

example/c/main.c

Manishearth · 2024-08-06T08:48:00Z

Sounds good. I'll try to merge as soon as possible to avoid you needing to patch stack

robertbastian · 2024-08-06T09:56:00Z

example/c/main.c

-    DiplomatStringView bn = {"bn", 2};
-
-    Locale* locale = icu4x_Locale_new_mv1(bn);
+    Locale* locale = icu4x_Locale_new_mv1((DiplomatStringView){.data = "bn", .len = 2});


does this really need the cast? 😕

it fails to parse without the cast. I don't understand it either.

C is weird and has terrible errors, if you can figure out how to avoid the cast be my guest 🙃🙃

ew. in that case I'm not sure if the inline is actually nicer 🤷

yeah feel free to change it back if you'd like. I'm very ambivalent here

Manishearth added 4 commits August 1, 2024 04:47

Add test for slice codegen

cfde8d7

Add DiplomatSlice and friends

6a41357

add owned versions

cf0e5e5

Use DiplomatSlice types at FFI boundary, unconditionally

beb73f7

Manishearth force-pushed the DiplomatSlice branch 2 times, most recently from 01c58b5 to 1ccf792 Compare August 1, 2024 12:13

Manishearth marked this pull request as ready for review August 1, 2024 12:13

Manishearth requested a review from sffc August 1, 2024 12:15

Manishearth marked this pull request as draft August 1, 2024 12:34

robertbastian reviewed Aug 1, 2024

View reviewed changes

track if type is specified as Rust or Diplomat

5002b76

Manishearth force-pushed the DiplomatSlice branch from 79d11b2 to a462c01 Compare August 1, 2024 13:10

Manishearth force-pushed the DiplomatSlice branch 2 times, most recently from 7b9435a to cfa3d00 Compare August 1, 2024 13:27

Manishearth added 5 commits August 1, 2024 07:29

Update C,C++ tool to generate the right thing

8e97434

update dart

3751527

Handle null slices

8754edd

rename

4c708ee

as_ptr

27d813a

Manishearth force-pushed the DiplomatSlice branch 2 times, most recently from bf2a12a to 44a2547 Compare August 1, 2024 15:42

Throw errors when specifying FFI-unsafe types

65f889b

Manishearth force-pushed the DiplomatSlice branch from 44a2547 to 65f889b Compare August 1, 2024 15:43

Manishearth marked this pull request as ready for review August 1, 2024 15:43

Manishearth requested a review from robertbastian August 1, 2024 15:44

Manishearth force-pushed the DiplomatSlice branch 3 times, most recently from b14c6d2 to cf81dc3 Compare August 1, 2024 16:55

Manishearth mentioned this pull request Aug 1, 2024

Add DiplomatOption<T> #558

Merged

clippy

340f0fa

Manishearth force-pushed the DiplomatSlice branch from cf81dc3 to 340f0fa Compare August 1, 2024 16:58

Manishearth force-pushed the DiplomatSlice branch from 34443be to 42ad7e7 Compare August 2, 2024 05:01

Replace boolean with StdlibOrDiplomat flag

8617720

Manishearth force-pushed the DiplomatSlice branch from 42ad7e7 to 8617720 Compare August 2, 2024 05:32

robertbastian approved these changes Aug 6, 2024

View reviewed changes

core/src/ast/types.rs Show resolved Hide resolved

example/c/main.c Outdated Show resolved Hide resolved

Manishearth added 2 commits August 6, 2024 01:45

rename get_rust_slice_type

fb9751c

inline C stuff

049bd4c

Manishearth merged commit 2754a5f into rust-diplomat:main Aug 6, 2024
15 checks passed

Manishearth deleted the DiplomatSlice branch August 6, 2024 08:52

robertbastian reviewed Aug 6, 2024

View reviewed changes

Manishearth mentioned this pull request Aug 7, 2024

Callback support in the AST/HIR/macro, and codegen for C #552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DiplomatSlice/etc instead of splitting params into data/len pairs #615

Use DiplomatSlice/etc instead of splitting params into data/len pairs #615

Manishearth commented Aug 1, 2024 •

edited

Loading

Manishearth commented Aug 1, 2024

robertbastian left a comment

robertbastian Aug 1, 2024

Manishearth Aug 1, 2024

robertbastian Aug 1, 2024

Manishearth Aug 1, 2024

Manishearth commented Aug 1, 2024

Manishearth commented Aug 1, 2024

ambiguousname commented Aug 1, 2024 •

edited

Loading

Manishearth commented Aug 1, 2024

ambiguousname commented Aug 1, 2024 •

edited

Loading

Manishearth commented Aug 1, 2024

Manishearth commented Aug 1, 2024

Manishearth commented Aug 1, 2024

robertbastian left a comment

Manishearth commented Aug 6, 2024

robertbastian Aug 6, 2024

Manishearth Aug 6, 2024

robertbastian Aug 6, 2024

Manishearth Aug 6, 2024

		final (utf8StrData, utf8StrLength) = utf8Str._utf8AllocIn(utf8StrArena.arena);
		final utf8Str = ffi.Struct.create<_SliceUtf8>();

Use DiplomatSlice/etc instead of splitting params into data/len pairs #615

Use DiplomatSlice/etc instead of splitting params into data/len pairs #615

Conversation

Manishearth commented Aug 1, 2024 • edited Loading

Manishearth commented Aug 1, 2024

robertbastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Aug 1, 2024

Manishearth commented Aug 1, 2024

ambiguousname commented Aug 1, 2024 • edited Loading

Manishearth commented Aug 1, 2024

ambiguousname commented Aug 1, 2024 • edited Loading

Manishearth commented Aug 1, 2024

Manishearth commented Aug 1, 2024

Manishearth commented Aug 1, 2024

robertbastian left a comment

Choose a reason for hiding this comment

Manishearth commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Aug 1, 2024 •

edited

Loading

ambiguousname commented Aug 1, 2024 •

edited

Loading

ambiguousname commented Aug 1, 2024 •

edited

Loading