From 2f9a0d45a1e3eaa10698197d0227133a66261ddd Mon Sep 17 00:00:00 2001 From: moxian Date: Sun, 30 Mar 2025 15:27:01 -0700 Subject: [PATCH] Touch up controller logic to handle nested items well. The test in the previous commit now passes --- full-tests/nested-items2.rs | 24 +++++++++++++ src/processor/checker.rs | 68 ++++++++++++++++++++++++------------- src/processor/mod.rs | 12 ++++--- 3 files changed, 77 insertions(+), 27 deletions(-) create mode 100644 full-tests/nested-items2.rs diff --git a/full-tests/nested-items2.rs b/full-tests/nested-items2.rs new file mode 100644 index 0000000..ed02dda --- /dev/null +++ b/full-tests/nested-items2.rs @@ -0,0 +1,24 @@ +// this should all get deleted in a single swoop *and* not panic about it +/// ~REQUIRE-DELETED l1 +mod l1 { + mod l2 { + mod l3 { + mod l4{ + mod l5 { + fn foo(){} + fn bar(){} + mod l6 { + fn x1(){} + } + fn x2(){} + } + } + mod l4_2 { + fn y(){} + } + } + } + fn x8(){} +} +/// ~MINIMIZE-ROOT main +fn main(){} diff --git a/src/processor/checker.rs b/src/processor/checker.rs index 748fda2..e7da7a8 100644 --- a/src/processor/checker.rs +++ b/src/processor/checker.rs @@ -6,6 +6,14 @@ use self::worklist::Worklist; #[derive(Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] struct AstPath(Vec); +impl AstPath { + fn has_prefix(&self, other: &AstPath) -> bool { + if self.0.len() < other.0.len() { + return false; + } + std::iter::zip(self.0.iter(), other.0.iter()).all(|(a, b)| a == b) + } +} impl Borrow<[String]> for AstPath { fn borrow(&self) -> &[String] { @@ -75,6 +83,20 @@ mod worklist { pub(super) fn pop(&mut self) -> Option> { self.0.pop() } + + // remove all the worklist items that would have been covered by the + // given ones. + // I.e. if we have already deleted the entire module, there's no need + // trying to delete that module's individual items anymore + pub(super) fn prune(&mut self, things: &std::collections::BTreeSet) { + for wl in &mut self.0 { + wl.retain(|path| { + // retain only if none of the things are a prefix of this path + things.iter().all(|thing| !path.has_prefix(thing)) + }) + } + self.0.retain(|wl| !wl.is_empty()); + } } } @@ -97,8 +119,9 @@ impl PassController { committed, failed: _, current, - worklist: _, + worklist, } => { + worklist.prune(current); committed.extend(mem::take(current)); self.next_in_worklist(); @@ -110,19 +133,8 @@ impl PassController { /// The changes did not reproduce the regression. Bisect further. pub fn does_not_reproduce(&mut self) { match &mut self.state { - PassControllerState::InitialCollection { candidates } => { - // Applying them all was too much, let's bisect! - let (current, first_worklist_item) = split_owned(mem::take(candidates)); - - let mut worklist = Worklist::new(); - worklist.push(first_worklist_item); - - self.state = PassControllerState::Bisecting { - committed: BTreeSet::new(), - failed: BTreeSet::new(), - current, - worklist, - }; + PassControllerState::InitialCollection { candidates: _ } => { + unreachable!("we should have made no changes on initial collection, what do you mean it does not reproduce?!?") } PassControllerState::Bisecting { committed, @@ -155,15 +167,24 @@ impl PassController { } } - /// The pass did not apply any changes. We're done. + /// The pass did not apply any changes. We're either done or just starting pub fn no_change(&mut self) { - match &self.state { + match &mut self.state { PassControllerState::InitialCollection { candidates } => { - assert!( - candidates.is_empty(), - "No change but received candidates. The responsible pass does not seem to track the ProcessState correctly: {candidates:?}" - ); - self.state = PassControllerState::Success; + if candidates.is_empty() { + self.state = PassControllerState::Success; + } else { + let current = mem::take(candidates) + .into_iter() + .collect::>(); + + self.state = PassControllerState::Bisecting { + committed: BTreeSet::new(), + failed: BTreeSet::new(), + current, + worklist: Worklist::new(), + }; + } } PassControllerState::Bisecting { current, .. } => { unreachable!("Pass said it didn't change anything in the bisection phase, nils forgot what this means: {current:?}"); @@ -184,9 +205,9 @@ impl PassController { pub fn can_process(&mut self, path: &[String]) -> bool { match &mut self.state { PassControllerState::InitialCollection { candidates } => { - // For the initial collection, we collect the candidate and apply them all. + // For the initial collection, we collect the candidate but don't apply anything candidates.push(AstPath(path.to_owned())); - true + false } PassControllerState::Bisecting { current, .. } => current.contains(path), PassControllerState::Success { .. } => { @@ -205,6 +226,7 @@ impl PassController { match worklist.pop() { Some(next) => { *current = next.into_iter().collect(); + trace!(?current, "current working set: "); } None => { self.state = PassControllerState::Success; diff --git a/src/processor/mod.rs b/src/processor/mod.rs index 3b7dadc..f167bda 100644 --- a/src/processor/mod.rs +++ b/src/processor/mod.rs @@ -187,6 +187,7 @@ impl Minimizer { // The logic for bisecting down lives in PassController. let mut checker = PassController::new(self.options.clone()); + let mut initial_pass = true; loop { let mut change = file.try_change(changes)?; let (_, krate) = change.before_content(); @@ -213,14 +214,17 @@ impl Minimizer { } } ProcessState::NoChange => { - if self.options.no_color { - info!("{file:?}: After {}: no changes", pass.name()); - } else { - info!("{file:?}: After {}: {}", pass.name(), "no changes".yellow()); + if !initial_pass { + if self.options.no_color { + info!("{file:?}: After {}: no changes", pass.name()); + } else { + info!("{file:?}: After {}: {}", pass.name(), "no changes".yellow()); + } } checker.no_change(); } } + initial_pass = false; if self.cancel.load(Ordering::SeqCst) { info!("Exiting early.");