Skip to content

Commit 6a7dc25

Browse files
tbermanhellovai
andauthored
Fix union encoding/decoding (#1898)
So there are a handful of union issues and this fixes some of them. First, type aliases that point at classes, enums or unions were not encoding properly. That is fixed by this PR. Second, unions were not decoding properly. That is also fixed. Third, type definitions that are unions are not decoding properly, not yet fixed but the first two felt meaningful enough to put up. <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > Fixes union encoding/decoding issues and adds recursive union test. > > - **Behavior**: > - Fixes type alias encoding for classes, enums, and unions in `cffi_generated.rs` and `ctypes.rs`. > - Fixes union decoding in `types-unions.go.j2` by switching from `ValueTypeIndex` to `VariantName`. > - **Schema**: > - Adds `variant_name` field to `CFFIValueUnionVariant` in `cffi.fbs`. > - **Tests**: > - Adds `recursive-union.baml` test file for recursive union testing. > - **Misc**: > - Updates `GoTypeAlias` in `generate_types.rs` to include `is_union` and `is_baml_serializable` flags. > - Modifies `EncodeUnion` function in `encode.go` to include `variantName`. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=BoundaryML%2Fbaml&utm_source=github&utm_medium=referral)<sup> for 7b1f7bc. You can [customize](https://app.ellipsis.dev/BoundaryML/settings/summaries) this summary. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN --> --------- Co-authored-by: hellovai <vbv@boundaryml.com>
1 parent 8104859 commit 6a7dc25

57 files changed

Lines changed: 1695 additions & 279 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

engine/language_client_cffi/src/cffi/cffi_generated.rs

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2336,9 +2336,10 @@ impl<'a> flatbuffers::Follow<'a> for CFFIValueUnionVariant<'a> {
23362336

23372337
impl<'a> CFFIValueUnionVariant<'a> {
23382338
pub const VT_NAME: flatbuffers::VOffsetT = 4;
2339-
pub const VT_FIELD_TYPES: flatbuffers::VOffsetT = 6;
2340-
pub const VT_VALUE_TYPE_INDEX: flatbuffers::VOffsetT = 8;
2341-
pub const VT_VALUE: flatbuffers::VOffsetT = 10;
2339+
pub const VT_VARIANT_NAME: flatbuffers::VOffsetT = 6;
2340+
pub const VT_FIELD_TYPES: flatbuffers::VOffsetT = 8;
2341+
pub const VT_VALUE_TYPE_INDEX: flatbuffers::VOffsetT = 10;
2342+
pub const VT_VALUE: flatbuffers::VOffsetT = 12;
23422343

23432344
#[inline]
23442345
pub unsafe fn init_from_table(table: flatbuffers::Table<'a>) -> Self {
@@ -2353,6 +2354,7 @@ impl<'a> CFFIValueUnionVariant<'a> {
23532354
if let Some(x) = args.value { builder.add_value(x); }
23542355
builder.add_value_type_index(args.value_type_index);
23552356
if let Some(x) = args.field_types { builder.add_field_types(x); }
2357+
if let Some(x) = args.variant_name { builder.add_variant_name(x); }
23562358
if let Some(x) = args.name { builder.add_name(x); }
23572359
builder.finish()
23582360
}
@@ -2366,6 +2368,13 @@ impl<'a> CFFIValueUnionVariant<'a> {
23662368
unsafe { self._tab.get::<flatbuffers::ForwardsUOffset<&str>>(CFFIValueUnionVariant::VT_NAME, None)}
23672369
}
23682370
#[inline]
2371+
pub fn variant_name(&self) -> Option<&'a str> {
2372+
// Safety:
2373+
// Created from valid Table for this object
2374+
// which contains a valid value in this slot
2375+
unsafe { self._tab.get::<flatbuffers::ForwardsUOffset<&str>>(CFFIValueUnionVariant::VT_VARIANT_NAME, None)}
2376+
}
2377+
#[inline]
23692378
pub fn field_types(&self) -> Option<flatbuffers::Vector<'a, flatbuffers::ForwardsUOffset<CFFIFieldTypeHolder<'a>>>> {
23702379
// Safety:
23712380
// Created from valid Table for this object
@@ -2396,6 +2405,7 @@ impl flatbuffers::Verifiable for CFFIValueUnionVariant<'_> {
23962405
use self::flatbuffers::Verifiable;
23972406
v.visit_table(pos)?
23982407
.visit_field::<flatbuffers::ForwardsUOffset<&str>>("name", Self::VT_NAME, false)?
2408+
.visit_field::<flatbuffers::ForwardsUOffset<&str>>("variant_name", Self::VT_VARIANT_NAME, false)?
23992409
.visit_field::<flatbuffers::ForwardsUOffset<flatbuffers::Vector<'_, flatbuffers::ForwardsUOffset<CFFIFieldTypeHolder>>>>("field_types", Self::VT_FIELD_TYPES, false)?
24002410
.visit_field::<i32>("value_type_index", Self::VT_VALUE_TYPE_INDEX, false)?
24012411
.visit_field::<flatbuffers::ForwardsUOffset<CFFIValueHolder>>("value", Self::VT_VALUE, false)?
@@ -2405,6 +2415,7 @@ impl flatbuffers::Verifiable for CFFIValueUnionVariant<'_> {
24052415
}
24062416
pub struct CFFIValueUnionVariantArgs<'a> {
24072417
pub name: Option<flatbuffers::WIPOffset<&'a str>>,
2418+
pub variant_name: Option<flatbuffers::WIPOffset<&'a str>>,
24082419
pub field_types: Option<flatbuffers::WIPOffset<flatbuffers::Vector<'a, flatbuffers::ForwardsUOffset<CFFIFieldTypeHolder<'a>>>>>,
24092420
pub value_type_index: i32,
24102421
pub value: Option<flatbuffers::WIPOffset<CFFIValueHolder<'a>>>,
@@ -2414,6 +2425,7 @@ impl<'a> Default for CFFIValueUnionVariantArgs<'a> {
24142425
fn default() -> Self {
24152426
CFFIValueUnionVariantArgs {
24162427
name: None,
2428+
variant_name: None,
24172429
field_types: None,
24182430
value_type_index: 0,
24192431
value: None,
@@ -2431,6 +2443,10 @@ impl<'a: 'b, 'b> CFFIValueUnionVariantBuilder<'a, 'b> {
24312443
self.fbb_.push_slot_always::<flatbuffers::WIPOffset<_>>(CFFIValueUnionVariant::VT_NAME, name);
24322444
}
24332445
#[inline]
2446+
pub fn add_variant_name(&mut self, variant_name: flatbuffers::WIPOffset<&'b str>) {
2447+
self.fbb_.push_slot_always::<flatbuffers::WIPOffset<_>>(CFFIValueUnionVariant::VT_VARIANT_NAME, variant_name);
2448+
}
2449+
#[inline]
24342450
pub fn add_field_types(&mut self, field_types: flatbuffers::WIPOffset<flatbuffers::Vector<'b , flatbuffers::ForwardsUOffset<CFFIFieldTypeHolder<'b >>>>) {
24352451
self.fbb_.push_slot_always::<flatbuffers::WIPOffset<_>>(CFFIValueUnionVariant::VT_FIELD_TYPES, field_types);
24362452
}
@@ -2461,6 +2477,7 @@ impl core::fmt::Debug for CFFIValueUnionVariant<'_> {
24612477
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
24622478
let mut ds = f.debug_struct("CFFIValueUnionVariant");
24632479
ds.field("name", &self.name());
2480+
ds.field("variant_name", &self.variant_name());
24642481
ds.field("field_types", &self.field_types());
24652482
ds.field("value_type_index", &self.value_type_index());
24662483
ds.field("value", &self.value());

engine/language_client_cffi/src/ctypes.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -489,14 +489,17 @@ where
489489
.iter()
490490
.position(|t| real_type == *t)
491491
.expect("Failed to find target_type in options");
492+
let variant_name = options[value_type_index].to_union_name();
492493
let options = builder.create_vector_from_iter(options_vec.into_iter());
493-
// TODO: get the name from the target_type
494-
let name = builder.create_string(&target_type.to_union_name());
494+
495+
let name_offset = builder.create_string(&target_type.to_union_name());
496+
let variant_name_offset = builder.create_string(&variant_name);
495497

496498
let value_union_variant = CFFIValueUnionVariant::create(
497499
&mut builder,
498500
&CFFIValueUnionVariantArgs {
499-
name: Some(name),
501+
name: Some(name_offset),
502+
variant_name: Some(variant_name_offset),
500503
field_types: Some(options),
501504
value_type_index: value_type_index as i32,
502505
value: Some(value_holder),

engine/language_client_cffi/types/cffi.fbs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ table CFFIValueTuple {
8484
// For the Rust variant `Union(Vec<CFFIFieldType>, Box<CFFIValue>)`
8585
table CFFIValueUnionVariant {
8686
name: string;
87+
variant_name: string;
8788
field_types: [CFFIFieldTypeHolder];
8889
value_type_index: int;
8990
value: CFFIValueHolder;

engine/language_client_codegen/src/go/generate_types.rs

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,15 +28,6 @@ pub(crate) fn cast_value(container_variable_name: &str, field_type: &GoType) ->
2828
.ok()
2929
.unwrap()
3030
);
31-
} else if field_type.is_slice {
32-
let inner_type = field_type.underlying_type.as_ref().unwrap();
33-
return format!(
34-
r#"castSlice({container_variable_name}, func(item any) {} {{
35-
return {}
36-
}})"#,
37-
inner_type.name,
38-
cast_value("item", inner_type),
39-
);
4031
} else if field_type.is_union {
4132
return format!("*({container_variable_name}).(*{})", field_type.name);
4233
} else if field_type.is_pointer {
@@ -249,6 +240,8 @@ pub struct GoType {
249240
struct GoTypeAlias<'ir> {
250241
name: Cow<'ir, str>,
251242
target: String,
243+
is_baml_serializable: bool,
244+
is_union: bool,
252245
}
253246

254247
#[derive(askama::Template)]
@@ -415,9 +408,12 @@ impl<'ir> From<ClassWalker<'ir>> for GoClass<'ir> {
415408
// TODO: Define AliasWalker to simplify type.
416409
impl<'ir> From<Walker<'ir, (&'ir String, &'ir FieldType)>> for GoTypeAlias<'ir> {
417410
fn from(walker: Walker<(&'ir String, &'ir FieldType)>) -> Self {
411+
let type_ref = walker.item.1.to_type_ref_2(walker.ir, false);
418412
GoTypeAlias {
419413
name: Cow::Borrowed(walker.item.0),
420-
target: walker.item.1.to_type_ref_2(walker.ir, false).name,
414+
target: type_ref.name,
415+
is_union: type_ref.is_union,
416+
is_baml_serializable: type_ref.is_class || type_ref.is_enum || type_ref.is_union,
421417
}
422418
}
423419
}

engine/language_client_codegen/src/go/templates/client.go.j2

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,6 @@ type stream struct {}
2929

3030
var Stream = &stream{}
3131

32-
func castSlice[T any](result any, castResult func(any) T) []T {
33-
items := result.([]any)
34-
casted := make([]T, len(items))
35-
for i, item := range items {
36-
casted[i] = castResult(item)
37-
}
38-
return casted
39-
}
40-
4132
func castOptional[T any](result any, castResult func(any) T) *T {
4233
if result == nil {
4334
return nil

engine/language_client_codegen/src/go/templates/types-unions.go.j2

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@ type {{ union.name }} struct {
1919

2020
func (u *{{ union.name }}) Decode(holder *cffi.CFFIValueUnionVariant) {
2121
valueHolder := holder.Value(nil)
22-
switch holder.ValueTypeIndex() {
22+
switch string(holder.VariantName()) {
2323
{% for variant in union.variants %}
24-
case {{ loop.index0 }}:
24+
case "{{ variant.0 }}":
2525
u.variant = "{{ variant.0 }}"
2626
value := {{ crate::go::generate_types::render_value_coercion("valueHolder", variant.1) }}
2727
u.variant_{{ variant.0 }} = &value
@@ -34,7 +34,7 @@ func (u {{ union.name }}) Encode(builder *flatbuffers.Builder) (cffi.CFFIValueUn
3434
switch u.variant {
3535
{% for variant in union.variants %}
3636
case "{{ variant.0 }}":
37-
return baml.EncodeUnion(builder, u.variant, u.variant_{{ variant.0 }})
37+
return baml.EncodeUnion(builder, "{{ union.name }}", u.variant, u.variant_{{ variant.0 }})
3838
{% endfor %}
3939
case "":
4040
return cffi.CFFIValueUnionNONE, 0, fmt.Errorf("invalid union variant: [unset]")

engine/language_client_codegen/src/go/templates/types.go.j2

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,4 +78,23 @@ func (c {{ class.name }}) BamlTypeName() string {
7878
{#- Type Aliases -#}
7979
{% for alias in structural_recursive_alias_cycles %}
8080
type {{alias.name}} {{alias.target}}
81+
82+
{% if alias.is_baml_serializable %}
83+
func (c {{alias.name}}) BamlTypeName() string {
84+
return {{alias.target}}(c).BamlTypeName()
85+
}
86+
87+
func (c {{alias.name}}) Encode(builder *flatbuffers.Builder) (cffi.CFFIValueUnion, flatbuffers.UOffsetT, error) {
88+
return {{alias.target}}(c).Encode(builder)
89+
}
90+
{% endif %}
91+
92+
{% if alias.is_union %}
93+
func (u *{{alias.name}}) Decode(holder *cffi.CFFIValueUnionVariant) {
94+
decodedUnion := {{alias.target}}{}
95+
decodedUnion.Decode(holder)
96+
*u = {{alias.name}}(decodedUnion)
97+
}
98+
{% endif %}
99+
81100
{% endfor %}

engine/language_client_go/pkg/cffi/CFFIValueUnionVariant.go

Lines changed: 20 additions & 9 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

engine/language_client_go/pkg/encode.go

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -63,21 +63,17 @@ func EncodeEnum(builder *flatbuffers.Builder, name string, value string, isDynam
6363
return cffi.CFFIValueUnionCFFIValueEnum, cffi.CFFIValueEnumEnd(builder), nil
6464
}
6565

66-
func EncodeUnion(builder *flatbuffers.Builder, variantName string, value any) (cffi.CFFIValueUnion, flatbuffers.UOffsetT, error) {
67-
nameOffset := builder.CreateString(variantName)
66+
func EncodeUnion(builder *flatbuffers.Builder, name string, variantName string, value any) (cffi.CFFIValueUnion, flatbuffers.UOffsetT, error) {
67+
nameOffset := builder.CreateString(name)
68+
variantNameOffset := builder.CreateString(variantName)
6869
valueHolderOffset, err := Encode(builder, value)
6970
if err != nil {
7071
return cffi.CFFIValueUnionNONE, 0, fmt.Errorf("encoding inner value for union variant '%s': %w", variantName, err)
7172
}
7273

73-
// Note: The CFFIValueUnionVariant schema has field_types and value_type_index.
74-
// The BamlUnionSerializer interface currently doesn't provide these.
75-
// For now, we are omitting them, which might be incomplete depending on
76-
// how the receiving end uses this structure. If they are needed, the
77-
// interface and this function must be updated.
78-
7974
cffi.CFFIValueUnionVariantStart(builder)
8075
cffi.CFFIValueUnionVariantAddName(builder, nameOffset)
76+
cffi.CFFIValueUnionVariantAddVariantName(builder, variantNameOffset)
8177
cffi.CFFIValueUnionVariantAddValue(builder, valueHolderOffset)
8278
// cffi.CFFIValueUnionVariantAddFieldTypes(builder, ...) // Omitted
8379
// cffi.CFFIValueUnionVariantAddValueTypeIndex(builder, ...) // Omitted
@@ -250,6 +246,7 @@ func encodeList(builder *flatbuffers.Builder, value reflect.Value) (flatbuffers.
250246

251247
// encodeMap now accepts and passes TypeMap
252248
func encodeMap(builder *flatbuffers.Builder, mapValue reflect.Value) (flatbuffers.UOffsetT, error) {
249+
253250
mapLength := mapValue.Len()
254251
entryOffsets := make([]flatbuffers.UOffsetT, 0, mapLength)
255252
// Iterate map and build entries (order doesn't strictly matter for map, but FB requires building bottom-up)
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
type RecursiveUnion = string | map<string, RecursiveUnion>
2+
3+
function RecursiveUnionTest(input: RecursiveUnion) -> RecursiveUnion {
4+
client GPT35
5+
prompt #"
6+
Return back the same value you were given: {{ input }}
7+
8+
{{ ctx.output_format }}
9+
"#
10+
}
11+
12+
test RecursiveUnionTest {
13+
functions [RecursiveUnionTest]
14+
args {
15+
input {
16+
"key" "value"
17+
"key2" {
18+
"key" "value2"
19+
"key2" "value3"
20+
}
21+
}
22+
}
23+
}

0 commit comments

Comments
 (0)