Support duplicate imports by default in wasm-tools component new
#2187
Conversation
This commit is a refinement to the support added in bytecodealliance#2145 to support duplicate imports by default in wasm-tools, namely by avoiding changing any binary offsets in a module except for the import section. This implements a strategy of renaming imports in the module that goes into a component to be unique. Classification of what an import maps to is then ensured to use the original names, not the current names, in the module.
|
cc @erikrose I had a long plane ride and figured I could give this a crack and I think it worked out pretty well but would be good to get your eyes on it too |
| let mut new_name = import.name.to_string(); | ||
| for i in 2.. { | ||
| new_name.truncate(import.name.len()); | ||
| write!(new_name, " [v{i}]").unwrap(); |
There was a problem hiding this comment.
Should we do something more graceful about the (admittedly extreme) corner case of exceeding the spec'd 256-char import name? It seems like everything returns Results all the way up, so it should be just as easy to toss a descriptive error message.
There was a problem hiding this comment.
Oh? Where did you see a 256-length limit? Wasmparser should have a limit of 100,000 as opposed to 256 to match web implementations. (or so we thought matched...)
There was a problem hiding this comment.
Oops, excuse me: the max is 2^32. So, even more extreme a corner case.
|
|
||
| ;; define `f2` before the second `f` import shows up | ||
| (import "cm32p2" "f" (func $f_v1)) | ||
| (import "cm32p2" "f2" (func $f2 (result i32))) |
There was a problem hiding this comment.
Perhaps we should test somewhere whether we gracefully compensate for collisions of new names with existing imports. For example, what if we import "f", then "f [v2]", then another "f"?
There was a problem hiding this comment.
Heh this test was actually originally for that where I appended "N" to the end of the import instead of [v2]. The previous syntax, as seen here, can clash with actual names that show up in the component model. I then had the idea of using a syntax which is impossible to show up in the component model (e.g. a space and brackets) so there's actually no worry of collisions any more. I figured I'd leave the test though as it might as well serve for future-proofing purposes.
There was a problem hiding this comment.
I dig it.:-D Hadn't seen the more limited import/export names for components before.
crates/wit-component/tests/components/deduplicate-imports/module.wat
Outdated
Show resolved
Hide resolved
crates/wit-component/tests/components/deduplicate-imports/module.wat
Outdated
Show resolved
Hide resolved
|
|
||
| ;; Call both copies of the import. They should both end up pointed to the same one. | ||
| (func | ||
| call $f_v1 |
There was a problem hiding this comment.
Is there something specific we're testing by doing both a series of fs and a series of gs? I could just be too dopey to figure it out. :-)
There was a problem hiding this comment.
Nah just wanted to make sure they were all referenced. In the original implementation of "just add N" to fully test the collision handling I needed to have different imports listed in different orders, but it's not really necessary any more but I figured I'd leave things.
|
LGTM! |
|
Boy, I'd love to get this in. Is Right Now good for you? I juuuust submitted an internal PR to pin to that wit-component release, because we need its functionality for a goal we're chasing. If you merge this now and cut a release, I'll pin to that instead, and we can forget the CLI flag ever existed! |
|
Yeah I'm happy to merge whenever but repo settings require 1+ approval from someone with write access, and 3/4 maintainers were on vacation last week so it may take a moment to work through the backlog to get to this. No urgency from my part though, I'm happy to just leave the argument there but hide it from |
|
@erikrose ok I've landed/published this as-is with dropping the CLI argument. If that causes issues though let me know and I can add it back in. |
|
I think it’s dandy. If you have a moment to cut a release (of wit-component at least, though I think it’s all tied together), I’ll skip straight to this approach and avoid any future concern about CLI compat.
|
|
Oh already done! |
|
Brilliant! :-D
|
This commit is a refinement to the support added in #2145 to support duplicate imports by default in wasm-tools, namely by avoiding changing any binary offsets in a module except for the import section. This implements a strategy of renaming imports in the module that goes into a component to be unique. Classification of what an import maps to is then ensured to use the original names, not the current names, in the module.