Conversation
Implement map type rendering plus lowering/lifting/deallocation support across the C, C++, C#, Go, MoonBit, and Markdown backends, and add map codegen/runtime tests. This aligns non-Rust generators with core map ABI support and fixes the Go test harness module replacement path needed for map codegen verification. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
| } | ||
| } | ||
|
|
||
| Instruction::MapLower { |
There was a problem hiding this comment.
Could support for other languages be omitted unless tests are also added? I'm not equipped to handle other languages and forgive me if I'm wrong, but if the other languages' support is LLM generated I would perfer someone review the code who knows about the language generation.
| &bindings_dir.join("go.mod"), | ||
| format!( | ||
| "module wit_component\n\ngo 1.25\n\nreplace go.bytecodealliance.org => {}", | ||
| "module wit_component\n\ngo 1.25\n\nreplace go.bytecodealliance.org/pkg => {}", |
There was a problem hiding this comment.
Is this perhaps a stray change? (otherwise I'm not sure where this comes from)
| #[derive(PartialEq, Eq, Clone, Copy, Hash, Debug)] | ||
| enum RuntimeItem { | ||
| AllocCrate, | ||
| MapType, |
There was a problem hiding this comment.
This is a bit of a dated enum which shouldn't be neede any more, cousl Map refer to {rt}::Map directly in various locations instead of requiring a pub use?
| if mode.lists_borrowed { | ||
| let lifetime = mode.lifetime.unwrap(); | ||
| self.push_str("&"); | ||
| if lifetime != "'_" { | ||
| self.push_str(lifetime); | ||
| self.push_str(" "); | ||
| } | ||
| self.push_str("[("); | ||
| self.print_ty(key, key_mode); | ||
| self.push_str(", "); | ||
| self.print_ty(value, value_mode); | ||
| self.push_str(")]"); |
There was a problem hiding this comment.
This looks like it might be copy/pasted from the lists side of things, but the slice syntax I don't think is intended here? Or is this intended? Should maps not be passed as &MapThing<K, V>?
| TypeDefKind::Map(key, value) => { | ||
| if mode.lifetime.is_some() { | ||
| self.print_map(key, value, mode); | ||
| return; | ||
| } | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
If adding this here, can you update the comment above?
| fn type_map(&mut self, id: TypeId, name: &str, key: &Type, value: &Type, docs: &Docs) { | ||
| let _ = (id, name, key, value, docs); | ||
| todo!("map types are not yet supported in this backend"); | ||
| } | ||
| fn type_builtin(&mut self, id: TypeId, name: &str, ty: &Type, docs: &Docs); |
There was a problem hiding this comment.
Can this be a required method like all other methods in this trait? (e.g. no default impl)
| TypeDefKind::Type(t) => self.read_from_memory(t, addr, offset), | ||
|
|
||
| TypeDefKind::List(_) => self.read_list_from_memory(ty, addr, offset), | ||
| TypeDefKind::Map(_, _) => self.read_list_from_memory(ty, addr, offset), |
There was a problem hiding this comment.
Can this have a comment for why read_list_from_memory works?
| Type::Id(id) => match &self.resolve.types[id].kind { | ||
| TypeDefKind::Type(t) => self.write_to_memory(t, addr, offset), | ||
| TypeDefKind::List(_) => self.write_list_to_memory(ty, addr, offset), | ||
| TypeDefKind::Map(_, _) => self.write_list_to_memory(ty, addr, offset), |
There was a problem hiding this comment.
Similar to below, can this have a comment for why write_list_to_memory works?
There was a problem hiding this comment.
Similar to other languages, I don't think this should be added without tests. Unlike other languages, though, I can review this, so mind adding some tests using the C bits?
| #[cfg(feature = "std")] | ||
| pub type Map<K, V> = std::collections::HashMap<K, V>; | ||
| #[cfg(not(feature = "std"))] | ||
| pub type Map<K, V> = alloc::collections::BTreeMap<K, V>; |
There was a problem hiding this comment.
This I'm hesitant to do myself, specifically switching based on std. That's not API-compatible unfortunately and puts this crate in a weird situation.
One possibility perhaps is to force some sort of configuration in bindgen! if map<K, V> is encountered which specifies which map type to use. Another is to always fall back to the more general BTreeMap in terms of availability. Another is to support both a fallback and a configuration. Another yet might be to avoid using HashMap and BTreeMap entirely and instead use Vec<(T, U)> by default. Overall I'm not sure what the best option is...
2 participants
Summary
map<K, V>support inwit-bindgencore ABI and backend codegen paths (C, C++, C#, Go, MoonBit), plus Markdown type renderingtests/codegen/map.witandtests/runtime/map/*crates/test/src/go.rsso generated map bindings resolvego.bytecodealliance.org/pkg/wit/*correctlyTest plan
cargo fmtcargo clippy --workspace --all-targets -- -D warningscargo checkcargo check -p wit-bindgen-corecargo check -p wit-bindgen-ccargo check -p wit-bindgen-cppcargo check -p wit-bindgen-csharpcargo check -p wit-bindgen-gocargo check -p wit-bindgen-moonbitcargo check -p wit-bindgen-markdowncargo run test --artifacts target/artifacts --runner cargo --languages go --filter map.wit tests/codegen