wit-component: Implement merging Resolves together#974
Merged
alexcrichton merged 2 commits intobytecodealliance:mainfrom Apr 11, 2023
Merged
wit-component: Implement merging Resolves together#974alexcrichton merged 2 commits intobytecodealliance:mainfrom
Resolves together#974alexcrichton merged 2 commits intobytecodealliance:mainfrom
Conversation
This commit addresses the fundamental issue exposed by bytecodealliance#967, namely that worlds were not managed well between adapters and the "main module". On the surface the issue is that wit-component maintains worlds separately for each adapter and for the main module, and then ends up tripping over itself when the worlds overlap in imports, for example. The first fix applied to handle this was to refactor wit-component to instead merge the worlds of the adapters into the main module's world. This merging operation is similar to the union-of-worlds proposed in WebAssembly/component-model#169. Prior to this commit, however, the merge operation between worlds was pretty naive and would bail out if there was any overlap at all. This meant that the panic from bytecodealliance#967 was turned into a first-class error message, but the use case shouldn't return an error and instead should succeed as the underlying packages and definitions were all shared. This comes back to bytecodealliance#962 which is an initial attempt at merging worlds which are structurally equivalent. The full implementation of this, however, was deeper than what bytecodealliance#962 uncovered and further required updating merging. One of the issues with bytecodealliance#962 (and initial implementations of this commit) is that a `Resolve` would still have multiple `Interface`s and `TypeDef`s and such floating around which are all the same where some were referred to by worlds and some weren't. What this commit chose to address this was to update the `Resolve::merge` operation. Previously this was an infallible simple "move the things from A to B" operation but now it's a much more involved "weaving things together" operation where overlap between A and B doesn't copy between them but instead uses items in-place. For example if a document in A isn't present in B, it's still added like before. If the document A is already present in B, though, then it's no longer added and instead it's recursed within to see if extra items are added to the new document. This new "fancy" merge operation is intended to be a more long-term implementation of this. At a high level this enables projects to separately depend on WASI, for example, without every project coordinating and using the exact same WIT-level dependency within one build invocation (which isn't possible with separate compilation). All WASI worlds will, in the end, get merged together into one picture of the WASI world when a component is produced. This was a fair bit of support which wasn't easily supported or tested before. I've modified the `components` test suite to be able to have shared WIT dependencies between the adapter and the main module to add tests for that. Additionally I've added a dedicated set of tests for just merging resolves which showcases some of the features at this time. With all that said, there are a few aspects and limitations of merging resolves that are worth mentioning: * Packages are merged as keyed by their name and URL. For now this basically means that only packages under `deps/` are candidates for merging (as only they have URLs) and they must be placed in same-named subfolders. * When merging packages are allowed to pick up more documents. This is intended to model where each resolve may have a subset of the whole picture file-wise. The use case in bytecodealliance#967 specifically exercises this, for example. * When merging documents it's allowed for new interfaces and worlds not previously present in the document to get merged in. This is intended to model a bit of forward-compatible-ness where for example the preview1 adapter may have been built against a historical version of WASI whereas a project using that may use a bleeding edge version that has more interfaces. This merge should still be ok since they're both only using what they declared, so by adding new interfaces and worlds it shouldn't ever tamper with the others' view of the world. * Merging interfaces and worlds, however, currently require exact "structural equivalence". The thinking behind this is that if one component exports an `interface` but then a merge operation suddenly adds items to that `interface` then the component would no longer validly export the interface. This should be possible for the converse, though, where adding items to an interface that's only ever imported should be valid. This doesn't implement this sort of detection logic and takes a more conservative stance that interfaces and worlds must exactly match. * Currently merging does not actually test for structural equivalence beyond top-level names. That's deferred to later if necessary, for example by updating bytecodealliance#962 to change where the tests happen. The result of all this is to fix the panic found in bytecodealliance#967 and more generally enable adapters and main modules in `wit-component` to import/use the same WIT interfaces. This means it's now possible to have a project use new preview2 interfaces with a preview1 standard library and the preview1 adapter and everything should work. Closes bytecodealliance#962 Closes bytecodealliance#967
pchickey
approved these changes
Apr 10, 2023
peterhuene
approved these changes
Apr 10, 2023
crates/wit-component/src/encoding.rs
Outdated
Comment on lines
1430
to
1432
| // Merge the adapter's document into our own document to have one large | ||
| // document, but the adapter's world isn't merged in to our world so | ||
| // retain it separately. |
Member
There was a problem hiding this comment.
Is this comment still correct given these changes?
Mossaka
pushed a commit
to Mossaka/wasm-tools-fork
that referenced
this pull request
May 29, 2023
…e#974) * wit-component: Implement merging `Resolve`s together This commit addresses the fundamental issue exposed by bytecodealliance#967, namely that worlds were not managed well between adapters and the "main module". On the surface the issue is that wit-component maintains worlds separately for each adapter and for the main module, and then ends up tripping over itself when the worlds overlap in imports, for example. The first fix applied to handle this was to refactor wit-component to instead merge the worlds of the adapters into the main module's world. This merging operation is similar to the union-of-worlds proposed in WebAssembly/component-model#169. Prior to this commit, however, the merge operation between worlds was pretty naive and would bail out if there was any overlap at all. This meant that the panic from bytecodealliance#967 was turned into a first-class error message, but the use case shouldn't return an error and instead should succeed as the underlying packages and definitions were all shared. This comes back to bytecodealliance#962 which is an initial attempt at merging worlds which are structurally equivalent. The full implementation of this, however, was deeper than what bytecodealliance#962 uncovered and further required updating merging. One of the issues with bytecodealliance#962 (and initial implementations of this commit) is that a `Resolve` would still have multiple `Interface`s and `TypeDef`s and such floating around which are all the same where some were referred to by worlds and some weren't. What this commit chose to address this was to update the `Resolve::merge` operation. Previously this was an infallible simple "move the things from A to B" operation but now it's a much more involved "weaving things together" operation where overlap between A and B doesn't copy between them but instead uses items in-place. For example if a document in A isn't present in B, it's still added like before. If the document A is already present in B, though, then it's no longer added and instead it's recursed within to see if extra items are added to the new document. This new "fancy" merge operation is intended to be a more long-term implementation of this. At a high level this enables projects to separately depend on WASI, for example, without every project coordinating and using the exact same WIT-level dependency within one build invocation (which isn't possible with separate compilation). All WASI worlds will, in the end, get merged together into one picture of the WASI world when a component is produced. This was a fair bit of support which wasn't easily supported or tested before. I've modified the `components` test suite to be able to have shared WIT dependencies between the adapter and the main module to add tests for that. Additionally I've added a dedicated set of tests for just merging resolves which showcases some of the features at this time. With all that said, there are a few aspects and limitations of merging resolves that are worth mentioning: * Packages are merged as keyed by their name and URL. For now this basically means that only packages under `deps/` are candidates for merging (as only they have URLs) and they must be placed in same-named subfolders. * When merging packages are allowed to pick up more documents. This is intended to model where each resolve may have a subset of the whole picture file-wise. The use case in bytecodealliance#967 specifically exercises this, for example. * When merging documents it's allowed for new interfaces and worlds not previously present in the document to get merged in. This is intended to model a bit of forward-compatible-ness where for example the preview1 adapter may have been built against a historical version of WASI whereas a project using that may use a bleeding edge version that has more interfaces. This merge should still be ok since they're both only using what they declared, so by adding new interfaces and worlds it shouldn't ever tamper with the others' view of the world. * Merging interfaces and worlds, however, currently require exact "structural equivalence". The thinking behind this is that if one component exports an `interface` but then a merge operation suddenly adds items to that `interface` then the component would no longer validly export the interface. This should be possible for the converse, though, where adding items to an interface that's only ever imported should be valid. This doesn't implement this sort of detection logic and takes a more conservative stance that interfaces and worlds must exactly match. * Currently merging does not actually test for structural equivalence beyond top-level names. That's deferred to later if necessary, for example by updating bytecodealliance#962 to change where the tests happen. The result of all this is to fix the panic found in bytecodealliance#967 and more generally enable adapters and main modules in `wit-component` to import/use the same WIT interfaces. This means it's now possible to have a project use new preview2 interfaces with a preview1 standard library and the preview1 adapter and everything should work. Closes bytecodealliance#962 Closes bytecodealliance#967 * Fix an outdated comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit addresses the fundamental issue exposed by #967, namely that worlds were not managed well between adapters and the "main module". On the surface the issue is that wit-component maintains worlds separately for each adapter and for the main module, and then ends up tripping over itself when the worlds overlap in imports, for example. The first fix applied to handle this was to refactor wit-component to instead merge the worlds of the adapters into the main module's world. This merging operation is similar to the union-of-worlds proposed in WebAssembly/component-model#169.
Prior to this commit, however, the merge operation between worlds was pretty naive and would bail out if there was any overlap at all. This meant that the panic from #967 was turned into a first-class error message, but the use case shouldn't return an error and instead should succeed as the underlying packages and definitions were all shared. This comes back to #962 which is an initial attempt at merging worlds which are structurally equivalent. The full implementation of this, however, was deeper than what #962 uncovered and further required updating merging.
One of the issues with #962 (and initial implementations of this commit) is that a
Resolvewould still have multipleInterfaces andTypeDefs and such floating around which are all the same where some were referred to by worlds and some weren't. What this commit chose to address this was to update theResolve::mergeoperation. Previously this was an infallible simple "move the things from A to B" operation but now it's a much more involved "weaving things together" operation where overlap between A and B doesn't copy between them but instead uses items in-place. For example if a document in A isn't present in B, it's still added like before. If the document A is already present in B, though, then it's no longer added and instead it's recursed within to see if extra items are added to the new document.This new "fancy" merge operation is intended to be a more long-term implementation of this. At a high level this enables projects to separately depend on WASI, for example, without every project coordinating and using the exact same WIT-level dependency within one build invocation (which isn't possible with separate compilation). All WASI worlds will, in the end, get merged together into one picture of the WASI world when a component is produced.
This was a fair bit of support which wasn't easily supported or tested before. I've modified the
componentstest suite to be able to have shared WIT dependencies between the adapter and the main module to add tests for that. Additionally I've added a dedicated set of tests for just merging resolves which showcases some of the features at this time.With all that said, there are a few aspects and limitations of merging resolves that are worth mentioning:
Packages are merged as keyed by their name and URL. For now this basically means that only packages under
deps/are candidates for merging (as only they have URLs) and they must be placed in same-named subfolders.When merging packages are allowed to pick up more documents. This is intended to model where each resolve may have a subset of the whole picture file-wise. The use case in Runtime panic encoding a guest component utilizing
wasi-http#967 specifically exercises this, for example.When merging documents it's allowed for new interfaces and worlds not previously present in the document to get merged in. This is intended to model a bit of forward-compatible-ness where for example the preview1 adapter may have been built against a historical version of WASI whereas a project using that may use a bleeding edge version that has more interfaces. This merge should still be ok since they're both only using what they declared, so by adding new interfaces and worlds it shouldn't ever tamper with the others' view of the world.
Merging interfaces and worlds, however, currently require exact "structural equivalence". The thinking behind this is that if one component exports an
interfacebut then a merge operation suddenly adds items to thatinterfacethen the component would no longer validly export the interface. This should be possible for the converse, though, where adding items to an interface that's only ever imported should be valid. This doesn't implement this sort of detection logic and takes a more conservative stance that interfaces and worlds must exactly match.Currently merging does not actually test for structural equivalence beyond top-level names. That's deferred to later if necessary, for example by updating Implement structural equivalence for interfaces when merging worlds. #962 to change where the tests happen.
The result of all this is to fix the panic found in #967 and more generally enable adapters and main modules in
wit-componentto import/use the same WIT interfaces. This means it's now possible to have a project use new preview2 interfaces with a preview1 standard library and the preview1 adapter and everything should work.Closes #962
Closes #967