Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion crates/wasmparser/src/validator/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4682,7 +4682,13 @@ impl ComponentNameContext {
let implements = ComponentName::new_with_features(implements, offset, *features)
.with_context(|| format!("`{implements}` is not a valid name"))?;
match implements.kind() {
ComponentNameKind::Interface(_) => {}
ComponentNameKind::Interface(iface_name) => {
if let Some(suffix) = version_suffix {
if let Err(e) = iface_name.version(Some(suffix)) {
bail!(offset, "invalid interface version: {e}");
}
}
}
_ => bail!(offset, "name `{implements}` must be an interface"),
}
}
Expand Down
69 changes: 61 additions & 8 deletions crates/wit-component/src/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ const TLS_BASE_SET: &str = "$set-tls-base";
pub(crate) mod fixup;

mod wit;
pub use wit::{encode, encode_world};
pub use wit::{encode, encode_with_options, encode_world};

mod types;
use types::{InstanceTypeEncoder, RootTypeEncoder, TypeEncodingMaps, ValtypeEncoder};
Expand Down Expand Up @@ -609,12 +609,29 @@ impl<'a> EncodingState<'a> {
let instance_type_idx = self
.component
.type_instance(Some(&format!("ty-{name}")), &ty);

let mut implements = info.implements.clone();
let (import_name, version_suffix) = if self.info.encoder.emit_canonical_names {
let suffix = resolve.version_suffix_of(interface_id);
if implements.is_some() {
implements = resolve.canon_id_of(interface_id);
(name.to_string(), suffix)
} else {
let canon_name = resolve
.canon_id_of(interface_id)
.unwrap_or_else(|| name.to_string());
(canon_name, suffix)
}
} else {
(name.to_string(), None)
};

let instance_idx = self.component.import(
wasm_encoder::ComponentExternName {
name: name.into(),
implements: info.implements.as_deref().map(|s| s.into()),
name: import_name.into(),
implements: implements.map(|s| s.into()),
external_id: info.external_id.as_deref().map(|s| s.into()),
version_suffix: None,
version_suffix: version_suffix.map(|s| s.into()),
},
ComponentTypeRef::Instance(instance_type_idx),
);
Expand Down Expand Up @@ -762,7 +779,11 @@ impl<'a> EncodingState<'a> {
let world = &resolve.worlds[self.info.encoder.metadata.world];

for export_name in exports {
let export_string = resolve.name_world_key(export_name);
let export_string = if self.info.encoder.emit_canonical_names {
resolve.name_canon_world_key(export_name)
} else {
resolve.name_world_key(export_name)
};
match &world.exports[export_name] {
WorldItem::Function(func) => {
let ty = self
Expand Down Expand Up @@ -993,12 +1014,25 @@ impl<'a> EncodingState<'a> {
component_index,
imports,
);
let mut implements = resolve.implements_value(key, item);
let export_version_suffix = if self.info.encoder.emit_canonical_names {
match (&implements, key, item) {
(Some(_), _, WorldItem::Interface { id, .. }) => {
implements = resolve.canon_id_of(*id);
resolve.version_suffix_of(*id)
}
(None, WorldKey::Interface(id), _) => resolve.version_suffix_of(*id),
_ => None,
}
} else {
None
};
Comment thread
chenyan2002 marked this conversation as resolved.
Outdated
let idx = self.component.export(
wasm_encoder::ComponentExternName {
name: export_name.into(),
implements: resolve.implements_value(key, item).map(|s| s.into()),
implements: implements.map(|s| s.into()),
external_id: resolve.external_id_value(key, item).map(|s| s.into()),
version_suffix: None,
version_suffix: export_version_suffix.map(|s| s.into()),
},
ComponentExportKind::Instance,
instance_index,
Expand Down Expand Up @@ -3290,6 +3324,7 @@ pub struct ComponentEncoder {
pub(super) reject_legacy_names: bool,
debug_names: bool,
shim_return_call_ref: bool,
emit_canonical_names: bool,
}

impl ComponentEncoder {
Expand Down Expand Up @@ -3357,6 +3392,20 @@ impl ComponentEncoder {
self
}

/// Sets whether to emit canonical interface names in the component binary.
///
/// When enabled, import/export names use canonical version prefixes (e.g.,
/// `wasi:cli/exit@0.2` instead of `wasi:cli/exit@0.2.1`) and the
/// `version_suffix` field is populated. This also forces merging of
/// imports that share the same canonical version prefix.
/// This flag subsumes the `merge_imports_based_on_semver` flag.
///
/// This is disabled by default.
pub fn emit_canonical_names(&mut self, emit: bool) -> &mut Self {
self.emit_canonical_names = emit;
self
}

/// Sets whether to reject the historical mangling/name scheme for core wasm
/// imports/exports as they map to the component model.
///
Expand Down Expand Up @@ -3509,7 +3558,11 @@ impl ComponentEncoder {
bail!("a module is required when encoding a component");
}

if self.merge_imports_based_on_semver.unwrap_or(true) {
if self.emit_canonical_names {
self.metadata
.resolve
.merge_world_imports_based_on_canonical_version(self.metadata.world)?;
} else if self.merge_imports_based_on_semver.unwrap_or(true) {
self.metadata
.resolve
.merge_world_imports_based_on_semver(self.metadata.world)?;
Expand Down
85 changes: 68 additions & 17 deletions crates/wit-component/src/encoding/wit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,16 @@ use wit_parser::*;
/// The binary returned can be [`decode`d](crate::decode) to recover the WIT
/// package provided.
pub fn encode(resolve: &Resolve, package: PackageId) -> Result<Vec<u8>> {
let mut component = encode_component(resolve, package)?;
encode_with_options(resolve, package, false)
}

/// Same as [`encode`] but with an option to emit canonical interface names.
pub fn encode_with_options(
resolve: &Resolve,
package: PackageId,
canonical_names: bool,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

) -> Result<Vec<u8>> {
let mut component = encode_component_with_options(resolve, package, canonical_names)?;
component.raw_custom_section(&crate::base_producers().raw_custom_section());
Ok(component.finish())
}
Expand All @@ -48,11 +57,16 @@ pub fn encode(resolve: &Resolve, package: PackageId) -> Result<Vec<u8>> {
///
/// The binary returned can be [`decode`d](crate::decode) to recover the WIT
/// package provided.
pub fn encode_component(resolve: &Resolve, package: PackageId) -> Result<ComponentBuilder> {
pub fn encode_component_with_options(
resolve: &Resolve,
package: PackageId,
canonical_names: bool,
) -> Result<ComponentBuilder> {
let mut encoder = Encoder {
component: ComponentBuilder::default(),
resolve,
package,
canonical_names,
};
encoder.run()?;

Expand All @@ -67,6 +81,15 @@ pub fn encode_component(resolve: &Resolve, package: PackageId) -> Result<Compone

/// Encodes a `world` as a component type.
pub fn encode_world(resolve: &Resolve, world_id: WorldId) -> Result<ComponentType> {
encode_world_with_options(resolve, world_id, false)
}

/// Same as [`encode_world`] but with an option to emit canonical names.
pub fn encode_world_with_options(
resolve: &Resolve,
world_id: WorldId,
canonical_names: bool,
) -> Result<ComponentType> {
let mut component = InterfaceEncoder::new(resolve);
let world = &resolve.worlds[world_id];
log::trace!("encoding world {}", world.name);
Expand All @@ -93,9 +116,10 @@ pub fn encode_world(resolve: &Resolve, world_id: WorldId) -> Result<ComponentTyp
continue;
}
};
component
.outer
.import(component_extern_name(resolve, key, import), ty);
component.outer.import(
component_extern_name(resolve, key, import, canonical_names),
ty,
);
}
// Encode the exports
for (key, export) in world.exports.iter() {
Expand All @@ -113,9 +137,10 @@ pub fn encode_world(resolve: &Resolve, world_id: WorldId) -> Result<ComponentTyp
}
WorldItem::Type { .. } => unreachable!(),
};
component
.outer
.export(component_extern_name(resolve, key, export), ty);
component.outer.export(
component_extern_name(resolve, key, export, canonical_names),
ty,
);
}

Ok(component.outer)
Expand All @@ -125,19 +150,40 @@ fn component_extern_name(
resolve: &Resolve,
key: &WorldKey,
item: &WorldItem,
canonical_names: bool,
) -> wasm_encoder::ComponentExternName<'static> {
ComponentExternName {
name: resolve.name_world_key(key).into(),
implements: resolve.implements_value(key, item).map(|s| s.into()),
external_id: resolve.external_id_value(key, item).map(|s| s.into()),
version_suffix: None,
if canonical_names {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

  1. here
  2. below in this file in for interface in interfaces
  3. in encode_interface_import in encoding.rs
  4. in encode_interface_export in encoding.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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will think about it tomorrow.

let name = resolve.name_canon_world_key(key);
let mut implements = resolve.implements_value(key, item);
let version_suffix = match (&implements, key, item) {
(Some(_), _, WorldItem::Interface { id, .. }) => {
implements = resolve.canon_id_of(*id);
resolve.version_suffix_of(*id)
}
(None, WorldKey::Interface(id), _) => resolve.version_suffix_of(*id),
_ => None,
};
ComponentExternName {
name: name.into(),
implements: implements.map(|s| s.into()),
external_id: resolve.external_id_value(key, item).map(|s| s.into()),
version_suffix: version_suffix.map(|s| s.into()),
}
} else {
ComponentExternName {
name: resolve.name_world_key(key).into(),
implements: resolve.implements_value(key, item).map(|s| s.into()),
external_id: resolve.external_id_value(key, item).map(|s| s.into()),
version_suffix: None,
}
}
}

struct Encoder<'a> {
component: ComponentBuilder,
resolve: &'a Resolve,
package: PackageId,
canonical_names: bool,
}

impl Encoder<'_> {
Expand All @@ -153,7 +199,8 @@ impl Encoder<'_> {
// For each `world` encode it directly as a component and then create a
// wrapper component that exports that component.
for (name, &world) in self.resolve.packages[self.package].worlds.iter() {
let component_ty = encode_world(self.resolve, world)?;
let component_ty =
encode_world_with_options(self.resolve, world, self.canonical_names)?;

let world = &self.resolve.worlds[world];
let mut wrapper = ComponentType::new();
Expand Down Expand Up @@ -197,11 +244,15 @@ impl Encoder<'_> {
for interface in interfaces {
encoder.interface = Some(interface);
let iface = &self.resolve.interfaces[interface];
let name = self.resolve.id_of(interface).unwrap();
let name = if self.canonical_names {
self.resolve.canon_id_of(interface).unwrap()
} else {
self.resolve.id_of(interface).unwrap()
};
if interface == id {
let idx = encoder.encode_instance(interface)?;
log::trace!("exporting self as {idx}");
encoder.outer.export(name, ComponentTypeRef::Instance(idx));
encoder.outer.export(&name, ComponentTypeRef::Instance(idx));
Comment thread
chenyan2002 marked this conversation as resolved.
Outdated
} else {
encoder.push_instance();
for (_, id) in iface.types.iter() {
Expand All @@ -212,7 +263,7 @@ impl Encoder<'_> {
encoder.outer.ty().instance(&instance);
encoder.import_map.insert(interface, encoder.instances);
encoder.instances += 1;
encoder.outer.import(name, ComponentTypeRef::Instance(idx));
encoder.outer.import(&name, ComponentTypeRef::Instance(idx));
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/wit-component/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ mod printing;
mod targets;
mod validation;

pub use encoding::{ComponentEncoder, LibraryInfo, encode};
pub use encoding::{ComponentEncoder, LibraryInfo, encode, encode_with_options};
pub use linking::Linker;
pub use printing::*;
pub use targets::*;
Expand Down
2 changes: 2 additions & 0 deletions crates/wit-component/tests/components.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ fn run_test(path: &Path) -> Result<()> {
.debug_names(true)
.shim_return_call_ref(config.return_call_ref)
.realloc_via_memory_grow(config.realloc_via_memory_grow)
.emit_canonical_names(config.merge_imports_based_on_canonical_version)
.module(&module)?;
for adapter in adapters {
let (name, wasm) = read_name_and_module("adapt-", &adapter?, &resolve, pkg_id)?;
Expand Down Expand Up @@ -247,6 +248,7 @@ struct Config {
use_built_in_libdl: bool,
return_call_ref: bool,
realloc_via_memory_grow: bool,
merge_imports_based_on_canonical_version: bool,
}

/// Reads the configuration for the test located at `path`.
Expand Down
Loading
Loading