From 51e8f0d7bd07e776c2c46911bdc06535c0721806 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 5 Mar 2025 18:45:31 +0000 Subject: [PATCH 1/7] toml_edit tables: Use TableLike and .as_table_like Fixes #57 --- src/lib.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 020d315..19c371a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -94,7 +94,7 @@ use std::{ time::SystemTime, }; -use toml_edit::{DocumentMut, Item, Table, TomlError}; +use toml_edit::{DocumentMut, Item, TableLike, TomlError}; /// Error type used by this crate. pub enum Error { @@ -308,6 +308,7 @@ fn extract_workspace_dependencies( ) -> Result, Error> { Ok(workspace_dep_tables(&workspace_toml) .into_iter() + .map(|t| t.iter()) .flatten() .map(move |(dep_name, dep_value)| { let pkg_name = dep_value.get("package").and_then(|i| i.as_str()).unwrap_or(dep_name); @@ -318,10 +319,10 @@ fn extract_workspace_dependencies( } /// Return an iterator over all `[workspace.dependencies]` -fn workspace_dep_tables(cargo_toml: &DocumentMut) -> Option<&Table> { +fn workspace_dep_tables(cargo_toml: &DocumentMut) -> Option<&dyn TableLike> { cargo_toml .get("workspace") - .and_then(|w| w.as_table()?.get("dependencies")?.as_table()) + .and_then(|w| w.as_table_like()?.get("dependencies")?.as_table_like()) } /// Make sure that the given crate name is a valid rust identifier. @@ -354,8 +355,8 @@ fn extract_crate_names( (name.to_string(), cr) }); - let dep_tables = dep_tables(cargo_toml).chain(target_dep_tables(cargo_toml)); - let dep_pkgs = dep_tables.flatten().filter_map(move |(dep_name, dep_value)| { + let dep_tables = dep_tables(cargo_toml.as_table()).chain(target_dep_tables(cargo_toml)); + let dep_pkgs = dep_tables.map(|t| t.iter()).flatten().filter_map(move |(dep_name, dep_value)| { let pkg_name = dep_value.get("package").and_then(|i| i.as_str()).unwrap_or(dep_name); // We already handle this via `root_pkg` above. @@ -383,18 +384,18 @@ fn extract_package_name(cargo_toml: &DocumentMut) -> Option<&str> { cargo_toml.get("package")?.get("name")?.as_str() } -fn target_dep_tables(cargo_toml: &DocumentMut) -> impl Iterator { - cargo_toml.get("target").into_iter().filter_map(Item::as_table).flat_map(|t| { - t.iter().map(|(_, value)| value).filter_map(Item::as_table).flat_map(dep_tables) +fn target_dep_tables(cargo_toml: &DocumentMut) -> impl Iterator { + cargo_toml.get("target").into_iter().filter_map(Item::as_table_like).flat_map(|t| { + t.iter().map(|(_, value)| value).filter_map(Item::as_table_like).flat_map(dep_tables) }) } -fn dep_tables(table: &Table) -> impl Iterator { +fn dep_tables(table: &dyn TableLike) -> impl Iterator { table .get("dependencies") .into_iter() .chain(table.get("dev-dependencies")) - .filter_map(Item::as_table) + .filter_map(Item::as_table_like) } #[cfg(test)] From 5ce9214530b1b727729fdc0625258e23d5b19e2d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 5 Mar 2025 18:45:52 +0000 Subject: [PATCH 2/7] toml_edit tables: clippy.toml forbidden methods I used this file and clippy -- -A clippy::all -D clippy::disallowed_methods to find the places I needed to fix. --- clippy.toml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000..3177464 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +disallowed-methods = [ + { path = "toml_edit::Item::as_table", reason = "Use .as_table_like instead!" }, + { path = "toml_edit::Item::is_table", reason = "Use .is_table_like instead!" }, +] From a1380664ee29899a48979337e30e246beeda3a0c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 6 Mar 2025 11:46:23 +0000 Subject: [PATCH 3/7] fixup! toml_edit tables: clippy.toml forbidden methods --- clippy.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy.toml b/clippy.toml index 3177464..679700c 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,4 +1,5 @@ disallowed-methods = [ + # Avoid a repeat of https://github.com/bkchr/proc-macro-crate/issues/57 { path = "toml_edit::Item::as_table", reason = "Use .as_table_like instead!" }, { path = "toml_edit::Item::is_table", reason = "Use .is_table_like instead!" }, ] From fe807ed96cc964fb187a1c5b2877fd0205a39dc9 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 6 Mar 2025 11:49:29 +0000 Subject: [PATCH 4/7] CI: check that we don't call toml_edit::Item::as_table This will stop this bug sneaking back in. Doing this here saves having to duplicate all the tests with versions that use inline tables instead. (We'll have one test, though.) --- .github/workflows/rust.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 6d77c77..f5e455a 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -44,3 +44,12 @@ jobs: - uses: Swatinem/rust-cache@v2 - name: Check formatting run: cargo fmt --all -- --check + test: + name: cargo clippy (forbidden methods) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: dtolnay/rust-toolchain@stable + # We just use clippy to spot forbidden methods. + # We disable the default lint set which has much questionable output. + - run: cargo clippy --all -- -A clippy::all -D clippy::disallowed_methods From 1ded013e9c0126bae48425799649b6424298ed73 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 6 Mar 2025 11:50:29 +0000 Subject: [PATCH 5/7] tests: One test of an inline table --- src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 19c371a..b1218b5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -439,6 +439,16 @@ mod tests { Ok(Some(FoundCrate::Name(name))) if name == "my_crate" } + // forbidding toml_edit::Item::as_table ought to mean this is OK, but let's have a test too + create_test! { + deps_with_crate_inline_table, + r#" + dependencies = { my_crate = "0.1" } + "#, + "", + Ok(Some(FoundCrate::Name(name))) if name == "my_crate" + } + create_test! { dev_deps_with_crate, r#" From b11171a1d29bf06d028d546e6971e1e823356143 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 6 Mar 2025 12:58:51 +0100 Subject: [PATCH 6/7] Update .github/workflows/rust.yml --- .github/workflows/rust.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index f5e455a..12b532e 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -44,7 +44,7 @@ jobs: - uses: Swatinem/rust-cache@v2 - name: Check formatting run: cargo fmt --all -- --check - test: + clippy: name: cargo clippy (forbidden methods) runs-on: ubuntu-latest steps: From 1bcfd8887fe691f69ca88a791efc95e4b6e8d491 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 6 Mar 2025 13:01:32 +0100 Subject: [PATCH 7/7] FMT --- src/lib.rs | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index b1218b5..ff7e959 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -356,26 +356,28 @@ fn extract_crate_names( }); let dep_tables = dep_tables(cargo_toml.as_table()).chain(target_dep_tables(cargo_toml)); - let dep_pkgs = dep_tables.map(|t| t.iter()).flatten().filter_map(move |(dep_name, dep_value)| { - let pkg_name = dep_value.get("package").and_then(|i| i.as_str()).unwrap_or(dep_name); + let dep_pkgs = + dep_tables.map(|t| t.iter()).flatten().filter_map(move |(dep_name, dep_value)| { + let pkg_name = dep_value.get("package").and_then(|i| i.as_str()).unwrap_or(dep_name); - // We already handle this via `root_pkg` above. - if package_name.as_ref().map_or(false, |n| *n == pkg_name) { - return None - } + // We already handle this via `root_pkg` above. + if package_name.as_ref().map_or(false, |n| *n == pkg_name) { + return None + } - // Check if this is a workspace dependency. - let workspace = dep_value.get("workspace").and_then(|w| w.as_bool()).unwrap_or_default(); + // Check if this is a workspace dependency. + let workspace = + dep_value.get("workspace").and_then(|w| w.as_bool()).unwrap_or_default(); - let pkg_name = workspace - .then(|| workspace_dependencies.get(pkg_name).map(|p| p.as_ref())) - .flatten() - .unwrap_or(pkg_name); + let pkg_name = workspace + .then(|| workspace_dependencies.get(pkg_name).map(|p| p.as_ref())) + .flatten() + .unwrap_or(pkg_name); - let cr = FoundCrate::Name(sanitize_crate_name(dep_name)); + let cr = FoundCrate::Name(sanitize_crate_name(dep_name)); - Some((pkg_name.to_owned(), cr)) - }); + Some((pkg_name.to_owned(), cr)) + }); Ok(root_pkg.into_iter().chain(dep_pkgs).collect()) } @@ -385,9 +387,16 @@ fn extract_package_name(cargo_toml: &DocumentMut) -> Option<&str> { } fn target_dep_tables(cargo_toml: &DocumentMut) -> impl Iterator { - cargo_toml.get("target").into_iter().filter_map(Item::as_table_like).flat_map(|t| { - t.iter().map(|(_, value)| value).filter_map(Item::as_table_like).flat_map(dep_tables) - }) + cargo_toml + .get("target") + .into_iter() + .filter_map(Item::as_table_like) + .flat_map(|t| { + t.iter() + .map(|(_, value)| value) + .filter_map(Item::as_table_like) + .flat_map(dep_tables) + }) } fn dep_tables(table: &dyn TableLike) -> impl Iterator {