Reject worlds with interfaces that can access both imports and exports

This commit is a fix for the issue described in
WebAssembly/component-model#208. The process of elaborating a world's
exports is no longer as straightforward as imports and additionally is
no longer infallible. Instead metadata is tracked to ensure that
transitively added imports are always used as imports and never both as
imports and exports.

crates/wit-parser/src/resolve.rs

@@ -1038,13 +1038,15 @@ impl Remap {
         }
 
         let mut export_funcs = Vec::new();
+        let mut export_interfaces = IndexMap::new();
         for ((mut name, item), span) in mem::take(&mut world.exports).into_iter().zip(export_spans)
         {
             self.update_world_key(&mut name);
             match item {
                 WorldItem::Interface(id) => {
                     let id = self.interfaces[id.index()];
-                    self.add_world_export(resolve, world, name, id);
+                    let prev = export_interfaces.insert(id, (name, *span));
+                    assert!(prev.is_none());
                 }
                 WorldItem::Function(mut f) => {
                     self.update_function(&mut f);
@@ -1058,6 +1060,8 @@ impl Remap {
             }
         }
 
+        self.add_world_exports(resolve, world, &export_interfaces)?;
+
         for (name, id, span) in import_types {
             let prev = world
                 .imports
@@ -1122,34 +1126,150 @@ impl Remap {
         if world.imports.contains_key(&key) {
             return;
         }
-
-        foreach_interface_dep(resolve, id, |dep| {
+        let ok = foreach_interface_dep(resolve, id, |dep| {
             self.add_world_import(resolve, world, WorldKey::Interface(dep), dep);
+            true
         });
+        assert!(ok);
         let prev = world.imports.insert(key, WorldItem::Interface(id));
         assert!(prev.is_none());
     }
 
-    fn add_world_export(
+    /// This function adds all of the interfaces in `export_interfaces` to the
+    /// list of exports of the `world` specified.
+    ///
+    /// This method is more involved than adding imports because it is fallible.
+    /// Chiefly what can happen is that the dependencies of all exports must be
+    /// satisfied by other exports or imports, but not both. For example given a
+    /// situation such as:
+    ///
+    /// ```wit
+    /// interface a {
+    ///     type t = u32
+    /// }
+    /// interface b {
+    ///     use a.{t}
+    /// }
+    /// interface c {
+    ///     use a.{t}
+    ///     use b.{t as t2}
+    /// }
+    /// ```
+    ///
+    /// where `c` depends on `b` and `a` where `b` depends on `a`, then the
+    /// purpose of this method is to reject this world:
+    ///
+    /// ```wit
+    /// world foo {
+    ///     export a
+    ///     export c
+    /// }
+    /// ```
+    ///
+    /// The reasoning here is unfortunately subtle and is additionally the
+    /// subject of WebAssembly/component-model#208. Effectively the `c`
+    /// interface depends on `b`, but it's not listed explicitly as an import,
+    /// so it's then implicitly added as an import. This then transitively
+    /// depends on `a` so it's also added as an import. At this point though `c`
+    /// also depends on `a`, and it's also exported, so naively it should depend
+    /// on the export and not implicitly add an import. This means though that
+    /// `c` has access to two copies of `a`, one imported and one exported. This
+    /// is not valid, especially in the face of resource types.
+    ///
+    /// Overall this method is tasked with rejecting the above world by walking
+    /// over all the exports and adding their dependencies. Each dependency is
+    /// recorded with whether it's required to be imported, and then if an
+    /// export is added for something that's required to be an error then the
+    /// operation fails.
+    fn add_world_exports(
         &self,
         resolve: &Resolve,
         world: &mut World,
-        key: WorldKey,
-        id: InterfaceId,
-    ) {
-        if world
-            .exports
-            .insert(key, WorldItem::Interface(id))
-            .is_some()
-        {
-            return;
+        export_interfaces: &IndexMap<InterfaceId, (WorldKey, Span)>,
+    ) -> Result<()> {
+        let mut required_imports = HashSet::new();
+        for (id, (key, span)) in export_interfaces.iter() {
+            let ok = add_world_export(
+                resolve,
+                world,
+                export_interfaces,
+                &mut required_imports,
+                *id,
+                key,
+                true,
+            );
+            if !ok {
+                bail!(Error {
+                    // FIXME: this is not a great error message and basically no
+                    // one will know what to do when it gets printed. Improving
+                    // this error message, however, is a chunk of work that may
+                    // not be best spent doing this at this time, so I'm writing
+                    // this comment instead.
+                    //
+                    // More-or-less what should happen here is that a "path"
+                    // from this interface to the conflicting interface should
+                    // be printed. It should be explained why an import is being
+                    // injected, why that's conflicting with an export, and
+                    // ideally with a suggestion of "add this interface to the
+                    // export list to fix this error".
+                    //
+                    // That's a lot of info that's not easy to get at without
+                    // more refactoring, so it's left to a future date in the
+                    // hopes that most folks won't actually run into this for
+                    // the time being.
+                    msg: format!(
+                        "interface transitively depends on both an \
+                         imported and exported copy of the same interface"
+                    ),
+                    span: *span,
+                });
+            }
         }
-
-        foreach_interface_dep(resolve, id, |dep| {
-            if !world.exports.contains_key(&WorldKey::Interface(dep)) {
-                self.add_world_import(resolve, world, WorldKey::Interface(dep), dep);
+        return Ok(());
+
+        fn add_world_export(
+            resolve: &Resolve,
+            world: &mut World,
+            export_interfaces: &IndexMap<InterfaceId, (WorldKey, Span)>,
+            required_imports: &mut HashSet<InterfaceId>,
+            id: InterfaceId,
+            key: &WorldKey,
+            add_export: bool,
+        ) -> bool {
+            if world.exports.contains_key(key) {
+                if add_export {
+                    return true;
+                } else {
+                    return false;
+                }
             }
-        });
+            let ok = foreach_interface_dep(resolve, id, |dep| {
+                let key = WorldKey::Interface(dep);
+                let add_export = add_export && export_interfaces.contains_key(&dep);
+                add_world_export(
+                    resolve,
+                    world,
+                    export_interfaces,
+                    required_imports,
+                    dep,
+                    &key,
+                    add_export,
+                )
+            });
+            if !ok {
+                return false;
+            }
+            if add_export {
+                if required_imports.contains(&id) {
+                    return false;
+                }
+                world.exports.insert(key.clone(), WorldItem::Interface(id));
+            } else {
+                required_imports.insert(id);
+                world.imports.insert(key.clone(), WorldItem::Interface(id));
+            }
+            true
+        }
     }
 
     fn resolve_include(
@@ -1215,8 +1335,8 @@ impl Remap {
 fn foreach_interface_dep(
     resolve: &Resolve,
     interface: InterfaceId,
-    mut f: impl FnMut(InterfaceId),
-) {
+    mut f: impl FnMut(InterfaceId) -> bool,
+) -> bool {
     for (_, ty) in resolve.interfaces[interface].types.iter() {
         let ty = match resolve.types[*ty].kind {
             TypeDefKind::Type(Type::Id(id)) => id,
@@ -1228,9 +1348,12 @@ fn foreach_interface_dep(
             TypeOwner::World(_) => unreachable!(),
         };
         if dep != interface {
-            f(dep);
+            if !f(dep) {
+                return false;
+            }
         }
     }
+    true
 }
 
 struct MergeMap<'a> {

crates/wit-parser/tests/ui/parse-fail/import-and-export1.wit.result

@@ -0,0 +1,5 @@
+interface transitively depends on both an imported and exported copy of the same interface
+     --> tests/ui/parse-fail/import-and-export1.wit:16:10
+      |
+   16 |   export i3
+      |          ^-

crates/wit-parser/tests/ui/parse-fail/import-and-export2.wit

@@ -0,0 +1,18 @@
+package foo:foo
+
+interface foo {
+  type t = u32
+}
+
+interface bar {
+  use foo.{t}
+}
+
+world baz {
+  export foo
+  import bar
+  export anon: interface {
+    use foo.{t}
+    use bar.{t as t2}
+  }
+}

crates/wit-parser/tests/ui/parse-fail/import-and-export2.wit.result

@@ -0,0 +1,5 @@
+interface transitively depends on both an imported and exported copy of the same interface
+     --> tests/ui/parse-fail/import-and-export2.wit:14:10
+      |
+   14 |   export anon: interface {
+      |          ^---

crates/wit-parser/tests/ui/parse-fail/import-and-export3.wit.result

@@ -0,0 +1,5 @@
+interface transitively depends on both an imported and exported copy of the same interface
+     --> tests/ui/parse-fail/import-and-export3.wit:36:10
+      |
+   36 |   export i3
+      |          ^-

crates/wit-parser/tests/ui/worlds-union.wit → crates/wit-parser/tests/ui/parse-fail/import-and-export4.wit

@@ -41,4 +41,4 @@ world test {
   import i3
   export i1
   export i3
-}
+}

crates/wit-parser/tests/ui/parse-fail/import-and-export4.wit.result

@@ -0,0 +1,5 @@
+interface transitively depends on both an imported and exported copy of the same interface
+     --> tests/ui/parse-fail/import-and-export4.wit:43:10
+      |
+   43 |   export i3
+      |          ^-

crates/wit-parser/tests/ui/parse-fail/import-and-export5.wit

@@ -0,0 +1,18 @@
+package foo:bar
+
+interface a {
+  record x {
+  }
+}
+
+interface b {
+  use a.{x}
+}
+
+world w {
+  export anon: interface {
+    use b.{x as x2}
+    use a.{x}
+  }
+  export a
+}

crates/wit-parser/tests/ui/parse-fail/import-and-export5.wit.result

@@ -0,0 +1,5 @@
+interface transitively depends on both an imported and exported copy of the same interface
+     --> tests/ui/parse-fail/import-and-export5.wit:13:10
+      |
+   13 |   export anon: interface {
+      |          ^---