Skip to content

Commit cf1f5ee

Browse files
committed
cargo-rail: meilisearch PR revealed a bug in Meilisearch and in cargo-rail; fixing 'undeclard features detection' to avoid 'borrowed features' sneaking through. added 'auto-fix' undeclared features w/ warning for manual. This is a big win.
1 parent 1a73394 commit cf1f5ee

File tree

8 files changed

+839
-6
lines changed

8 files changed

+839
-6
lines changed

docs/config.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ Controls workspace dependency unification behavior. All options are optional wit
107107
| `remove_unused` | `bool` | `true` | Automatically remove unused dependencies during unification. Requires `detect_unused = true`. |
108108
| `prune_dead_features` | `bool` | `true` | Remove features that are never enabled in the resolved dependency graph across all targets. Only prunes empty no-ops (`feature = []`). Features with actual dependencies are preserved. |
109109
| `preserve_features` | `string[]` | `[]` | Features to preserve from dead feature pruning. Supports glob patterns (e.g., `"unstable-*"`, `"bench*"`). Use this to keep features intended for future use or external consumers. |
110+
| `detect_undeclared_features` | `bool` | `true` | Detect crates that rely on Cargo's feature unification to "borrow" features from other workspace members. These crates will fail when built standalone after unification. Reports as warnings (or auto-fixes if `fix_undeclared_features` is enabled). |
111+
| `fix_undeclared_features` | `bool` | `true` | Auto-fix undeclared feature dependencies by adding missing features to each crate's Cargo.toml. Produces a cleaner graph where standalone builds work correctly. Requires `detect_undeclared_features = true`. |
110112
| `max_backups` | `usize` | `3` | Maximum number of backup archives to keep. Older backups are automatically cleaned up after successful operations. Set to `0` to disable backup creation entirely. |
111113

112114
**Example:**
@@ -119,6 +121,8 @@ detect_unused = true
119121
remove_unused = true
120122
prune_dead_features = true
121123
preserve_features = ["future-api", "unstable-*"] # Keep these from pruning
124+
detect_undeclared_features = true # Catch borrowed features
125+
fix_undeclared_features = true # Auto-fix them (default)
122126
max_backups = 5
123127
```
124128

src/cargo/manifest_writer.rs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,4 +263,89 @@ impl ManifestWriter {
263263

264264
Ok(())
265265
}
266+
267+
/// Add features to an existing dependency in a member's Cargo.toml
268+
///
269+
/// This is used to fix undeclared feature dependencies - when a crate relies on
270+
/// Cargo's feature unification to "borrow" features from other workspace members.
271+
///
272+
/// # Arguments
273+
/// * `member_toml_path` - Path to the member's Cargo.toml
274+
/// * `dep_name` - Name of the dependency to update
275+
/// * `dep_kind` - Type of dependency (Normal, Dev, Build)
276+
/// * `target` - Optional target platform constraint (e.g., "cfg(unix)")
277+
/// * `features_to_add` - Features to add to the dependency
278+
pub fn add_features(
279+
&self,
280+
member_toml_path: &Path,
281+
dep_name: &str,
282+
dep_kind: DepKind,
283+
target: Option<&str>,
284+
features_to_add: &[String],
285+
) -> RailResult<()> {
286+
// Read member Cargo.toml
287+
let mut doc = manifest_ops::read_toml_file(member_toml_path)?;
288+
289+
// Get section name from kind
290+
let kind_section = self.dep_kind_to_section(dep_kind);
291+
292+
// Handle target-specific vs regular sections
293+
let dep_item = if let Some(target_cfg) = target {
294+
// Target-specific: look in [target.'cfg(...)'.dependencies]
295+
let path = format!("target.{}.{}", target_cfg, kind_section);
296+
let table = manifest_ops::get_or_create_table(&mut doc, &path)?;
297+
table.get_mut(dep_name)
298+
} else {
299+
// Regular section
300+
doc
301+
.get_mut(kind_section)
302+
.and_then(|t| t.as_table_mut())
303+
.and_then(|t| t.get_mut(dep_name))
304+
};
305+
306+
let Some(dep_item) = dep_item else {
307+
// Dependency not found in manifest - skip silently
308+
// This can legitimately happen with:
309+
// - Renamed dependencies (package = "other-name")
310+
// - Target-specific deps where the target section doesn't exist
311+
// - Dependencies that were already converted to workspace = true
312+
return Ok(());
313+
};
314+
315+
// Get existing features (if any)
316+
let mut existing_features: Vec<String> = manifest_ops::extract_features(dep_item).unwrap_or_default();
317+
318+
// Add new features (dedup)
319+
for feature in features_to_add {
320+
if !existing_features.contains(feature) {
321+
existing_features.push(feature.clone());
322+
}
323+
}
324+
325+
// Sort for consistency
326+
existing_features.sort();
327+
328+
// Update the dependency entry
329+
// If it's a simple string like `serde = "1.0"`, we need to convert it to a table
330+
if dep_item.is_str() {
331+
// Convert simple string (e.g., `dep = "1.0"`) to inline table with features
332+
let version = dep_item
333+
.as_str()
334+
.expect("dep_item.is_str() was true but as_str() returned None")
335+
.to_string();
336+
let mut inline_table = toml_edit::InlineTable::new();
337+
inline_table.insert("version", toml_edit::Value::from(version));
338+
inline_table.insert("features", manifest_ops::build_feature_array(&existing_features));
339+
*dep_item = toml_edit::Item::Value(toml_edit::Value::InlineTable(inline_table));
340+
} else {
341+
// Already a table, just update features
342+
manifest_ops::set_features(dep_item, existing_features)?;
343+
}
344+
345+
// Format and write
346+
self.formatter.format_manifest(&mut doc)?;
347+
manifest_ops::write_toml_file(member_toml_path, &doc)?;
348+
349+
Ok(())
350+
}
266351
}

src/cargo/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,6 @@ pub use multi_target_metadata::{ComputedMsrv, FragmentedTransitive, MsrvSourceUs
2626
pub use unify_analyzer::UnifyAnalyzer;
2727
pub use unify_report::UnifyReport;
2828
pub use unify_types::{
29-
DuplicateCleanup, IssueSeverity, MemberEdit, OptionalFeature, PrunedFeature, TransitivePin, UnificationPlan,
30-
UnifiedDep, UnifyIssue, UnusedDep, UnusedReason, ValidationResult, VersionMismatch,
29+
DuplicateCleanup, IssueSeverity, MemberEdit, OptionalFeature, PrunedFeature, TransitivePin, UndeclaredFeature,
30+
UnificationPlan, UnifiedDep, UnifyIssue, UnusedDep, UnusedReason, ValidationResult, VersionMismatch,
3131
};

src/cargo/unify_analyzer.rs

Lines changed: 144 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ use crate::cargo::{
99
unify::version_utils::{find_major_version_conflicts, is_exact_pin, versions_compatible},
1010
unify::{CandidateIterator, FeaturePruner, TransitivePlanner, UnusedDepFinder},
1111
unify_types::{
12-
DuplicateCleanup, IssueSeverity, MemberEdit, UnificationPlan, UnifiedDep, UnifyIssue, ValidationResult,
13-
VersionMismatch,
12+
DuplicateCleanup, IssueSeverity, MemberEdit, UndeclaredFeature, UnificationPlan, UnifiedDep, UnifyIssue,
13+
ValidationResult, VersionMismatch,
1414
},
1515
};
1616
use crate::config::{ExactPinHandling, MajorVersionConflict, UnifyConfig};
@@ -473,6 +473,56 @@ impl UnifyAnalyzer {
473473
member_edits.entry(crate_name).or_default().extend(edits);
474474
}
475475

476+
// === Detect undeclared features ===
477+
// Find cases where a crate uses features that it didn't declare in Cargo.toml
478+
// This happens when Cargo's feature unification "borrows" features from other members
479+
let undeclared_features = if self.config.detect_undeclared_features {
480+
progress!("Checking for undeclared feature dependencies...");
481+
self.detect_undeclared_features()
482+
} else {
483+
Vec::new()
484+
};
485+
486+
// Generate fixes or warnings for undeclared features
487+
if self.config.fix_undeclared_features && !undeclared_features.is_empty() {
488+
progress!(
489+
"Generating fixes for {} undeclared feature issues...",
490+
undeclared_features.len()
491+
);
492+
for uf in &undeclared_features {
493+
let edit = MemberEdit::AddFeatures {
494+
dep_name: uf.dep_name.clone(),
495+
dep_kind: uf.dep_kind,
496+
target: uf.target.clone(),
497+
features_to_add: uf.undeclared_features.clone(),
498+
};
499+
member_edits.entry(uf.member.clone()).or_default().push(edit);
500+
}
501+
} else {
502+
// Add issues for undeclared features (warn mode)
503+
for uf in &undeclared_features {
504+
issues.push(UnifyIssue {
505+
dep_name: uf.dep_name.clone(),
506+
severity: IssueSeverity::Warning,
507+
message: format!(
508+
"{} uses {} features [{}] but only declares [{}]. \
509+
This crate relies on feature unification from other workspace members. \
510+
After unification, standalone builds will fail. \
511+
Fix: Add the missing features to {}'s Cargo.toml.",
512+
uf.member,
513+
uf.dep_name,
514+
uf.undeclared_features.join(", "),
515+
if uf.declared_features.is_empty() {
516+
"none".to_string()
517+
} else {
518+
uf.declared_features.join(", ")
519+
},
520+
uf.member,
521+
),
522+
});
523+
}
524+
}
525+
476526
// Run validation
477527
let validation_results = self.validate_targets()?;
478528

@@ -489,6 +539,7 @@ impl UnifyAnalyzer {
489539
optional_features,
490540
version_mismatches,
491541
unused_deps,
542+
undeclared_features,
492543
})
493544
}
494545

@@ -534,4 +585,95 @@ impl UnifyAnalyzer {
534585
)
535586
}
536587
}
588+
589+
/// Detect dependencies where resolved features exceed declared features
590+
///
591+
/// This catches cases where a crate relies on Cargo's feature unification to
592+
/// "borrow" features from other workspace members. After workspace dependency
593+
/// unification, standalone builds (cargo test -p <crate>) will fail because
594+
/// the borrowed features are no longer available.
595+
///
596+
/// Example:
597+
/// - crate A declares: `backoff = "0.4"` (no features)
598+
/// - crate B declares: `backoff = { version = "0.4", features = ["tokio"] }`
599+
/// - crate A uses `backoff::future::retry()` which requires `features = ["futures"]`
600+
/// - When building the whole workspace, A gets `futures` from B via unification
601+
/// - After unification, A's standalone build fails
602+
fn detect_undeclared_features(&self) -> Vec<UndeclaredFeature> {
603+
let mut undeclared = Vec::new();
604+
605+
// Common features that are typically not actionable:
606+
// - "default", "std" - usually implicit
607+
// - Internal features like "*_backend", "*_impl" - implementation details
608+
let should_skip_feature = |f: &str| -> bool {
609+
f == "default" || f == "std" || f == "alloc" || f.ends_with("_backend") || f.ends_with("_impl")
610+
};
611+
612+
// Build a map of what each member explicitly declares for each dependency
613+
// We want to find cases where member A would need a feature that ONLY comes from member B
614+
let mut member_declared_features: HashMap<String, HashMap<String, HashSet<String>>> = HashMap::new();
615+
616+
for member in &self.manifests.members {
617+
let mut dep_features: HashMap<String, HashSet<String>> = HashMap::new();
618+
for (dep_key, usage) in &member.dependencies {
619+
let mut feats: HashSet<String> = usage.unconditional_features.iter().cloned().collect();
620+
// If default_features = true (default), add "default"
621+
if usage.default_features {
622+
feats.insert("default".to_string());
623+
}
624+
dep_features.insert(dep_key.name.to_string(), feats);
625+
}
626+
member_declared_features.insert(member.package_name.clone(), dep_features);
627+
}
628+
629+
// Compute the unified features (what will end up in workspace.dependencies)
630+
// This is the UNION of all declared features across all members
631+
let mut unified_features: HashMap<String, HashSet<String>> = HashMap::new();
632+
for dep_features in member_declared_features.values() {
633+
for (dep_name, features) in dep_features {
634+
unified_features
635+
.entry(dep_name.clone())
636+
.or_default()
637+
.extend(features.iter().cloned());
638+
}
639+
}
640+
641+
// Now check: for each member, are there features in unified that it doesn't declare?
642+
// These are features "borrowed" from other workspace members
643+
for member in &self.manifests.members {
644+
for (dep_key, usage) in &member.dependencies {
645+
let Some(unified) = unified_features.get(&*dep_key.name) else {
646+
continue;
647+
};
648+
649+
// Get declared features from this member's Cargo.toml
650+
let mut declared: HashSet<String> = usage.unconditional_features.iter().cloned().collect();
651+
if usage.default_features {
652+
declared.insert("default".to_string());
653+
}
654+
655+
// Find features that are unified but not declared by this member
656+
// These are features this member will lose access to after unification
657+
let borrowed: Vec<String> = unified
658+
.difference(&declared)
659+
.filter(|f| !should_skip_feature(f))
660+
.cloned()
661+
.collect();
662+
663+
if !borrowed.is_empty() {
664+
undeclared.push(UndeclaredFeature {
665+
member: member.package_name.clone(),
666+
dep_name: dep_key.name.to_string(),
667+
undeclared_features: borrowed,
668+
declared_features: declared.into_iter().collect(),
669+
resolved_features: unified.iter().cloned().collect(),
670+
dep_kind: usage.kind,
671+
target: usage.target.clone(),
672+
});
673+
}
674+
}
675+
}
676+
677+
undeclared
678+
}
537679
}

0 commit comments

Comments
 (0)