From 4a389516ecaf30bcd2b47bece15ea72fd736662e Mon Sep 17 00:00:00 2001 From: Shadaj Laddad Date: Wed, 8 Jul 2026 23:16:38 +0000 Subject: [PATCH] fix(stageleft_tool): include strong-ref-enabled and target-specific deps in `__deps` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the fix for hydro-project/stageleft#62. PR #65 made optional dependencies show up in `__deps` behind `#[cfg(any(feature = ...))]` gates, but three gaps remained (the first two are hit by real manifests such as hydro_lang's): 1. Strong `pkg/feature` references were not treated as enabling an optional dependency. Cargo's `feat = ["tokio-stream/sync"]` enables the optional dep `tokio-stream`, but `optional_dep_features` only matched `dep:` refs, so the re-export was cfg'd out when the dep was enabled solely via a strong ref. The old `dep:/...` prefix check was dead code (not valid Cargo syntax) and has been removed. Weak `pkg?/feature` refs correctly do not count as gating. The implicit feature is now appended only when no `dep:` reference exists anywhere (matching Cargo semantics). 2. `[target.'cfg(...)'.dependencies]` tables were ignored entirely, so target-specific deps (e.g. hydro_lang's linux-only `procfs`) never appeared in `__deps`. They are now collected and gated on the target's cfg predicate, composed with feature gates for optional deps: `#[cfg(all(, any(feature = ...)))]`. A dep declared in multiple tables is re-exported once, gated by `any()` of its per-declaration predicates, or ungated if any declaration is unconditional. Non-`cfg(...)` target keys (plain target triples) are skipped with a `cargo::warning` since they cannot be expressed as a cfg gate. 3. A missing `[dependencies]` table no longer panics. Implementation: `gen_deps_module` now builds a list of `Dep` entries (name, package alias, `Vec`) collected from the main and target dependency tables via `collect_deps_from_table`, and `cfg_attr_for_declarations` derives the combined `#[cfg(...)]` attribute used for both the `pub use` re-exports and the runtime `add_deps_reexport` registrations. Tests: - 7 new unit tests in stageleft_tool covering strong refs, weak refs, target-specific deps (plain and optional), deps declared in both the main and target tables, deps in multiple target tables, and a manifest with no `[dependencies]` table. - stageleft_test_no_entry now declares `my_strong_ref_feature` (strong `once_cell/std` ref), `my_weak_ref_feature` (weak `once_cell?/std` ref), and a `[target.'cfg(unix)'.dependencies]` libc dep; the codegen snapshot shows the new gating, e.g. `#[cfg(any(feature = "my_strong_ref_feature", feature = "once_cell"))]` and `#[cfg(unix)] pub use libc;`. Note: the snapshot must be regenerated with a full workspace build (`cargo test --workspace --all-targets`, as CI does) — building the crate alone changes prettyplease's output formatting due to feature unification of syn's `full` feature. Verified with `cargo test --workspace --all-targets` (61 tests pass), `cargo fmt --all -- --check`, and `cargo clippy --all-targets -- -D warnings`. Co-authored-by: Infinity 🤖 PR: #89 --- Cargo.lock | 1 + stageleft_test_no_entry/Cargo.toml | 9 +- .../codegen_snapshot__staged_deps.snap | 14 +- stageleft_tool/src/lib.rs | 426 +++++++++++++++--- 4 files changed, 392 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d48d5ad..468c9d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -490,6 +490,7 @@ version = "0.0.0" dependencies = [ "cfg-if", "insta", + "libc", "once_cell", "slotmap", "stageleft", diff --git a/stageleft_test_no_entry/Cargo.toml b/stageleft_test_no_entry/Cargo.toml index c322c8f..dd1743b 100644 --- a/stageleft_test_no_entry/Cargo.toml +++ b/stageleft_test_no_entry/Cargo.toml @@ -17,7 +17,8 @@ stageleft = { path = "../stageleft", version = "^0.15.0" } slotmap = "1.0.0" -# optional dep with implicit feature (feature "once_cell" auto-created) +# optional dep with implicit feature (feature "once_cell" auto-created), +# also enabled by the strong `once_cell/...` reference in `my_strong_ref_feature` once_cell = { version = "1.20", optional = true } # optional dep gated by explicit feature via dep: syntax @@ -25,6 +26,12 @@ cfg-if = { version = "1.0", optional = true } [features] my_cfg_feature = ["dep:cfg-if"] +my_strong_ref_feature = ["once_cell/std"] +my_weak_ref_feature = ["once_cell?/std"] + +# target-specific dependencies should be re-exported gated on the target cfg +[target.'cfg(unix)'.dependencies] +libc = "0.2" [build-dependencies] stageleft_tool = { path = "../stageleft_tool", version = "^0.15.0" } diff --git a/stageleft_test_no_entry/tests/snapshots/codegen_snapshot__staged_deps.snap b/stageleft_test_no_entry/tests/snapshots/codegen_snapshot__staged_deps.snap index 946c43a..a77f296 100644 --- a/stageleft_test_no_entry/tests/snapshots/codegen_snapshot__staged_deps.snap +++ b/stageleft_test_no_entry/tests/snapshots/codegen_snapshot__staged_deps.snap @@ -5,10 +5,12 @@ expression: "include_str!(concat!(env!(\"OUT_DIR\"), stageleft::PATH_SEPARATOR!( pub mod __deps { pub use stageleft; pub use slotmap; - #[cfg(any(feature = "once_cell"))] + #[cfg(any(feature = "my_strong_ref_feature", feature = "once_cell"))] pub use once_cell; #[cfg(any(feature = "my_cfg_feature"))] pub use cfg_if; + #[cfg(unix)] + pub use libc; stageleft::internal::ctor::declarative::ctor! { #[ctor(unsafe)] fn __init() { { stageleft::internal::add_deps_reexport(vec!["stageleft"], @@ -21,7 +23,8 @@ pub mod __deps { .replace("-", "_"), ::std::borrow::ToOwned::to_owned("__staged"), ::std::borrow::ToOwned::to_owned("__deps"), ::std::borrow::ToOwned::to_owned("slotmap"),]); } #[cfg(any(feature = - "once_cell"))] { stageleft::internal::add_deps_reexport(vec!["once_cell"], + "my_strong_ref_feature", feature = "once_cell"))] { + stageleft::internal::add_deps_reexport(vec!["once_cell"], vec![option_env!("STAGELEFT_FINAL_CRATE_NAME") .unwrap_or(env!("CARGO_PKG_NAME")) .replace("-", "_"), ::std::borrow::ToOwned::to_owned("__staged"), ::std::borrow::ToOwned::to_owned("__deps"), @@ -30,7 +33,12 @@ pub mod __deps { vec![option_env!("STAGELEFT_FINAL_CRATE_NAME") .unwrap_or(env!("CARGO_PKG_NAME")) .replace("-", "_"), ::std::borrow::ToOwned::to_owned("__staged"), ::std::borrow::ToOwned::to_owned("__deps"), - ::std::borrow::ToOwned::to_owned("cfg_if"),]); } + ::std::borrow::ToOwned::to_owned("cfg_if"),]); } #[cfg(unix)] { + stageleft::internal::add_deps_reexport(vec!["libc"], + vec![option_env!("STAGELEFT_FINAL_CRATE_NAME") .unwrap_or(env!("CARGO_PKG_NAME")) + .replace("-", "_"), ::std::borrow::ToOwned::to_owned("__staged"), + ::std::borrow::ToOwned::to_owned("__deps"), + ::std::borrow::ToOwned::to_owned("libc"),]); } stageleft::internal::add_crate_with_staged(env!("CARGO_PKG_NAME") .replace("-", "_")); } } diff --git a/stageleft_tool/src/lib.rs b/stageleft_tool/src/lib.rs index 5e28593..0de9016 100644 --- a/stageleft_tool/src/lib.rs +++ b/stageleft_tool/src/lib.rs @@ -444,40 +444,143 @@ impl VisitMut for GenFinalPubVisitor { } /// For an optional dependency, compute the list of features that enable it. -/// If no feature references `dep:`, Cargo creates an implicit feature with the dep's name. -/// If one or more features reference `dep:`, those are the gating features. +/// +/// A feature enables the optional dependency if its list contains `dep:` or a "strong" +/// dependency feature reference `/` (but not a "weak" reference `?/`, +/// which only applies if the dependency is enabled by something else). +/// +/// Additionally, if no feature references `dep:`, Cargo creates an implicit feature with +/// the dep's name, which is included in the returned list. fn optional_dep_features(dep_name: &str, features_table: Option<&toml_edit::Table>) -> Vec { - let dep_prefix = format!("dep:{dep_name}"); - let dep_prefix_slash = format!("{dep_prefix}/"); - let explicit: Vec = features_table - .into_iter() - .flat_map(|t| t.iter()) - .filter(|(_, vals)| { - vals.as_array().is_some_and(|arr| { - arr.iter().any(|v| { - v.as_str() - .is_some_and(|s| s == dep_prefix || s.starts_with(&dep_prefix_slash)) - }) + let dep_ref = format!("dep:{dep_name}"); + let strong_ref_prefix = format!("{dep_name}/"); + // Whether any feature references `dep:`, which suppresses the implicit feature. + let mut any_dep_ref = false; + let mut gating: Vec = Vec::new(); + for (feat, vals) in features_table.into_iter().flat_map(|t| t.iter()) { + let enables = vals.as_array().is_some_and(|arr| { + arr.iter().filter_map(|v| v.as_str()).fold(false, |acc, s| { + if s == dep_ref { + any_dep_ref = true; + true + } else { + // A strong `/` reference also enables the optional dep. + // (A weak `?/` reference does not.) + acc || s.starts_with(&strong_ref_prefix) + } }) - }) - .map(|(feat, _)| feat.to_owned()) - .collect(); - if explicit.is_empty() { - vec![dep_name.to_owned()] + }); + if enables { + gating.push(feat.to_owned()); + } + } + if !any_dep_ref { + // No `dep:` references, so Cargo creates an implicit feature named after the dep. + let implicit = dep_name.to_owned(); + if !gating.contains(&implicit) { + gating.push(implicit); + } + } + gating +} + +/// A single declaration of a dependency, from `[dependencies]` or a +/// `[target.'cfg(...)'.dependencies]` table. +struct DepDeclaration { + /// The `cfg(...)` predicate from the `[target.'cfg(...)']` key, or `None` for the plain + /// `[dependencies]` table. + target_cfg: Option, + /// The features which enable the dependency, empty if the dependency is not optional. + gating_features: Vec, +} + +impl DepDeclaration { + /// The `cfg` predicate under which this declaration makes the dependency available, or + /// `None` if it is unconditionally available. + fn cfg_predicate(&self) -> Option { + let feature_pred: Option = if self.gating_features.is_empty() { + None + } else { + let preds = self + .gating_features + .iter() + .map(|f| -> syn::Meta { parse_quote!(feature = #f) }); + Some(parse_quote!(any(#(#preds),*))) + }; + match (&self.target_cfg, feature_pred) { + (Some(target), Some(features)) => Some(parse_quote!(all(#target, #features))), + (Some(target), None) => Some(target.clone()), + (None, features) => features, + } + } +} + +/// Build a `#[cfg(...)]` attribute gating a dependency declared by `declarations`, or `None` if +/// the dependency is unconditionally available. +fn cfg_attr_for_declarations(declarations: &[DepDeclaration]) -> Option { + let mut preds = Vec::with_capacity(declarations.len()); + for declaration in declarations { + // If any declaration is unconditional, the dependency is always available. + let pred = declaration.cfg_predicate()?; + preds.push(pred); + } + if let [pred] = &*preds { + Some(parse_quote!(#[cfg(#pred)])) } else { - explicit + Some(parse_quote!(#[cfg(any(#(#preds),*))])) } } -/// Build a `#[cfg(...)]` attribute for the given feature list, or `None` if not optional. -fn cfg_attr_for_features(features: &[String]) -> Option { - if features.is_empty() { - return None; +/// Parse a `[target.'']` key such as `cfg(target_os = "linux")` into a `cfg` predicate. +/// Returns `None` for keys which are not `cfg(...)` expressions (i.e. plain target triples). +fn parse_target_cfg_key(target_key: &str) -> Option { + let inner = target_key.trim().strip_prefix("cfg(")?.strip_suffix(')')?; + syn::parse_str::(inner).ok() +} + +/// A dependency of the staged crate, possibly declared in multiple tables +/// (e.g. `[dependencies]` and/or one or more `[target.'cfg(...)'.dependencies]` tables). +struct Dep { + /// Dependency name with `-` replaced by `_`. + name: String, + /// `package = "..."` renamed crate name (with `-` replaced by `_`), if any. + original_crate_name: Option, + declarations: Vec, +} + +/// Collect dependencies from a `[dependencies]`-shaped table into `deps`, merging entries for +/// dependencies which are declared in multiple tables. +fn collect_deps_from_table( + deps_table: &dyn toml_edit::TableLike, + target_cfg: Option<&syn::Meta>, + features_table: Option<&toml_edit::Table>, + deps: &mut Vec, +) { + for (name, v) in deps_table.iter() { + let is_optional = v.get("optional").and_then(|o| o.as_bool()).unwrap_or(false); + let gating_features = if is_optional { + optional_dep_features(name, features_table) + } else { + vec![] + }; + let declaration = DepDeclaration { + target_cfg: target_cfg.cloned(), + gating_features, + }; + + let name_underscored = name.replace('-', "_"); + if let Some(existing) = deps.iter_mut().find(|d| d.name == name_underscored) { + existing.declarations.push(declaration); + } else { + deps.push(Dep { + name: name_underscored, + original_crate_name: v + .get("package") + .map(|v| v.as_str().unwrap().replace("-", "_")), + declarations: vec![declaration], + }); + } } - let preds = features - .iter() - .map(|f| -> syn::Meta { parse_quote!(feature = #f) }); - Some(parse_quote!(#[cfg(any(#(#preds),*))])) } fn gen_deps_module(stageleft_name: syn::Ident, manifest_path: &Path) -> syn::ItemMod { @@ -487,31 +590,41 @@ fn gen_deps_module(stageleft_name: syn::Ident, manifest_path: &Path) -> syn::Ite .parse::() .unwrap(); let features_table = toml_parsed.get("features").and_then(|v| v.as_table()); - let all_crate_names = toml_parsed["dependencies"] - .as_table() - .unwrap() - .iter() - .map(|(name, v)| { - let is_optional = v.get("optional").and_then(|o| o.as_bool()).unwrap_or(false); - let gating_features = if is_optional { - optional_dep_features(name, features_table) - } else { - vec![] - }; - ( - name.replace('-', "_"), - v.get("package") - .map(|v| v.as_str().unwrap().replace("-", "_")), - gating_features, - ) - }) - .collect::>(); - let deps_reexported = all_crate_names + let mut all_deps = Vec::new(); + if let Some(deps_table) = toml_parsed + .get("dependencies") + .and_then(|v| v.as_table_like()) + { + collect_deps_from_table(deps_table, None, features_table, &mut all_deps); + } + // Also collect target-specific dependencies from `[target.'cfg(...)'.dependencies]` tables. + for (target_key, target_val) in toml_parsed + .get("target") + .and_then(|v| v.as_table_like()) + .into_iter() + .flat_map(|t| t.iter()) + { + let Some(deps_table) = target_val + .get("dependencies") + .and_then(|v| v.as_table_like()) + else { + continue; + }; + if let Some(target_cfg) = parse_target_cfg_key(target_key) { + collect_deps_from_table(deps_table, Some(&target_cfg), features_table, &mut all_deps); + } else { + println!( + "cargo::warning=stageleft: skipping `[target.'{target_key}'.dependencies]`: only `cfg(...)` target keys are supported in `__deps`" + ); + } + } + + let deps_reexported = all_deps .iter() - .map(|(name, _, gating_features)| { - let name_ident = syn::Ident::new(name, Span::call_site()); - let cfg = cfg_attr_for_features(gating_features); + .map(|dep| { + let name_ident = syn::Ident::new(&dep.name, Span::call_site()); + let cfg = cfg_attr_for_declarations(&dep.declarations); parse_quote! { #cfg pub use #name_ident; @@ -519,11 +632,12 @@ fn gen_deps_module(stageleft_name: syn::Ident, manifest_path: &Path) -> syn::Ite }) .collect::>(); - let deps_reexported_runtime = all_crate_names + let deps_reexported_runtime = all_deps .iter() - .map(|(name, original_crate_name, gating_features)| { - let original_crate_name_or_alias = original_crate_name.as_deref().unwrap_or(name); - let cfg = cfg_attr_for_features(gating_features); + .map(|dep| { + let name = &dep.name; + let original_crate_name_or_alias = dep.original_crate_name.as_deref().unwrap_or(name); + let cfg = cfg_attr_for_declarations(&dep.declarations); parse_quote! { #cfg { @@ -969,4 +1083,208 @@ mod tests { generated_code ); } + + #[test] + fn test_gen_deps_module_optional_strong_dep_feature_ref() { + // A strong `dep/feat` reference enables the optional dependency, so a feature + // containing one must be included as a gating feature. + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!(temp_file, "[dependencies]").unwrap(); + writeln!( + temp_file, + r#"optional_dep = {{ version = "1.0", optional = true }}"# + ) + .unwrap(); + writeln!(temp_file, "\n[features]").unwrap(); + writeln!(temp_file, r#"feat_a = ["dep:optional_dep"]"#).unwrap(); + writeln!(temp_file, r#"feat_b = ["optional_dep/some_feat"]"#).unwrap(); + temp_file.flush().unwrap(); + + let stageleft_name = syn::Ident::new("stageleft", Span::call_site()); + let deps_module = gen_deps_module(stageleft_name, temp_file.path()); + let generated_code = quote::quote!(#deps_module).to_string(); + + assert!( + generated_code.contains(r#"any (feature = "feat_a" , feature = "feat_b")"#), + "Both the dep: feature and the strong dep/feat feature should gate: {}", + generated_code + ); + assert!( + !generated_code.contains(r#"feature = "optional_dep""#), + "Implicit feature should be suppressed when dep: is used: {}", + generated_code + ); + } + + #[test] + fn test_gen_deps_module_optional_weak_dep_feature_ref() { + // A weak `dep?/feat` reference does not enable the optional dependency, so a feature + // containing one must not be included as a gating feature. + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!(temp_file, "[dependencies]").unwrap(); + writeln!( + temp_file, + r#"optional_dep = {{ version = "1.0", optional = true }}"# + ) + .unwrap(); + writeln!(temp_file, "\n[features]").unwrap(); + writeln!(temp_file, r#"feat_a = ["dep:optional_dep"]"#).unwrap(); + writeln!(temp_file, r#"feat_w = ["optional_dep?/some_feat"]"#).unwrap(); + temp_file.flush().unwrap(); + + let stageleft_name = syn::Ident::new("stageleft", Span::call_site()); + let deps_module = gen_deps_module(stageleft_name, temp_file.path()); + let generated_code = quote::quote!(#deps_module).to_string(); + + assert!( + generated_code.contains(r#"any (feature = "feat_a")"#), + "Should gate on the dep: feature: {}", + generated_code + ); + assert!( + !generated_code.contains(r#"feature = "feat_w""#), + "Weak dep?/feat reference should not gate: {}", + generated_code + ); + } + + #[test] + fn test_gen_deps_module_target_specific_dep() { + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!(temp_file, "[dependencies]").unwrap(); + writeln!(temp_file, r#"regular_dep = "1.0""#).unwrap(); + writeln!( + temp_file, + "\n[target.'cfg(target_os = \"linux\")'.dependencies]" + ) + .unwrap(); + writeln!(temp_file, r#"linux_dep = "1.0""#).unwrap(); + temp_file.flush().unwrap(); + + let stageleft_name = syn::Ident::new("stageleft", Span::call_site()); + let deps_module = gen_deps_module(stageleft_name, temp_file.path()); + let generated_code = quote::quote!(#deps_module).to_string(); + + assert!( + generated_code.contains("pub use linux_dep"), + "Target-specific dep should be re-exported: {}", + generated_code + ); + assert!( + generated_code.contains(r#"# [cfg (target_os = "linux")] pub use linux_dep"#), + "Target-specific dep should be gated by the target cfg: {}", + generated_code + ); + } + + #[test] + fn test_gen_deps_module_target_specific_optional_dep() { + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!( + temp_file, + "[target.'cfg(target_os = \"linux\")'.dependencies]" + ) + .unwrap(); + writeln!( + temp_file, + r#"procfs = {{ version = "0.17", optional = true }}"# + ) + .unwrap(); + writeln!(temp_file, "\n[features]").unwrap(); + writeln!(temp_file, r#"runtime_measure = ["dep:procfs"]"#).unwrap(); + temp_file.flush().unwrap(); + + let stageleft_name = syn::Ident::new("stageleft", Span::call_site()); + let deps_module = gen_deps_module(stageleft_name, temp_file.path()); + let generated_code = quote::quote!(#deps_module).to_string(); + + assert!( + generated_code.contains( + r#"# [cfg (all (target_os = "linux" , any (feature = "runtime_measure")))] pub use procfs"# + ), + "Optional target-specific dep should be gated by both the target cfg and feature: {}", + generated_code + ); + } + + #[test] + fn test_gen_deps_module_dep_in_main_and_target_table() { + // A dep declared unconditionally in [dependencies] and also in a target table + // should be re-exported unconditionally. + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!(temp_file, "[dependencies]").unwrap(); + writeln!(temp_file, r#"tokio = "1.0""#).unwrap(); + writeln!(temp_file, "\n[target.'cfg(unix)'.dependencies]").unwrap(); + writeln!( + temp_file, + r#"tokio = {{ version = "1.0", features = ["fs"] }}"# + ) + .unwrap(); + temp_file.flush().unwrap(); + + let stageleft_name = syn::Ident::new("stageleft", Span::call_site()); + let deps_module = gen_deps_module(stageleft_name, temp_file.path()); + let generated_code = quote::quote!(#deps_module).to_string(); + + assert!( + generated_code.contains("pub use tokio"), + "Dep should be re-exported: {}", + generated_code + ); + assert!( + !generated_code.contains("cfg"), + "Dep available unconditionally should not be cfg-gated: {}", + generated_code + ); + assert_eq!( + generated_code.matches("pub use tokio").count(), + 1, + "Dep should only be re-exported once: {}", + generated_code + ); + } + + #[test] + fn test_gen_deps_module_dep_in_multiple_target_tables() { + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!(temp_file, "[target.'cfg(unix)'.dependencies]").unwrap(); + writeln!(temp_file, r#"platform_dep = "1.0""#).unwrap(); + writeln!(temp_file, "\n[target.'cfg(windows)'.dependencies]").unwrap(); + writeln!(temp_file, r#"platform_dep = "1.0""#).unwrap(); + temp_file.flush().unwrap(); + + let stageleft_name = syn::Ident::new("stageleft", Span::call_site()); + let deps_module = gen_deps_module(stageleft_name, temp_file.path()); + let generated_code = quote::quote!(#deps_module).to_string(); + + assert!( + generated_code.contains(r#"# [cfg (any (unix , windows))] pub use platform_dep"#), + "Dep in multiple target tables should be gated by any() of the target cfgs: {}", + generated_code + ); + assert_eq!( + generated_code.matches("pub use platform_dep").count(), + 1, + "Dep should only be re-exported once: {}", + generated_code + ); + } + + #[test] + fn test_gen_deps_module_no_dependencies_table() { + let mut temp_file = NamedTempFile::new().unwrap(); + writeln!(temp_file, "[package]").unwrap(); + writeln!(temp_file, r#"name = "empty_crate""#).unwrap(); + temp_file.flush().unwrap(); + + let stageleft_name = syn::Ident::new("stageleft", Span::call_site()); + let deps_module = gen_deps_module(stageleft_name, temp_file.path()); + let generated_code = quote::quote!(#deps_module).to_string(); + + assert!( + !generated_code.contains("pub use"), + "No deps should be re-exported: {}", + generated_code + ); + } }