Skip to content

Aggressively check for non-PartialEq types in const_to_pat #72184

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

Closed
wants to merge 3 commits into from
Closed
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
204 changes: 133 additions & 71 deletions src/librustc_mir_build/hair/pattern/const_to_pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,82 +97,67 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> {

let inlined_const_as_pat = self.recur(cv);

if self.include_lint_checks && !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 self.saw_const_match_error.get() {
return inlined_const_as_pat;
}

let structural = self.search_for_structural_match_violation(cv.ty);
debug!(
"search_for_structural_match_violation cv.ty: {:?} returned: {:?}",
cv.ty, structural
);
// We eventually lower to a call to `PartialEq::eq` for this type, so ensure that this
// method actually exists.
if !self.ty_has_partial_eq_impl(cv.ty) {
let msg = if cv.ty.is_trait() {
"trait objects cannot be used in patterns".to_string()
} else {
format!("`{:?}` must implement `PartialEq` to be used in a pattern", cv.ty)
};

if structural.is_none() && mir_structural_match_violation {
bug!("MIR const-checker found novel structural match violation");
}
// Codegen will ICE if we continue compilation, so abort here.
self.tcx().sess.span_fatal(self.span, &msg);
}

if let Some(non_sm_ty) = structural {
let msg = match non_sm_ty {
traits::NonStructuralMatchTy::Adt(adt_def) => {
let path = self.tcx().def_path_str(adt_def.did);
format!(
"to use a constant of type `{}` in a pattern, \
if !self.include_lint_checks {
return inlined_const_as_pat;
}

// If we were able to successfully convert the const to some pat,
// double-check that all types in the const implement `Structural`.

let ty_violation = self.search_for_structural_match_violation(cv.ty);
debug!(
"search_for_structural_match_violation cv.ty: {:?} returned: {:?}",
cv.ty, ty_violation,
);

if mir_structural_match_violation {
let non_sm_ty =
ty_violation.expect("MIR const-checker found novel structural match violation");
let msg = match non_sm_ty {
traits::NonStructuralMatchTy::Adt(adt_def) => {
let path = self.tcx().def_path_str(adt_def.did);
format!(
"to use a constant of type `{}` in a pattern, \
`{}` must be annotated with `#[derive(PartialEq, Eq)]`",
path, path,
)
}
traits::NonStructuralMatchTy::Dynamic => {
"trait objects cannot be used in patterns".to_string()
}
traits::NonStructuralMatchTy::Param => {
bug!("use of constant whose type is a parameter inside a pattern")
}
};

// double-check there even *is* a semantic `PartialEq` to dispatch to.
//
// (If there isn't, then we can safely issue a hard
// error, because that's never worked, due to compiler
// using `PartialEq::eq` in this scenario in the past.)
//
// Note: To fix rust-lang/rust#65466, one could lift this check
// *before* any structural-match checking, and unconditionally error
// if `PartialEq` is not implemented. However, that breaks stable
// code at the moment, because types like `for <'a> fn(&'a ())` do
// not *yet* implement `PartialEq`. So for now we leave this here.
let ty_is_partial_eq: bool = {
let partial_eq_trait_id =
self.tcx().require_lang_item(EqTraitLangItem, Some(self.span));
let obligation: PredicateObligation<'_> = predicate_for_trait_def(
self.tcx(),
self.param_env,
ObligationCause::misc(self.span, self.id),
partial_eq_trait_id,
0,
cv.ty,
&[],
);
// FIXME: should this call a `predicate_must_hold` variant instead?
self.infcx.predicate_may_hold(&obligation)
};

if !ty_is_partial_eq {
// span_fatal avoids ICE from resolution of non-existent method (rare case).
self.tcx().sess.span_fatal(self.span, &msg);
} else if mir_structural_match_violation {
self.tcx().struct_span_lint_hir(
lint::builtin::INDIRECT_STRUCTURAL_MATCH,
self.id,
self.span,
|lint| lint.build(&msg).emit(),
);
} else {
debug!(
"`search_for_structural_match_violation` found one, but `CustomEq` was \
not in the qualifs for that `const`"
);
path, path,
)
}
}
traits::NonStructuralMatchTy::Dynamic => {
"trait objects cannot be used in patterns".to_string()
}
traits::NonStructuralMatchTy::Param => {
bug!("use of constant whose type is a parameter inside a pattern")
}
};

self.tcx().struct_span_lint_hir(
lint::builtin::INDIRECT_STRUCTURAL_MATCH,
self.id,
self.span,
|lint| lint.build(&msg).emit(),
);
} else if ty_violation.is_some() {
debug!(
"`search_for_structural_match_violation` found one, but `CustomEq` was \
not in the qualifs for that `const`"
);
}

inlined_const_as_pat
Expand Down Expand Up @@ -280,4 +265,81 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> {

Pat { span, ty: cv.ty, kind: Box::new(kind) }
}

fn ty_has_partial_eq_impl(&self, ty: Ty<'tcx>) -> bool {
let tcx = self.tcx();

let is_partial_eq = |ty| {
let partial_eq_trait_id = tcx.require_lang_item(EqTraitLangItem, Some(self.span));
let obligation: PredicateObligation<'_> = predicate_for_trait_def(
tcx,
self.param_env,
ObligationCause::misc(self.span, self.id),
partial_eq_trait_id,
0,
ty,
&[],
);

// FIXME: should this call a `predicate_must_hold` variant instead?
self.infcx.predicate_may_hold(&obligation)
};

// Higher-ranked function pointers, such as `for<'r> fn(&'r i32)` are allowed in patterns
// but do not satisfy `Self: PartialEq` due to shortcomings in the trait solver.
// Check for bare function pointers first since it is cheap to do so.
if let ty::FnPtr(_) = ty.kind {
return true;
}

// In general, types that appear in patterns need to implement `PartialEq`.
if is_partial_eq(ty) {
return true;
}

// HACK: The check for bare function pointers will miss generic types that are instantiated
// with a higher-ranked type (`for<'r> fn(&'r i32)`) as a parameter. To preserve backwards
// compatibility in this case, we must continue to allow types such as `Option<fn(&i32)>`.
//
//
// We accomplish this by replacing *all* late-bound lifetimes in the type with concrete
// ones. This leverages the fact that function pointers with no late-bound lifetimes do
// satisfy `PartialEq`. In other words, we transform `Option<for<'r> fn(&'r i32)>` to
// `Option<fn(&'erased i32)>` and again check whether `PartialEq` is satisfied.
// Obviously this is too permissive, but it is better than the old behavior, which
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be a good idea to construct an example of something that runs into this (and presumably causes the same ICE that this is fixing most other occurrences of), and then file a bug about fixing that remaining hole, right?

@ecstatic-morse , can you provide a piece of example code that runs into that scenario?

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse May 20, 2020

Choose a reason for hiding this comment

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

My thought was that you could write something like impl PartialEq for &'static MyStruct and then somehow write a higher-ranked reference to MyStruct in a const. Depending on how the trait solver treats 'erased, that might still cause an ICE.

However, I think the only way we let the user write higher-ranked types is with fn(&i32) or Fn(&i32), neither of which can run into that problem. Additionally, users can no longer use trait objects in patterns since #71038. If these really are all the ways that the user can write higher-ranked types, perhaps this exactly the right amount of permissive?

// allowed *all* types to reach codegen and caused issues like #65466.
let erased_ty = erase_all_late_bound_regions(tcx, ty);
if is_partial_eq(erased_ty) {
warn!("Non-function pointer only satisfied `PartialEq` after regions were erased");
return true;
}

false
}
}

/// Erase *all* late bound regions, ignoring their debruijn index.
///
/// This is a terrible hack. Do not use it elsewhere.
fn erase_all_late_bound_regions<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {
use ty::fold::TypeFoldable;

struct Eraser<'tcx> {
tcx: TyCtxt<'tcx>,
}

impl<'tcx> ty::fold::TypeFolder<'tcx> for Eraser<'tcx> {
fn tcx(&self) -> TyCtxt<'tcx> {
self.tcx
}

fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
match r {
ty::ReLateBound(_, _) => &ty::ReErased,
r => r.super_fold_with(self),
}
}
}

ty.fold_with(&mut Eraser { tcx })
}
5 changes: 2 additions & 3 deletions src/test/ui/consts/const_in_pattern/issue-65466.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
// FIXME: This still ICEs.
//
// ignore-test
// This used to ICE in #65466.

#![deny(indirect_structural_match)]

Expand All @@ -18,6 +16,7 @@ fn main() {
let x = O::None;
match &[x][..] {
C => (),
//~^ must implement `PartialEq`
_ => (),
}
}
15 changes: 4 additions & 11 deletions src/test/ui/consts/const_in_pattern/issue-65466.stderr
Original file line number Diff line number Diff line change
@@ -1,15 +1,8 @@
error[E0601]: `main` function not found in crate `issue_65466`
--> $DIR/issue-65466.rs:1:1
error: `&[O<B>]` must implement `PartialEq` to be used in a pattern
--> $DIR/issue-65466.rs:18:9
|
LL | / #![deny(indirect_structural_match)]
LL | |
LL | | #[derive(PartialEq, Eq)]
LL | | enum O<T> {
... |
LL | | }
LL | | }
| |_^ consider adding a `main` function to `$DIR/issue-65466.rs`
LL | C => (),
| ^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0601`.
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn main() {

match None {
NO_PARTIAL_EQ_NONE => println!("NO_PARTIAL_EQ_NONE"),
//~^ ERROR must be annotated with `#[derive(PartialEq, Eq)]`
//~^ ERROR must implement `PartialEq`
_ => panic!("whoops"),
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: to use a constant of type `NoPartialEq` in a pattern, `NoPartialEq` must be annotated with `#[derive(PartialEq, Eq)]`
error: `std::option::Option<NoPartialEq>` must implement `PartialEq` to be used in a pattern
--> $DIR/reject_non_partial_eq.rs:28:9
|
LL | NO_PARTIAL_EQ_NONE => println!("NO_PARTIAL_EQ_NONE"),
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/match/issue-70972-dyn-trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ fn main() {
let a: &dyn Send = &7u32;
match a {
F => panic!(),
//~^ ERROR trait objects cannot be used in patterns
//~^ ERROR must implement `PartialEq`
_ => {}
}
}
2 changes: 1 addition & 1 deletion src/test/ui/match/issue-70972-dyn-trait.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: trait objects cannot be used in patterns
error: `&dyn std::marker::Send` must implement `PartialEq` to be used in a pattern
--> $DIR/issue-70972-dyn-trait.rs:6:9
|
LL | F => panic!(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const A: &[B] = &[];
pub fn main() {
match &[][..] {
A => (),
//~^ ERROR must be annotated with `#[derive(PartialEq, Eq)]`
//~^ ERROR must implement `PartialEq`
_ => (),
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: to use a constant of type `B` in a pattern, `B` must be annotated with `#[derive(PartialEq, Eq)]`
error: `&[B]` must implement `PartialEq` to be used in a pattern
--> $DIR/issue-61188-match-slice-forbidden-without-eq.rs:15:9
|
LL | A => (),
Expand Down