Skip to content

Make WIT merging commutative#2451

Open
fibonacci1729 wants to merge 1 commit intobytecodealliance:mainfrom
fibonacci1729:wit-commutativity
Open

Make WIT merging commutative#2451
fibonacci1729 wants to merge 1 commit intobytecodealliance:mainfrom
fibonacci1729:wit-commutativity

Conversation

@fibonacci1729
Copy link
Contributor

@fibonacci1729 fibonacci1729 commented Feb 25, 2026

Closes #1897

This PR makes the following changes made to mod.rs to make WIT merging commutative:

  • MergeMap::build_interface — Previously, every type and function in from's interface was required to exist in into'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 only from has extras, they're skipped in the mapping phase and handled later during the merge.

  • Resolve::merge interface processing — When a mapped interface has extra types/functions from from, those extras are now added to the into interface (types added to the types map, functions added to the functions map with proper span adjustment and remapping).

  • Adds topologically_sort_interfaces method on Resolve that 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.

Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
}

#[test]
fn merging_is_commutative() -> Result<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Would this test be better put into wit-component/tests/merge?

Copy link
Member

Choose a reason for hiding this comment

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

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)

@fibonacci1729 fibonacci1729 marked this pull request as ready for review February 26, 2026 21:05
@fibonacci1729 fibonacci1729 requested a review from a team as a code owner February 26, 2026 21:05
@fibonacci1729 fibonacci1729 requested review from fitzgen and removed request for a team February 26, 2026 21:05
@alexcrichton
Copy link
Member

Wanted to say I haven't forgotten this, and thanks! I'll do my best to set aside time tomorrow to review this

@fibonacci1729
Copy link
Contributor Author

Oh no rush at all!

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

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?

Comment on lines +4151 to +4166
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],);
}
Copy link
Member

Choose a reason for hiding this comment

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

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 new

where the rough idea here is:

  • Create two components, each with half of a wit interface (one missing f1, one missing f2).
  • Take those components and generate their WIT, renaming one of them from root to root2 to 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 new step 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's an excellent idea. I'll start looking into that! Thanks!

}

#[test]
fn merging_is_commutative() -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

merging wit is not commutative

2 participants