cargo-rail: fixing all-features issues; transitive chains aren't working

Author: loadingalias

src/cargo/manifest.rs

@@ -292,29 +292,38 @@ impl CargoTransform {
     for unified in unified_deps {
       // Determine which table to write to based on target
       let deps_table = if let Some(ref target) = unified.target {
-        // Platform-specific dependency: write to [target.'<target>'.dependencies]
-        let target_key = format!("target.'{}'", target);
-
-        // Ensure target section exists in workspace
+        // Platform-specific dependency: write to [workspace.target.'<target>'.dependencies]
+        // IMPORTANT: We need to create workspace.target (as a table), then target.'cfg(...)' as a subtable
         let workspace = doc["workspace"]
           .as_table_mut()
           .ok_or_else(|| RailError::message("[workspace] is not a table"))?;
 
-        if !workspace.contains_key(&target_key) {
-          workspace[&target_key] = table();
+        // Ensure [workspace.target] table exists
+        if !workspace.contains_key("target") {
+          workspace["target"] = table();
+        }
+
+        let target_table = workspace["target"]
+          .as_table_mut()
+          .ok_or_else(|| RailError::message("[workspace.target] is not a table"))?;
+
+        // Create the cfg-specific section within target: [workspace.target.'cfg(...)']
+        if !target_table.contains_key(target) {
+          target_table[target] = table();
         }
 
-        let target_section = workspace[&target_key]
+        let cfg_section = target_table[target]
           .as_table_mut()
-          .ok_or_else(|| RailError::message(format!("[workspace.{}] is not a table", target_key)))?;
+          .ok_or_else(|| RailError::message(format!("[workspace.target.'{}'] is not a table", target)))?;
 
-        if !target_section.contains_key("dependencies") {
-          target_section["dependencies"] = table();
+        // Create dependencies section: [workspace.target.'cfg(...)'.dependencies]
+        if !cfg_section.contains_key("dependencies") {
+          cfg_section["dependencies"] = table();
         }
 
-        target_section["dependencies"]
+        cfg_section["dependencies"]
           .as_table_mut()
-          .ok_or_else(|| RailError::message(format!("[workspace.{}.dependencies] is not a table", target_key)))?
+          .ok_or_else(|| RailError::message(format!("[workspace.target.'{}'.dependencies] is not a table", target)))?
       } else {
         // Regular dependency: write to [workspace.dependencies]
         let workspace = doc["workspace"]

src/cargo/unify/collector.rs

@@ -10,7 +10,10 @@ use std::collections::{HashMap, HashSet};
 /// Uses RESOLVED features from the dependency graph, not declared features.
 /// This ensures we capture features enabled transitively and provides the
 /// TRUE feature union across the workspace.
-pub fn collect_dependencies(metadata: &WorkspaceMetadata) -> Vec<DependencyInstance> {
+///
+/// The `use_all_features` parameter indicates whether metadata was collected with --all-features,
+/// which affects how we classify unidentified feature sources.
+pub fn collect_dependencies(metadata: &WorkspaceMetadata, use_all_features: bool) -> Vec<DependencyInstance> {
   let mut instances = Vec::new();
 
   for pkg in metadata.list_crates() {
@@ -40,7 +43,7 @@ pub fn collect_dependencies(metadata: &WorkspaceMetadata) -> Vec<DependencyInsta
       };
 
       // Track feature provenance: WHY is each feature enabled?
-      let feature_provenance = determine_feature_provenance(&features, dep, &pkg.name, metadata);
+      let feature_provenance = determine_feature_provenance(&features, dep, &pkg.name, metadata, use_all_features);
 
       // Detect if this is a proc-macro crate
       // Proc-macros are build-time only and have different optimization strategies
@@ -127,32 +130,45 @@ fn get_member_workspace_deps(manifest_path: &cargo_metadata::camino::Utf8Path) -
 /// 1. Direct declaration in member's Cargo.toml
 /// 2. Default features
 /// 3. Target-specific
-/// 4. Transitive or --all-features
+/// 4. Transitive or --all-features (depending on use_all_features parameter)
 fn determine_feature_provenance(
   resolved_features: &[String],
   dep: &cargo_metadata::Dependency,
   member_name: &str,
   metadata: &WorkspaceMetadata,
+  use_all_features: bool,
 ) -> HashMap<String, FeatureSource> {
   let mut provenance = HashMap::new();
 
   // Get the actual package metadata for this dependency
   let dep_pkg = metadata.get_package(&dep.name);
 
+  // Build a set of "root" features that could transitively enable other features:
+  // 1. Features explicitly declared in Cargo.toml
+  // 2. Default features (if enabled)
+  let mut root_features = dep.features.to_vec();
+  if dep.uses_default_features {
+    root_features.push("default".to_string());
+  }
+
   for feature in resolved_features {
-    let source = determine_single_feature_source(feature, dep, member_name, dep_pkg);
+    let source = determine_single_feature_source(feature, dep, member_name, dep_pkg, &root_features, use_all_features);
     provenance.insert(feature.clone(), source);
   }
 
   provenance
 }
 
 /// Determine the source of a single feature
+///
+/// `root_features` are the features that were explicitly enabled (from Cargo.toml + default if enabled)
 fn determine_single_feature_source(
   feature: &str,
   dep: &cargo_metadata::Dependency,
   member_name: &str,
   dep_pkg: Option<&cargo_metadata::Package>,
+  root_features: &[String],
+  use_all_features: bool,
 ) -> FeatureSource {
   // 1. Check if declared directly in this member's Cargo.toml
   if dep.features.contains(&feature.to_string()) {
@@ -166,7 +182,7 @@ fn determine_single_feature_source(
     && let Some(pkg) = dep_pkg
     && let Some(default_features) = pkg.features.get("default")
   {
-    // Default features can reference other features
+    // Default features can reference other features directly
     if default_features.iter().any(|f| f.trim_start_matches("dep:") == feature) {
       return FeatureSource::Default;
     }
@@ -179,8 +195,52 @@ fn determine_single_feature_source(
     };
   }
 
-  // 4. Otherwise it's either transitive or from --all-features
-  // For now, we classify it as AllFeatures since we're using --all-features metadata
-  // In a future enhancement, we could trace the actual dependency chain
-  FeatureSource::AllFeatures
+  // 4. Check if it's enabled transitively through a feature dependency chain
+  // We need to trace from root_features (explicitly enabled) to this feature
+  if let Some(pkg) = dep_pkg {
+    // Try to find a path from any root feature to this feature
+    for root_feature in root_features {
+      if let Some(chain) = find_feature_chain(root_feature, feature, pkg) {
+        return FeatureSource::Transitive { through: chain };
+      }
+    }
+  }
+
+  // 5. If metadata was collected with --all-features, classify as such
+  // Otherwise, it's transitive but we couldn't trace the exact chain
+  if use_all_features {
+    FeatureSource::AllFeatures
+  } else {
+    // Transitive dependency - exact chain couldn't be determined
+    FeatureSource::Transitive { through: vec![] }
+  }
+}
+
+/// Find a chain of features from `start` to `target`
+///
+/// Returns the feature chain if found (e.g., ["perf", "perf-inline"])
+fn find_feature_chain(start: &str, target: &str, pkg: &cargo_metadata::Package) -> Option<Vec<String>> {
+  // Check if start directly enables target
+  if let Some(feature_deps) = pkg.features.get(start) {
+    for dep_feature in feature_deps {
+      let clean_name = dep_feature.trim_start_matches("dep:");
+      if clean_name == target {
+        return Some(vec![start.to_string()]);
+      }
+    }
+
+    // Check if start enables something that enables target (one level deep)
+    for dep_feature in feature_deps {
+      let intermediate = dep_feature.trim_start_matches("dep:");
+      if let Some(intermediate_deps) = pkg.features.get(intermediate) {
+        for sub_dep in intermediate_deps {
+          if sub_dep.trim_start_matches("dep:") == target {
+            return Some(vec![start.to_string(), intermediate.to_string()]);
+          }
+        }
+      }
+    }
+  }
+
+  None
 }

src/cargo/unify/config.rs

@@ -6,6 +6,9 @@ use std::collections::HashSet;
 /// Configuration for workspace unification
 #[derive(Debug, Clone)]
 pub struct UnifyConfig {
+  /// Whether metadata was collected with --all-features (default: true)
+  pub use_all_features: bool,
+
   /// Allow renamed dependencies to be unified (default: false)
   pub allow_renamed: bool,
 
@@ -28,6 +31,7 @@ pub struct UnifyConfig {
 impl Default for UnifyConfig {
   fn default() -> Self {
     Self {
+      use_all_features: true,
       allow_renamed: false,
       exclude: HashSet::new(),
       include: HashSet::new(),
@@ -41,6 +45,7 @@ impl Default for UnifyConfig {
 impl From<&RailConfig> for UnifyConfig {
   fn from(rail_config: &RailConfig) -> Self {
     Self {
+      use_all_features: rail_config.unify.use_all_features,
       allow_renamed: rail_config.unify.allow_renamed,
       exclude: rail_config.unify.exclude.iter().cloned().collect(),
       include: rail_config.unify.include.iter().cloned().collect(),

src/cargo/unify/issue_detection.rs

@@ -136,36 +136,36 @@ pub fn detect_issues(
   }
 
   // Check if all are target-specific
+  // IMPORTANT: Cargo does NOT support target-specific dependencies in [workspace.dependencies]
+  // or [workspace.target.'cfg(...)'] sections. Platform-specific deps must stay in member crates.
+  // See: https://doc.rust-lang.org/cargo/reference/workspaces.html
   if instances.iter().all(|i| i.target.is_some()) {
     let targets: Vec<_> = instances.iter().filter_map(|i| i.target.clone()).collect();
-
-    // Check if all instances use the SAME target
     let unique_targets: HashSet<_> = targets.iter().collect();
 
-    if unique_targets.len() == 1 {
-      // All instances use the same target - we CAN unify this!
-      // This is NOT an issue, it's a unification opportunity
-      // Don't return an issue here - let the normal unification flow handle it
-      // The unified dep can use [target.'cfg(...)'.dependencies]
-      return None;
-    }
-
-    // Multiple different targets - this is more complex
+    // Return Info-level issue to inform user that platform-specific deps can't be unified
     return Some(UnificationIssue {
       dep_name: dep_name.to_string(),
       issue_type: IssueType::AllTargetSpecific {
         targets: targets.clone(),
       },
       severity: IssueSeverity::Info, // Just informational
       affected_members: instances.iter().map(|i| i.member.clone()).collect(),
-      suggestion: format!(
-        "Multiple platform-specific targets detected: {}. Consider per-platform unification.",
-        unique_targets
-          .iter()
-          .map(|t| format!("'{}'", t))
-          .collect::<Vec<_>>()
-          .join(", ")
-      ),
+      suggestion: if unique_targets.len() == 1 {
+        format!(
+          "All uses are platform-specific for target {}. Cargo does not support target-specific dependencies in workspace.dependencies - these will remain in member crates.",
+          unique_targets.iter().next().unwrap()
+        )
+      } else {
+        format!(
+          "Multiple platform-specific targets detected: {}. Cargo does not support target-specific dependencies in workspace.dependencies - these will remain in member crates.",
+          unique_targets
+            .iter()
+            .map(|t| format!("'{}'", t))
+            .collect::<Vec<_>>()
+            .join(", ")
+        )
+      },
     });
   }
 

src/cargo/unify/mod.rs

@@ -79,7 +79,7 @@ impl<'a> WorkspaceUnifier<'a> {
   /// Analyze workspace and generate unification plan
   pub fn analyze(&self) -> RailResult<UnificationPlan> {
     // 1. Collect all dependency instances from workspace members
-    let dep_instances = collector::collect_dependencies(self.metadata);
+    let dep_instances = collector::collect_dependencies(self.metadata, self.config.use_all_features);
 
     // 2. Group by dependency name
     let grouped = collector::group_by_name(dep_instances, self.metadata);

src/cargo/unify/plan.rs

@@ -129,11 +129,16 @@ impl UnificationPlan {
               let transitive_chains: Vec<_> = sources
                 .iter()
                 .filter_map(|s| match s {
-                  FeatureSource::Transitive { through } => Some(through.join(" → ")),
+                  FeatureSource::Transitive { through } if !through.is_empty() => Some(through.join(" → ")),
                   _ => None,
                 })
                 .collect();
 
+              let has_transitive_unknown = sources.iter().any(|s| match s {
+                FeatureSource::Transitive { through } => through.is_empty(),
+                _ => false,
+              });
+
               // Format the provenance
               if !direct_members.is_empty() {
                 output.push_str(&format!(
@@ -149,6 +154,11 @@ impl UnificationPlan {
                   feature,
                   transitive_chains.first().unwrap()
                 ));
+              } else if has_transitive_unknown {
+                output.push_str(&format!(
+                  "       ⚠ \"{}\" - Enabled transitively (chain unknown)\n",
+                  feature
+                ));
               } else if has_all_features {
                 output.push_str(&format!("       ⚠ \"{}\" - From --all-features flag\n", feature));
               }