Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple package in a single WIT source #1577

Merged
merged 11 commits into from
Jun 6, 2024
30 changes: 15 additions & 15 deletions crates/wit-component/src/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2133,33 +2133,33 @@ mod test {

use super::*;
use std::path::Path;
use wit_parser::UnresolvedPackage;
use wit_parser::UnresolvedPackageGroup;

#[test]
fn it_renames_imports() {
let mut resolve = Resolve::new();
let pkg = resolve
.push(
UnresolvedPackage::parse(
Path::new("test.wit"),
r#"
let UnresolvedPackageGroup {
mut packages,
source_map,
} = UnresolvedPackageGroup::parse(
Path::new("test.wit"),
r#"
package test:wit;

interface i {
f: func();
f: func();
}

world test {
import i;
import foo: interface {
f: func();
}
import i;
import foo: interface {
f: func();
}
}
"#,
)
.unwrap(),
)
.unwrap();
)
.unwrap();
let pkg = resolve.push(packages.remove(0), &source_map).unwrap();

let world = resolve.select_world(pkg, None).unwrap();

Expand Down
50 changes: 45 additions & 5 deletions crates/wit-component/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
use std::str::FromStr;
use std::{borrow::Cow, fmt::Display};

use anyhow::{bail, Result};
use anyhow::{bail, Context, Result};
use wasm_encoder::{CanonicalOption, Encode, Section};
use wit_parser::{Resolve, WorldId};
use wit_parser::{parse_use_path, PackageId, ParsedUsePath, Resolve, WorldId};

mod encoding;
mod gc;
Expand Down Expand Up @@ -79,6 +79,43 @@ impl From<StringEncoding> for wasm_encoder::CanonicalOption {
}
}

/// Handles world name resolution for cases when multiple packages may have been resolved. If this
/// is the case, and we're dealing with input that contains a user-supplied world name (like via a
/// CLI command, for instance), we want to ensure that the world name follows the following rules:
///
/// * If there is a single resolved package with a single world, the world name name MAY be
/// omitted.
/// * If there is a single resolved package with multiple worlds, the world name MUST be supplied,
/// but MAY or MAY NOT be fully-qualified.
/// * If there are multiple resolved packages, the world name MUST be fully-qualified.
pub fn resolve_world_from_name(
Copy link
Member

Choose a reason for hiding this comment

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

This is more-or-less a replacement of Resolve::select_world. In that case what would you think of basically replacing select_world with this? That way callers can largely stay the same where they'll pass in a list of ids here rather than a single id and the "foo" selector will only work if the list of packages is of length 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially had the same instinct, but felt like the current approach may be a little more typesafe?

In either case, the caller has to make sure of something at the call site: in the current setup, they have to make sure to select the right PackageId for the world_name they're interested in (I can add a comment to the doc-string to help point them to the right helper for doing this), but at least the type signature gives a hint that they need to do this. In the alternative world where select_world always takes a Vec<PackageId>, I could easily imagine authors forgetting to check that their input world_name formatting conforms to the rules dictated by the Vec<PackageId> count, and hitting this error condition in (probably relatively rarely tested) multi package scenarios.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right, but I also think we'll still want to change select_world, lemme expand on this though. You're right in that if a single PackageId was what the caller already had then it's best to have the current signature of select_world. Note though that the world argument of select_world is not just "foo" but it can also be the fully qualified name of foo:bar/baz.

Otherwise though, why I think this still needs to change, is that the select_world is a method that has not aged well. The intention of the method was to be "here are the two inputs bindings generators typically have and then here's the canonical way to acquire a world". In that sense this is supposed to be the helper to acquire a WorldId to generate bindings for (and all bindings generators operate at the level of a world). The history of this method looked like:

  • Originally there weren't worlds
  • Then there were worlds, but there was a default world in each package
  • Then we wanted the option to select a non-default world, hence the world: Option<&str> argument
  • Then we removed default world but the Option remained for "if there's only one world otherwise disambiguate with the name here"
  • Then we wanted to select worlds outside of this package so the string syntax was extended to foo:bar/baz.

Basically what I'm getting at is that the exact signature of select_world is not really all that important because it's pretty rare to have a single PackageId after this change. The input from all bindings generators is going to be "here's the set of packages that was defined in the 'main package' now please select a world from here". Given that I think it's best to switch this to &[PackageId]. I'd love one day to be able to remove the package argument entirely, but I think that's for a bit further down the road.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If possible, I'd like to save such a refactor for a follow on PR, to get a good baseline landed and free of merge conflicts here.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍

resolve: &Resolve,
resolved_packages: Vec<PackageId>,
world_name: Option<&str>,
) -> Result<WorldId> {
match resolved_packages.len() {
0 => bail!("all of the supplied WIT source files were empty"),
1 => resolve.select_world(resolved_packages[0], world_name.as_deref()),
_ => match world_name.as_deref() {
Some(name) => {
let world_path = parse_use_path(name).with_context(|| {
format!("failed to parse world specifier `{name}`")
})?;
match world_path {
ParsedUsePath::Name(name) => bail!("the world specifier must be of the fully-qualified, id-based form (ex: \"wasi:http/proxy\" rather than \"proxy\"); you used {name}"),
ParsedUsePath::Package(pkg_name, _) => {
match resolve.package_names.get(&pkg_name) {
Some(pkg_id) => resolve.select_world(pkg_id.clone(), world_name.as_deref()),
None => bail!("the world specifier you provided named {pkg_name}, but no package with that name was found"),
}
}
}
}
None => bail!("the supplied WIT source files describe multiple packages; please provide a fully-qualified world-specifier to the `embed` command"),
},
}
}

/// A producer section to be added to all modules and components synthesized by
/// this crate
pub(crate) fn base_producers() -> wasm_metadata::Producers {
Expand Down Expand Up @@ -112,7 +149,7 @@ mod tests {

use anyhow::Result;
use wasmparser::Payload;
use wit_parser::{Resolve, UnresolvedPackage};
use wit_parser::{Resolve, UnresolvedPackageGroup};

use super::{embed_component_metadata, StringEncoding};

Expand Down Expand Up @@ -147,8 +184,11 @@ world test-world {}

// Parse pre-canned WIT to build resolver
let mut resolver = Resolve::default();
let pkg = UnresolvedPackage::parse(&Path::new("in-code.wit"), COMPONENT_WIT)?;
let pkg_id = resolver.push(pkg)?;
let UnresolvedPackageGroup {
mut packages,
source_map,
} = UnresolvedPackageGroup::parse(&Path::new("in-code.wit"), COMPONENT_WIT)?;
let pkg_id = resolver.push(packages.remove(0), &source_map)?;
let world = resolver.select_world(pkg_id, Some("test-world").into())?;

// Embed component metadata
Expand Down
10 changes: 5 additions & 5 deletions crates/wit-component/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
//! the three arguments originally passed to `encode`.

use crate::validation::BARE_FUNC_MODULE_NAME;
use crate::{DecodedWasm, StringEncoding};
use crate::{resolve_world_from_name, DecodedWasm, StringEncoding};
use anyhow::{bail, Context, Result};
use indexmap::IndexMap;
use std::borrow::Cow;
Expand Down Expand Up @@ -259,12 +259,12 @@ impl Bindgen {
let world_name = reader.read_string()?;
wasm = &data[reader.original_position()..];

let (r, pkg) = match crate::decode(wasm)? {
DecodedWasm::WitPackage(resolve, pkg) => (resolve, pkg),
DecodedWasm::Component(..) => bail!("expected an encoded wit package"),
let (r, pkgs) = match crate::decode(wasm)? {
DecodedWasm::WitPackages(resolve, pkgs) => (resolve, pkgs),
DecodedWasm::Component(..) => bail!("expected encoded wit package(s)"),
};
resolve = r;
world = resolve.packages[pkg].worlds[world_name];
world = resolve_world_from_name(&resolve, pkgs, Some(world_name.into()))?;
}

// Current format where `data` is a wasm component itself.
Expand Down
79 changes: 49 additions & 30 deletions crates/wit-component/src/printing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,37 +50,56 @@ impl WitPrinter {
self
}

/// Print the given WIT package to a string.
pub fn print(&mut self, resolve: &Resolve, pkgid: PackageId) -> Result<String> {
let pkg = &resolve.packages[pkgid];
self.print_docs(&pkg.docs);
self.output.push_str("package ");
self.print_name(&pkg.name.namespace);
self.output.push_str(":");
self.print_name(&pkg.name.name);
if let Some(version) = &pkg.name.version {
self.output.push_str(&format!("@{version}"));
}
self.print_semicolon();
self.output.push_str("\n\n");
for (name, id) in pkg.interfaces.iter() {
self.print_docs(&resolve.interfaces[*id].docs);
self.print_stability(&resolve.interfaces[*id].stability);
self.output.push_str("interface ");
self.print_name(name);
self.output.push_str(" {\n");
self.print_interface(resolve, *id)?;
writeln!(&mut self.output, "}}\n")?;
}
/// Print a set of one or more WIT packages into a string.
pub fn print(&mut self, resolve: &Resolve, pkg_ids: &[PackageId]) -> Result<String> {
let has_multiple_packages = pkg_ids.len() > 1;
for (i, pkg_id) in pkg_ids.into_iter().enumerate() {
if i > 0 {
self.output.push_str("\n\n");
}

for (name, id) in pkg.worlds.iter() {
self.print_docs(&resolve.worlds[*id].docs);
self.print_stability(&resolve.worlds[*id].stability);
self.output.push_str("world ");
self.print_name(name);
self.output.push_str(" {\n");
self.print_world(resolve, *id)?;
writeln!(&mut self.output, "}}")?;
let pkg = &resolve.packages[pkg_id.clone()];
self.print_docs(&pkg.docs);
self.output.push_str("package ");
self.print_name(&pkg.name.namespace);
self.output.push_str(":");
self.print_name(&pkg.name.name);
if let Some(version) = &pkg.name.version {
self.output.push_str(&format!("@{version}"));
}

if has_multiple_packages {
self.output.push_str("{");
self.output.indent += 1
} else {
self.print_semicolon();
self.output.push_str("\n\n");
}

for (name, id) in pkg.interfaces.iter() {
self.print_docs(&resolve.interfaces[*id].docs);
self.print_stability(&resolve.interfaces[*id].stability);
self.output.push_str("interface ");
self.print_name(name);
self.output.push_str(" {\n");
self.print_interface(resolve, *id)?;
writeln!(&mut self.output, "}}\n")?;
}

for (name, id) in pkg.worlds.iter() {
self.print_docs(&resolve.worlds[*id].docs);
self.print_stability(&resolve.worlds[*id].stability);
self.output.push_str("world ");
self.print_name(name);
self.output.push_str(" {\n");
self.print_world(resolve, *id)?;
writeln!(&mut self.output, "}}")?;
}

if has_multiple_packages {
self.output.push_str("}");
self.output.indent -= 1
}
}

Ok(std::mem::take(&mut self.output).into())
Expand Down
14 changes: 13 additions & 1 deletion crates/wit-component/src/semver_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::{
dummy_module, embed_component_metadata, encoding::encode_world, ComponentEncoder,
StringEncoding,
};
use anyhow::{Context, Result};
use anyhow::{bail, Context, Result};
use wasm_encoder::{ComponentBuilder, ComponentExportKind, ComponentTypeRef};
use wasmparser::{Validator, WasmFeatures};
use wit_parser::{Resolve, WorldId};
Expand Down Expand Up @@ -47,6 +47,18 @@ pub fn semver_check(mut resolve: Resolve, prev: WorldId, new: WorldId) -> Result
pkg.name.version = None;
}

let old_pkg_id = resolve.worlds[prev]
.package
.context("old world not in named package")?;
let old_pkg_name = &resolve.packages[old_pkg_id].name;
let new_pkg_id = resolve.worlds[new]
.package
.context("new world not in named package")?;
let new_pkg_name = &resolve.packages[new_pkg_id].name;
if old_pkg_id != new_pkg_id {
bail!("the old world is in package {old_pkg_name}, which is not the same as the new world, which is in package {new_pkg_name}", )
}

// Component that will be validated at the end.
let mut root_component = ComponentBuilder::default();

Expand Down
Loading