Take default-features into account (#30)

* Add tests for old behaviour Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Refactor Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Refactor Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * More refactor Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Add tests and cleanup Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * fmt Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ggwpez · Aug 28, 2023 · 22e04c5 · 22e04c5
1 parent 69f0a66
commit 22e04c5
Show file tree

Hide file tree

Showing 10 changed files with 10,848 additions and 4,026 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "zepter"
-version = "0.11.0"
+version = "0.11.1"
 edition = "2021"
 authors = [ "Oliver Tale-Yazdi" ]
 description = "Analyze, Fix and Format features in your Rust workspace."

diff --git a/src/cmd/lint.rs b/src/cmd/lint.rs
@@ -341,6 +341,7 @@ impl PropagateFeatureCmd {
 		let allowed_dir = allowed_dir.parent().unwrap();
 		let feature = self.feature.clone();
 		let meta = self.cargo_args.load_metadata().expect("Loads metadata");
+		let dag = build_feature_dag(&meta, &meta.packages);
 		let pkgs = meta.packages.iter().collect::<Vec<_>>();
 		let mut to_check = pkgs.clone();
 		if !self.packages.is_empty() {
@@ -362,11 +363,8 @@ impl PropagateFeatureCmd {
 		let mut propagate_missing = BTreeMap::<CrateId, BTreeSet<RenamedPackage>>::new();
 		// (Crate that missing the feature) -> (Dependency that has it)
 		let mut feature_missing = BTreeMap::<CrateId, BTreeSet<RenamedPackage>>::new();
-		// Crate that has the feature but does not need it.
-		let mut feature_maybe_unused = BTreeSet::<CrateId>::new();
 
 		for pkg in to_check.iter() {
-			let mut feature_used = false;
 			// TODO that it does not enable other features.
 
 			for dep in pkg.dependencies.iter() {
@@ -375,38 +373,50 @@ impl PropagateFeatureCmd {
 				let Some(dep) = resolve_dep(pkg, dep, &meta) else {
 					// Either outside workspace or not resolved, possibly due to not being used at
 					// all because of the target or whatever.
-					feature_used = true;
 					continue
 				};
 
-				if dep.pkg.features.contains_key(&feature) {
-					match pkg.features.get(&feature) {
-						None =>
-							if self.left_side_feature_missing == MuteSetting::Error {
-								feature_missing.entry(pkg.id.to_string()).or_default().insert(dep);
-							},
-						Some(enabled) => {
-							let want_opt = format!("{}?/{}", dep.name(), feature);
-							let want_req = format!("{}/{}", dep.name(), feature);
-							// TODO check that optional deps are only enabled as optional unless
-							// overwritten with `--feature-enables-dep`.
-
-							if !enabled.contains(&want_opt) && !enabled.contains(&want_req) {
-								propagate_missing
-									.entry(pkg.id.to_string())
-									.or_default()
-									.insert(dep);
-							} else {
-								// All ok
-								feature_used = true;
-							}
-						},
+				if !dep.pkg.features.contains_key(&feature) {
+					continue
+				}
+				if pkg.features.get(&feature).is_none() {
+					if self.left_side_feature_missing == MuteSetting::Error {
+						feature_missing.entry(pkg.id.to_string()).or_default().insert(dep);
 					}
+					continue
 				}
-			}
 
-			if !feature_used && pkg.features.contains_key(&feature) {
-				feature_maybe_unused.insert(pkg.id.to_string());
+				// TODO check that optional deps are only enabled as optional unless
+				// overwritten with `--feature-enables-dep`.
+				let target = CrateAndFeature(dep.pkg.id.repr.clone(), feature.clone());
+				let want_opt = CrateAndFeature(format!("{}?", &pkg.id), feature.clone());
+				let want_req = CrateAndFeature(pkg.id.repr.clone(), feature.clone());
+
+				if dag.adjacent(&want_opt, &target) || dag.adjacent(&want_req, &target) {
+					// Easy case, all good.
+					continue
+				}
+				let default_entrypoint = CrateAndFeature(pkg.id.repr.clone(), "#entrypoint".into());
+				let sub_dag = dag.sub(|CrateAndFeature(p, f)| {
+					(p == &pkg.id.repr && f == "#entrypoint") || (p == &dep.pkg.id.repr)
+				});
+				if let Some(p) = sub_dag.any_path(&default_entrypoint, &target) {
+					// Easy case, all good.
+					log::debug!(
+						"Reachable from the default entrypoint: {:?} vis {:?}",
+						target,
+						p.0
+					);
+					continue
+				}
+				// Now the more complicated case where `pkg/F -> dep/G .. -> dep/F`. So to say a
+				// multi-hop internal transitive propagation of the feature on the dependency side.
+				/*let sub_dag = dag.sub(|CrateAndFeature(p, f)| {
+					(p == &dep.pkg.id.repr)
+				});*/
+				//panic!("Not reachable: sub_dag:\n\n{:#?}\n\n", dag.edges);
+
+				propagate_missing.entry(pkg.id.to_string()).or_default().insert(dep);
 			}
 		}
 		let faulty_crates: BTreeSet<CrateId> = propagate_missing
@@ -640,6 +650,15 @@ fn build_feature_dag(meta: &Metadata, pkgs: &[Package]) -> Dag<CrateAndFeature>
 					CrateAndFeature(pkg.id.to_string(), "default".into()),
 					CrateAndFeature(dep.name.clone(), "default".into()),
 				);
+
+				let Some(dep_id) = resolve_dep(pkg, dep, meta) else { continue };
+
+				// Hacky…
+				dag.add_edge(
+					CrateAndFeature(pkg.id.to_string(), "#entrypoint".into()),
+					CrateAndFeature(dep_id.pkg.id.repr.clone(), "default".into()),
+				);
+				log::debug!("Adding default entrypoint for {} on {}", dep.name, pkg.name);
 			}
 			for feature in &dep.features {
 				dag.add_edge(

diff --git a/src/dag.rs b/src/dag.rs
@@ -139,13 +139,18 @@ where
 		self.edges.entry(node).or_default();
 	}
 
-	/// Whether `from` is directly connected to `to`.
+	/// Whether `from` is directly adjacent to `to`.
 	///
 	/// *Directly* means with via an edge.
-	pub fn connected(&self, from: &T, to: &T) -> bool {
+	pub fn adjacent(&self, from: &T, to: &T) -> bool {
 		self.edges.get(from).map_or(false, |v| v.contains(to))
 	}
 
+	/// Whether `from` is reachable to `to` via.
+	pub fn reachable(&self, from: &T, to: &T) -> bool {
+		self.any_path(from, to).is_some()
+	}
+
 	/// Whether `from` appears on the lhs of the edge relation.
 	///
 	/// Aka: Whether `self` has any dependencies nodes.
@@ -171,6 +176,16 @@ where
 		Self { edges }
 	}
 
+	pub fn sub(&self, pred: impl Fn(&T) -> bool) -> Self {
+		let mut edges = BTreeMap::new();
+		for (k, v) in self.edges.iter() {
+			if pred(k) {
+				edges.insert(k.clone(), v.clone());
+			}
+		}
+		Self { edges }
+	}
+
 	/// Get get a ref to the a LHS node.
 	pub fn lhs_node(&self, from: &T) -> Option<&T> {
 		self.edges.get_key_value(from).map(|(k, _)| k)

diff --git a/tests/integration/polkadot/issue-7261.yaml b/tests/integration/polkadot/issue-7261.yaml
@@ -8,9 +8,7 @@ cases:
       feature 'std'
         must propagate to:
           polkadot-runtime-parachains
-          serde_json
           beefy-primitives (renamed from sp-consensus-beefy)
           sp-mmr-primitives
-          sp-trie
-    Found 5 issues and fixed 5.
-  diff: "diff --git runtime/test-runtime/Cargo.toml runtime/test-runtime/Cargo.toml\nindex 6d38a0..8f3418 100644\n--- runtime/test-runtime/Cargo.toml\n+++ runtime/test-runtime/Cargo.toml\n@@ -130,0 +131,5 @@ std = [\n+\t\"polkadot-runtime-parachains/std\",\n+\t\"serde_json/std\",\n+\t\"beefy-primitives/std\",\n+\t\"sp-mmr-primitives/std\",\n+\t\"sp-trie/std\"\n"
+    Found 3 issues and fixed 3.
+  diff: "diff --git runtime/test-runtime/Cargo.toml runtime/test-runtime/Cargo.toml\nindex 6d38a0..972533 100644\n--- runtime/test-runtime/Cargo.toml\n+++ runtime/test-runtime/Cargo.toml\n@@ -130,0 +131,3 @@ std = [\n+\t\"polkadot-runtime-parachains/std\",\n+\t\"beefy-primitives/std\",\n+\t\"sp-mmr-primitives/std\"\n"