From ff267d688327a7765462860cf4ebf93b26a63a3c Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 10:29:38 -0400 Subject: [PATCH] refactor(xtask): split parity evidence and layers dependency checks (Sonar S3776) (#4406) Co-authored-by: Claude Sonnet 5.5 --- xtask/src/layers.rs | 53 +++++++++++++++++++++++++-------------------- xtask/src/parity.rs | 22 ++++++++++++------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/xtask/src/layers.rs b/xtask/src/layers.rs index 8ad15733a2..b961ebef66 100644 --- a/xtask/src/layers.rs +++ b/xtask/src/layers.rs @@ -182,6 +182,34 @@ impl std::fmt::Display for Violation { } } +/// Layer and testkit rules for one dependency of `c` that is itself a workspace crate. +fn check_workspace_dep(c: &Crate, class: Class, layer: u8, d: &Dep, out: &mut Vec) { + let dep_class = classify(&d.name); + // Test support is a dev-dependency only (`Class::Testkit`) and so never reaches + // the product graph a standalone crate publishes. Checking it against the + // standalone allow-list would stop `cos` using the shared oracles in its own + // tests, which is the one thing those crates exist for. + let testkit_dev = dep_class == Some(Class::Testkit) && d.kind == DepKind::Dev; + if matches!(class, Class::Standalone(_)) && !testkit_dev && !standalone_allowed(&c.name, &d.name) { + out.push(Violation::StandaloneHasWorkspaceDep { krate: c.name.clone(), dep: d.name.clone() }); + return; + } + match dep_class { + None => {} + Some(Class::Testkit) if d.kind != DepKind::Dev && class != Class::Testkit => { + out.push(Violation::TestkitAsNormalDep { krate: c.name.clone(), dep: d.name.clone() }); + } + Some(Class::Testkit) => {} + Some(dc) => { + let to = dc.layer().unwrap_or(u8::MAX); + let ok = to < layer || (to == layer && sideways_allowed(&c.name, &d.name)); + if !ok { + out.push(Violation::Upward { krate: c.name.clone(), dep: d.name.clone(), from: layer, to: to.min(8), kind: d.kind }); + } + } + } +} + pub fn check(crates: &[Crate]) -> Vec { let mut out = Vec::new(); for c in crates { @@ -198,30 +226,7 @@ pub fn check(crates: &[Crate]) -> Vec { continue; } if d.workspace { - let dep_class = classify(&d.name); - // Test support is a dev-dependency only (`Class::Testkit`) and so never reaches - // the product graph a standalone crate publishes. Checking it against the - // standalone allow-list would stop `cos` using the shared oracles in its own - // tests, which is the one thing those crates exist for. - let testkit_dev = dep_class == Some(Class::Testkit) && d.kind == DepKind::Dev; - if matches!(class, Class::Standalone(_)) && !testkit_dev && !standalone_allowed(&c.name, &d.name) { - out.push(Violation::StandaloneHasWorkspaceDep { krate: c.name.clone(), dep: d.name.clone() }); - continue; - } - match dep_class { - None => {} - Some(Class::Testkit) if d.kind != DepKind::Dev && class != Class::Testkit => { - out.push(Violation::TestkitAsNormalDep { krate: c.name.clone(), dep: d.name.clone() }); - } - Some(Class::Testkit) => {} - Some(dc) => { - let to = dc.layer().unwrap_or(u8::MAX); - let ok = to < layer || (to == layer && sideways_allowed(&c.name, &d.name)); - if !ok { - out.push(Violation::Upward { krate: c.name.clone(), dep: d.name.clone(), from: layer, to: to.min(8), kind: d.kind }); - } - } - } + check_workspace_dep(c, class, layer, d, &mut out); } else if layer < UI_MIN_LAYER && is_ui_crate(&d.name) && !(class == Class::Testkit || d.kind == DepKind::Dev && d.name == "egui_kittest") { out.push(Violation::UiBelowL7 { krate: c.name.clone(), dep: d.name.clone(), layer }); diff --git a/xtask/src/parity.rs b/xtask/src/parity.rs index 5466776102..ac32f32e85 100644 --- a/xtask/src/parity.rs +++ b/xtask/src/parity.rs @@ -104,6 +104,18 @@ fn valid_milestone(m: &str) -> bool { m == "none" || m.strip_prefix('M').and_then(|n| n.parse::().ok()).is_some_and(|n| n <= 14) } +/// Why an `evidence` entry (`path.rs::test_fn`) does not resolve to a test in the source tree, if it doesn't. +fn evidence_error(ev: &str, repo: &Path) -> Option { + let Some((path, func)) = ev.split_once("::") else { + return Some(format!("evidence {ev:?} must be `path.rs::test_fn`")); + }; + match std::fs::read_to_string(repo.join(path)) { + Ok(src) if src.contains(&format!("fn {func}(")) => None, + Ok(_) => Some(format!("evidence: no `fn {func}` in {path}")), + Err(_) => Some(format!("evidence: {path} does not exist")), + } +} + /// Validate features against the registry, tool table and source tree; returns the errors. pub fn validate(features: &[Feature], commands: &HashSet, tools: &HashSet, repo: &Path) -> Vec { let mut errors = Vec::new(); @@ -143,14 +155,8 @@ pub fn validate(features: &[Feature], commands: &HashSet, tools: &HashSe errors.push(e("shipped features must cite at least one test in `evidence`".into())); } for ev in &f.evidence { - let Some((path, func)) = ev.split_once("::") else { - errors.push(e(format!("evidence {ev:?} must be `path.rs::test_fn`"))); - continue; - }; - match std::fs::read_to_string(repo.join(path)) { - Ok(src) if src.contains(&format!("fn {func}(")) => {} - Ok(_) => errors.push(e(format!("evidence: no `fn {func}` in {path}"))), - Err(_) => errors.push(e(format!("evidence: {path} does not exist"))), + if let Some(m) = evidence_error(ev, repo) { + errors.push(e(m)); } } }