Code review feedback.

Author: peterhuene

crates/wac-graph/src/graph.rs

@@ -294,17 +294,17 @@ impl RegisteredPackage {
 #[derive(Debug, Clone)]
 pub struct Node {
     /// The node kind.
-    pub kind: NodeKind,
+    kind: NodeKind,
     /// The package associated with the node, if any.
-    pub package: Option<PackageId>,
+    package: Option<PackageId>,
     /// The item kind of the node.
-    pub item_kind: ItemKind,
+    item_kind: ItemKind,
     /// The optional name to associate with the node.
     ///
     /// When the graph is encoded, node names are recorded in a `names` custom section.
-    pub name: Option<String>,
+    name: Option<String>,
     /// The name to use for exporting the node.
-    pub export: Option<String>,
+    export: Option<String>,
 }
 
 impl Node {
@@ -318,13 +318,49 @@ impl Node {
         }
     }
 
-    fn import_name(&self) -> Option<&str> {
+    /// Gets the kind of the node.
+    pub fn kind(&self) -> &NodeKind {
+        &self.kind
+    }
+
+    /// Gets the package id associated with the node.
+    ///
+    /// Returns `None` if the node is not directly associated with a package.
+    pub fn package(&self) -> Option<PackageId> {
+        self.package
+    }
+
+    /// Gets the item kind of the node.
+    pub fn item_kind(&self) -> ItemKind {
+        self.item_kind
+    }
+
+    /// Gets the name of the node.
+    ///
+    /// Node names are encoded in a `names` custom section.
+    ///
+    /// Returns `None` if the node is unnamed.
+    pub fn name(&self) -> Option<&str> {
+        self.name.as_deref()
+    }
+
+    /// Gets the import name of the node.
+    ///
+    /// Returns `Some` if the node is an import or `None` if the node is not an import.
+    pub fn import_name(&self) -> Option<&str> {
         match &self.kind {
             NodeKind::Import(name) => Some(name),
             _ => None,
         }
     }
 
+    /// Gets the export name of the node.
+    ///
+    /// Returns `None` if the node is not exported.
+    pub fn export_name(&self) -> Option<&str> {
+        self.export.as_deref()
+    }
+
     fn add_satisfied_arg(&mut self, index: usize) {
         match &mut self.kind {
             NodeKind::Instantiation(satisfied) => {
@@ -532,7 +568,10 @@ impl CompositionGraph {
         let key = entry.package.as_ref().unwrap().key();
         log::debug!("unregistering package `{key}` with the graph");
         let prev = self.package_map.remove(&key as &dyn BorrowedKey);
-        assert!(prev.is_some());
+        assert!(
+            prev.is_some(),
+            "package should exist in the package map (this is a bug)"
+        );
 
         // Finally free the package
         *entry = RegisteredPackage::new(entry.generation.wrapping_add(1));

crates/wac-parser/src/resolution.rs

@@ -737,7 +737,7 @@ enum Item {
 impl Item {
     fn kind(&self, graph: &CompositionGraph) -> ItemKind {
         match self {
-            Self::Node(id) => graph[*id].item_kind,
+            Self::Node(id) => graph[*id].item_kind(),
             Self::Use(ty) => ItemKind::Type(*ty),
             Self::Type(ty) => ItemKind::Type(*ty),
         }
@@ -788,7 +788,7 @@ impl State {
         if let Item::Node(node) = item {
             // Use only the first name encountered for the node, ignoring
             // aliasing in the form of `let x = y;`
-            if self.graph[node].name.is_none() {
+            if self.graph[node].name().is_none() {
                 self.graph.set_node_name(node, id.string.to_owned());
             }
         }
@@ -1133,10 +1133,10 @@ impl<'a> AstResolver<'a> {
     ) -> Result<(), Error> {
         if let Some((item, prev_span)) = state.root_scope().get(&name) {
             let node = &state.graph[item.node()];
-            if let NodeKind::Definition = node.kind {
+            if let NodeKind::Definition = node.kind() {
                 return Err(Error::ExportConflict {
                     name,
-                    kind: node.item_kind.desc(state.graph.types()).to_string(),
+                    kind: node.item_kind().desc(state.graph.types()).to_string(),
                     span,
                     definition: prev_span,
                     help: if !show_hint {
@@ -2720,7 +2720,7 @@ impl<'a> AstResolver<'a> {
         for (name, import) in state
             .graph
             .node_ids()
-            .filter_map(|n| match &state.graph[n].kind {
+            .filter_map(|n| match &state.graph[n].kind() {
                 NodeKind::Import(name) => Some((name, n)),
                 _ => None,
             })
@@ -2738,7 +2738,7 @@ impl<'a> AstResolver<'a> {
                 .is_subtype(
                     expected.promote(),
                     state.graph.types(),
-                    state.graph[import].item_kind,
+                    state.graph[import].item_kind(),
                     state.graph.types(),
                 )
                 .map_err(|e| Error::TargetMismatch {
@@ -2767,7 +2767,7 @@ impl<'a> AstResolver<'a> {
 
             checker
                 .is_subtype(
-                    state.graph[export].item_kind,
+                    state.graph[export].item_kind(),
                     state.graph.types(),
                     expected.promote(),
                     state.graph.types(),

src/commands/encode.rs

@@ -64,7 +64,7 @@ pub struct EncodeCommand {
     #[clap(long, value_name = "URL")]
     pub registry: Option<String>,
 
-    /// The path to the source file.
+    /// The path to the source WAC file.
     #[clap(value_name = "PATH")]
     pub path: PathBuf,
 }

src/commands/parse.rs

@@ -8,7 +8,7 @@ use wac_parser::Document;
 #[derive(Args)]
 #[clap(disable_version_flag = true)]
 pub struct ParseCommand {
-    /// The path to the source file.
+    /// The path to the source WAC file.
     #[clap(value_name = "PATH")]
     pub path: PathBuf,
 }

src/commands/resolve.rs

@@ -36,7 +36,7 @@ pub struct ResolveCommand {
     #[clap(long, value_name = "URL")]
     pub registry: Option<String>,
 
-    /// The path to the source file.
+    /// The path to the source WAC file.
     #[clap(value_name = "PATH")]
     pub path: PathBuf,
 }