Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #641 #644

Merged
merged 15 commits into from
Aug 21, 2024
Merged

Fix #641 #644

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
96c8bec
Adding conversion parameter to gen_js_to_c
ambiguousname Aug 17, 2024
1fba11e
Formatting
ambiguousname Aug 17, 2024
88e1037
Merge branch 'main' into splat-fix
ambiguousname Aug 19, 2024
5b1b585
Make .splat a natural consequence of gen_js_to_c
ambiguousname Aug 19, 2024
57cebc5
Default ... ____ .splat(), add carve outs for freeing allocations
ambiguousname Aug 21, 2024
048771f
Generation
ambiguousname Aug 21, 2024
dca57fb
We can't use .alloc directly, so adding a garbage collector type
ambiguousname Aug 21, 2024
6700bf7
Generation
ambiguousname Aug 21, 2024
3c83c7b
Quick bugfix
ambiguousname Aug 21, 2024
b99d71b
Merge branch 'main' into splat-fix
ambiguousname Aug 21, 2024
1e43acc
Regeneration to bugfix
ambiguousname Aug 21, 2024
a515999
Merge branch 'main' into splat-fix
ambiguousname Aug 21, 2024
e979043
Fixing generation bug from main merge
ambiguousname Aug 21, 2024
3b46708
Formatting
ambiguousname Aug 21, 2024
8ad07b5
Whitespace fix
ambiguousname Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 51 additions & 40 deletions tool/src/js/type_generation/converter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,50 +532,61 @@ impl<'jsctx, 'tcx> TyGenContext<'jsctx, 'tcx> {
js_name: Cow<'tcx, str>,
struct_borrow_info: Option<&StructBorrowContext<'tcx>>,
alloc: Option<&str>,
) -> Cow<'tcx, str> {
match *ty {
Type::Primitive(..) => js_name.clone(),
Type::Opaque(ref op) if op.is_optional() => format!("{js_name}.ffiValue ?? 0").into(),
Type::Enum(..) | Type::Opaque(..) => format!("{js_name}.ffiValue").into(),
Type::Struct(..) => {
self.gen_js_to_c_for_struct_type(js_name, struct_borrow_info, alloc.unwrap())
}
Type::Slice(slice) => {
if let Some(hir::MaybeStatic::Static) = slice.lifetime() {
panic!("'static not supported for JS backend.")
} else {
let base_statement = match slice {
hir::Slice::Str(_, encoding) => match encoding {
hir::StringEncoding::UnvalidatedUtf8 | hir::StringEncoding::Utf8 => {
format!("diplomatRuntime.DiplomatBuf.str8(wasm, {js_name})")
}
_ => {
format!("diplomatRuntime.DiplomatBuf.str16(wasm, {js_name})")
}
},
hir::Slice::Strs(encoding) => format!(
r#"diplomatRuntime.DiplomatBuf.strs(wasm, {js_name}, "{}")"#,
match encoding {
hir::StringEncoding::UnvalidatedUtf16 => "string16",
_ => "string8",
}
),
hir::Slice::Primitive(_, p) => format!(
r#"diplomatRuntime.DiplomatBuf.slice(wasm, {js_name}, "{}")"#,
self.formatter.fmt_primitive_list_view(p)
),
_ => unreachable!("Unknown Slice variant {ty:?}"),
}
.into();
if let Some(a) = alloc {
format!("{a}.alloc({base_statement})").into()
conversion: Option<(&str, &str)>,
ambiguousname marked this conversation as resolved.
Show resolved Hide resolved
) -> String {
let (start, end) = if let Some((s, e)) = conversion {
(s, e)
} else {
("", "")
};
format!(
"{start}{}{end}",
match *ty {
Type::Primitive(..) => js_name.clone(),
Type::Opaque(ref op) if op.is_optional() =>
format!("{js_name}.ffiValue ?? 0").into(),
Type::Enum(..) | Type::Opaque(..) => format!("{js_name}.ffiValue").into(),
Type::Struct(..) => {
self.gen_js_to_c_for_struct_type(js_name, struct_borrow_info, alloc.unwrap())
}
Type::Slice(slice) => {
if let Some(hir::MaybeStatic::Static) = slice.lifetime() {
panic!("'static not supported for JS backend.")
} else {
base_statement
let base_statement = match slice {
hir::Slice::Str(_, encoding) => match encoding {
hir::StringEncoding::UnvalidatedUtf8
| hir::StringEncoding::Utf8 => {
format!("diplomatRuntime.DiplomatBuf.str8(wasm, {js_name})")
}
_ => {
format!("diplomatRuntime.DiplomatBuf.str16(wasm, {js_name})")
}
},
hir::Slice::Strs(encoding) => format!(
r#"diplomatRuntime.DiplomatBuf.strs(wasm, {js_name}, "{}")"#,
match encoding {
hir::StringEncoding::UnvalidatedUtf16 => "string16",
_ => "string8",
}
),
hir::Slice::Primitive(_, p) => format!(
r#"diplomatRuntime.DiplomatBuf.slice(wasm, {js_name}, "{}")"#,
self.formatter.fmt_primitive_list_view(p)
),
_ => unreachable!("Unknown Slice variant {ty:?}"),
}
.into();
if let Some(a) = alloc {
format!("{a}.alloc({base_statement})").into()
} else {
base_statement
}
}
}
_ => unreachable!("Unknown AST/HIR variant {ty:?}"),
}
_ => unreachable!("Unknown AST/HIR variant {ty:?}"),
}
)
}

pub(super) fn gen_js_to_c_for_struct_type(
Expand Down
32 changes: 15 additions & 17 deletions tool/src/js/type_generation/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,17 +195,12 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
None
};

let js_to_c = format!("{}{}{}",
match &field.ty {
Type::Slice(..) => "...",
_ => "",
},
self.gen_js_to_c_for_type(&field.ty, format!("this.#{}", field_name.clone()).into(), maybe_struct_borrow_info.as_ref(), alloc.as_deref()),
match &field.ty {
Type::Slice(..) => ".splat()",
_ => ""
}
);
let conversion = match field.ty {
hir::Type::Slice(..) => Some(("...", ".splat()")),
_ => None,
};

let js_to_c = self.gen_js_to_c_for_type(&field.ty, format!("this.#{}", field_name.clone()).into(), maybe_struct_borrow_info.as_ref(), alloc.as_deref(), conversion);

FieldInfo {
field_name,
Expand Down Expand Up @@ -316,8 +311,9 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {

// If we're a slice of strings or primitives. See [`hir::Types::Slice`].
if let hir::Type::Slice(slice) = param.ty {
let slice_expr =
self.gen_js_to_c_for_type(&param.ty, param_info.name.clone(), None, None);
let slice_expr = self
.gen_js_to_c_for_type(&param.ty, param_info.name.clone(), None, None, None)
.into();

let is_borrowed = match param_borrow_kind {
ParamBorrowInfo::TemporarySlice => false,
Expand Down Expand Up @@ -371,14 +367,16 @@ impl<'ctx, 'tcx> TyGenContext<'ctx, 'tcx> {
} else {
None
};
method_info
.param_conversions
.push(self.gen_js_to_c_for_type(
method_info.param_conversions.push(
self.gen_js_to_c_for_type(
&param.ty,
param_info.name.clone(),
struct_borrow_info.as_ref(),
alloc,
));
None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I still don't think this is the best way to structure this

I don't think this parameter should be necessary. We're manually doing slice.ptr, slice.size above, so what we have is that two codepaths calling gen_js_to_c_for_type, one which uses ...{}.splat() to splat it, and one which manually spreads it, and we have this conversion parameter to deal with that divergence.

gen_js_to_c_for_type already unconditionally returns a ...struct.toFFI() for structs, so all of its call sites already expect spread operators. I think it should unconditionally return ...slice.splat() for slices (with caveats on the arena behavior). The call sites should be adjusted to handle that for slices; that will simplify them since currently this call site special-cases slices and manually spreads it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(all of this is an unfortunate consequence of how slices need to be special snowflakes for borrowing, which makes it easy to accidentally write code tha treats them as more special than they need to be)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's probably for the best that we try to aim for near-universal usage of ...splat() in gen_js_to_c, it's just an absolute pain for freeing. I think I'll be able to make a clean solution though.

)
.into(),
);
}

method_info.parameters.push(param_info);
Expand Down
Loading