From 40e82d9f5d750362e361b9007314cf342dc64ff0 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Tue, 28 Feb 2023 17:10:52 -0300 Subject: [PATCH 1/2] Rename Inserted to OverlapResult --- .../traits/specialize/specialization_graph.rs | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs b/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs index 61ed9ef2ec184..14d4bc6855c8e 100644 --- a/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs +++ b/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs @@ -20,16 +20,17 @@ pub struct FutureCompatOverlapError<'tcx> { pub kind: FutureCompatOverlapErrorKind, } -/// The result of attempting to insert an impl into a group of children. -enum Inserted<'tcx> { - /// The impl was inserted as a new child in this group of children. - BecameNewSibling(Option>), +/// The result of overlap_check, we would attempt to insert an impl into a group of children +/// afterwards. +enum OverlapResult<'tcx> { + /// The impl is going to be inserted as a new child in this group of children. + NoOverlap(Option>), /// The impl should replace existing impls [X1, ..], because the impl specializes X1, X2, etc. - ReplaceChildren(Vec), + SpecializeAll(Vec), /// The impl is a specialization of an existing child. - ShouldRecurseOn(DefId), + SpecializeOne(DefId), } trait ChildrenExt<'tcx> { @@ -42,7 +43,7 @@ trait ChildrenExt<'tcx> { impl_def_id: DefId, simplified_self: Option, overlap_mode: OverlapMode, - ) -> Result, OverlapError<'tcx>>; + ) -> Result, OverlapError<'tcx>>; } impl<'tcx> ChildrenExt<'tcx> for Children { @@ -86,7 +87,7 @@ impl<'tcx> ChildrenExt<'tcx> for Children { impl_def_id: DefId, simplified_self: Option, overlap_mode: OverlapMode, - ) -> Result, OverlapError<'tcx>> { + ) -> Result, OverlapError<'tcx>> { let mut last_lint = None; let mut replace_children = Vec::new(); @@ -185,7 +186,7 @@ impl<'tcx> ChildrenExt<'tcx> for Children { ); // The impl specializes `possible_sibling`. - return Ok(Inserted::ShouldRecurseOn(possible_sibling)); + return Ok(OverlapResult::SpecializeOne(possible_sibling)); } else if ge && !le { debug!( "placing as parent of TraitRef {:?}", @@ -200,13 +201,13 @@ impl<'tcx> ChildrenExt<'tcx> for Children { } if !replace_children.is_empty() { - return Ok(Inserted::ReplaceChildren(replace_children)); + return Ok(OverlapResult::SpecializeAll(replace_children)); } // No overlap with any potential siblings, so add as a new sibling. debug!("placing as new sibling"); self.insert_blindly(tcx, impl_def_id); - Ok(Inserted::BecameNewSibling(last_lint)) + Ok(OverlapResult::NoOverlap(last_lint)) } } @@ -306,7 +307,7 @@ impl<'tcx> GraphExt<'tcx> for Graph { // Descend the specialization tree, where `parent` is the current parent node. loop { - use self::Inserted::*; + use self::OverlapResult::*; let insert_result = self.children.entry(parent).or_default().insert( tcx, @@ -316,11 +317,11 @@ impl<'tcx> GraphExt<'tcx> for Graph { )?; match insert_result { - BecameNewSibling(opt_lint) => { + NoOverlap(opt_lint) => { last_lint = opt_lint; break; } - ReplaceChildren(grand_children_to_be) => { + SpecializeAll(grand_children_to_be) => { // We currently have // // P @@ -359,7 +360,7 @@ impl<'tcx> GraphExt<'tcx> for Graph { } break; } - ShouldRecurseOn(new_parent) => { + SpecializeOne(new_parent) => { parent = new_parent; } } From a73d57b9b1e81779394d20012a8041ccc9107508 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Tue, 28 Feb 2023 15:52:45 -0300 Subject: [PATCH 2/2] Extract overlap_check_considering_specialization from insert --- .../traits/specialize/specialization_graph.rs | 208 ++++++++++-------- 1 file changed, 112 insertions(+), 96 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs b/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs index 14d4bc6855c8e..d04dfa58e6fb8 100644 --- a/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs +++ b/compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs @@ -88,9 +88,6 @@ impl<'tcx> ChildrenExt<'tcx> for Children { simplified_self: Option, overlap_mode: OverlapMode, ) -> Result, OverlapError<'tcx>> { - let mut last_lint = None; - let mut replace_children = Vec::new(); - debug!("insert(impl_def_id={:?}, simplified_self={:?})", impl_def_id, simplified_self,); let possible_siblings = match simplified_self { @@ -98,117 +95,136 @@ impl<'tcx> ChildrenExt<'tcx> for Children { None => PotentialSiblings::Unfiltered(iter_children(self)), }; - for possible_sibling in possible_siblings { - debug!( - "insert: impl_def_id={:?}, simplified_self={:?}, possible_sibling={:?}", - impl_def_id, simplified_self, possible_sibling, - ); + let result = overlap_check_considering_specialization( + tcx, + impl_def_id, + possible_siblings, + overlap_mode, + ); - let create_overlap_error = |overlap: traits::coherence::OverlapResult<'tcx>| { - let trait_ref = overlap.impl_header.trait_ref.unwrap(); - let self_ty = trait_ref.self_ty(); - - OverlapError { - with_impl: possible_sibling, - trait_ref, - // Only report the `Self` type if it has at least - // some outer concrete shell; otherwise, it's - // not adding much information. - self_ty: self_ty.has_concrete_skeleton().then_some(self_ty), - intercrate_ambiguity_causes: overlap.intercrate_ambiguity_causes, - involves_placeholder: overlap.involves_placeholder, - } - }; - - let report_overlap_error = |overlap: traits::coherence::OverlapResult<'tcx>, - last_lint: &mut _| { - // Found overlap, but no specialization; error out or report future-compat warning. - - // Do we *still* get overlap if we disable the future-incompatible modes? - let should_err = traits::overlapping_impls( - tcx, - possible_sibling, - impl_def_id, - traits::SkipLeakCheck::default(), - overlap_mode, - ) - .is_some(); - - let error = create_overlap_error(overlap); - - if should_err { - Err(error) - } else { - *last_lint = Some(FutureCompatOverlapError { - error, - kind: FutureCompatOverlapErrorKind::LeakCheck, - }); - - Ok((false, false)) - } - }; + if let Ok(OverlapResult::NoOverlap(_)) = result { + // No overlap with any potential siblings, so add as a new sibling. + debug!("placing as new sibling"); + self.insert_blindly(tcx, impl_def_id); + } - let last_lint_mut = &mut last_lint; - let (le, ge) = traits::overlapping_impls( + result + } +} + +fn overlap_check_considering_specialization<'tcx>( + tcx: TyCtxt<'tcx>, + impl_def_id: DefId, + possible_siblings: PotentialSiblings, impl Iterator>, + overlap_mode: OverlapMode, +) -> Result, OverlapError<'tcx>> { + let mut last_lint = None; + let mut replace_children = Vec::new(); + + for possible_sibling in possible_siblings { + debug!("insert: impl_def_id={:?}, possible_sibling={:?}", impl_def_id, possible_sibling); + + let create_overlap_error = |overlap: traits::coherence::OverlapResult<'tcx>| { + let trait_ref = overlap.impl_header.trait_ref.unwrap(); + let self_ty = trait_ref.self_ty(); + + OverlapError { + with_impl: possible_sibling, + trait_ref, + // Only report the `Self` type if it has at least + // some outer concrete shell; otherwise, it's + // not adding much information. + self_ty: self_ty.has_concrete_skeleton().then_some(self_ty), + intercrate_ambiguity_causes: overlap.intercrate_ambiguity_causes, + involves_placeholder: overlap.involves_placeholder, + } + }; + + let report_overlap_error = |overlap: traits::coherence::OverlapResult<'tcx>, + last_lint: &mut _| { + // Found overlap, but no specialization; error out or report future-compat warning. + + // Do we *still* get overlap if we disable the future-incompatible modes? + let should_err = traits::overlapping_impls( tcx, possible_sibling, impl_def_id, - traits::SkipLeakCheck::Yes, + traits::SkipLeakCheck::default(), overlap_mode, ) - .map_or(Ok((false, false)), |overlap| { - if let Some(overlap_kind) = - tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) - { - match overlap_kind { - ty::ImplOverlapKind::Permitted { marker: _ } => {} - ty::ImplOverlapKind::Issue33140 => { - *last_lint_mut = Some(FutureCompatOverlapError { - error: create_overlap_error(overlap), - kind: FutureCompatOverlapErrorKind::Issue33140, - }); - } - } + .is_some(); + + let error = create_overlap_error(overlap); - return Ok((false, false)); + if should_err { + Err(error) + } else { + *last_lint = Some(FutureCompatOverlapError { + error, + kind: FutureCompatOverlapErrorKind::LeakCheck, + }); + + Ok((false, false)) + } + }; + + let last_lint_mut = &mut last_lint; + let (le, ge) = traits::overlapping_impls( + tcx, + possible_sibling, + impl_def_id, + traits::SkipLeakCheck::Yes, + overlap_mode, + ) + .map_or(Ok((false, false)), |overlap| { + if let Some(overlap_kind) = + tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) + { + match overlap_kind { + ty::ImplOverlapKind::Permitted { marker: _ } => {} + ty::ImplOverlapKind::Issue33140 => { + *last_lint_mut = Some(FutureCompatOverlapError { + error: create_overlap_error(overlap), + kind: FutureCompatOverlapErrorKind::Issue33140, + }); + } } - let le = tcx.specializes((impl_def_id, possible_sibling)); - let ge = tcx.specializes((possible_sibling, impl_def_id)); + return Ok((false, false)); + } - if le == ge { report_overlap_error(overlap, last_lint_mut) } else { Ok((le, ge)) } - })?; + let le = tcx.specializes((impl_def_id, possible_sibling)); + let ge = tcx.specializes((possible_sibling, impl_def_id)); - if le && !ge { - debug!( - "descending as child of TraitRef {:?}", - tcx.impl_trait_ref(possible_sibling).unwrap().subst_identity() - ); + if le == ge { report_overlap_error(overlap, last_lint_mut) } else { Ok((le, ge)) } + })?; - // The impl specializes `possible_sibling`. - return Ok(OverlapResult::SpecializeOne(possible_sibling)); - } else if ge && !le { - debug!( - "placing as parent of TraitRef {:?}", - tcx.impl_trait_ref(possible_sibling).unwrap().subst_identity() - ); + if le && !ge { + debug!( + "descending as child of TraitRef {:?}", + tcx.impl_trait_ref(possible_sibling).unwrap().subst_identity() + ); - replace_children.push(possible_sibling); - } else { - // Either there's no overlap, or the overlap was already reported by - // `overlap_error`. - } - } + // The impl specializes `possible_sibling`. + return Ok(OverlapResult::SpecializeOne(possible_sibling)); + } else if ge && !le { + debug!( + "placing as parent of TraitRef {:?}", + tcx.impl_trait_ref(possible_sibling).unwrap().subst_identity() + ); - if !replace_children.is_empty() { - return Ok(OverlapResult::SpecializeAll(replace_children)); + replace_children.push(possible_sibling); + } else { + // Either there's no overlap, or the overlap was already reported by + // `overlap_error`. } + } - // No overlap with any potential siblings, so add as a new sibling. - debug!("placing as new sibling"); - self.insert_blindly(tcx, impl_def_id); - Ok(OverlapResult::NoOverlap(last_lint)) + if !replace_children.is_empty() { + return Ok(OverlapResult::SpecializeAll(replace_children)); } + + Ok(OverlapResult::NoOverlap(last_lint)) } fn iter_children(children: &mut Children) -> impl Iterator + '_ {