-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
match: Use an aggregate equality comparison for constant array/slice patterns #155216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9bb14fa
085b0a4
9665678
6171edd
4df802d
4c496f9
ca8b70b
0da5fed
776ace0
9f195dc
10cbf96
f89a54e
c02f658
2ea4f27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,40 @@ use crate::builder::matches::{ | |||||||||||||
| FlatPat, MatchPairTree, PatConstKind, PatternExtraData, SliceLenOp, TestableCase, | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| /// Below this length, an array or slice pattern is compared element by element | ||||||||||||||
| /// rather than as a single aggregate, since the per-element comparisons are | ||||||||||||||
| /// unlikely to be more expensive than a `PartialEq::eq` call. | ||||||||||||||
| const AGGREGATE_EQ_MIN_LEN: usize = 4; | ||||||||||||||
|
|
||||||||||||||
| impl<'a, 'tcx> Builder<'a, 'tcx> { | ||||||||||||||
| /// Check if we can use aggregate `PartialEq::eq` comparisons for constant array/slice patterns. | ||||||||||||||
| /// This is not possible in const contexts, because `PartialEq` is not const-stable yet. | ||||||||||||||
| fn can_use_aggregate_eq(&self) -> bool { | ||||||||||||||
| let in_const_context = self.tcx.is_const_fn(self.def_id.to_def_id()) | ||||||||||||||
| || !self.tcx.hir_body_owner_kind(self.def_id).is_fn_or_closure(); | ||||||||||||||
| !in_const_context | ||||||||||||||
| } | ||||||||||||||
|
jakubadamw marked this conversation as resolved.
|
||||||||||||||
|
|
||||||||||||||
| /// If the given array or slice pattern node was expanded from a constant | ||||||||||||||
| /// by `const_to_pat` and an aggregate comparison is both possible and | ||||||||||||||
| /// worthwhile, returns the original constant value, so that the scrutinee | ||||||||||||||
| /// can be compared against it as a whole via `PartialEq::eq`. | ||||||||||||||
| /// | ||||||||||||||
| /// Note that this deliberately does not apply to hand-written array or | ||||||||||||||
| /// slice patterns, which only ever match element by element. | ||||||||||||||
| fn aggregate_const_value( | ||||||||||||||
| &self, | ||||||||||||||
| pattern: &Pat<'tcx>, | ||||||||||||||
| element_count: usize, | ||||||||||||||
| ) -> Option<ty::Value<'tcx>> { | ||||||||||||||
| let value = pattern.extra.as_deref()?.expanded_const_value?; | ||||||||||||||
| if element_count < AGGREGATE_EQ_MIN_LEN || !self.can_use_aggregate_eq() { | ||||||||||||||
| return None; | ||||||||||||||
| } | ||||||||||||||
| Some(value) | ||||||||||||||
|
Comment on lines
+42
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to check that the element type is such that we'll be using a known rust/library/core/src/cmp/bytewise.rs Lines 31 to 36 in 88f7399
Potentially this could be extended to arbitrary |
||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// For an array or slice pattern's subpatterns (prefix/slice/suffix), returns a list | ||||||||||||||
| /// of those subpatterns, each paired with a suitably-projected [`PlaceBuilder`]. | ||||||||||||||
| fn prefix_slice_suffix<'a, 'tcx>( | ||||||||||||||
|
|
@@ -344,10 +378,26 @@ impl<'tcx> InterPat<'tcx> { | |||||||||||||
| _ => None, | ||||||||||||||
| }; | ||||||||||||||
| if let Some(array_len) = array_len { | ||||||||||||||
| for (subplace, subpat) in | ||||||||||||||
| prefix_slice_suffix(&place_builder, Some(array_len), prefix, slice, suffix) | ||||||||||||||
| { | ||||||||||||||
| subpats.push(InterPat::lower_thir_pat(cx, subplace, subpat)); | ||||||||||||||
| // If this pattern was expanded from a constant, compare | ||||||||||||||
| // the whole array against that constant at once via | ||||||||||||||
| // `PartialEq::eq` rather than element by element. | ||||||||||||||
| if let Some(aggregate_value) = cx.aggregate_const_value(pattern, prefix.len()) { | ||||||||||||||
| debug_assert!(slice.is_none() && suffix.is_empty()); | ||||||||||||||
| Some(TestableCase::Constant { | ||||||||||||||
| value: aggregate_value, | ||||||||||||||
| kind: PatConstKind::Aggregate, | ||||||||||||||
| }) | ||||||||||||||
| } else { | ||||||||||||||
| for (subplace, subpat) in prefix_slice_suffix( | ||||||||||||||
| &place_builder, | ||||||||||||||
| Some(array_len), | ||||||||||||||
| prefix, | ||||||||||||||
| slice, | ||||||||||||||
| suffix, | ||||||||||||||
| ) { | ||||||||||||||
| subpats.push(InterPat::lower_thir_pat(cx, subplace, subpat)); | ||||||||||||||
| } | ||||||||||||||
| None | ||||||||||||||
| } | ||||||||||||||
| } else { | ||||||||||||||
| // If the array length couldn't be determined, ignore the | ||||||||||||||
|
|
@@ -359,33 +409,57 @@ impl<'tcx> InterPat<'tcx> { | |||||||||||||
| pattern.ty | ||||||||||||||
| ), | ||||||||||||||
| ); | ||||||||||||||
| None | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| None | ||||||||||||||
| } | ||||||||||||||
| PatKind::Slice { ref prefix, ref slice, ref suffix } => { | ||||||||||||||
| for (subplace, subpat) in | ||||||||||||||
| prefix_slice_suffix(&place_builder, None, prefix, slice, suffix) | ||||||||||||||
| { | ||||||||||||||
| subpats.push(InterPat::lower_thir_pat(cx, subplace, subpat)); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if prefix.is_empty() && slice.is_some() && suffix.is_empty() { | ||||||||||||||
| // A slice pattern shaped like `[..]` is irrefutable. | ||||||||||||||
| // It can match a slice of any length, so no length test is needed. | ||||||||||||||
| None | ||||||||||||||
| } else { | ||||||||||||||
| // Any other shape of slice pattern requires a length test. | ||||||||||||||
| // Slice patterns with a `..` subpattern require a minimum | ||||||||||||||
| // length; those without `..` require an exact length. | ||||||||||||||
| // If this pattern was expanded from a constant, compare the | ||||||||||||||
| // whole slice against that constant at once via | ||||||||||||||
| // `PartialEq::eq` after the length check, rather than | ||||||||||||||
| // element by element. | ||||||||||||||
| if let Some(aggregate_value) = cx.aggregate_const_value(pattern, prefix.len()) { | ||||||||||||||
| debug_assert!(slice.is_none() && suffix.is_empty()); | ||||||||||||||
| subpats.push(InterPat { | ||||||||||||||
| place, | ||||||||||||||
| testable_case: Some(TestableCase::Constant { | ||||||||||||||
| value: aggregate_value, | ||||||||||||||
| kind: PatConstKind::Aggregate, | ||||||||||||||
| }), | ||||||||||||||
| subpats: Vec::new(), | ||||||||||||||
| or_subpats: None, | ||||||||||||||
| ascriptions: Vec::new(), | ||||||||||||||
| binding: None, | ||||||||||||||
| pattern_span: pattern.span, | ||||||||||||||
| is_never: false, | ||||||||||||||
| }); | ||||||||||||||
| Some(TestableCase::Slice { | ||||||||||||||
| len: u64::try_from(prefix.len() + suffix.len()).unwrap(), | ||||||||||||||
| op: if slice.is_some() { | ||||||||||||||
| SliceLenOp::GreaterOrEqual | ||||||||||||||
| } else { | ||||||||||||||
| SliceLenOp::Equal | ||||||||||||||
| }, | ||||||||||||||
| len: u64::try_from(prefix.len()).unwrap(), | ||||||||||||||
| op: SliceLenOp::Equal, | ||||||||||||||
| }) | ||||||||||||||
| } else { | ||||||||||||||
| for (subplace, subpat) in | ||||||||||||||
| prefix_slice_suffix(&place_builder, None, prefix, slice, suffix) | ||||||||||||||
| { | ||||||||||||||
| subpats.push(InterPat::lower_thir_pat(cx, subplace, subpat)); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if prefix.is_empty() && slice.is_some() && suffix.is_empty() { | ||||||||||||||
| // A slice pattern shaped like `[..]` is irrefutable. | ||||||||||||||
| // It can match a slice of any length, so no length test is needed. | ||||||||||||||
| None | ||||||||||||||
| } else { | ||||||||||||||
| // Any other shape of slice pattern requires a length test. | ||||||||||||||
| // Slice patterns with a `..` subpattern require a minimum | ||||||||||||||
| // length; those without `..` require an exact length. | ||||||||||||||
| Some(TestableCase::Slice { | ||||||||||||||
| len: u64::try_from(prefix.len() + suffix.len()).unwrap(), | ||||||||||||||
| op: if slice.is_some() { | ||||||||||||||
| SliceLenOp::GreaterOrEqual | ||||||||||||||
| } else { | ||||||||||||||
| SliceLenOp::Equal | ||||||||||||||
| }, | ||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | |
| TestableCase::Constant { value, kind: PatConstKind::String } => { | ||
| TestKind::StringEq { value } | ||
| } | ||
| TestableCase::Constant { value, kind: PatConstKind::Aggregate } => { | ||
| TestKind::AggregateEq { value } | ||
| } | ||
| TestableCase::Constant { value, kind: PatConstKind::Float | PatConstKind::Other } => { | ||
| TestKind::ScalarEq { value } | ||
| } | ||
|
|
@@ -137,44 +140,59 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | |
| self.cfg.terminate(block, self.source_info(match_start_span), terminator); | ||
| } | ||
|
|
||
| TestKind::StringEq { value } => { | ||
| TestKind::StringEq { value } | TestKind::AggregateEq { value } => { | ||
| let tcx = self.tcx; | ||
| let success_block = target_block(TestBranch::Success); | ||
| let fail_block = target_block(TestBranch::Failure); | ||
|
|
||
| let ref_str_ty = Ty::new_imm_ref(tcx, tcx.lifetimes.re_erased, tcx.types.str_); | ||
| assert!(ref_str_ty.is_imm_ref_str(), "{ref_str_ty:?}"); | ||
|
|
||
| // The string constant we're testing against has type `str`, but | ||
| // calling `<str as PartialEq>::eq` requires `&str` operands. | ||
| // | ||
| // Because `str` and `&str` have the same valtree representation, | ||
| // we can "cast" to the desired type by just replacing the type. | ||
| assert!(value.ty.is_str(), "unexpected value type for StringEq test: {value:?}"); | ||
| let expected_value = ty::Value { ty: ref_str_ty, valtree: value.valtree }; | ||
| let inner_ty = value.ty; | ||
| if matches!(test.kind, TestKind::StringEq { .. }) { | ||
| assert!( | ||
| inner_ty.is_str(), | ||
| "unexpected value type for StringEq test: {value:?}" | ||
| ); | ||
| } | ||
| let ref_ty = Ty::new_imm_ref(tcx, tcx.lifetimes.re_erased, inner_ty); | ||
|
|
||
| // The constant we're testing against has type `str`, `[T; N]`, or `[T]`, | ||
| // but calling `<T as PartialEq>::eq` requires a reference operand | ||
| // (`&str`, `&[T; N]`, or `&[T]`). Valtree representations are the same | ||
| // with or without the reference wrapper, so we can "cast" to the | ||
| // desired type by just replacing the type. | ||
| let expected_value = ty::Value { ty: ref_ty, valtree: value.valtree }; | ||
| let expected_value_operand = | ||
| self.literal_operand(test.span, Const::from_ty_value(tcx, expected_value)); | ||
|
|
||
| // Similarly, the scrutinized place has type `str`, but we need `&str`. | ||
| // Get a reference by doing `let actual_value_ref_place: &str = &place`. | ||
| let actual_value_ref_place = self.temp(ref_str_ty, test.span); | ||
| // Similarly, the scrutinised place has the inner type, but we need a | ||
| // reference. Get one by doing `let actual_value_ref_place = &place`. | ||
| let actual_value_ref_place = self.temp(ref_ty, test.span); | ||
| self.cfg.push_assign( | ||
| block, | ||
| self.source_info(test.span), | ||
| actual_value_ref_place, | ||
| Rvalue::Ref(tcx.lifetimes.re_erased, BorrowKind::Shared, place), | ||
| ); | ||
|
|
||
| // Compare two strings using `<str as std::cmp::PartialEq>::eq`. | ||
| // (Interestingly this means that exhaustiveness analysis relies, for soundness, | ||
| // on the `PartialEq` impl for `str` to be correct!) | ||
| self.string_compare( | ||
| // Compare the two values using `<T as std::cmp::PartialEq>::eq`. | ||
| // (Interestingly this means that, for `str`, exhaustiveness analysis | ||
| // relies for soundness on the `PartialEq` impl for `str` to be correct!) | ||
|
Comment on lines
+177
to
+178
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This applies to aggregates too. We need |
||
| // | ||
| // The aggregate comparisons, unlike the long-standing string ones, are | ||
| // asserted not to unwind, since an unwind edge would make | ||
| // borrow-checking stricter than for the `SwitchInt`s they replace. | ||
| // That is sound because a constant is only allowed in a pattern if its | ||
| // type is structural match, so the array/slice impl and every element | ||
| // impl it delegates to are derived or primitive, and cannot panic. | ||
|
Comment on lines
+183
to
+185
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only be doing this with element types where we know the In general, being marked |
||
| let can_unwind = matches!(test.kind, TestKind::StringEq { .. }); | ||
| self.non_scalar_compare( | ||
| block, | ||
| success_block, | ||
| fail_block, | ||
| source_info, | ||
| inner_ty, | ||
| expected_value_operand, | ||
| Operand::Copy(actual_value_ref_place), | ||
| can_unwind, | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -409,19 +427,31 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | |
| ); | ||
| } | ||
|
|
||
| /// Compare two values of type `&str` using `<str as std::cmp::PartialEq>::eq`. | ||
| fn string_compare( | ||
| /// Compare two reference values using `<T as PartialEq>::eq`. | ||
| /// | ||
| /// `compared_ty` is the *inner* type (e.g. `str`, `[u8; 64]`); | ||
| /// `expect` and `val` must already be references to that type. | ||
| /// | ||
| /// When `can_unwind` is false, the call is given `UnwindAction::Unreachable` | ||
| /// and no unwind edge, asserting that the `PartialEq::eq` implementation | ||
| /// cannot panic. This matters beyond codegen: an unwinding call would make | ||
| /// borrow-checking of the surrounding match stricter, because the unwind | ||
| /// path can create drop-order conflicts that the ordinary path does not | ||
| /// have. | ||
| fn non_scalar_compare( | ||
| &mut self, | ||
| block: BasicBlock, | ||
| success_block: BasicBlock, | ||
| fail_block: BasicBlock, | ||
| source_info: SourceInfo, | ||
| compared_ty: Ty<'tcx>, | ||
| expect: Operand<'tcx>, | ||
| val: Operand<'tcx>, | ||
| can_unwind: bool, | ||
| ) { | ||
| let str_ty = self.tcx.types.str_; | ||
| let eq_def_id = self.tcx.require_lang_item(LangItem::PartialEq, source_info.span); | ||
| let method = trait_method(self.tcx, eq_def_id, sym::eq, &[str_ty.into(), str_ty.into()]); | ||
| let method = | ||
| trait_method(self.tcx, eq_def_id, sym::eq, &[compared_ty.into(), compared_ty.into()]); | ||
|
|
||
| let bool_ty = self.tcx.types.bool; | ||
| let eq_result = self.temp(bool_ty, source_info.span); | ||
|
|
@@ -448,12 +478,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | |
| .into(), | ||
| destination: eq_result, | ||
| target: Some(eq_block), | ||
| unwind: UnwindAction::Continue, | ||
| unwind: if can_unwind { UnwindAction::Continue } else { UnwindAction::Unreachable }, | ||
| call_source: CallSource::MatchCmp, | ||
| fn_span: source_info.span, | ||
| }, | ||
| ); | ||
| self.diverge_from(block); | ||
| if can_unwind { | ||
| self.diverge_from(block); | ||
| } | ||
|
|
||
| // check the result | ||
| self.cfg.terminate( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.