Auto merge of #67343 - ecstatic-morse:qualif-structural-match, r=pnkfelix

Const qualification for `StructuralEq`

Furthers #62411. Resolves #62614.

The goal of this PR is to implement the logic in #67088 on the MIR instead of the HIR. It uses the `Qualif` trait to track `StructuralPartialEq`/`StructuralEq` in the final value of a `const`. Then, if we encounter a constant during HAIR lowering whose value may not be structurally matchable, we emit the `indirect_structural_match` lint.

This PR contains all the tests present in #67088 and emits the proper warnings for the corner cases. This PR does not handle #65466, which would require that we be [more aggressive](https://github.com/rust-lang/rust/blob/42abbd8878d3b67238f3611b0587c704ba94f39c/src/librustc_mir_build/hair/pattern/const_to_pat.rs#L126-L130) when checking matched types for `PartialEq`. I think that should be done separately.

Because this works on MIR and uses dataflow, this PR should accept more cases than #67088. Notably, the qualifs in the final value of a const are encoded cross-crate, so matching on a constant whose value is defined in another crate to be `Option::<TyWithCustomEqImpl>::None` should work. Additionally, if a `const` has branching/looping, we will only emit the warning if any possible control flow path could result in a type with a custom `PartialEq` impl ending up as the final value of a `const`. I'm not sure how #67088 handled this.

AFAIK, it's not settled that these are the semantics we actually want: it's just how the `Qualif` framework happens to work. If the cross-crate part is undesirable, it would be quite easy to change the result of `mir_const_qualif().custom_eq` to `true` before encoding it in the crate metadata. This way, other crates would have to assume that all publicly exported constants may not be safe for matching.

r? @pnkfelix
cc @eddyb

Author: bors

src/librustc_middle/mir/query.rs

@@ -80,6 +80,7 @@ pub struct BorrowCheckResult<'tcx> {
 pub struct ConstQualifs {
     pub has_mut_interior: bool,
     pub needs_drop: bool,
+    pub custom_eq: bool,
 }
 
 /// After we borrow check a closure, we are left with various

src/librustc_mir/transform/check_consts/qualifs.rs

@@ -2,16 +2,19 @@
 //!
 //! See the `Qualif` trait for more info.
 
+use rustc_infer::infer::TyCtxtInferExt;
 use rustc_middle::mir::*;
-use rustc_middle::ty::{self, AdtDef, Ty};
+use rustc_middle::ty::{self, subst::SubstsRef, AdtDef, Ty};
 use rustc_span::DUMMY_SP;
+use rustc_trait_selection::traits;
 
 use super::ConstCx;
 
 pub fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> ConstQualifs {
     ConstQualifs {
         has_mut_interior: HasMutInterior::in_any_value_of_ty(cx, ty),
         needs_drop: NeedsDrop::in_any_value_of_ty(cx, ty),
+        custom_eq: CustomEq::in_any_value_of_ty(cx, ty),
     }
 }
 
@@ -53,7 +56,11 @@ pub trait Qualif {
     /// with a custom `Drop` impl is inherently `NeedsDrop`.
     ///
     /// Returning `true` for `in_adt_inherently` but `false` for `in_any_value_of_ty` is unsound.
-    fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &AdtDef) -> bool;
+    fn in_adt_inherently(
+        cx: &ConstCx<'_, 'tcx>,
+        adt: &'tcx AdtDef,
+        substs: SubstsRef<'tcx>,
+    ) -> bool;
 }
 
 /// Constant containing interior mutability (`UnsafeCell<T>`).
@@ -74,7 +81,7 @@ impl Qualif for HasMutInterior {
         !ty.is_freeze(cx.tcx, cx.param_env, DUMMY_SP)
     }
 
-    fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &AdtDef) -> bool {
+    fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &'tcx AdtDef, _: SubstsRef<'tcx>) -> bool {
         // Exactly one type, `UnsafeCell`, has the `HasMutInterior` qualif inherently.
         // It arises structurally for all other types.
         Some(adt.did) == cx.tcx.lang_items().unsafe_cell_type()
@@ -99,11 +106,44 @@ impl Qualif for NeedsDrop {
         ty.needs_drop(cx.tcx, cx.param_env)
     }
 
-    fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &AdtDef) -> bool {
+    fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &'tcx AdtDef, _: SubstsRef<'tcx>) -> bool {
         adt.has_dtor(cx.tcx)
     }
 }
 
+/// A constant that cannot be used as part of a pattern in a `match` expression.
+pub struct CustomEq;
+
+impl Qualif for CustomEq {
+    const ANALYSIS_NAME: &'static str = "flow_custom_eq";
+
+    fn in_qualifs(qualifs: &ConstQualifs) -> bool {
+        qualifs.custom_eq
+    }
+
+    fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool {
+        // If *any* component of a composite data type does not implement `Structural{Partial,}Eq`,
+        // we know that at least some values of that type are not structural-match. I say "some"
+        // because that component may be part of an enum variant (e.g.,
+        // `Option::<NonStructuralMatchTy>::Some`), in which case some values of this type may be
+        // structural-match (`Option::None`).
+        let id = cx.tcx.hir().local_def_id_to_hir_id(cx.def_id.as_local().unwrap());
+        traits::search_for_structural_match_violation(id, cx.body.span, cx.tcx, ty).is_some()
+    }
+
+    fn in_adt_inherently(
+        cx: &ConstCx<'_, 'tcx>,
+        adt: &'tcx AdtDef,
+        substs: SubstsRef<'tcx>,
+    ) -> bool {
+        let ty = cx.tcx.mk_ty(ty::Adt(adt, substs));
+        let id = cx.tcx.hir().local_def_id_to_hir_id(cx.def_id.as_local().unwrap());
+        cx.tcx
+            .infer_ctxt()
+            .enter(|infcx| !traits::type_marked_structural(id, cx.body.span, &infcx, ty))
+    }
+}
+
 // FIXME: Use `mir::visit::Visitor` for the `in_*` functions if/when it supports early return.
 
 /// Returns `true` if this `Rvalue` contains qualif `Q`.
@@ -147,8 +187,8 @@ where
         Rvalue::Aggregate(kind, operands) => {
             // Return early if we know that the struct or enum being constructed is always
             // qualified.
-            if let AggregateKind::Adt(def, ..) = **kind {
-                if Q::in_adt_inherently(cx, def) {
+            if let AggregateKind::Adt(def, _, substs, ..) = **kind {
+                if Q::in_adt_inherently(cx, def, substs) {
                     return true;
                 }
             }

src/librustc_mir/transform/check_consts/validation.rs

@@ -17,7 +17,7 @@ use std::borrow::Cow;
 use std::ops::Deref;
 
 use super::ops::{self, NonConstOp};
-use super::qualifs::{self, HasMutInterior, NeedsDrop};
+use super::qualifs::{self, CustomEq, HasMutInterior, NeedsDrop};
 use super::resolver::FlowSensitiveAnalysis;
 use super::{is_lang_panic_fn, ConstCx, ConstKind, Qualif};
 use crate::const_eval::{is_const_fn, is_unstable_const_fn};
@@ -142,9 +142,35 @@ impl Qualifs<'mir, 'tcx> {
 
         let return_loc = ccx.body.terminator_loc(return_block);
 
+        let custom_eq = match ccx.const_kind() {
+            // We don't care whether a `const fn` returns a value that is not structurally
+            // matchable. Functions calls are opaque and always use type-based qualification, so
+            // this value should never be used.
+            ConstKind::ConstFn => true,
+
+            // If we know that all values of the return type are structurally matchable, there's no
+            // need to run dataflow.
+            ConstKind::Const | ConstKind::Static | ConstKind::StaticMut
+                if !CustomEq::in_any_value_of_ty(ccx, ccx.body.return_ty()) =>
+            {
+                false
+            }
+
+            ConstKind::Const | ConstKind::Static | ConstKind::StaticMut => {
+                let mut cursor = FlowSensitiveAnalysis::new(CustomEq, ccx)
+                    .into_engine(ccx.tcx, &ccx.body, ccx.def_id)
+                    .iterate_to_fixpoint()
+                    .into_results_cursor(&ccx.body);
+
+                cursor.seek_after(return_loc);
+                cursor.contains(RETURN_PLACE)
+            }
+        };
+
         ConstQualifs {
             needs_drop: self.needs_drop(ccx, RETURN_PLACE, return_loc),
             has_mut_interior: self.has_mut_interior(ccx, RETURN_PLACE, return_loc),
+            custom_eq,
         }
     }
 }

src/librustc_mir_build/hair/pattern/const_to_pat.rs

@@ -22,13 +22,14 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> {
         cv: &'tcx ty::Const<'tcx>,
         id: hir::HirId,
         span: Span,
+        mir_structural_match_violation: bool,
     ) -> Pat<'tcx> {
         debug!("const_to_pat: cv={:#?} id={:?}", cv, id);
         debug!("const_to_pat: cv.ty={:?} span={:?}", cv.ty, span);
 
         self.tcx.infer_ctxt().enter(|infcx| {
             let mut convert = ConstToPat::new(self, id, span, infcx);
-            convert.to_pat(cv)
+            convert.to_pat(cv, mir_structural_match_violation)
         })
     }
 }
@@ -81,7 +82,11 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> {
         traits::type_marked_structural(self.id, self.span, &self.infcx, ty)
     }
 
-    fn to_pat(&mut self, cv: &'tcx ty::Const<'tcx>) -> Pat<'tcx> {
+    fn to_pat(
+        &mut self,
+        cv: &'tcx ty::Const<'tcx>,
+        mir_structural_match_violation: bool,
+    ) -> Pat<'tcx> {
         // This method is just a wrapper handling a validity check; the heavy lifting is
         // performed by the recursive `recur` method, which is not meant to be
         // invoked except by this method.
@@ -100,6 +105,11 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> {
                 "search_for_structural_match_violation cv.ty: {:?} returned: {:?}",
                 cv.ty, structural
             );
+
+            if structural.is_none() && mir_structural_match_violation {
+                bug!("MIR const-checker found novel structural match violation");
+            }
+
             if let Some(non_sm_ty) = structural {
                 let adt_def = match non_sm_ty {
                     traits::NonStructuralMatchTy::Adt(adt_def) => adt_def,
@@ -146,13 +156,18 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> {
                 if !ty_is_partial_eq {
                     // span_fatal avoids ICE from resolution of non-existent method (rare case).
                     self.tcx().sess.span_fatal(self.span, &make_msg());
-                } else {
+                } else if mir_structural_match_violation {
                     self.tcx().struct_span_lint_hir(
                         lint::builtin::INDIRECT_STRUCTURAL_MATCH,
                         self.id,
                         self.span,
                         |lint| lint.build(&make_msg()).emit(),
                     );
+                } else {
+                    debug!(
+                        "`search_for_structural_match_violation` found one, but `CustomEq` was \
+                          not in the qualifs for that `const`"
+                    );
                 }
             }
         }

src/librustc_mir_build/hair/pattern/mod.rs

@@ -15,7 +15,7 @@ use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
 use rustc_hir::pat_util::EnumerateAndAdjustIterator;
 use rustc_hir::RangeEnd;
 use rustc_index::vec::Idx;
-use rustc_middle::mir::interpret::{get_slice_bytes, sign_extend, ConstValue, ErrorHandled};
+use rustc_middle::mir::interpret::{get_slice_bytes, sign_extend, ConstValue};
 use rustc_middle::mir::interpret::{LitToConstError, LitToConstInput};
 use rustc_middle::mir::UserTypeProjection;
 use rustc_middle::mir::{BorrowKind, Field, Mutability};
@@ -762,69 +762,80 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> {
     fn lower_path(&mut self, qpath: &hir::QPath<'_>, id: hir::HirId, span: Span) -> Pat<'tcx> {
         let ty = self.tables.node_type(id);
         let res = self.tables.qpath_res(qpath, id);
-        let is_associated_const = match res {
-            Res::Def(DefKind::AssocConst, _) => true,
-            _ => false,
+
+        let pat_from_kind = |kind| Pat { span, ty, kind: Box::new(kind) };
+
+        let (def_id, is_associated_const) = match res {
+            Res::Def(DefKind::Const, def_id) => (def_id, false),
+            Res::Def(DefKind::AssocConst, def_id) => (def_id, true),
+
+            _ => return pat_from_kind(self.lower_variant_or_leaf(res, id, span, ty, vec![])),
         };
-        let kind = match res {
-            Res::Def(DefKind::Const | DefKind::AssocConst, def_id) => {
-                let substs = self.tables.node_substs(id);
-                // Use `Reveal::All` here because patterns are always monomorphic even if their function isn't.
-                match self.tcx.const_eval_resolve(
-                    self.param_env.with_reveal_all(),
-                    def_id,
-                    substs,
-                    None,
-                    Some(span),
-                ) {
-                    Ok(value) => {
-                        let const_ =
-                            ty::Const::from_value(self.tcx, value, self.tables.node_type(id));
-
-                        let pattern = self.const_to_pat(&const_, id, span);
-                        if !is_associated_const {
-                            return pattern;
-                        }
 
-                        let user_provided_types = self.tables().user_provided_types();
-                        return if let Some(u_ty) = user_provided_types.get(id) {
-                            let user_ty = PatTyProj::from_user_type(*u_ty);
-                            Pat {
-                                span,
-                                kind: Box::new(PatKind::AscribeUserType {
-                                    subpattern: pattern,
-                                    ascription: Ascription {
-                                        /// Note that use `Contravariant` here. See the
-                                        /// `variance` field documentation for details.
-                                        variance: ty::Variance::Contravariant,
-                                        user_ty,
-                                        user_ty_span: span,
-                                    },
-                                }),
-                                ty: const_.ty,
-                            }
-                        } else {
-                            pattern
-                        };
-                    }
-                    Err(ErrorHandled::TooGeneric) => {
-                        self.errors.push(if is_associated_const {
-                            PatternError::AssocConstInPattern(span)
-                        } else {
-                            PatternError::StaticInPattern(span)
-                        });
-                        PatKind::Wild
-                    }
-                    Err(_) => {
-                        self.tcx.sess.span_err(span, "could not evaluate constant pattern");
-                        PatKind::Wild
-                    }
-                }
+        // Use `Reveal::All` here because patterns are always monomorphic even if their function
+        // isn't.
+        let param_env_reveal_all = self.param_env.with_reveal_all();
+        let substs = self.tables.node_substs(id);
+        let instance = match ty::Instance::resolve(self.tcx, param_env_reveal_all, def_id, substs) {
+            Ok(Some(i)) => i,
+            Ok(None) => {
+                self.errors.push(if is_associated_const {
+                    PatternError::AssocConstInPattern(span)
+                } else {
+                    PatternError::StaticInPattern(span)
+                });
+
+                return pat_from_kind(PatKind::Wild);
+            }
+
+            Err(_) => {
+                self.tcx.sess.span_err(span, "could not evaluate constant pattern");
+                return pat_from_kind(PatKind::Wild);
             }
-            _ => self.lower_variant_or_leaf(res, id, span, ty, vec![]),
         };
 
-        Pat { span, ty, kind: Box::new(kind) }
+        // `mir_const_qualif` must be called with the `DefId` of the item where the const is
+        // defined, not where it is declared. The difference is significant for associated
+        // constants.
+        let mir_structural_match_violation = self.tcx.mir_const_qualif(instance.def_id()).custom_eq;
+        debug!("mir_structural_match_violation({:?}) -> {}", qpath, mir_structural_match_violation);
+
+        match self.tcx.const_eval_instance(param_env_reveal_all, instance, Some(span)) {
+            Ok(value) => {
+                let const_ = ty::Const::from_value(self.tcx, value, self.tables.node_type(id));
+
+                let pattern = self.const_to_pat(&const_, id, span, mir_structural_match_violation);
+
+                if !is_associated_const {
+                    return pattern;
+                }
+
+                let user_provided_types = self.tables().user_provided_types();
+                if let Some(u_ty) = user_provided_types.get(id) {
+                    let user_ty = PatTyProj::from_user_type(*u_ty);
+                    Pat {
+                        span,
+                        kind: Box::new(PatKind::AscribeUserType {
+                            subpattern: pattern,
+                            ascription: Ascription {
+                                /// Note that use `Contravariant` here. See the
+                                /// `variance` field documentation for details.
+                                variance: ty::Variance::Contravariant,
+                                user_ty,
+                                user_ty_span: span,
+                            },
+                        }),
+                        ty: const_.ty,
+                    }
+                } else {
+                    pattern
+                }
+            }
+            Err(_) => {
+                self.tcx.sess.span_err(span, "could not evaluate constant pattern");
+                pat_from_kind(PatKind::Wild)
+            }
+        }
     }
 
     /// Converts literals, paths and negation of literals to patterns.
@@ -849,7 +860,7 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> {
 
             let lit_input = LitToConstInput { lit: &lit.node, ty: self.tables.expr_ty(expr), neg };
             match self.tcx.at(expr.span).lit_to_const(lit_input) {
-                Ok(val) => *self.const_to_pat(val, expr.hir_id, lit.span).kind,
+                Ok(val) => *self.const_to_pat(val, expr.hir_id, lit.span, false).kind,
                 Err(LitToConstError::UnparseableFloat) => {
                     self.errors.push(PatternError::FloatBug);
                     PatKind::Wild