Make WIT merging commutative#2451
Make WIT merging commutative#2451fibonacci1729 wants to merge 2 commits intobytecodealliance:mainfrom
Conversation
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
|
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?
|
@alexcrichton in the last commit here, i added a test to demonstrate why the toposort is needed post merge and updated the fuzzer to run the real-world workflow. |
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
6740381 to
8cf874e
Compare
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.