From 05c39438e295389e30a580a5376bbed83b1ddfa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 26 Dec 2024 01:30:29 +0000 Subject: [PATCH 1/4] Account for `for<'a>` types when checking for non-structural type in constant as pattern When we encounter a constant in a pattern, we check if it is non-structural. If so, we check if the type implements `PartialEq`, but for types with escaping bound vars the check would be incorrect as is, so we break early. This is ok because these types would be filtered anyways. Fix #134764. --- .../src/thir/pattern/const_to_pat.rs | 13 ++++++++++--- .../non_structural_with_escaping_bounds.rs | 15 +++++++++++++++ .../non_structural_with_escaping_bounds.stderr | 15 +++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.rs create mode 100644 tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr diff --git a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs index 2b3c98db966cd..a1d9b9024ab32 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs @@ -8,7 +8,9 @@ use rustc_infer::infer::TyCtxtInferExt; use rustc_infer::traits::Obligation; use rustc_middle::mir::interpret::ErrorHandled; use rustc_middle::thir::{FieldPat, Pat, PatKind}; -use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt, TypeVisitor, ValTree}; +use rustc_middle::ty::{ + self, Ty, TyCtxt, TypeSuperVisitable, TypeVisitableExt, TypeVisitor, ValTree, +}; use rustc_middle::{mir, span_bug}; use rustc_span::def_id::DefId; use rustc_span::{Span, sym}; @@ -387,7 +389,9 @@ fn extend_type_not_partial_eq<'tcx>( impl<'tcx> TypeVisitor> for UsedParamsNeedInstantiationVisitor<'tcx> { fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result { - if let ty::Adt(def, _args) = ty.kind() { + if let ty::Adt(def, _args) = ty.kind() + && !ty.has_escaping_bound_vars() + { let ty_def_id = def.did(); let ty_def_span = self.tcx.def_span(ty_def_id); let (impls_partial_eq, derived, structural, impl_def_id) = @@ -412,7 +416,6 @@ fn extend_type_not_partial_eq<'tcx>( _ => {} }; } - use rustc_middle::ty::TypeSuperVisitable; ty.super_visit_with(self) } } @@ -468,6 +471,10 @@ fn type_has_partial_eq_impl<'tcx>( let partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::PartialEq, None); let structural_partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::StructuralPeq, None); + if ty.has_escaping_bound_vars() { + return (false, false, false, None); + } + let partial_eq_obligation = Obligation::new( tcx, ObligationCause::dummy(), diff --git a/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.rs b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.rs new file mode 100644 index 0000000000000..e5d095fd61780 --- /dev/null +++ b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.rs @@ -0,0 +1,15 @@ +#![feature(structural_match)] +impl std::marker::StructuralPartialEq for O { } + +enum O { + Some(*const T), + None, +} + +const C: O Fn(Box)> = O::None; + +fn main() { + match O::None { + C => (), //~ ERROR constant of non-structural type + } +} diff --git a/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr new file mode 100644 index 0000000000000..47378569084a4 --- /dev/null +++ b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr @@ -0,0 +1,15 @@ +error: constant of non-structural type `O Fn(Box)>` in a pattern + --> $DIR/non_structural_with_escaping_bounds.rs:13:9 + | +LL | const C: O Fn(Box)> = O::None; + | ----------------------------------------------- constant defined here +... +LL | C => (), + | ^ constant of non-structural type + | + = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details + = note: `std::alloc::Global` must be annotated with `#[derive(PartialEq)]` to be usable in patterns + = note: `std::alloc::Global` must be annotated with `#[derive(PartialEq)]` to be usable in patterns + +error: aborting due to 1 previous error + From 91425d0ef8bfdfb7f11b7528d026d1a245743c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 26 Dec 2024 01:35:43 +0000 Subject: [PATCH 2/4] Avoid duplicated note --- .../src/thir/pattern/const_to_pat.rs | 14 ++++++++------ .../non_structural_with_escaping_bounds.stderr | 1 - 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs index a1d9b9024ab32..814da66ff0a91 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs @@ -381,10 +381,10 @@ fn extend_type_not_partial_eq<'tcx>( adts_without_partialeq: FxHashSet, /// The user has written `impl PartialEq for Ty` which means it's non-structual, /// but we don't have a span to point at, so we'll just add them as a `note`. - manual: Vec>, + manual: FxHashSet>, /// The type has no `PartialEq` implementation, neither manual or derived, but /// we don't have a span to point at, so we'll just add them as a `note`. - without: Vec>, + without: FxHashSet>, } impl<'tcx> TypeVisitor> for UsedParamsNeedInstantiationVisitor<'tcx> { @@ -408,10 +408,10 @@ fn extend_type_not_partial_eq<'tcx>( self.adts_without_partialeq.insert(ty_def_span); } (true, false, _, _) => { - self.manual.push(ty); + self.manual.insert(ty); } (false, _, _, _) => { - self.without.push(ty); + self.without.insert(ty); } _ => {} }; @@ -424,8 +424,8 @@ fn extend_type_not_partial_eq<'tcx>( typing_env, adts_with_manual_partialeq: FxHashSet::default(), adts_without_partialeq: FxHashSet::default(), - manual: vec![], - without: vec![], + manual: FxHashSet::default(), + without: FxHashSet::default(), }; v.visit_ty(ty); #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering @@ -439,11 +439,13 @@ fn extend_type_not_partial_eq<'tcx>( "must be annotated with `#[derive(PartialEq)]` to be usable in patterns", ); } + #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering for ty in v.manual { err.note(format!( "`{ty}` must be annotated with `#[derive(PartialEq)]` to be usable in patterns, manual `impl`s are not sufficient; see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details" )); } + #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering for ty in v.without { err.note(format!( "`{ty}` must be annotated with `#[derive(PartialEq)]` to be usable in patterns" diff --git a/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr index 47378569084a4..5ec6ecc3070fd 100644 --- a/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr +++ b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr @@ -9,7 +9,6 @@ LL | C => (), | = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details = note: `std::alloc::Global` must be annotated with `#[derive(PartialEq)]` to be usable in patterns - = note: `std::alloc::Global` must be annotated with `#[derive(PartialEq)]` to be usable in patterns error: aborting due to 1 previous error From 919f672c3dabfe33c641a29bbe51011936a82896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 26 Dec 2024 01:40:43 +0000 Subject: [PATCH 3/4] Avoid unnecessary note when type has escaping bounds --- .../rustc_mir_build/src/thir/pattern/const_to_pat.rs | 12 +++++++++--- .../non_structural_with_escaping_bounds.stderr | 1 - 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs index 814da66ff0a91..b53099b0abbb7 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs @@ -385,13 +385,15 @@ fn extend_type_not_partial_eq<'tcx>( /// The type has no `PartialEq` implementation, neither manual or derived, but /// we don't have a span to point at, so we'll just add them as a `note`. without: FxHashSet>, + /// If any of the subtypes has escaping bounds (`for<'a>`), we won't emit a note. + has_escaping_bound_vars: bool, } impl<'tcx> TypeVisitor> for UsedParamsNeedInstantiationVisitor<'tcx> { fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result { - if let ty::Adt(def, _args) = ty.kind() - && !ty.has_escaping_bound_vars() - { + if ty.has_escaping_bound_vars() { + self.has_escaping_bound_vars = true; + } else if let ty::Adt(def, _args) = ty.kind() { let ty_def_id = def.did(); let ty_def_span = self.tcx.def_span(ty_def_id); let (impls_partial_eq, derived, structural, impl_def_id) = @@ -426,8 +428,12 @@ fn extend_type_not_partial_eq<'tcx>( adts_without_partialeq: FxHashSet::default(), manual: FxHashSet::default(), without: FxHashSet::default(), + has_escaping_bound_vars: false, }; v.visit_ty(ty); + if v.has_escaping_bound_vars { + return; + } #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering for span in v.adts_with_manual_partialeq { err.span_note(span, "the `PartialEq` trait must be derived, manual `impl`s are not sufficient; see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details"); diff --git a/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr index 5ec6ecc3070fd..371be9982f708 100644 --- a/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr +++ b/tests/ui/consts/const_in_pattern/non_structural_with_escaping_bounds.stderr @@ -8,7 +8,6 @@ LL | C => (), | ^ constant of non-structural type | = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details - = note: `std::alloc::Global` must be annotated with `#[derive(PartialEq)]` to be usable in patterns error: aborting due to 1 previous error From 857918e9bcb22476d7d4477ca335b7c44d315d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 7 Jan 2025 22:33:27 +0000 Subject: [PATCH 4/4] review comments Replace tuple with struct and remove unnecessary early return. --- .../src/thir/pattern/const_to_pat.rs | 124 ++++++++++-------- 1 file changed, 67 insertions(+), 57 deletions(-) diff --git a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs index b53099b0abbb7..3853b95f78b4c 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs @@ -1,3 +1,5 @@ +use core::ops::ControlFlow; + use rustc_abi::{FieldIdx, VariantIdx}; use rustc_apfloat::Float; use rustc_data_structures::fx::FxHashSet; @@ -187,7 +189,7 @@ impl<'tcx> ConstToPat<'tcx> { if !inlined_const_as_pat.references_error() { // Always check for `PartialEq` if we had no other errors yet. - if !type_has_partial_eq_impl(self.tcx, typing_env, ty).0 { + if !type_has_partial_eq_impl(self.tcx, typing_env, ty).has_impl { let mut err = self.tcx.dcx().create_err(TypeNotPartialEq { span: self.span, ty }); extend_type_not_partial_eq(self.tcx, typing_env, ty, &mut err); return self.mk_err(err, ty); @@ -221,12 +223,13 @@ impl<'tcx> ConstToPat<'tcx> { // Extremely important check for all ADTs! Make sure they opted-in to be used in // patterns. debug!("adt_def {:?} has !type_marked_structural for cv.ty: {:?}", adt_def, ty); - let (_impls_partial_eq, derived, structural, impl_def_id) = - type_has_partial_eq_impl(self.tcx, self.typing_env, ty); + let PartialEqImplStatus { + is_derived, structural_partial_eq, non_blanket_impl, .. + } = type_has_partial_eq_impl(self.tcx, self.typing_env, ty); let (manual_partialeq_impl_span, manual_partialeq_impl_note) = - match (structural, impl_def_id) { + match (structural_partial_eq, non_blanket_impl) { (true, _) => (None, false), - (_, Some(def_id)) if def_id.is_local() && !derived => { + (_, Some(def_id)) if def_id.is_local() && !is_derived => { (Some(tcx.def_span(def_id)), false) } _ => (None, true), @@ -385,40 +388,46 @@ fn extend_type_not_partial_eq<'tcx>( /// The type has no `PartialEq` implementation, neither manual or derived, but /// we don't have a span to point at, so we'll just add them as a `note`. without: FxHashSet>, - /// If any of the subtypes has escaping bounds (`for<'a>`), we won't emit a note. - has_escaping_bound_vars: bool, } impl<'tcx> TypeVisitor> for UsedParamsNeedInstantiationVisitor<'tcx> { + type Result = ControlFlow<()>; fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result { - if ty.has_escaping_bound_vars() { - self.has_escaping_bound_vars = true; - } else if let ty::Adt(def, _args) = ty.kind() { - let ty_def_id = def.did(); - let ty_def_span = self.tcx.def_span(ty_def_id); - let (impls_partial_eq, derived, structural, impl_def_id) = - type_has_partial_eq_impl(self.tcx, self.typing_env, ty); - match (impls_partial_eq, derived, structural, impl_def_id) { - (_, _, true, _) => {} - (true, false, _, Some(def_id)) if def_id.is_local() => { - self.adts_with_manual_partialeq.insert(self.tcx.def_span(def_id)); - } - (true, false, _, _) if ty_def_id.is_local() => { - self.adts_with_manual_partialeq.insert(ty_def_span); - } - (false, _, _, _) if ty_def_id.is_local() => { - self.adts_without_partialeq.insert(ty_def_span); - } - (true, false, _, _) => { - self.manual.insert(ty); - } - (false, _, _, _) => { - self.without.insert(ty); - } - _ => {} - }; + match ty.kind() { + ty::Dynamic(..) => return ControlFlow::Break(()), + ty::FnPtr(..) => return ControlFlow::Continue(()), + ty::Adt(def, _args) => { + let ty_def_id = def.did(); + let ty_def_span = self.tcx.def_span(ty_def_id); + let PartialEqImplStatus { + has_impl, + is_derived, + structural_partial_eq, + non_blanket_impl, + } = type_has_partial_eq_impl(self.tcx, self.typing_env, ty); + match (has_impl, is_derived, structural_partial_eq, non_blanket_impl) { + (_, _, true, _) => {} + (true, false, _, Some(def_id)) if def_id.is_local() => { + self.adts_with_manual_partialeq.insert(self.tcx.def_span(def_id)); + } + (true, false, _, _) if ty_def_id.is_local() => { + self.adts_with_manual_partialeq.insert(ty_def_span); + } + (false, _, _, _) if ty_def_id.is_local() => { + self.adts_without_partialeq.insert(ty_def_span); + } + (true, false, _, _) => { + self.manual.insert(ty); + } + (false, _, _, _) => { + self.without.insert(ty); + } + _ => {} + }; + ty.super_visit_with(self) + } + _ => ty.super_visit_with(self), } - ty.super_visit_with(self) } } let mut v = UsedParamsNeedInstantiationVisitor { @@ -428,10 +437,8 @@ fn extend_type_not_partial_eq<'tcx>( adts_without_partialeq: FxHashSet::default(), manual: FxHashSet::default(), without: FxHashSet::default(), - has_escaping_bound_vars: false, }; - v.visit_ty(ty); - if v.has_escaping_bound_vars { + if v.visit_ty(ty).is_break() { return; } #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering @@ -445,31 +452,38 @@ fn extend_type_not_partial_eq<'tcx>( "must be annotated with `#[derive(PartialEq)]` to be usable in patterns", ); } - #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering - for ty in v.manual { + #[allow(rustc::potential_query_instability)] + let mut manual: Vec<_> = v.manual.into_iter().map(|t| t.to_string()).collect(); + manual.sort(); + for ty in manual { err.note(format!( "`{ty}` must be annotated with `#[derive(PartialEq)]` to be usable in patterns, manual `impl`s are not sufficient; see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details" )); } - #[allow(rustc::potential_query_instability)] // Span labels will be sorted by the rendering - for ty in v.without { + #[allow(rustc::potential_query_instability)] + let mut without: Vec<_> = v.without.into_iter().map(|t| t.to_string()).collect(); + without.sort(); + for ty in without { err.note(format!( "`{ty}` must be annotated with `#[derive(PartialEq)]` to be usable in patterns" )); } } +#[derive(Debug)] +struct PartialEqImplStatus { + has_impl: bool, + is_derived: bool, + structural_partial_eq: bool, + non_blanket_impl: Option, +} + #[instrument(level = "trace", skip(tcx), ret)] fn type_has_partial_eq_impl<'tcx>( tcx: TyCtxt<'tcx>, typing_env: ty::TypingEnv<'tcx>, ty: Ty<'tcx>, -) -> ( - /* has impl */ bool, - /* is derived */ bool, - /* structural partial eq */ bool, - /* non-blanket impl */ Option, -) { +) -> PartialEqImplStatus { let (infcx, param_env) = tcx.infer_ctxt().build_with_typing_env(typing_env); // double-check there even *is* a semantic `PartialEq` to dispatch to. // @@ -479,10 +493,6 @@ fn type_has_partial_eq_impl<'tcx>( let partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::PartialEq, None); let structural_partial_eq_trait_id = tcx.require_lang_item(hir::LangItem::StructuralPeq, None); - if ty.has_escaping_bound_vars() { - return (false, false, false, None); - } - let partial_eq_obligation = Obligation::new( tcx, ObligationCause::dummy(), @@ -510,10 +520,10 @@ fn type_has_partial_eq_impl<'tcx>( // that patterns can only do things that the code could also do without patterns, but it is // needed for backwards compatibility. The actual pattern matching compares primitive values, // `PartialEq::eq` never gets invoked, so there's no risk of us running non-const code. - ( - infcx.predicate_must_hold_modulo_regions(&partial_eq_obligation), - automatically_derived, - structural_peq, - impl_def_id, - ) + PartialEqImplStatus { + has_impl: infcx.predicate_must_hold_modulo_regions(&partial_eq_obligation), + is_derived: automatically_derived, + structural_partial_eq: structural_peq, + non_blanket_impl: impl_def_id, + } }