From 2ad780eaf03816ae26cd4285eb379e2bb0c55672 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Wed, 10 Jan 2024 22:18:48 +0100 Subject: [PATCH 1/5] Clarify that the status of `&!` is undecided --- compiler/rustc_mir_build/src/thir/pattern/check_match.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs index f6c5e4a5cd6..1b7be98600f 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs @@ -289,7 +289,8 @@ fn lower_pattern( fn is_known_valid_scrutinee(&self, scrutinee: &Expr<'tcx>) -> bool { use ExprKind::*; match &scrutinee.kind { - // Both pointers and references can validly point to a place with invalid data. + // Pointers can validly point to a place with invalid data. It is undecided whether + // references can too, so we conservatively assume they can. Deref { .. } => false, // Inherit validity of the parent place, unless the parent is an union. Field { lhs, .. } => { From bf913ad0aee7e97b93f50824e9b2292414670954 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Wed, 10 Jan 2024 22:42:23 +0100 Subject: [PATCH 2/5] Simplify use of `ValidityConstraint` We had reached a point where the shenanigans about omitting empty arms are unnecessary. --- .../rustc_pattern_analysis/src/usefulness.rs | 27 +++---------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs index 6244cf0ff7d..72459582113 100644 --- a/compiler/rustc_pattern_analysis/src/usefulness.rs +++ b/compiler/rustc_pattern_analysis/src/usefulness.rs @@ -768,9 +768,6 @@ pub(crate) fn ctors_for_ty(&self) -> Result, Cx::Error> { pub enum ValidityConstraint { ValidOnly, MaybeInvalid, - /// Option for backwards compatibility: the place is not known to be valid but we allow omitting - /// `useful && !reachable` arms anyway. - MaybeInvalidButAllowOmittingArms, } impl ValidityConstraint { @@ -778,20 +775,9 @@ pub fn from_bool(is_valid_only: bool) -> Self { if is_valid_only { ValidOnly } else { MaybeInvalid } } - fn allow_omitting_side_effecting_arms(self) -> Self { - match self { - MaybeInvalid | MaybeInvalidButAllowOmittingArms => MaybeInvalidButAllowOmittingArms, - // There are no side-effecting empty arms here, nothing to do. - ValidOnly => ValidOnly, - } - } - fn is_known_valid(self) -> bool { matches!(self, ValidOnly) } - fn allows_omitting_empty_arms(self) -> bool { - matches!(self, ValidOnly | MaybeInvalidButAllowOmittingArms) - } /// If the place has validity given by `self` and we read that the value at the place has /// constructor `ctor`, this computes what we can assume about the validity of the constructor @@ -814,7 +800,7 @@ impl fmt::Display for ValidityConstraint { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let s = match self { ValidOnly => "✓", - MaybeInvalid | MaybeInvalidButAllowOmittingArms => "?", + MaybeInvalid => "?", }; write!(f, "{s}") } @@ -1460,8 +1446,6 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( // Whether the place/column we are inspecting is known to contain valid data. let place_validity = matrix.place_validity[0]; - // For backwards compability we allow omitting some empty arms that we ideally shouldn't. - let place_validity = place_validity.allow_omitting_side_effecting_arms(); // Analyze the constructors present in this column. let ctors = matrix.heads().map(|p| p.ctor()); @@ -1486,12 +1470,9 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( // Whether we should report "Enum::A and Enum::C are missing" or "_ is missing". let report_individual_missing_ctors = always_report_all || !all_missing; // Which constructors are considered missing. We ensure that `!missing_ctors.is_empty() => - // split_ctors.contains(Missing)`. The converse usually holds except in the - // `MaybeInvalidButAllowOmittingArms` backwards-compatibility case. - let mut missing_ctors = split_set.missing; - if !place_validity.allows_omitting_empty_arms() { - missing_ctors.extend(split_set.missing_empty); - } + // split_ctors.contains(Missing)`. The converse usually holds except when + // `!place_validity.is_known_valid()`. + let missing_ctors = split_set.missing; let mut ret = WitnessMatrix::empty(); for ctor in split_ctors { From edb27a306a29b5b3e9f63da3e74e985736905bc2 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Wed, 10 Jan 2024 22:23:21 +0100 Subject: [PATCH 3/5] Make all the empty pattern decisions in `usefulness` --- .../rustc_pattern_analysis/src/constructor.rs | 19 +++++-------------- compiler/rustc_pattern_analysis/src/lints.rs | 2 +- .../rustc_pattern_analysis/src/usefulness.rs | 18 +++++++++++++----- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_pattern_analysis/src/constructor.rs b/compiler/rustc_pattern_analysis/src/constructor.rs index c1042d5b66e..af553840898 100644 --- a/compiler/rustc_pattern_analysis/src/constructor.rs +++ b/compiler/rustc_pattern_analysis/src/constructor.rs @@ -858,12 +858,14 @@ impl ConstructorSet { /// any) are missing; 2/ split constructors to handle non-trivial intersections e.g. on ranges /// or slices. This can get subtle; see [`SplitConstructorSet`] for details of this operation /// and its invariants. - #[instrument(level = "debug", skip(self, pcx, ctors), ret)] + #[instrument(level = "debug", skip(self, ctors), ret)] pub(crate) fn split<'a>( &self, - pcx: &PlaceCtxt<'a, Cx>, ctors: impl Iterator> + Clone, - ) -> SplitConstructorSet { + ) -> SplitConstructorSet + where + Cx: 'a, + { let mut present: SmallVec<[_; 1]> = SmallVec::new(); // Empty constructors found missing. let mut missing_empty = Vec::new(); @@ -1003,17 +1005,6 @@ pub(crate) fn split<'a>( } } - // We have now grouped all the constructors into 3 buckets: present, missing, missing_empty. - // In the absence of the `exhaustive_patterns` feature however, we don't count nested empty - // types as empty. Only non-nested `!` or `enum Foo {}` are considered empty. - if !pcx.mcx.tycx.is_exhaustive_patterns_feature_on() - && !(pcx.is_scrutinee && matches!(self, Self::NoConstructors)) - { - // Treat all missing constructors as nonempty. - // This clears `missing_empty`. - missing.append(&mut missing_empty); - } - SplitConstructorSet { present, missing, missing_empty } } } diff --git a/compiler/rustc_pattern_analysis/src/lints.rs b/compiler/rustc_pattern_analysis/src/lints.rs index cfe4ca3ce93..cf3dddfafeb 100644 --- a/compiler/rustc_pattern_analysis/src/lints.rs +++ b/compiler/rustc_pattern_analysis/src/lints.rs @@ -59,7 +59,7 @@ fn analyze_ctors( ) -> Result, ErrorGuaranteed> { let column_ctors = self.patterns.iter().map(|p| p.ctor()); let ctors_for_ty = &pcx.ctors_for_ty()?; - Ok(ctors_for_ty.split(pcx, column_ctors)) + Ok(ctors_for_ty.split(column_ctors)) } /// Does specialization: given a constructor, this takes the patterns from the column that match diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs index 72459582113..490a84c65db 100644 --- a/compiler/rustc_pattern_analysis/src/usefulness.rs +++ b/compiler/rustc_pattern_analysis/src/usefulness.rs @@ -737,15 +737,13 @@ pub(crate) struct PlaceCtxt<'a, Cx: TypeCx> { pub(crate) mcx: MatchCtxt<'a, Cx>, /// Type of the place under investigation. pub(crate) ty: Cx::Ty, - /// Whether the place is the original scrutinee place, as opposed to a subplace of it. - pub(crate) is_scrutinee: bool, } impl<'a, Cx: TypeCx> PlaceCtxt<'a, Cx> { /// A `PlaceCtxt` when code other than `is_useful` needs one. #[cfg_attr(not(feature = "rustc"), allow(dead_code))] pub(crate) fn new_dummy(mcx: MatchCtxt<'a, Cx>, ty: Cx::Ty) -> Self { - PlaceCtxt { mcx, ty, is_scrutinee: false } + PlaceCtxt { mcx, ty } } pub(crate) fn ctor_arity(&self, ctor: &Constructor) -> usize { @@ -1442,7 +1440,7 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( }; debug!("ty: {ty:?}"); - let pcx = &PlaceCtxt { mcx, ty, is_scrutinee: is_top_level }; + let pcx = &PlaceCtxt { mcx, ty }; // Whether the place/column we are inspecting is known to contain valid data. let place_validity = matrix.place_validity[0]; @@ -1451,7 +1449,17 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( let ctors = matrix.heads().map(|p| p.ctor()); let ctors_for_ty = pcx.ctors_for_ty()?; let is_integers = matches!(ctors_for_ty, ConstructorSet::Integers { .. }); // For diagnostics. - let split_set = ctors_for_ty.split(pcx, ctors); + let mut split_set = ctors_for_ty.split(ctors); + // We have now grouped all the constructors into 3 buckets: present, missing, missing_empty. + // In the absence of the `exhaustive_patterns` feature however, we don't count nested empty + // types as empty. Only non-nested `!` or `enum Foo {}` are considered empty. + if !pcx.mcx.tycx.is_exhaustive_patterns_feature_on() + && !(is_top_level && matches!(ctors_for_ty, ConstructorSet::NoConstructors)) + { + // Treat all missing constructors as nonempty. + // This clears `missing_empty`. + split_set.missing.append(&mut split_set.missing_empty); + } let all_missing = split_set.present.is_empty(); // Build the set of constructors we will specialize with. It must cover the whole type. From de77f1a86ef732d3cbffd284bb32c5908a535748 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Wed, 10 Jan 2024 22:47:52 +0100 Subject: [PATCH 4/5] Simplify empty pattern logic a bit --- .../rustc_pattern_analysis/src/usefulness.rs | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs index 490a84c65db..ec9cbeb4fef 100644 --- a/compiler/rustc_pattern_analysis/src/usefulness.rs +++ b/compiler/rustc_pattern_analysis/src/usefulness.rs @@ -1441,21 +1441,20 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( debug!("ty: {ty:?}"); let pcx = &PlaceCtxt { mcx, ty }; + let ctors_for_ty = pcx.ctors_for_ty()?; // Whether the place/column we are inspecting is known to contain valid data. let place_validity = matrix.place_validity[0]; + // We treat match scrutinees of type `!` or `EmptyEnum` differently. + let is_toplevel_exception = + is_top_level && matches!(ctors_for_ty, ConstructorSet::NoConstructors); + // Whether empty patterns can be omitted for exhaustiveness. + let can_omit_empty_arms = is_toplevel_exception || mcx.tycx.is_exhaustive_patterns_feature_on(); // Analyze the constructors present in this column. let ctors = matrix.heads().map(|p| p.ctor()); - let ctors_for_ty = pcx.ctors_for_ty()?; - let is_integers = matches!(ctors_for_ty, ConstructorSet::Integers { .. }); // For diagnostics. let mut split_set = ctors_for_ty.split(ctors); - // We have now grouped all the constructors into 3 buckets: present, missing, missing_empty. - // In the absence of the `exhaustive_patterns` feature however, we don't count nested empty - // types as empty. Only non-nested `!` or `enum Foo {}` are considered empty. - if !pcx.mcx.tycx.is_exhaustive_patterns_feature_on() - && !(is_top_level && matches!(ctors_for_ty, ConstructorSet::NoConstructors)) - { + if !can_omit_empty_arms { // Treat all missing constructors as nonempty. // This clears `missing_empty`. split_set.missing.append(&mut split_set.missing_empty); @@ -1463,17 +1462,17 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( let all_missing = split_set.present.is_empty(); // Build the set of constructors we will specialize with. It must cover the whole type. + // We need to iterate over a full set of constructors, so we add `Missing` to represent the + // missing ones. This is explained under "Constructor Splitting" at the top of this file. let mut split_ctors = split_set.present; - if !split_set.missing.is_empty() { - // We need to iterate over a full set of constructors, so we add `Missing` to represent the - // missing ones. This is explained under "Constructor Splitting" at the top of this file. - split_ctors.push(Constructor::Missing); - } else if !split_set.missing_empty.is_empty() && !place_validity.is_known_valid() { - // The missing empty constructors are reachable if the place can contain invalid data. + if !(split_set.missing.is_empty() + && (split_set.missing_empty.is_empty() || place_validity.is_known_valid())) + { split_ctors.push(Constructor::Missing); } // Decide what constructors to report. + let is_integers = matches!(ctors_for_ty, ConstructorSet::Integers { .. }); let always_report_all = is_top_level && !is_integers; // Whether we should report "Enum::A and Enum::C are missing" or "_ is missing". let report_individual_missing_ctors = always_report_all || !all_missing; From d95644d3ae276ba787cdf9214a97f27b82c1348e Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Wed, 10 Jan 2024 23:08:31 +0100 Subject: [PATCH 5/5] Simplify empty pattern logic some more --- compiler/rustc_pattern_analysis/src/usefulness.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs index ec9cbeb4fef..d8c63e0571d 100644 --- a/compiler/rustc_pattern_analysis/src/usefulness.rs +++ b/compiler/rustc_pattern_analysis/src/usefulness.rs @@ -1450,23 +1450,19 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( is_top_level && matches!(ctors_for_ty, ConstructorSet::NoConstructors); // Whether empty patterns can be omitted for exhaustiveness. let can_omit_empty_arms = is_toplevel_exception || mcx.tycx.is_exhaustive_patterns_feature_on(); + // Whether empty patterns are counted as useful or not. + let empty_arms_are_unreachable = place_validity.is_known_valid() && can_omit_empty_arms; // Analyze the constructors present in this column. let ctors = matrix.heads().map(|p| p.ctor()); let mut split_set = ctors_for_ty.split(ctors); - if !can_omit_empty_arms { - // Treat all missing constructors as nonempty. - // This clears `missing_empty`. - split_set.missing.append(&mut split_set.missing_empty); - } let all_missing = split_set.present.is_empty(); - // Build the set of constructors we will specialize with. It must cover the whole type. // We need to iterate over a full set of constructors, so we add `Missing` to represent the // missing ones. This is explained under "Constructor Splitting" at the top of this file. let mut split_ctors = split_set.present; if !(split_set.missing.is_empty() - && (split_set.missing_empty.is_empty() || place_validity.is_known_valid())) + && (split_set.missing_empty.is_empty() || empty_arms_are_unreachable)) { split_ctors.push(Constructor::Missing); } @@ -1479,7 +1475,10 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( // Which constructors are considered missing. We ensure that `!missing_ctors.is_empty() => // split_ctors.contains(Missing)`. The converse usually holds except when // `!place_validity.is_known_valid()`. - let missing_ctors = split_set.missing; + let mut missing_ctors = split_set.missing; + if !can_omit_empty_arms { + missing_ctors.append(&mut split_set.missing_empty); + } let mut ret = WitnessMatrix::empty(); for ctor in split_ctors {