Rollup merge of #150238 - Zalathar:pat-const-kind, r=Nadrieril

mir_build: Classify `TestableCase::Constant` into multiple sub-kinds

In match lowering, when choosing a test for a `TestableCase::Constant`, there is some ad-hoc logic for inspecting the pattern type and deciding what kind of test is suitable. There is also some very similar logic later, when partitioning cases into buckets based on the chosen test.

Instead of having that ad-hoc logic in multiple places, I think it's better to perform an up-front classification when lowering `thir::PatKind::Constant` to `TestableCase::Constant`, and then have the later steps simply match on an enum variant.

There should be no change to the resulting built MIR.

(I will note that the logic/invariants involved are a bit unclear, so there is a risk of accidental minor differences.)

Author: jhpratt

compiler/rustc_mir_build/src/builder/matches/buckets.rs

@@ -6,8 +6,7 @@ use rustc_middle::span_bug;
 use tracing::debug;
 
 use crate::builder::Builder;
-use crate::builder::matches::test::is_switch_ty;
-use crate::builder::matches::{Candidate, Test, TestBranch, TestKind, TestableCase};
+use crate::builder::matches::{Candidate, PatConstKind, Test, TestBranch, TestKind, TestableCase};
 
 /// Output of [`Builder::partition_candidates_into_buckets`].
 pub(crate) struct PartitionedCandidates<'tcx, 'b, 'c> {
@@ -157,11 +156,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             //
             // FIXME(#29623) we could use PatKind::Range to rule
             // things out here, in some cases.
-            //
-            // FIXME(Zalathar): Is the `is_switch_ty` test unnecessary?
-            (TestKind::SwitchInt, &TestableCase::Constant { value })
-                if is_switch_ty(match_pair.pattern_ty) =>
-            {
+            (
+                TestKind::SwitchInt,
+                &TestableCase::Constant { value, kind: PatConstKind::IntOrChar },
+            ) => {
                 // An important invariant of candidate bucketing is that a candidate
                 // must not match in multiple branches. For `SwitchInt` tests, adding
                 // a new value might invalidate that property for range patterns that
@@ -206,7 +204,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 })
             }
 
-            (TestKind::If, TestableCase::Constant { value }) => {
+            (TestKind::If, TestableCase::Constant { value, kind: PatConstKind::Bool }) => {
                 fully_matched = true;
                 let value = value.try_to_bool().unwrap_or_else(|| {
                     span_bug!(test.span, "expected boolean value but got {value:?}")
@@ -291,7 +289,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     if !test.overlaps(pat, self.tcx)? { Some(TestBranch::Failure) } else { None }
                 }
             }
-            (TestKind::Range(range), &TestableCase::Constant { value }) => {
+            (
+                TestKind::Range(range),
+                &TestableCase::Constant {
+                    value,
+                    kind: PatConstKind::Bool | PatConstKind::IntOrChar | PatConstKind::Float,
+                },
+            ) => {
                 fully_matched = false;
                 if !range.contains(value, self.tcx)? {
                     // `value` is not contained in the testing range,
@@ -302,7 +306,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 }
             }
 
-            (TestKind::Eq { value: test_val, .. }, TestableCase::Constant { value: case_val }) => {
+            (
+                TestKind::Eq { value: test_val, .. },
+                TestableCase::Constant {
+                    value: case_val,
+                    kind: PatConstKind::Float | PatConstKind::Other,
+                },
+            ) => {
                 if test_val == case_val {
                     fully_matched = true;
                     Some(TestBranch::Success)

compiler/rustc_mir_build/src/builder/matches/match_pair.rs

@@ -7,7 +7,9 @@ use rustc_middle::ty::{self, Ty, TypeVisitableExt};
 
 use crate::builder::Builder;
 use crate::builder::expr::as_place::{PlaceBase, PlaceBuilder};
-use crate::builder::matches::{FlatPat, MatchPairTree, PatternExtraData, TestableCase};
+use crate::builder::matches::{
+    FlatPat, MatchPairTree, PatConstKind, PatternExtraData, TestableCase,
+};
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// Builds and pushes [`MatchPairTree`] subtrees, one for each pattern in
@@ -156,7 +158,29 @@ impl<'tcx> MatchPairTree<'tcx> {
                 }
             }
 
-            PatKind::Constant { value } => Some(TestableCase::Constant { value }),
+            PatKind::Constant { value } => {
+                // CAUTION: The type of the pattern node (`pattern.ty`) is
+                // _often_ the same as the type of the const value (`value.ty`),
+                // but there are some cases where those types differ
+                // (e.g. when `deref!(..)` patterns interact with `String`).
+
+                // Classify the constant-pattern into further kinds, to
+                // reduce the number of ad-hoc type tests needed later on.
+                let pat_ty = pattern.ty;
+                let const_kind = if pat_ty.is_bool() {
+                    PatConstKind::Bool
+                } else if pat_ty.is_integral() || pat_ty.is_char() {
+                    PatConstKind::IntOrChar
+                } else if pat_ty.is_floating_point() {
+                    PatConstKind::Float
+                } else {
+                    // FIXME(Zalathar): This still covers several different
+                    // categories (e.g. raw pointer, string, pattern-type)
+                    // which could be split out into their own kinds.
+                    PatConstKind::Other
+                };
+                Some(TestableCase::Constant { value, kind: const_kind })
+            }
 
             PatKind::AscribeUserType {
                 ascription: Ascription { ref annotation, variance },

compiler/rustc_mir_build/src/builder/matches/mod.rs

@@ -1262,7 +1262,7 @@ struct Ascription<'tcx> {
 #[derive(Debug, Clone)]
 enum TestableCase<'tcx> {
     Variant { adt_def: ty::AdtDef<'tcx>, variant_index: VariantIdx },
-    Constant { value: ty::Value<'tcx> },
+    Constant { value: ty::Value<'tcx>, kind: PatConstKind },
     Range(Arc<PatRange<'tcx>>),
     Slice { len: u64, variable_length: bool },
     Deref { temp: Place<'tcx>, mutability: Mutability },
@@ -1276,6 +1276,28 @@ impl<'tcx> TestableCase<'tcx> {
     }
 }
 
+/// Sub-classification of [`TestableCase::Constant`], which helps to avoid
+/// some redundant ad-hoc checks when preparing and lowering tests.
+#[derive(Debug, Clone)]
+enum PatConstKind {
+    /// The primitive `bool` type, which is like an integer but simpler,
+    /// having only two values.
+    Bool,
+    /// Primitive unsigned/signed integer types, plus `char`.
+    /// These types interact nicely with `SwitchInt`.
+    IntOrChar,
+    /// Floating-point primitives, e.g. `f32`, `f64`.
+    /// These types don't support `SwitchInt` and require an equality test,
+    /// but can also interact with range pattern tests.
+    Float,
+    /// Any other constant-pattern is usually tested via some kind of equality
+    /// check. Types that might be encountered here include:
+    /// - `&str`
+    /// - raw pointers derived from integer values
+    /// - pattern types, e.g. `pattern_type!(u32 is 1..)`
+    Other,
+}
+
 /// Node in a tree of "match pairs", where each pair consists of a place to be
 /// tested, and a test to perform on that place.
 ///

compiler/rustc_mir_build/src/builder/matches/test.rs

@@ -19,7 +19,9 @@ use rustc_span::{DUMMY_SP, Span, Symbol, sym};
 use tracing::{debug, instrument};
 
 use crate::builder::Builder;
-use crate::builder::matches::{MatchPairTree, Test, TestBranch, TestKind, TestableCase};
+use crate::builder::matches::{
+    MatchPairTree, PatConstKind, Test, TestBranch, TestKind, TestableCase,
+};
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// Identifies what test is needed to decide if `match_pair` is applicable.
@@ -32,11 +34,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         let kind = match match_pair.testable_case {
             TestableCase::Variant { adt_def, variant_index: _ } => TestKind::Switch { adt_def },
 
-            TestableCase::Constant { .. } if match_pair.pattern_ty.is_bool() => TestKind::If,
-            TestableCase::Constant { .. } if is_switch_ty(match_pair.pattern_ty) => {
+            TestableCase::Constant { value: _, kind: PatConstKind::Bool } => TestKind::If,
+            TestableCase::Constant { value: _, kind: PatConstKind::IntOrChar } => {
                 TestKind::SwitchInt
             }
-            TestableCase::Constant { value } => {
+            TestableCase::Constant { value, kind: PatConstKind::Float } => {
+                TestKind::Eq { value, cast_ty: match_pair.pattern_ty }
+            }
+            TestableCase::Constant { value, kind: PatConstKind::Other } => {
                 TestKind::Eq { value, cast_ty: match_pair.pattern_ty }
             }
 
@@ -491,11 +496,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     }
 }
 
-/// Returns true if this type be used with [`TestKind::SwitchInt`].
-pub(crate) fn is_switch_ty(ty: Ty<'_>) -> bool {
-    ty.is_integral() || ty.is_char()
-}
-
 fn trait_method<'tcx>(
     tcx: TyCtxt<'tcx>,
     trait_def_id: DefId,