Implement canonical interface names in wit-component - #2622
Conversation
Tweak the string-splitting to more closely match the intent of versions found in tooling, specifically: * Document that build metadata (`+foo` in `0.0.1+foo`) is always split out and not part of the canonical name. * Don't split out pre-release information since pre-release versions are incompatible with all other versions. This additionally matches preexisting merging behavior in `wit-component` as discovered in bytecodealliance/wasm-tools#2622
| bail!(offset, "invalid interface version: {e}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this validation move up to the above method? I think it'd be a bit clearer to perform this validation when implements is specified rather than deferring it to happen later here. That'd probably require validating the version_suffix field before implements and reorderin the above if statements, but that should be fine to do.
There was a problem hiding this comment.
Done. Neither way is perfect. We validate version_suffix with ComponentName here. Ideally, we would also validate the suffix with implements here. But moving up does make a smaller diff.
| pub fn encode_with_options( | ||
| resolve: &Resolve, | ||
| package: PackageId, | ||
| canonical_names: bool, |
There was a problem hiding this comment.
In retrospect reading over this again, I think let's just add the bool parameter to the preexisting function. Given the nature of publishing for this crate, which is that each release is a semver-major release, it's ok to change API signatures. And given that I think it makes more sense rather than to start a *_with_options convention because if more options are added in the future it'll just end up breaking this signature anyway. Having just one function helps reduce duplication and cognitive overhead too I think.
|
|
||
| fn encode_interface_export( | ||
| &mut self, | ||
| export_name: &str, |
There was a problem hiding this comment.
Right now this name logic is now split between the caller and the end of this function, but perhaps this parameter could be changed to ComponentExternName directly to avoid this duplication and instead have the logic in just one place?
| external_id: resolve.external_id_value(key, item).map(|s| s.into()), | ||
| version_suffix: None, | ||
| let implements = resolve.implements_interface(key, item); | ||
| if canonical_names { |
There was a problem hiding this comment.
In a previous review I was curiuos if it would be possible to deduplicate the number of places that a canonical-names option was taken into account and a ComponentExternName were created. I count currently four different locations doing very similar things:
- here
- below in this file in
for interface in interfaces - in
encode_interface_importinencoding.rs - in
encode_interface_exportinencoding.rs(split across two functions)
Were you able to take a look and see if these locations could be unified? Is there perhaps one, or maybe two at most, helpers that could be used to construct these names?
There was a problem hiding this comment.
Will think about it tomorrow.
| (import "a:b/c@0.1.1" "x" (func (param i32 i32))) | ||
| (import "a:b/c@0.1.1" "y" (func)) |
There was a problem hiding this comment.
In addition to importing these two functions could this test additionally import the 0.1.0 version of a:b/c to test that version-merging logic too?
| continue; | ||
| if let WorldKey::Interface(_) = key { | ||
| continue; | ||
| } | ||
| // Keep labeled imports with `implements` version unchanged |
There was a problem hiding this comment.
I'm a bit surprised that this is necessary since it's expected that there'd be a unique InterfaceId for each named import. If this is removed, do tests fail, however?
There was a problem hiding this comment.
Yes, merge-canon-with-implements.wit test would fail:
import a:b/c@0.1.0;
import a:b/c@0.1.1;
import my-thing: a:b/c@0.1.0;
import my-thing: a:b/c@0.1.0 gets deleted, because 0.1.0 have been removed by the above imports.
2 participants
Follow-up to #2556 and replaces #2602.
emit_canonical_namesinComponentEncoderto emit canonical version.merge_world_imports_based_on_semverthatimplementsversion is not updated during the merge.--emit-canonical-namestowasm-tools component newDuring the transition period, upstream libraries, e.g., wac, can decode the original binary and re-encode with the
emit_canonical_namesflag enabled, so that we convert all binaries into their canonical encoding as a preprocessing step. Thenwaccan merge the interface via string matching directly.