Fixing rust map lowering interfaces by scoping each use. - #1722
mpawliszyn wants to merge 3 commits into
Conversation
Rust bindings fail to compile for functions that are directly imported or exported from a world. Existing map tests use interfaces. `Interface::finish` only imports `WitMap` when the path to root is non-empty so worlds never get it.
This currently fails in rust since it ends up clashing with the generated WitMap.
Importing the trait anonymously inside the lowering block rather than trying to combine imports and correctly importing once per interface. This missed both maps existing in the world and caused a potential name collision. I claim this should not change the final compiled code nor change the compile time much. I also clean up the logic trying to import once per interface.
| )); | ||
| self.push_str(&format!("let {len} = {map}.wit_map_len();\n")); | ||
| self.push_str(&format!( | ||
| "let {len} = {{ use {wit_map} as _; {map}.wit_map_len() }};\n" |
There was a problem hiding this comment.
This might actually be best as {wit_map}::wit_map_len(&{map}) perhaps? That way it's unambiguous what's being used here
There was a problem hiding this comment.
I tried to do that that but some of the values are owned and others are borrowed so I need something that will dereference appropriately.
If I try to do what you suggest some of the map tests fail saying that I don't need the & but when I remove it, others fail.
I am a newbie at rust so I could be missing something here.
There was a problem hiding this comment.
Hm could you make the change here so I could see the failures in CI? It might be something unrelated, but if I can't figure it out then what you have here works too
There was a problem hiding this comment.
Sure I pushed the &{map} version. It will fail and the rust error will recommend removing the & but removing the & other failures will show up that ask you to place the & back.
Let me know if you want to also see the & less version too.
There was a problem hiding this comment.
Actually since it is failing already I uploaded without the & so you can see what I mean. You can see below that both fail.
If there is a better way I am all ears though.
There was a problem hiding this comment.
Aha ok this makes sense, thanks! I agree that the original "just have a use" is the best way to go here. Basically this requires the .-resolution the Rust compiler does for traits since the trait isn't actually implemented for &T
There was a problem hiding this comment.
Note that I am back to the original use situation now.
25805d5 to
172582b
Compare
This branch has not been deployed
2 participants
Importing the trait anonymously inside the lowering block rather than
trying to combine imports and correctly importing once per interface.
This missed both maps existing in the world and caused a potential name
collision.
I claim this should not change the final compiled code nor change the
compile time much. I also clean up the logic trying to import once per
interface.
I also added tests for the failing cases.