cargo-rail: fixing the last real bug in unify before v1?!

Author: loadingalias

src/cargo/manifest_analyzer.rs

@@ -33,10 +33,13 @@ impl DepKey {
   }
 }
 
-// Only compare by name, not renamed_from
+// Compare by both name AND renamed_from to distinguish:
+// - getrandom (direct dependency)
+// - old_getrandom (renamed from getrandom, i.e., package = "getrandom")
+// These are different dependencies that should NOT have their features merged.
 impl PartialEq for DepKey {
   fn eq(&self, other: &Self) -> bool {
-    self.name == other.name
+    self.name == other.name && self.renamed_from == other.renamed_from
   }
 }
 
@@ -45,6 +48,7 @@ impl Eq for DepKey {}
 impl std::hash::Hash for DepKey {
   fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
     self.name.hash(state);
+    self.renamed_from.hash(state);
   }
 }
 
@@ -56,7 +60,10 @@ impl PartialOrd for DepKey {
 
 impl Ord for DepKey {
   fn cmp(&self, other: &Self) -> std::cmp::Ordering {
-    self.name.cmp(&other.name)
+    match self.name.cmp(&other.name) {
+      std::cmp::Ordering::Equal => self.renamed_from.cmp(&other.renamed_from),
+      other => other,
+    }
   }
 }
 
@@ -164,31 +171,17 @@ impl ManifestAnalyzer {
     }
 
     // Build usage index
-    // When merging dependencies with same name, preserve renamed_from if any
+    // DepKey now includes renamed_from in equality, so:
+    // - `getrandom` (direct) and `old_getrandom` (package = "getrandom") are separate keys
+    // - This prevents feature confusion between renamed and non-renamed deps
     let mut usage_index: HashMap<DepKey, Vec<DepUsage>> = HashMap::new();
-    let mut renamed_info: HashMap<String, Option<String>> = HashMap::new();
 
     for member in &parsed_members {
       for (dep_key, usage) in &member.dependencies {
-        // Track if any usage of this dep is renamed
-        if dep_key.renamed_from.is_some() {
-          renamed_info.insert(dep_key.name.clone(), dep_key.renamed_from.clone());
-        }
         usage_index.entry(dep_key.clone()).or_default().push(usage.clone());
       }
     }
 
-    // Update keys with renamed_from info if any usage was renamed
-    let usage_index: HashMap<DepKey, Vec<DepUsage>> = usage_index
-      .into_iter()
-      .map(|(mut key, usages)| {
-        if let Some(renamed_from) = renamed_info.get(&key.name) {
-          key.renamed_from = renamed_from.clone();
-        }
-        (key, usages)
-      })
-      .collect();
-
     // Pre-compute usage counts for O(1) lookup (20-30% speedup)
     let usage_counts: HashMap<DepKey, usize> = usage_index
       .iter()

src/cargo/unify_analyzer.rs

@@ -688,8 +688,12 @@ impl UnifyAnalyzer {
           .cloned()
           .collect();
 
+        // For renamed dependencies (package = "..."), use the ALIAS name for manifest editing
+        // e.g., for `old_getrandom = { package = "getrandom", ... }`, use "old_getrandom"
+        let manifest_dep_name = dep_key.renamed_from.clone().unwrap_or_else(|| dep_key.name.clone());
+
         let edit = MemberEdit::UseWorkspace {
-          dep_name: dep_key.name.clone(),
+          dep_name: manifest_dep_name,
           dep_kind: usage.kind,
           target: usage.target.clone(), // Preserve target for correct section
           local_features,

tests/integration/test_unify_comprehensive.rs

@@ -242,14 +242,16 @@ root = "."
 }
 
 // ============================================================================
-// SCENARIO 7: Renamed Dependencies (Hard Blocker)
+// SCENARIO 7: Renamed Dependencies (Treated Separately)
 // ============================================================================
 
 #[test]
 fn test_unify_renamed_dependencies_hard_blocker() -> Result<()> {
   let workspace = TestWorkspace::new()?;
 
   // Create crates with renamed dependency
+  // With the bug fix, renamed deps (package = "...") are now properly separated
+  // from direct deps of the same package. This prevents feature confusion.
   workspace.add_crate("crate-a", "0.1.0", &[("serde", r#""1.0""#)])?;
 
   // Manually create crate-b with renamed serde
@@ -276,37 +278,39 @@ serde_crate = { package = "serde", version = "1.0" }
 
   workspace.commit("Add crates with renamed dependency")?;
 
-  // Configure rail.toml (allow_renamed = false by default)
+  // Configure rail.toml (include_renamed = false by default)
   std::fs::write(
     workspace.path.join("rail.toml"),
     r#"[workspace]
 root = "."
 
 [unify]
-allow_renamed = false
+include_renamed = false
 "#,
   )?;
 
-  // Run analyze - should show Hard blocker
+  // Run analyze - with the fix, renamed deps are now treated separately
+  // Since each version of serde (direct vs renamed) only has 1 user,
+  // neither qualifies for unification (needs 2+ users)
   let analyze_output = run_cargo_rail(&workspace.path, &["rail", "unify", "--check"])?;
   let analyze_stdout = String::from_utf8_lossy(&analyze_output.stdout);
 
+  // Should show no unification opportunities since each has only 1 user
   assert!(
-    analyze_stdout.contains("BLOCKING") || analyze_stdout.contains("Renamed"),
-    "Should show blocking issue for renamed dependency.\nOutput:\n{}",
+    analyze_stdout.contains("No unification opportunities") || analyze_stdout.contains("No dependencies to unify"),
+    "Should show no unification opportunities when deps are properly separated.\nOutput:\n{}",
     analyze_stdout
   );
 
-  // Run apply - should FAIL
+  // Run apply - should succeed (no changes needed)
   let apply_output = run_cargo_rail(&workspace.path, &["rail", "unify"])?;
   let apply_stdout = String::from_utf8_lossy(&apply_output.stdout);
-  let apply_stderr = String::from_utf8_lossy(&apply_output.stderr);
 
+  // Should indicate no changes
   assert!(
-    !apply_output.status.success(),
-    "Apply should fail with renamed dependency.\nstdout:\n{}\nstderr:\n{}",
-    apply_stdout,
-    apply_stderr
+    apply_stdout.contains("No unification opportunities") || apply_stdout.contains("No dependencies to unify"),
+    "Apply should indicate no changes needed.\nstdout:\n{}",
+    apply_stdout
   );
 
   // Check that members were NOT converted to workspace inheritance
@@ -322,7 +326,7 @@ allow_renamed = false
   // Should NOT have "serde = { workspace = true }"
   assert!(
     !crate_a_member.contains("serde = { workspace = true }") && !crate_a_member.contains("serde = {workspace=true}"),
-    "crate-a should not use workspace = true for serde (apply failed, so no conversion).\ncrate-a Cargo.toml:\n{}",
+    "crate-a should not use workspace = true for serde.\ncrate-a Cargo.toml:\n{}",
     crate_a_member
   );