Skip to content
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

lint towards rejecting consts in patterns that do not implement PartialEq #115893

Merged
merged 4 commits into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2310,6 +2310,57 @@ declare_lint! {
};
}

declare_lint! {
/// 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(const_patterns_without_partial_eq)]
///
/// trait EnumSetType {
/// type Repr;
/// }
///
/// enum Enum8 { }
/// impl EnumSetType for Enum8 {
/// type Repr = u8;
/// }
///
/// #[derive(PartialEq, Eq)]
/// struct EnumSet<T: EnumSetType> {
/// __enumset_underlying: T::Repr,
/// }
///
/// const CONST_SET: EnumSet<Enum8> = 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 CONST_PATTERNS_WITHOUT_PARTIAL_EQ,
Warn,
"constant in pattern does not implement `PartialEq`",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
reference: "issue #116122 <https://github.com/rust-lang/rust/issues/116122>",
};
}

declare_lint! {
/// The `ambiguous_associated_items` lint detects ambiguity between
/// [associated items] and [enum variants].
Expand Down Expand Up @@ -3356,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,
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_mir_build/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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)]`

Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_mir_build/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
37 changes: 28 additions & 9 deletions compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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());
Expand All @@ -178,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 };
Expand All @@ -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() {
Expand Down Expand Up @@ -238,13 +241,24 @@ 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_has_partial_eq_impl(cv.ty()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the potential alternative of checking for Eq here instead of PartialEq. This would then subsume the "disallow floats in patterns" lint as well. However with t-lang in the past having been disinclined to disallow float patterns, it's not clear that we want to require Eq.

self.tcx().emit_spanned_lint(
lint::builtin::CONST_PATTERNS_WITHOUT_PARTIAL_EQ,
self.id,
self.span,
NonPartialEqMatch { non_peq_ty: cv.ty() },
);
}
}

inlined_const_as_pat
}

#[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
Expand All @@ -259,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(
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/consts/const_in_pattern/issue-65466.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const C: &[O<B>] = &[O::None];
fn main() {
let x = O::None;
match &[x][..] {
C => (),
C => (), //~WARN: the type must implement `PartialEq`
//~| previously accepted
_ => (),
}
}
23 changes: 23 additions & 0 deletions tests/ui/consts/const_in_pattern/issue-65466.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
warning: to use a constant of type `&[O<B>]` 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 #116122 <https://github.com/rust-lang/rust/issues/116122>
= note: `#[warn(const_patterns_without_partial_eq)]` on by default

warning: 1 warning emitted

Future incompatibility report: Future breakage diagnostic:
warning: to use a constant of type `&[O<B>]` 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 #116122 <https://github.com/rust-lang/rust/issues/116122>
= note: `#[warn(const_patterns_without_partial_eq)]` on by default

Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ const CONST_SET: EnumSet<Enum8> = 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?"),
}
}
23 changes: 23 additions & 0 deletions tests/ui/match/issue-72896-non-partial-eq-const.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
warning: to use a constant of type `EnumSet<Enum8>` 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 #116122 <https://github.com/rust-lang/rust/issues/116122>
= note: `#[warn(const_patterns_without_partial_eq)]` on by default

warning: 1 warning emitted

Future incompatibility report: Future breakage diagnostic:
warning: to use a constant of type `EnumSet<Enum8>` 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 #116122 <https://github.com/rust-lang/rust/issues/116122>
= note: `#[warn(const_patterns_without_partial_eq)]` on by default

Loading