From 3cf5cc37f7ca3aa459fe21af0b571d05d60d04af Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 6 Feb 2025 19:31:25 +0000 Subject: [PATCH 1/5] insurer code is dead by runing tests with a panic! call --- src/cargo/core/resolver/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index ba8566f71c5..8f29dd5c644 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -964,7 +964,7 @@ fn find_candidate( .remaining_candidates .next(&mut frame.conflicting_activations, &frame.context); let Some((candidate, has_another)) = next else { - continue; + panic!("why did we save a frame that has no next?"); }; // If all members of `conflicting_activations` are still From 0716b882ba6e8c92ab65d5436aaeb16ba7702928 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 6 Feb 2025 19:35:07 +0000 Subject: [PATCH 2/5] move code out of loop --- src/cargo/core/resolver/mod.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 8f29dd5c644..0d89bde2121 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -960,13 +960,6 @@ fn find_candidate( }; while let Some(mut frame) = backtrack_stack.pop() { - let next = frame - .remaining_candidates - .next(&mut frame.conflicting_activations, &frame.context); - let Some((candidate, has_another)) = next else { - panic!("why did we save a frame that has no next?"); - }; - // If all members of `conflicting_activations` are still // active in this back up we know that we're guaranteed to not actually // make any progress. As a result if we hit this condition we can @@ -999,6 +992,11 @@ fn find_candidate( } } + let (candidate, has_another) = frame + .remaining_candidates + .next(&mut frame.conflicting_activations, &frame.context) + .expect("why did we save a frame that has no next?"); + return Some((candidate, has_another, frame)); } None From 2cce8b4be9a41247d8e5a6b9550db146bf4c5adf Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 6 Feb 2025 20:03:12 +0000 Subject: [PATCH 3/5] deduplicate the check --- src/cargo/core/resolver/mod.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 0d89bde2121..c573c4a4b6d 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -965,7 +965,17 @@ fn find_candidate( // make any progress. As a result if we hit this condition we can // completely skip this backtrack frame and move on to the next. if let Some(age) = age { - if frame.context.age >= age { + // Above we use `cx` to determine if this is going to be conflicting. + // But lets just double check if the `pop`ed frame agrees. + let frame_to_new = frame.context.age >= age; + debug_assert!( + frame + .context + .is_conflicting(Some(parent.package_id()), conflicting_activations) + == frame_to_new.then_some(age) + ); + + if frame_to_new { trace!( "{} = \"{}\" skip as not solving {}: {:?}", frame.dep.package_name(), @@ -973,22 +983,7 @@ fn find_candidate( parent.package_id(), conflicting_activations ); - // above we use `cx` to determine that this is still going to be conflicting. - // but lets just double check. - debug_assert!( - frame - .context - .is_conflicting(Some(parent.package_id()), conflicting_activations) - == Some(age) - ); continue; - } else { - // above we use `cx` to determine that this is not going to be conflicting. - // but lets just double check. - debug_assert!(frame - .context - .is_conflicting(Some(parent.package_id()), conflicting_activations) - .is_none()); } } From a9da3c8345ef17fe48beb2e31fdf3656df04d1d3 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 6 Feb 2025 16:44:19 -0500 Subject: [PATCH 4/5] to to too Co-authored-by: Ed Page --- src/cargo/core/resolver/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index c573c4a4b6d..c0411121a94 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -967,12 +967,12 @@ fn find_candidate( if let Some(age) = age { // Above we use `cx` to determine if this is going to be conflicting. // But lets just double check if the `pop`ed frame agrees. - let frame_to_new = frame.context.age >= age; + let frame_too_new = frame.context.age >= age; debug_assert!( frame .context .is_conflicting(Some(parent.package_id()), conflicting_activations) - == frame_to_new.then_some(age) + == frame_too_new.then_some(age) ); if frame_to_new { From 801409161210856c0f3453086040aaceccdbf6f3 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 6 Feb 2025 22:07:05 +0000 Subject: [PATCH 5/5] reorganize the loop --- src/cargo/core/resolver/mod.rs | 43 +++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index c0411121a94..82fa57b1b0e 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -958,13 +958,14 @@ fn find_candidate( } else { None }; + let mut new_frame = None; + if let Some(age) = age { + while let Some(frame) = backtrack_stack.pop() { + // If all members of `conflicting_activations` are still + // active in this back up we know that we're guaranteed to not actually + // make any progress. As a result if we hit this condition we can + // completely skip this backtrack frame and move on to the next. - while let Some(mut frame) = backtrack_stack.pop() { - // If all members of `conflicting_activations` are still - // active in this back up we know that we're guaranteed to not actually - // make any progress. As a result if we hit this condition we can - // completely skip this backtrack frame and move on to the next. - if let Some(age) = age { // Above we use `cx` to determine if this is going to be conflicting. // But lets just double check if the `pop`ed frame agrees. let frame_too_new = frame.context.age >= age; @@ -975,26 +976,30 @@ fn find_candidate( == frame_too_new.then_some(age) ); - if frame_to_new { - trace!( - "{} = \"{}\" skip as not solving {}: {:?}", - frame.dep.package_name(), - frame.dep.version_req(), - parent.package_id(), - conflicting_activations - ); - continue; + if !frame_too_new { + new_frame = Some(frame); + break; } + trace!( + "{} = \"{}\" skip as not solving {}: {:?}", + frame.dep.package_name(), + frame.dep.version_req(), + parent.package_id(), + conflicting_activations + ); } + } else { + // If we're here then we are in abnormal situations and need to just go one frame at a time. + new_frame = backtrack_stack.pop(); + } + new_frame.map(|mut frame| { let (candidate, has_another) = frame .remaining_candidates .next(&mut frame.conflicting_activations, &frame.context) .expect("why did we save a frame that has no next?"); - - return Some((candidate, has_another, frame)); - } - None + (candidate, has_another, frame) + }) } fn check_cycles(resolve: &Resolve) -> CargoResult<()> {