Implement structural equivalence for interfaces when merging worlds.#962
Closed
sunfishcode wants to merge 1 commit into
Closed
Implement structural equivalence for interfaces when merging worlds.#962sunfishcode wants to merge 1 commit into
sunfishcode wants to merge 1 commit into
Conversation
When merging two worlds together, coalesce interfaces that are structurally equivalent to each other. The motivation for this is that I want to be able to write library code that uses wit bindings that can be used in any world that contains that interface. One such library is std, which will want to depend on wasi-filesystem and other interfaces, but remain independent of the specific world that the user will ultimately chose to link everything into.
Member
|
Thanks! I am personally wary to perform this merging purely based on structural equivalence however since that definitely gets more funky with resource types in the mix. Instead though a rough idea for handling this sort of case would be to use the URLs and/or unique IDs of interfaces. This is something that hasn't really been gamed out a ton right now but the thinking would be the when the same interface is imported or exported twice when merging worlds the interfaces are merged together based on their unique ID. If the IDs don't match then merging doesn't happen and an error is emitted, otherwise merging happens. One important case I think this is necessary for is when one world imports one function from |
alexcrichton
added a commit
to alexcrichton/wasm-tools
that referenced
this pull request
Apr 7, 2023
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
Member
Author
|
This is now superceded by #974. |
alexcrichton
added a commit
that referenced
this pull request
Apr 11, 2023
* wit-component: Implement merging `Resolve`s together 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 `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 #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 #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-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 #962 Closes #967 * Fix an outdated comment
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
2 participants
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.
When merging two worlds together, coalesce interfaces that are structurally equivalent to each other.
The motivation for this is that I want to be able to write library code that uses wit bindings that can be used in any world that contains that interface.
One such library is std, which will want to depend on wasi-filesystem and other interfaces, but remain independent of the specific world that the user will ultimately chose to link everything into.
This doesn't yet include any tests because I'm unsure how to add them for this. I have confirmed that a cargo component build with this is able to build a working component for a crate that uses its own
wit_bindgen::generate!invocation for interfaces that are also pulled in through Cargo.toml: