From c5fccb98ea8d853b7d332e077c3ed86401b160c7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 16 Sep 2023 14:15:48 +0200 Subject: [PATCH 1/4] work towards rejecting consts in patterns that do not implement PartialEq --- compiler/rustc_lint_defs/src/builtin.rs | 52 +++++++++++++++++++ compiler/rustc_mir_build/messages.ftl | 3 ++ compiler/rustc_mir_build/src/errors.rs | 6 +++ .../src/thir/pattern/const_to_pat.rs | 14 ++++- .../ui/consts/const_in_pattern/issue-65466.rs | 3 +- .../const_in_pattern/issue-65466.stderr | 23 ++++++++ ...rs => issue-72896-non-partial-eq-const.rs} | 3 +- .../issue-72896-non-partial-eq-const.stderr | 23 ++++++++ 8 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 tests/ui/consts/const_in_pattern/issue-65466.stderr rename tests/ui/match/{issue-72896.rs => issue-72896-non-partial-eq-const.rs} (78%) create mode 100644 tests/ui/match/issue-72896-non-partial-eq-const.stderr diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 1951db49e91ad..a749c52e2dd65 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -2310,6 +2310,57 @@ declare_lint! { }; } +declare_lint! { + /// The `match_without_partial_eq` lint detects constants that are used in patterns, + /// whose type does not implement `PartialEq`. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// #![deny(match_without_partial_eq)] + /// + /// trait EnumSetType { + /// type Repr; + /// } + /// + /// enum Enum8 { } + /// impl EnumSetType for Enum8 { + /// type Repr = u8; + /// } + /// + /// #[derive(PartialEq, Eq)] + /// struct EnumSet { + /// __enumset_underlying: T::Repr, + /// } + /// + /// const CONST_SET: EnumSet = EnumSet { __enumset_underlying: 3 }; + /// + /// fn main() { + /// match CONST_SET { + /// CONST_SET => { /* ok */ } + /// _ => panic!("match fell through?"), + /// } + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// Previous versions of Rust accepted constants in patterns, even if those constants' types + /// did not have `PartialEq` implemented. The compiler falls back to comparing the value + /// field-by-field. In the future we'd like to ensure that pattern matching always + /// follows `PartialEq` semantics, so that trait bound will become a requirement for + /// matching on constants. + pub MATCH_WITHOUT_PARTIAL_EQ, + Warn, + "constant in pattern does not implement `PartialEq`", + @future_incompatible = FutureIncompatibleInfo { + reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps, + reference: "issue #X ", + }; +} + declare_lint! { /// The `ambiguous_associated_items` lint detects ambiguity between /// [associated items] and [enum variants]. @@ -3389,6 +3440,7 @@ declare_lint_pass! { LOSSY_PROVENANCE_CASTS, MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS, MACRO_USE_EXTERN_CRATE, + MATCH_WITHOUT_PARTIAL_EQ, META_VARIABLE_MISUSE, MISSING_ABI, MISSING_FRAGMENT_SPECIFIER, diff --git a/compiler/rustc_mir_build/messages.ftl b/compiler/rustc_mir_build/messages.ftl index 938f3edd31b05..ce021923f647f 100644 --- a/compiler/rustc_mir_build/messages.ftl +++ b/compiler/rustc_mir_build/messages.ftl @@ -229,6 +229,9 @@ mir_build_non_exhaustive_patterns_type_not_empty = non-exhaustive patterns: type .suggestion = ensure that all possible cases are being handled by adding a match arm with a wildcard pattern as shown .help = ensure that all possible cases are being handled by adding a match arm with a wildcard pattern +mir_build_non_partial_eq_match = + to use a constant of type `{$non_peq_ty}` in a pattern, the type must implement `PartialEq` + mir_build_nontrivial_structural_match = to use a constant of type `{$non_sm_ty}` in a pattern, the constant's initializer must be trivial or `{$non_sm_ty}` must be annotated with `#[derive(PartialEq, Eq)]` diff --git a/compiler/rustc_mir_build/src/errors.rs b/compiler/rustc_mir_build/src/errors.rs index 3ff3387a78114..bee5ac550ddce 100644 --- a/compiler/rustc_mir_build/src/errors.rs +++ b/compiler/rustc_mir_build/src/errors.rs @@ -748,6 +748,12 @@ pub struct NontrivialStructuralMatch<'tcx> { pub non_sm_ty: Ty<'tcx>, } +#[derive(LintDiagnostic)] +#[diag(mir_build_non_partial_eq_match)] +pub struct NonPartialEqMatch<'tcx> { + pub non_peq_ty: Ty<'tcx>, +} + #[derive(LintDiagnostic)] #[diag(mir_build_overlapping_range_endpoints)] #[note] 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 00d9fe72cfcb3..41f31beff778e 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 @@ -16,8 +16,8 @@ use std::cell::Cell; use super::PatCtxt; use crate::errors::{ - FloatPattern, IndirectStructuralMatch, InvalidPattern, NontrivialStructuralMatch, - PointerPattern, TypeNotStructural, UnionPattern, UnsizedPattern, + FloatPattern, IndirectStructuralMatch, InvalidPattern, NonPartialEqMatch, + NontrivialStructuralMatch, PointerPattern, TypeNotStructural, UnionPattern, UnsizedPattern, }; impl<'a, 'tcx> PatCtxt<'a, 'tcx> { @@ -235,6 +235,16 @@ impl<'tcx> ConstToPat<'tcx> { PointerPattern, ); } + _ if !self.type_may_have_partial_eq_impl(cv.ty()) => { + // Value is structural-match but the type doesn't even implement `PartialEq`... + self.saw_const_match_lint.set(true); + self.tcx().emit_spanned_lint( + lint::builtin::MATCH_WITHOUT_PARTIAL_EQ, + self.id, + self.span, + NonPartialEqMatch { non_peq_ty: cv.ty() }, + ); + } _ => {} } } diff --git a/tests/ui/consts/const_in_pattern/issue-65466.rs b/tests/ui/consts/const_in_pattern/issue-65466.rs index 2b421f4c705ec..d45c32e170a6a 100644 --- a/tests/ui/consts/const_in_pattern/issue-65466.rs +++ b/tests/ui/consts/const_in_pattern/issue-65466.rs @@ -15,7 +15,8 @@ const C: &[O] = &[O::None]; fn main() { let x = O::None; match &[x][..] { - C => (), + C => (), //~WARN: the type must implement `PartialEq` + //~| previously accepted _ => (), } } diff --git a/tests/ui/consts/const_in_pattern/issue-65466.stderr b/tests/ui/consts/const_in_pattern/issue-65466.stderr new file mode 100644 index 0000000000000..89fddbf853d5f --- /dev/null +++ b/tests/ui/consts/const_in_pattern/issue-65466.stderr @@ -0,0 +1,23 @@ +warning: to use a constant of type `&[O]` in a pattern, the type must implement `PartialEq` + --> $DIR/issue-65466.rs:18:9 + | +LL | C => (), + | ^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #X + = note: `#[warn(match_without_partial_eq)]` on by default + +warning: 1 warning emitted + +Future incompatibility report: Future breakage diagnostic: +warning: to use a constant of type `&[O]` in a pattern, the type must implement `PartialEq` + --> $DIR/issue-65466.rs:18:9 + | +LL | C => (), + | ^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #X + = note: `#[warn(match_without_partial_eq)]` on by default + diff --git a/tests/ui/match/issue-72896.rs b/tests/ui/match/issue-72896-non-partial-eq-const.rs similarity index 78% rename from tests/ui/match/issue-72896.rs rename to tests/ui/match/issue-72896-non-partial-eq-const.rs index 3a8b82037310a..a3095f0be83c1 100644 --- a/tests/ui/match/issue-72896.rs +++ b/tests/ui/match/issue-72896-non-partial-eq-const.rs @@ -17,7 +17,8 @@ const CONST_SET: EnumSet = EnumSet { __enumset_underlying: 3 }; fn main() { match CONST_SET { - CONST_SET => { /* ok */ } + CONST_SET => { /* ok */ } //~WARN: must implement `PartialEq` + //~| previously accepted _ => panic!("match fell through?"), } } diff --git a/tests/ui/match/issue-72896-non-partial-eq-const.stderr b/tests/ui/match/issue-72896-non-partial-eq-const.stderr new file mode 100644 index 0000000000000..f32cf43953160 --- /dev/null +++ b/tests/ui/match/issue-72896-non-partial-eq-const.stderr @@ -0,0 +1,23 @@ +warning: to use a constant of type `EnumSet` in a pattern, the type must implement `PartialEq` + --> $DIR/issue-72896-non-partial-eq-const.rs:20:9 + | +LL | CONST_SET => { /* ok */ } + | ^^^^^^^^^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #X + = note: `#[warn(match_without_partial_eq)]` on by default + +warning: 1 warning emitted + +Future incompatibility report: Future breakage diagnostic: +warning: to use a constant of type `EnumSet` in a pattern, the type must implement `PartialEq` + --> $DIR/issue-72896-non-partial-eq-const.rs:20:9 + | +LL | CONST_SET => { /* ok */ } + | ^^^^^^^^^ + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #X + = note: `#[warn(match_without_partial_eq)]` on by default + From c3ed0c454eec4bb7fcf5b73f9d7584390fb692eb Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 17 Sep 2023 11:04:26 +0200 Subject: [PATCH 2/4] make sure we always emit the no-PartialEq lint, even if there were other lints --- .../src/thir/pattern/const_to_pat.rs | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 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 41f31beff778e..a390691c6756c 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 @@ -155,8 +155,9 @@ impl<'tcx> ConstToPat<'tcx> { }; if !self.saw_const_match_error.get() { - // If we were able to successfully convert the const to some pat, - // double-check that all types in the const implement `Structural`. + // If we were able to successfully convert the const to some pat (possibly with some + // lints, but no errors), double-check that all types in the const implement + // `Structural` and `PartialEq`. let structural = traits::search_for_structural_match_violation(self.span, self.tcx(), cv.ty()); @@ -192,8 +193,10 @@ impl<'tcx> ConstToPat<'tcx> { } else { let err = InvalidPattern { span: self.span, non_sm_ty }; self.tcx().sess.emit_err(err); - return Box::new(Pat { span: self.span, ty: cv.ty(), kind: PatKind::Wild }); } + // All branches above emitted an error. Don't print any more lints. + // The pattern we return is irrelevant since we errored. + return Box::new(Pat { span: self.span, ty: cv.ty(), kind: PatKind::Wild }); } else if !self.saw_const_match_lint.get() { if let Some(mir_structural_match_violation) = mir_structural_match_violation { match non_sm_ty.kind() { @@ -235,19 +238,20 @@ impl<'tcx> ConstToPat<'tcx> { PointerPattern, ); } - _ if !self.type_may_have_partial_eq_impl(cv.ty()) => { - // Value is structural-match but the type doesn't even implement `PartialEq`... - self.saw_const_match_lint.set(true); - self.tcx().emit_spanned_lint( - lint::builtin::MATCH_WITHOUT_PARTIAL_EQ, - self.id, - self.span, - NonPartialEqMatch { non_peq_ty: cv.ty() }, - ); - } _ => {} } } + + // Always check for `PartialEq`, even if we emitted other lints. (But not if there were + // any errors.) This ensures it shows up in cargo's future-compat reports as well. + if !self.type_may_have_partial_eq_impl(cv.ty()) { + self.tcx().emit_spanned_lint( + lint::builtin::MATCH_WITHOUT_PARTIAL_EQ, + self.id, + self.span, + NonPartialEqMatch { non_peq_ty: cv.ty() }, + ); + } } inlined_const_as_pat From b589976606c7f03b2000bed41ddf6149d96c587b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 24 Sep 2023 16:37:19 +0200 Subject: [PATCH 3/4] use a must_hold variant for checking PartialEq --- .../src/thir/pattern/const_to_pat.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 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 a390691c6756c..ed20ed4a84f0b 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 @@ -179,7 +179,7 @@ impl<'tcx> ConstToPat<'tcx> { } if let Some(non_sm_ty) = structural { - if !self.type_may_have_partial_eq_impl(cv.ty()) { + if !self.type_has_partial_eq_impl(cv.ty()) { if let ty::Adt(def, ..) = non_sm_ty.kind() { if def.is_union() { let err = UnionPattern { span: self.span }; @@ -244,7 +244,7 @@ impl<'tcx> ConstToPat<'tcx> { // Always check for `PartialEq`, even if we emitted other lints. (But not if there were // any errors.) This ensures it shows up in cargo's future-compat reports as well. - if !self.type_may_have_partial_eq_impl(cv.ty()) { + if !self.type_has_partial_eq_impl(cv.ty()) { self.tcx().emit_spanned_lint( lint::builtin::MATCH_WITHOUT_PARTIAL_EQ, self.id, @@ -258,7 +258,7 @@ impl<'tcx> ConstToPat<'tcx> { } #[instrument(level = "trace", skip(self), ret)] - fn type_may_have_partial_eq_impl(&self, ty: Ty<'tcx>) -> bool { + fn type_has_partial_eq_impl(&self, ty: Ty<'tcx>) -> bool { // double-check there even *is* a semantic `PartialEq` to dispatch to. // // (If there isn't, then we can safely issue a hard @@ -273,8 +273,13 @@ impl<'tcx> ConstToPat<'tcx> { ty::TraitRef::new(self.tcx(), partial_eq_trait_id, [ty, ty]), ); - // FIXME: should this call a `predicate_must_hold` variant instead? - self.infcx.predicate_may_hold(&partial_eq_obligation) + // This *could* accept a type that isn't actually `PartialEq`, because region bounds get + // ignored. However that should be pretty much impossible since consts that do not depend on + // generics can only mention the `'static` lifetime, and how would one have a type that's + // `PartialEq` for some lifetime but *not* for `'static`? If this ever becomes a problem + // we'll need to leave some sort of trace of this requirement in the MIR so that borrowck + // can ensure that the type really implements `PartialEq`. + self.infcx.predicate_must_hold_modulo_regions(&partial_eq_obligation) } fn field_pats( From a1d6fc43403fadc5564af50d6212d45d9aace84d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 25 Sep 2023 18:28:22 +0200 Subject: [PATCH 4/4] rename lint; add tracking issue --- compiler/rustc_lint_defs/src/builtin.rs | 10 +++++----- .../rustc_mir_build/src/thir/pattern/const_to_pat.rs | 2 +- tests/ui/consts/const_in_pattern/issue-65466.stderr | 8 ++++---- tests/ui/match/issue-72896-non-partial-eq-const.stderr | 8 ++++---- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index a749c52e2dd65..00d07cbb2e398 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -2311,13 +2311,13 @@ declare_lint! { } declare_lint! { - /// The `match_without_partial_eq` lint detects constants that are used in patterns, + /// The `const_patterns_without_partial_eq` lint detects constants that are used in patterns, /// whose type does not implement `PartialEq`. /// /// ### Example /// /// ```rust,compile_fail - /// #![deny(match_without_partial_eq)] + /// #![deny(const_patterns_without_partial_eq)] /// /// trait EnumSetType { /// type Repr; @@ -2352,12 +2352,12 @@ declare_lint! { /// field-by-field. In the future we'd like to ensure that pattern matching always /// follows `PartialEq` semantics, so that trait bound will become a requirement for /// matching on constants. - pub MATCH_WITHOUT_PARTIAL_EQ, + pub CONST_PATTERNS_WITHOUT_PARTIAL_EQ, Warn, "constant in pattern does not implement `PartialEq`", @future_incompatible = FutureIncompatibleInfo { reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps, - reference: "issue #X ", + reference: "issue #116122 ", }; } @@ -3407,6 +3407,7 @@ declare_lint_pass! { CONFLICTING_REPR_HINTS, CONST_EVALUATABLE_UNCHECKED, CONST_ITEM_MUTATION, + CONST_PATTERNS_WITHOUT_PARTIAL_EQ, DEAD_CODE, DEPRECATED, DEPRECATED_CFG_ATTR_CRATE_TYPE_NAME, @@ -3440,7 +3441,6 @@ declare_lint_pass! { LOSSY_PROVENANCE_CASTS, MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS, MACRO_USE_EXTERN_CRATE, - MATCH_WITHOUT_PARTIAL_EQ, META_VARIABLE_MISUSE, MISSING_ABI, MISSING_FRAGMENT_SPECIFIER, 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 ed20ed4a84f0b..4758ace73b61c 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 @@ -246,7 +246,7 @@ impl<'tcx> ConstToPat<'tcx> { // any errors.) This ensures it shows up in cargo's future-compat reports as well. if !self.type_has_partial_eq_impl(cv.ty()) { self.tcx().emit_spanned_lint( - lint::builtin::MATCH_WITHOUT_PARTIAL_EQ, + lint::builtin::CONST_PATTERNS_WITHOUT_PARTIAL_EQ, self.id, self.span, NonPartialEqMatch { non_peq_ty: cv.ty() }, diff --git a/tests/ui/consts/const_in_pattern/issue-65466.stderr b/tests/ui/consts/const_in_pattern/issue-65466.stderr index 89fddbf853d5f..9c80cb3a849c0 100644 --- a/tests/ui/consts/const_in_pattern/issue-65466.stderr +++ b/tests/ui/consts/const_in_pattern/issue-65466.stderr @@ -5,8 +5,8 @@ LL | C => (), | ^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #X - = note: `#[warn(match_without_partial_eq)]` on by default + = note: for more information, see issue #116122 + = note: `#[warn(const_patterns_without_partial_eq)]` on by default warning: 1 warning emitted @@ -18,6 +18,6 @@ LL | C => (), | ^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #X - = note: `#[warn(match_without_partial_eq)]` on by default + = note: for more information, see issue #116122 + = note: `#[warn(const_patterns_without_partial_eq)]` on by default diff --git a/tests/ui/match/issue-72896-non-partial-eq-const.stderr b/tests/ui/match/issue-72896-non-partial-eq-const.stderr index f32cf43953160..a7fc0cfc05457 100644 --- a/tests/ui/match/issue-72896-non-partial-eq-const.stderr +++ b/tests/ui/match/issue-72896-non-partial-eq-const.stderr @@ -5,8 +5,8 @@ LL | CONST_SET => { /* ok */ } | ^^^^^^^^^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #X - = note: `#[warn(match_without_partial_eq)]` on by default + = note: for more information, see issue #116122 + = note: `#[warn(const_patterns_without_partial_eq)]` on by default warning: 1 warning emitted @@ -18,6 +18,6 @@ LL | CONST_SET => { /* ok */ } | ^^^^^^^^^ | = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #X - = note: `#[warn(match_without_partial_eq)]` on by default + = note: for more information, see issue #116122 + = note: `#[warn(const_patterns_without_partial_eq)]` on by default