Make WIT merging commutative#2451
Make WIT merging commutative#2451fibonacci1729 wants to merge 1 commit intobytecodealliance:mainfrom
Conversation
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
| } | ||
|
|
||
| #[test] | ||
| fn merging_is_commutative() -> Result<()> { |
There was a problem hiding this comment.
TODO: Would this test be better put into wit-component/tests/merge?
There was a problem hiding this comment.
Yeah test-wise I think it'd be good to formalize how meging is tested with a suite like that. I realize though that the merge suite isn't great. Ideally what I'd love to see is what happens in the real-world today surfacing this issue which is something like:
- Start with a single WIT.
- Produce two components from this WIT, where each component imports a subset of the possible imports.
- Componentize the two components.
- Re-infer the WIT from each component, and attempt to merge that.
This would, if this existed, model how everyone's starting from the same source of truth. The handwritten merge tests are an approximation here where it's manually duplicating an interface across two locations and then merging those together.
Overall I suspect the merge suite is probably good enough for now, but yeah if you wouldn't mind adding this in there that'd be appreciated! The test suite might also be able to be bulked up by asserting that merging in both directions works (I'm not sure if it does this today)
|
Wanted to say I haven't forgotten this, and thanks! I'll do my best to set aside time tomorrow to review this |
|
Oh no rush at all! |
alexcrichton
left a comment
There was a problem hiding this comment.
I'm a bit wary of the top-o sort of the arena itself, but before reviewing that too too closely I would in theory prefer to have fuzzing/testing/something to show it's a problem before we'd identify it's a problem. To that end a question for you: how much time would you be willing to put into testing/fuzzing this?
I ask because much of the design of wit-component and wit-parser are heavily influenced by the roundtrip_wit.rs fuzzer in this repository. I discovered what felt like an endless long tail of issues only through fuzzing which highlighted all sorts of stuff I forgot when originally writing WIT support. I'd ideally like to lean on that for this as well as much as we can mainly because I don't necessarily trust myself to catch all the corner cases here. I highlighted one case below of a merging which I believe should work but isn't currently caught by the fuzzer (I've been running it locally for awhile now).
I describe below a test script as well as a sort of ideal testing strategy where we start from a single WIT, diverge, and assert the merge then later works. The roundtrip_wit.rs fuzzer is already almost doing this where it's taking worlds in a WIT package, creating a dummy component, deleting arbitrary imports, and then creating components. I've applied this diff locally as an attempt to be able to test like this, although it's turning up things that I think are preexisting issues as well.
Would you be up for helping to work on fuzzing side of this to help ensure this covers all sorts of various edge cases and such?
| let from_has_extra_funcs = from_interface | ||
| .functions | ||
| .keys() | ||
| .any(|name| !into_interface.functions.contains_key(name)); | ||
| let into_has_extra_funcs = into_interface | ||
| .functions | ||
| .keys() | ||
| .any(|name| !from_interface.functions.contains_key(name)); | ||
| if from_has_extra_funcs && into_has_extra_funcs { | ||
| let from_extra: Vec<_> = from_interface | ||
| .functions | ||
| .keys() | ||
| .filter(|n| !into_interface.functions.contains_key(n.as_str())) | ||
| .collect(); | ||
| bail!("expected function `{}` to be present", from_extra[0],); | ||
| } |
There was a problem hiding this comment.
This surprised me here insofar as I would expect that the only requirement is that the intersection of two interfaces perfectly matches but otherwise one shouldn't necessarily need to be a subset of another. I'm trying to make test case that showcases this via the CLI and it's unfortunately a bit difficult with merge not really being a first-class operation, nor shrinking a WIT to some subset necessarily. What I was able to do was this:
Using this WIT:
package a:b;
interface a {
f1: func();
f2: func();
}
world w {
import a;
}I've got this script
set -ex
wt=./target/debug/wasm-tools
$wt component embed --dummy src.wit -t | \
$wt print --name-unnamed | \
rg -v 'import.*f1' | \
$wt component new | \
$wt component wit -o a.wit
$wt component embed --dummy src.wit -t | \
$wt print --name-unnamed | \
rg -v 'import.*f2' | \
$wt component new | \
$wt component wit | \
sed 's/root/root2/g' > b.wit
$wt component embed --dummy a.wit | \
$wt component embed b.wit | \
$wt component new
$wt component embed --dummy b.wit | \
$wt component embed a.wit | \
$wt component newwhere the rough idea here is:
- Create two components, each with half of a wit interface (one missing
f1, one missingf2). - Take those components and generate their WIT, renaming one of them from
roottoroot2to avoid clashes. - Merge those two WITs by embedding the type information in one wasm binary and then turning that into a component. Here the
component newstep will implicitly merge all WITs found within a component.
This doesn't exactly match higher level workflows but it's somewhat close enough in theory.
With this PR, this test currently fails with this WIT, and I believe it's due to this subset check here. That being said, my hunch is that if this were to just be removed it would work out?
There was a problem hiding this comment.
Yeah that's an excellent idea. I'll start looking into that! Thanks!
| } | ||
|
|
||
| #[test] | ||
| fn merging_is_commutative() -> Result<()> { |
There was a problem hiding this comment.
Yeah test-wise I think it'd be good to formalize how meging is tested with a suite like that. I realize though that the merge suite isn't great. Ideally what I'd love to see is what happens in the real-world today surfacing this issue which is something like:
- Start with a single WIT.
- Produce two components from this WIT, where each component imports a subset of the possible imports.
- Componentize the two components.
- Re-infer the WIT from each component, and attempt to merge that.
This would, if this existed, model how everyone's starting from the same source of truth. The handwritten merge tests are an approximation here where it's manually duplicating an interface across two locations and then merging those together.
Overall I suspect the merge suite is probably good enough for now, but yeah if you wouldn't mind adding this in there that'd be appreciated! The test suite might also be able to be bulked up by asserting that merging in both directions works (I'm not sure if it does this today)
Closes #1897
This PR makes the following changes made to
mod.rsto make WIT merging commutative:MergeMap::build_interface— Previously, every type and function infrom's interface was required to exist ininto's interface (strict equality). Now, one interface can be a superset of the other. If both sides have exclusive items, it fails (genuinely incompatible). If onlyfromhas extras, they're skipped in the mapping phase and handled later during the merge.Resolve::mergeinterface processing — When a mapped interface has extra types/functions fromfrom, those extras are now added to theintointerface (types added to thetypesmap, functions added to thefunctionsmap with proper span adjustment and remapping).Adds
topologically_sort_interfacesmethod onResolvethat rebuilds the interfaces arena in topological order after a merge (newly-added interfaces may have been added after existing interfaces that now depend on them).NOTE: I made use of Copilot (Claude Opus 4.6) in implementing the topological sort bits.