Skip to content

Commit 3226b14

Browse files
authored
Maintain more internal consistency checks when merging worlds (#1792)
This commit fixes two issues, both preexisting, which represented violated invariants of a `Resolve` after worlds were merged. World merging is used during component generation and might be used for bindings generation so it's important to maintain the various dynamic invariants of `Resolve` to ensure that surrounding tooling works. An example of this is that printing a merged world doesn't produce parseable WIT today because some of the internal invariants aren't respected. This commit beefs up the `Resolve::assert_valid` function and then applies necessary fixes to resolve these assertion failures on preexisting test cases. The main difference is that anonymous interfaces/types are now "cloned" when they move from one world to another. This is required to ensure that all the various links between these items are consistent and point to the right place. This cloning infrastructure may end up being more generalized in the future if the need arises, but for now it's quite limited.
1 parent a099de3 commit 3226b14

8 files changed

Lines changed: 374 additions & 14 deletions

crates/wit-parser/src/resolve.rs

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ use crate::{
1919
WorldKey, WorldSpan,
2020
};
2121

22+
mod clone;
23+
2224
/// Representation of a fully resolved set of WIT packages.
2325
///
2426
/// This structure contains a graph of WIT packages and all of their contents
@@ -747,6 +749,8 @@ package {name} is defined in two different locations:\n\
747749
let from_world = &self.worlds[from];
748750
let into_world = &self.worlds[into];
749751

752+
log::trace!("merging {} into {}", from_world.name, into_world.name);
753+
750754
// First walk over all the imports of `from` world and figure out what
751755
// to do with them.
752756
//
@@ -755,13 +759,15 @@ package {name} is defined in two different locations:\n\
755759
// same. Otherwise queue up a new import since if `from` has more
756760
// imports than `into` then it's fine to add new imports.
757761
for (name, from_import) in from_world.imports.iter() {
762+
let name_str = self.name_world_key(name);
758763
match into_world.imports.get(name) {
759764
Some(into_import) => {
760-
let name = self.name_world_key(name);
765+
log::trace!("info/from shared import on `{name_str}`");
761766
self.merge_world_item(from_import, into_import)
762-
.with_context(|| format!("failed to merge world import {name}"))?;
767+
.with_context(|| format!("failed to merge world import {name_str}"))?;
763768
}
764769
None => {
770+
log::trace!("new import: `{name_str}`");
765771
new_imports.push((name.clone(), from_import.clone()));
766772
}
767773
}
@@ -788,13 +794,15 @@ package {name} is defined in two different locations:\n\
788794
// Next walk over exports of `from` and process these similarly to
789795
// imports.
790796
for (name, from_export) in from_world.exports.iter() {
797+
let name_str = self.name_world_key(name);
791798
match into_world.exports.get(name) {
792799
Some(into_export) => {
793-
let name = self.name_world_key(name);
800+
log::trace!("info/from shared export on `{name_str}`");
794801
self.merge_world_item(from_export, into_export)
795-
.with_context(|| format!("failed to merge world export {name}"))?;
802+
.with_context(|| format!("failed to merge world export {name_str}"))?;
796803
}
797804
None => {
805+
log::trace!("new export `{name_str}`");
798806
// See comments in `ensure_can_add_world_export` for why
799807
// this is slightly different than imports.
800808
self.ensure_can_add_world_export(
@@ -811,14 +819,33 @@ package {name} is defined in two different locations:\n\
811819
}
812820
}
813821

822+
// For all the new imports and exports they may need to be "cloned" to
823+
// be able to belong to the new world. For example:
824+
//
825+
// * Anonymous interfaces have a `package` field which points to the
826+
// package of the containing world, but `from` and `into` may not be
827+
// in the same package.
828+
//
829+
// * Type imports have an `owner` field that point to `from`, but they
830+
// now need to point to `into` instead.
831+
//
832+
// Cloning is no trivial task, however, so cloning is delegated to a
833+
// submodule to perform a "deep" clone and copy items into new arena
834+
// entries as necessary.
835+
let mut cloner = clone::Cloner::new(self, TypeOwner::World(from), TypeOwner::World(into));
836+
cloner.register_world_type_overlap(from, into);
837+
for (name, item) in new_imports.iter_mut().chain(&mut new_exports) {
838+
cloner.world_item(name, item);
839+
}
840+
814841
// Insert any new imports and new exports found first.
815-
let into = &mut self.worlds[into];
842+
let into_world = &mut self.worlds[into];
816843
for (name, import) in new_imports {
817-
let prev = into.imports.insert(name, import);
844+
let prev = into_world.imports.insert(name, import);
818845
assert!(prev.is_none());
819846
}
820847
for (name, export) in new_exports {
821-
let prev = into.exports.insert(name, export);
848+
let prev = into_world.exports.insert(name, export);
822849
assert!(prev.is_none());
823850
}
824851

@@ -1428,7 +1455,13 @@ package {name} is defined in two different locations:\n\
14281455
for (name, item) in world.imports.iter().chain(world.exports.iter()) {
14291456
log::debug!("validating world item: {}", self.name_world_key(name));
14301457
match item {
1431-
WorldItem::Interface { .. } => {}
1458+
WorldItem::Interface { id, .. } => {
1459+
// anonymous interfaces must belong to the same package
1460+
// as the world's package.
1461+
if matches!(name, WorldKey::Name(_)) {
1462+
assert_eq!(self.interfaces[*id].package, world.package);
1463+
}
1464+
}
14321465
WorldItem::Function(f) => {
14331466
assert!(!matches!(name, WorldKey::Interface(_)));
14341467
assert_eq!(f.name, name.clone().unwrap_name());
@@ -1438,12 +1471,7 @@ package {name} is defined in two different locations:\n\
14381471
assert!(types.insert(*ty));
14391472
let ty = &self.types[*ty];
14401473
assert_eq!(ty.name, Some(name.clone().unwrap_name()));
1441-
1442-
// TODO: `Resolve::merge_worlds` doesn't uphold this
1443-
// invariant, and that should be fixed.
1444-
if false {
1445-
assert_eq!(ty.owner, TypeOwner::World(id));
1446-
}
1474+
assert_eq!(ty.owner, TypeOwner::World(id));
14471475
}
14481476
}
14491477
}
Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
//! A helper, non public, module to assist with cloning items within a
2+
//! `Resolve`.
3+
//!
4+
//! Cloning items is not as simple as calling `Clone` due to the nature of how
5+
//! `TypeId` tracks relationships between interfaces and types. A full deep
6+
//! clone requires walking the full structure and allocating new id links. This
7+
//! is akin to, for example, creating a deep copy of an `Rc<T>` by calling
8+
//! `Clone for T`.
9+
//!
10+
//! This is currently used when merging worlds together to help copy anonymously
11+
//! named items from one world to another.
12+
//!
13+
//! The general structure of this module is that each method takes a mutable
14+
//! reference to an AST item and updates it as necessary internally, delegating
15+
//! to other methods for internal AST items.
16+
//!
17+
//! This module does not at the time of this writing have full support for
18+
//! cloning everything within a `Resolve`.
19+
20+
use crate::*;
21+
use std::collections::HashMap;
22+
23+
pub struct Cloner<'a> {
24+
resolve: &'a mut Resolve,
25+
prev_owner: TypeOwner,
26+
new_owner: TypeOwner,
27+
28+
/// This map keeps track, in the current scope of types, of all copied
29+
/// types. This deduplicates copying types to ensure that they're only
30+
/// copied at most once.
31+
types: HashMap<TypeId, TypeId>,
32+
}
33+
34+
impl<'a> Cloner<'a> {
35+
pub fn new(
36+
resolve: &'a mut Resolve,
37+
prev_owner: TypeOwner,
38+
new_owner: TypeOwner,
39+
) -> Cloner<'a> {
40+
Cloner {
41+
prev_owner,
42+
new_owner,
43+
resolve,
44+
types: Default::default(),
45+
}
46+
}
47+
48+
pub fn register_world_type_overlap(&mut self, from: WorldId, into: WorldId) {
49+
let into = &self.resolve.worlds[into];
50+
let from = &self.resolve.worlds[from];
51+
for (name, into_import) in into.imports.iter() {
52+
let WorldKey::Name(_) = name else { continue };
53+
let WorldItem::Type(into_id) = into_import else {
54+
continue;
55+
};
56+
let Some(WorldItem::Type(from_id)) = from.imports.get(name) else {
57+
continue;
58+
};
59+
self.types.insert(*from_id, *into_id);
60+
}
61+
}
62+
63+
pub fn world_item(&mut self, key: &WorldKey, item: &mut WorldItem) {
64+
match key {
65+
WorldKey::Name(_) => {}
66+
WorldKey::Interface(_) => return,
67+
}
68+
69+
match item {
70+
WorldItem::Type(t) => {
71+
self.type_id(t);
72+
}
73+
WorldItem::Function(f) => {
74+
self.function(f);
75+
}
76+
WorldItem::Interface { id, .. } => {
77+
self.interface(id);
78+
}
79+
}
80+
}
81+
82+
fn type_id(&mut self, ty: &mut TypeId) {
83+
if !self.types.contains_key(ty) {
84+
let mut new = self.resolve.types[*ty].clone();
85+
self.type_def(&mut new);
86+
let id = self.resolve.types.alloc(new);
87+
self.types.insert(*ty, id);
88+
}
89+
*ty = self.types[ty];
90+
}
91+
92+
fn type_def(&mut self, def: &mut TypeDef) {
93+
if def.owner != TypeOwner::None {
94+
assert_eq!(def.owner, self.prev_owner);
95+
def.owner = self.new_owner;
96+
}
97+
match &mut def.kind {
98+
TypeDefKind::Type(Type::Id(id)) => {
99+
if self.resolve.types[*id].owner == self.prev_owner {
100+
self.type_id(id);
101+
} else {
102+
// ..
103+
}
104+
}
105+
TypeDefKind::Type(_)
106+
| TypeDefKind::Resource
107+
| TypeDefKind::Flags(_)
108+
| TypeDefKind::Enum(_) => {}
109+
TypeDefKind::Handle(Handle::Own(ty) | Handle::Borrow(ty)) => {
110+
self.type_id(ty);
111+
}
112+
TypeDefKind::Option(ty) | TypeDefKind::List(ty) => {
113+
self.ty(ty);
114+
}
115+
TypeDefKind::Tuple(list) => {
116+
for ty in list.types.iter_mut() {
117+
self.ty(ty);
118+
}
119+
}
120+
TypeDefKind::Record(r) => {
121+
for field in r.fields.iter_mut() {
122+
self.ty(&mut field.ty);
123+
}
124+
}
125+
TypeDefKind::Variant(r) => {
126+
for case in r.cases.iter_mut() {
127+
if let Some(ty) = &mut case.ty {
128+
self.ty(ty);
129+
}
130+
}
131+
}
132+
TypeDefKind::Result(r) => {
133+
if let Some(ok) = &mut r.ok {
134+
self.ty(ok);
135+
}
136+
if let Some(err) = &mut r.err {
137+
self.ty(err);
138+
}
139+
}
140+
TypeDefKind::Future(f) => {
141+
if let Some(ty) = f {
142+
self.ty(ty);
143+
}
144+
}
145+
TypeDefKind::Stream(_) => unimplemented!(),
146+
TypeDefKind::Unknown => {}
147+
}
148+
}
149+
150+
fn ty(&mut self, ty: &mut Type) {
151+
match ty {
152+
Type::Id(id) => self.type_id(id),
153+
_ => {}
154+
}
155+
}
156+
157+
fn function(&mut self, func: &mut Function) {
158+
match &mut func.kind {
159+
FunctionKind::Freestanding => {}
160+
FunctionKind::Method(id) | FunctionKind::Static(id) | FunctionKind::Constructor(id) => {
161+
self.type_id(id)
162+
}
163+
}
164+
for (_, ty) in func.params.iter_mut() {
165+
self.ty(ty);
166+
}
167+
match &mut func.results {
168+
Results::Named(named) => {
169+
for (_, ty) in named {
170+
self.ty(ty);
171+
}
172+
}
173+
Results::Anon(ty) => self.ty(ty),
174+
}
175+
}
176+
177+
fn interface(&mut self, id: &mut InterfaceId) {
178+
let mut new = self.resolve.interfaces[*id].clone();
179+
let next_id = self.resolve.interfaces.next_id();
180+
let mut clone = Cloner::new(
181+
self.resolve,
182+
TypeOwner::Interface(*id),
183+
TypeOwner::Interface(next_id),
184+
);
185+
for id in new.types.values_mut() {
186+
clone.type_id(id);
187+
}
188+
for func in new.functions.values_mut() {
189+
clone.function(func);
190+
}
191+
new.package = Some(match self.new_owner {
192+
TypeOwner::Interface(id) => self.resolve.interfaces[id].package.unwrap(),
193+
TypeOwner::World(id) => self.resolve.worlds[id].package.unwrap(),
194+
TypeOwner::None => unreachable!(),
195+
});
196+
*id = self.resolve.interfaces.alloc(new);
197+
assert_eq!(*id, next_id);
198+
}
199+
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// RUN: component wit %
2+
3+
package foo:bar;
4+
5+
world foo {
6+
import a: interface {
7+
use local.{t1};
8+
use a:b/foreign.{t2};
9+
}
10+
export a: interface {
11+
use local.{t1};
12+
use a:b/foreign.{t2};
13+
}
14+
}
15+
16+
interface local {
17+
type t1 = u32;
18+
}
19+
20+
package a:b {
21+
interface foreign {
22+
type t2 = u32;
23+
}
24+
}
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/// RUN: component wit %
2+
package foo:bar;
3+
4+
interface local {
5+
type t1 = u32;
6+
}
7+
8+
world foo {
9+
import local;
10+
import a:b/foreign;
11+
import a: interface {
12+
use local.{t1};
13+
use a:b/foreign.{t2};
14+
}
15+
16+
export a: interface {
17+
use local.{t1};
18+
use a:b/foreign.{t2};
19+
}
20+
}
21+
package a:b {
22+
interface foreign {
23+
type t2 = u32;
24+
}
25+
}
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// RUN: component embed --dummy --world w1 % | \
2+
// component embed --world w2 % | \
3+
// component embed --world w3 % | \
4+
// component embed --world w4 % | \
5+
// component wit
6+
7+
package a:b;
8+
9+
world w1 {}
10+
world w2 {
11+
import a: interface {}
12+
}
13+
world w3 {
14+
export a: interface {}
15+
}
16+
world w4 {
17+
import b: interface {}
18+
export b: interface {}
19+
}

0 commit comments

Comments
 (0)