Replace toml dependency with toml_edit to preserve comments (#407)

---------

Co-authored-by: Ivan Petkov <ivanppetkov@gmail.com>

CHANGELOG.md

@@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## Unreleased
 
+### Fixed
+* Fixed handling of Cargo workspace inheritance for git-dependencies where said
+  crate relies on reading non-TOML metadata (i.e. comments) from its Cargo.toml
+  at build time. ([#407](https://github.com/ipetkov/crane/pull/407))
+
 ## [0.14.1] - 2023-09-23
 
 ### Fixed

pkgs/crane-utils/Cargo.lock

@@ -16,7 +16,7 @@ dependencies = [
  "pretty_assertions",
  "serde",
  "serde_json",
- "toml",
+ "toml_edit",
 ]
 
 [[package]]
@@ -124,15 +124,6 @@ dependencies = [
  "serde",
 ]
 
-[[package]]
-name = "serde_spanned"
-version = "0.6.3"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "96426c9936fd7a0124915f9185ea1d20aa9445cc9821142f0a73bc9207a2e186"
-dependencies = [
- "serde",
-]
-
 [[package]]
 name = "syn"
 version = "2.0.28"
@@ -144,37 +135,19 @@ dependencies = [
  "unicode-ident",
 ]
 
-[[package]]
-name = "toml"
-version = "0.7.6"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c17e963a819c331dcacd7ab957d80bc2b9a9c1e71c804826d2f283dd65306542"
-dependencies = [
- "indexmap",
- "serde",
- "serde_spanned",
- "toml_datetime",
- "toml_edit",
-]
-
 [[package]]
 name = "toml_datetime"
 version = "0.6.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "7cda73e2f1397b1262d6dfdcef8aafae14d1de7748d66822d3bfeeb6d03e5e4b"
-dependencies = [
- "serde",
-]
 
 [[package]]
 name = "toml_edit"
-version = "0.19.14"
+version = "0.20.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f8123f27e969974a3dfba720fdb560be359f57b44302d280ba72e76a74480e8a"
+checksum = "ca676d9ba1a322c1b64eb8045a5ec5c0cfb0c9d08e15e9ff622589ad5221c8fe"
 dependencies = [
  "indexmap",
- "serde",
- "serde_spanned",
  "toml_datetime",
  "winnow",
 ]

pkgs/crane-utils/Cargo.toml

@@ -8,7 +8,7 @@ publish = false
 anyhow = "1"
 serde_json = "1"
 serde = { version = "1", features = ["derive"] }
-toml = { version = "0.7.3", features = ["preserve_order"] }
+toml_edit = "0.20.1"
 
 [dev-dependencies]
 pretty_assertions = "1.3.0"

pkgs/crane-utils/src/bin/crane-resolve-workspace-inheritance.rs

@@ -7,8 +7,9 @@ use std::{
     mem,
     path::Path,
     process::{Command, Stdio},
+    str::FromStr,
 };
-use toml::value::Table;
+use toml_edit::Item;
 
 fn main() {
     let mut args = env::args();
@@ -64,39 +65,29 @@ fn resolve_and_print_cargo_toml(cargo_toml: &Path) -> anyhow::Result<()> {
     let mut cargo_toml = parse_toml(cargo_toml)?;
     merge(&mut cargo_toml, &parse_toml(&root_toml.join("Cargo.toml"))?);
 
-    toml::to_string(&cargo_toml)
+    stdout()
-        .context("failed to serialize updated Cargo.toml")
+        .write_all(cargo_toml.to_string().as_bytes())
-        .and_then(|string| {
+        .context("failed to print updated Cargo.toml")
-            stdout()
-                .write_all(string.as_bytes())
-                .context("failed to print updated Cargo.toml")
-                .map(drop)
-        })
 }
 
-fn parse_toml(path: &Path) -> anyhow::Result<toml::Value> {
+fn parse_toml(path: &Path) -> anyhow::Result<toml_edit::Document> {
     let mut buf = String::new();
     File::open(path)
         .and_then(|mut file| file.read_to_string(&mut buf))
         .with_context(|| format!("cannot read {}", path.display()))?;
 
-    toml::from_str(&buf).with_context(|| format!("cannot parse {}", path.display()))
+    toml_edit::Document::from_str(&buf).with_context(|| format!("cannot parse {}", path.display()))
 }
 
-fn merge(cargo_toml: &mut toml::Value, root: &toml::Value) {
+/// Merge the workspace `root` toml into the specified crate's `cargo_toml`
-    let (t, rt) = match (cargo_toml, root) {
+fn merge(cargo_toml: &mut toml_edit::Document, root: &toml_edit::Document) {
-        (toml::Value::Table(t), toml::Value::Table(rt)) => (t, rt),
+    let w: &dyn toml_edit::TableLike =
-
+        if let Some(w) = root.get("workspace").and_then(try_as_table_like) {
-        // Bail if cargo_toml or workspace root are malformed
+            w
-        _ => return,
+        } else {
-    };
+            // no "workspace" entry, nothing to merge
-
+            return;
-    let w = if let Some(toml::Value::Table(w)) = rt.get("workspace") {
+        };
-        w
-    } else {
-        // no "workspace" entry, nothing to merge
-        return;
-    };
 
     // https://doc.rust-lang.org/cargo/reference/workspaces.html#workspaces
     for (key, ws_key) in [
@@ -105,90 +96,200 @@ fn merge(cargo_toml: &mut toml::Value, root: &toml::Value) {
         ("dev-dependencies", "dependencies"),
         ("build-dependencies", "dependencies"),
     ] {
-        if let (Some(toml::Value::Table(p)), Some(toml::Value::Table(wp))) =
+        if let Some((cargo_toml, root)) = cargo_toml.get_mut(key).zip(w.get(ws_key)) {
-            (t.get_mut(key), w.get(ws_key))
+            try_merge_cargo_tables(cargo_toml, root);
-        {
-            merge_tables(p, wp);
         };
 
-        if let Some(toml::Value::Table(targets)) = t.get_mut("target") {
+        if let Some(targets) = cargo_toml.get_mut("target").and_then(try_as_table_like_mut) {
-            for (_, tp) in targets {
+            for (_, tp) in targets.iter_mut() {
-                if let (Some(toml::Value::Table(p)), Some(toml::Value::Table(wp))) =
+                if let Some((cargo_toml, root)) = tp.get_mut(key).zip(w.get(ws_key)) {
-                    (tp.get_mut(key), w.get(ws_key))
+                    try_merge_cargo_tables(cargo_toml, root);
-                {
+                }
-                    merge_tables(p, wp);
-                };
             }
         }
     }
 }
 
-fn merge_tables(cargo_toml: &mut Table, root: &Table) {
+/// Return a [`toml_edit::TableLike`] representation of the [`Item`] (if any)
+fn try_as_table_like(item: &Item) -> Option<&dyn toml_edit::TableLike> {
+    match item {
+        Item::Table(w) => Some(w),
+        Item::Value(toml_edit::Value::InlineTable(w)) => Some(w),
+        _ => None,
+    }
+}
+
+/// Return a mutable [`toml_edit::TableLike`] representation of the [`Item`] (if any)
+fn try_as_table_like_mut(item: &mut Item) -> Option<&mut dyn toml_edit::TableLike> {
+    match item {
+        Item::Table(w) => Some(w),
+        Item::Value(toml_edit::Value::InlineTable(w)) => Some(w),
+        _ => None,
+    }
+}
+
+/// Merge the specified `cargo_toml` and workspace `root` if both are tables
+fn try_merge_cargo_tables(cargo_toml: &mut Item, root: &Item) {
+    let cargo_toml = try_as_table_like_mut(cargo_toml);
+    let root = try_as_table_like(root);
+
+    if let Some((cargo_toml, root)) = cargo_toml.zip(root) {
+        merge_cargo_tables(cargo_toml, root);
+    }
+}
+/// Merge the specified `cargo_toml` and workspace `root` tables
+fn merge_cargo_tables<T, U>(cargo_toml: &mut T, root: &U)
+where
+    T: toml_edit::TableLike + ?Sized,
+    U: toml_edit::TableLike + ?Sized,
+{
     cargo_toml.iter_mut().for_each(|(k, v)| {
         // Bail if:
         // - cargo_toml isn't a table (otherwise `workspace = true` can't show up
         // - the workspace root doesn't have this key
-        let (t, root_val) = match (&mut *v, root.get(k)) {
+        let (t, root_val) = match try_as_table_like_mut(&mut *v).zip(root.get(&k)) {
-            (toml::Value::Table(t), Some(root_val)) => (t, root_val),
+            Some((t, root_val)) => (t, root_val),
             _ => return,
         };
 
-        if let Some(toml::Value::Boolean(true)) = t.get("workspace") {
+        if let Some(Item::Value(toml_edit::Value::Boolean(bool_value))) = t.get("workspace") {
-            t.remove("workspace");
+            if *bool_value.value() {
-            let orig_val = mem::replace(v, root_val.clone());
+                t.remove("workspace");
-            merge_into(v, orig_val);
+                let orig_val = mem::replace(v, root_val.clone());
+                merge_items(v, orig_val);
+            }
         }
     });
 }
 
-fn merge_into(dest: &mut toml::Value, additional: toml::Value) {
+/// Recursively merge the `additional` item into the specified `dest`
-    match additional {
+fn merge_items(dest: &mut Item, additional: Item) {
-        toml::Value::String(_)
+    use toml_edit::Value;
-        | toml::Value::Integer(_)
-        | toml::Value::Float(_)
-        | toml::Value::Boolean(_)
-        | toml::Value::Datetime(_) => {
-            // Override dest completely for raw values
-            *dest = additional;
-        }
 
-        toml::Value::Array(additional) => {
+    match additional {
-            if let toml::Value::Array(dest) = dest {
+        Item::Value(additional) => match additional {
+            Value::String(_)
+            | Value::Integer(_)
+            | Value::Float(_)
+            | Value::Boolean(_)
+            | Value::Datetime(_) => {
+                // Override dest completely for raw values
+                *dest = Item::Value(additional);
+            }
+
+            Value::Array(additional) => {
+                if let Item::Value(Value::Array(dest)) = dest {
+                    dest.extend(additional);
+                } else {
+                    // Override dest completely if types don't match
+                    *dest = Item::Value(Value::Array(additional));
+                }
+            }
+
+            Value::InlineTable(additional) => {
+                merge_tables(dest, additional);
+            }
+        },
+        Item::Table(additional) => {
+            merge_tables(dest, additional);
+        }
+        Item::None => {}
+        Item::ArrayOfTables(additional) => {
+            if let Item::ArrayOfTables(dest) = dest {
                 dest.extend(additional);
             } else {
                 // Override dest completely if types don't match
-                *dest = toml::Value::Array(additional);
+                *dest = Item::ArrayOfTables(additional);
             }
         }
+    }
+}
 
-        toml::Value::Table(additional) => {
+use table_like_ext::merge_tables;
-            if let toml::Value::Table(dest) = dest {
+mod table_like_ext {
-                additional
+    //! Helper functions to merge values in any combination of the two [`TableLike`] items
-                    .into_iter()
+    //! found in [`toml_edit`]
-                    .for_each(|(k, v)| match dest.get_mut(&k) {
+
-                        Some(existing) => merge_into(existing, v),
+    use toml_edit::{Item, TableLike};
-                        None => {
+
-                            dest.insert(k, v);
+    /// Recursively merge the `additional` table into `dest` (overwriting if `dest` is not a table)
-                        }
+    pub(super) fn merge_tables<T>(dest: &mut Item, additional: T)
-                    });
+    where
-            } else {
+        T: TableLikeExt,
+    {
+        match dest {
+            Item::Table(dest) => merge_table_like(dest, additional),
+            Item::Value(toml_edit::Value::InlineTable(dest)) => merge_table_like(dest, additional),
+            _ => {
                 // Override dest completely if types don't match, but also
                 // skip empty tables (i.e. if we had `key = { workspace = true }`
                 if !additional.is_empty() {
-                    *dest = toml::Value::Table(additional);
+                    *dest = additional.into_item();
                 }
             }
         }
     }
+
+    /// Recursively merge two tables
+    fn merge_table_like<T, U>(dest: &mut T, additional: U)
+    where
+        T: TableLike,
+        U: TableLikeExt,
+    {
+        additional
+            .into_iter()
+            .map(U::map_iter_item)
+            .for_each(|(k, v)| match dest.get_mut(&k) {
+                Some(existing) => super::merge_items(existing, v),
+                None => {
+                    dest.insert(&k, v);
+                }
+            });
+    }
+
+    /// Generalized form of the item yielded by [`IntoIterator`] for the two [`TableLike`] types
+    /// in [`toml_edit`]
+    type CommonIterItem = (toml_edit::InternalString, Item);
+
+    /// Extension trait to iterate [`Item`]s from a [`TableLike`] item
+    pub(super) trait TableLikeExt: TableLike + IntoIterator {
+        /// Convert the iterator item to a common type
+        fn map_iter_item(item: Self::Item) -> CommonIterItem;
+
+        /// Convert the table into an [`Item`]
+        fn into_item(self) -> Item;
+    }
+
+    impl TableLikeExt for toml_edit::Table {
+        fn map_iter_item(item: Self::Item) -> CommonIterItem {
+            item
+        }
+
+        fn into_item(self) -> Item {
+            Item::Table(self)
+        }
+    }
+
+    impl TableLikeExt for toml_edit::InlineTable {
+        fn map_iter_item(item: Self::Item) -> CommonIterItem {
+            let (k, v) = item;
+            (k, Item::Value(v))
+        }
+
+        fn into_item(self) -> Item {
+            Item::Value(toml_edit::Value::InlineTable(self))
+        }
+    }
 }
 
 #[cfg(test)]
 mod tests {
     use pretty_assertions::assert_eq;
+    use std::str::FromStr;
 
     #[test]
     fn smoke() {
-        let mut cargo_toml = toml::from_str(
+        let mut cargo_toml = toml_edit::Document::from_str(
             r#"
             [package]
             authors.workspace = true
@@ -209,6 +310,7 @@ mod tests {
             version.workspace = true
 
             [dependencies]
+            # the `foo` dependency is most imporant, so it goes first
             foo.workspace = true
             bar.workspace = true
             baz.workspace = true
@@ -240,11 +342,15 @@ mod tests {
             grault = { version = "grault-vers" }
             garply = "garply-vers"
             waldo = "waldo-vers"
+
+            [features]
+            # this feature is a demonstration that comments are preserved
+            my_feature = []
         "#,
         )
         .unwrap();
 
-        let root_toml = toml::from_str(
+        let root_toml = toml_edit::Document::from_str(
             r#"
             [workspace.package]
             authors = ["first author", "second author"]
@@ -265,6 +371,7 @@ mod tests {
             version = "some version"
 
             [workspace.dependencies]
+            # top-level workspace comments are not copied - only the values are merged
             foo = { version = "foo-vers" }
             bar = { version = "bar-vers", default-features = false }
             baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] }
@@ -278,64 +385,68 @@ mod tests {
         )
         .unwrap();
 
-        let expected_toml = toml::from_str::<toml::Value>(
+        // NOTE: The nonstandard spacing is due to reusing decorations from original keys/values
-            r#"
+        // in cargo_toml
+        let expected_toml_str = r#"
             [package]
-            authors = ["first author", "second author"]
+            authors= ["first author", "second author"]
-            categories = ["first category", "second category" ]
+            categories= ["first category", "second category" ]
-            description = "some description"
+            description= "some description"
-            documentation = "some doc url"
+            documentation= "some doc url"
-            edition = "2021"
+            edition= "2021"
-            exclude = ["first exclusion", "second exclusion"]
+            exclude= ["first exclusion", "second exclusion"]
-            homepage = "some home page"
+            homepage= "some home page"
-            include = ["first inclusion", "second inclusion"]
+            include= ["first inclusion", "second inclusion"]
-            keyword = ["first keyword", "second keyword"]
+            keyword= ["first keyword", "second keyword"]
-            license = "some license"
+            license= "some license"
-            license-file = "some license-file"
+            license-file= "some license-file"
-            publish = true
+            publish= true
-            readme = "some readme"
+            readme= "some readme"
-            repository = "some repository"
+            repository= "some repository"
-            rust-version = "some rust-version"
+            rust-version= "some rust-version"
-            version = "some version"
+            version= "some version"
 
             [dependencies]
-            foo = { version = "foo-vers" }
+            # the `foo` dependency is most imporant, so it goes first
-            bar = { version = "bar-vers", default-features = false }
+            foo= { version = "foo-vers" }
-            baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] }
+            bar= { version = "bar-vers", default-features = false }
-            qux = { version = "qux-vers", features = ["qux-feat", "qux-additional"] }
+            baz= { version = "baz-vers", features = ["baz-feat", "baz-feat2"] }
-            corge = { version = "corge-vers-override", features = ["qux-feat"] }
+            qux = { version = "qux-vers", features = ["qux-feat","qux-additional"] }
+            corge = { version = "corge-vers-override" , features = ["qux-feat"] }
             grault = { version = "grault-vers" }
             garply = "garply-vers"
             waldo = "waldo-vers"
 
             [target.'cfg(unix)'.dependencies]
-            unix = { version = "unix-vers", features = ["some"] }
+            unix = { version = "unix-vers" , features = ["some"] }
 
             [dev-dependencies]
-            foo = { version = "foo-vers" }
+            foo= { version = "foo-vers" }
-            bar = { version = "bar-vers", default-features = false }
+            bar= { version = "bar-vers", default-features = false }
-            baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] }
+            baz= { version = "baz-vers", features = ["baz-feat", "baz-feat2"] }
-            qux = { version = "qux-vers", features = ["qux-feat", "qux-additional"] }
+            qux = { version = "qux-vers", features = ["qux-feat","qux-additional"] }
-            corge = { version = "corge-vers-override", features = ["qux-feat"] }
+            corge = { version = "corge-vers-override" , features = ["qux-feat"] }
             grault = { version = "grault-vers" }
             garply = "garply-vers"
             waldo = "waldo-vers"
 
             [build-dependencies]
-            foo = { version = "foo-vers" }
+            foo= { version = "foo-vers" }
-            bar = { version = "bar-vers", default-features = false }
+            bar= { version = "bar-vers", default-features = false }
-            baz = { version = "baz-vers", features = ["baz-feat", "baz-feat2"] }
+            baz= { version = "baz-vers", features = ["baz-feat", "baz-feat2"] }
-            qux = { version = "qux-vers", features = ["qux-feat", "qux-additional"] }
+            qux = { version = "qux-vers", features = ["qux-feat","qux-additional"] }
-            corge = { version = "corge-vers-override", features = ["qux-feat"] }
+            corge = { version = "corge-vers-override" , features = ["qux-feat"] }
             grault = { version = "grault-vers" }
             garply = "garply-vers"
             waldo = "waldo-vers"
-        "#,
+
-        )
+            [features]
-        .unwrap();
+            # this feature is a demonstration that comments are preserved
+            my_feature = []
+        "#;
 
         super::merge(&mut cargo_toml, &root_toml);
 
-        assert_eq!(expected_toml, cargo_toml);
+        assert_eq!(expected_toml_str, cargo_toml.to_string());
     }
 }