Skip to content

use visitor for #[structural_match] check #62339

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

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
6 changes: 6 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,12 @@ declare_lint! {
"outlives requirements can be inferred"
}

declare_lint! {
pub INDIRECT_STRUCTURAL_MATCH,
Warn,
"pattern with const indirectly referencing non-`#[structural_match]` type"
}

/// Some lints that are buffered from `libsyntax`. See `syntax::early_buffered_lints`.
pub mod parser {
declare_lint! {
Expand Down
1 change: 1 addition & 0 deletions src/librustc/middle/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ language_item_table! {
UnpinTraitLangItem, "unpin", unpin_trait, Target::Trait;
PinTypeLangItem, "pin", pin_type, Target::Struct;

// Don't be fooled by the naming here: this lang item denotes `PartialEq`, not `Eq`.
EqTraitLangItem, "eq", eq_trait, Target::Trait;
PartialOrdTraitLangItem, "partial_ord", partial_ord_trait, Target::Trait;
OrdTraitLangItem, "ord", ord_trait, Target::Trait;
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
id: LintId::of(MUTABLE_BORROW_RESERVATION_CONFLICT),
reference: "issue #59159 <https://github.com/rust-lang/rust/issues/59159>",
edition: None,
},
FutureIncompatibleInfo {
id: LintId::of(INDIRECT_STRUCTURAL_MATCH),
reference: "issue #62411 <https://github.com/rust-lang/rust/issues/62411>",
edition: None,
}
]);

Expand Down
2 changes: 2 additions & 0 deletions src/librustc_mir/hair/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
let mut patcx = PatternContext::new(self.tcx,
self.param_env.and(self.identity_substs),
self.tables);
patcx.include_lint_checks();
let pattern = expand_pattern(cx, patcx.lower_pattern(&pat));
if !patcx.errors.is_empty() {
patcx.report_inlining_errors(pat.span);
Expand Down Expand Up @@ -266,6 +267,7 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> {
let mut patcx = PatternContext::new(self.tcx,
self.param_env.and(self.identity_substs),
self.tables);
patcx.include_lint_checks();
let pattern = patcx.lower_pattern(pat);
let pattern_ty = pattern.ty;
let pats: Matrix<'_, '_> = vec![smallvec![
Expand Down
213 changes: 207 additions & 6 deletions src/librustc_mir/hair/pattern/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ use crate::const_eval::const_variant_index;
use crate::hair::util::UserAnnotatedTyHelpers;
use crate::hair::constant::*;

use rustc::lint;
use rustc::mir::{Field, BorrowKind, Mutability};
use rustc::mir::{UserTypeProjection};
use rustc::mir::interpret::{GlobalId, ConstValue, sign_extend, AllocId, Pointer};
use rustc::traits::{ObligationCause, PredicateObligation};
use rustc::ty::{self, Region, TyCtxt, AdtDef, Ty, UserType, DefIdTree};
use rustc::ty::{CanonicalUserType, CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations};
use rustc::ty::subst::{SubstsRef, Kind};
Expand All @@ -22,6 +24,7 @@ use rustc::hir::def::{CtorOf, Res, DefKind, CtorKind};
use rustc::hir::pat_util::EnumerateAndAdjustIterator;

use rustc_data_structures::indexed_vec::Idx;
use rustc_data_structures::fx::FxHashSet;

use std::cmp::Ordering;
use std::fmt;
Expand Down Expand Up @@ -332,6 +335,7 @@ pub struct PatternContext<'a, 'tcx> {
pub tables: &'a ty::TypeckTables<'tcx>,
pub substs: SubstsRef<'tcx>,
pub errors: Vec<PatternError>,
include_lint_checks: bool,
}

impl<'a, 'tcx> Pattern<'tcx> {
Expand Down Expand Up @@ -363,10 +367,16 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
param_env: param_env_and_substs.param_env,
tables,
substs: param_env_and_substs.value,
errors: vec![]
errors: vec![],
include_lint_checks: false,
}
}

pub fn include_lint_checks(&mut self) -> &mut Self {
self.include_lint_checks = true;
self
}

pub fn lower_pattern(&mut self, pat: &'tcx hir::Pat) -> Pattern<'tcx> {
// When implicit dereferences have been inserted in this pattern, the unadjusted lowered
// pattern has the type that results *after* dereferencing. For example, in this code:
Expand Down Expand Up @@ -942,23 +952,94 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {

/// Converts an evaluated constant to a pattern (if possible).
/// This means aggregate values (like structs and enums) are converted
/// to a pattern that matches the value (as if you'd compared via equality).
/// to a pattern that matches the value (as if you'd compared via structural equality).
fn const_to_pat(
&self,
instance: ty::Instance<'tcx>,
cv: &'tcx ty::Const<'tcx>,
id: hir::HirId,
span: Span,
) -> Pattern<'tcx> {
// This method is just a warpper handling a validity check; the heavy lifting is
// performed by the recursive const_to_pat_inner method, which is not meant to be
// invoked except by this method.
//
// once indirect_structural_match is a full fledged error, this
// level of indirection can be eliminated

debug!("const_to_pat: cv={:#?} id={:?}", cv, id);
let adt_subpattern = |i, variant_opt| {
debug!("const_to_pat: cv.ty={:?} span={:?}", cv.ty, span);

let mut saw_error = false;
let inlined_const_as_pat = self.const_to_pat_inner(instance, cv, id, span, &mut saw_error);

if self.include_lint_checks && !saw_error {
// If we were able to successfully convert the const to some pat, double-check
// that the type of the const obeys `#[structural_match]` constraint.
if let Some(adt_def) = search_for_adt_without_structural_match(self.tcx, cv.ty) {

let path = self.tcx.def_path_str(adt_def.did);
let msg = format!(
"to use a constant of type `{}` in a pattern, \
`{}` must be annotated with `#[derive(PartialEq, Eq)]`",
path,
path,
);

// before issuing lint, double-check there even *is* a
// semantic PartialEq for us 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.)

let ty_is_partial_eq: bool = {
let partial_eq_trait_id = self.tcx.lang_items().eq_trait().unwrap();
let obligation: PredicateObligation<'_> =
self.tcx.predicate_for_trait_def(self.param_env,
ObligationCause::misc(span, id),
partial_eq_trait_id,
0,
cv.ty,
&[]);
self.tcx
.infer_ctxt()
.enter(|infcx| 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(span, &msg);
} else {
self.tcx.lint_hir(lint::builtin::INDIRECT_STRUCTURAL_MATCH, id, span, &msg);
}
}
}

inlined_const_as_pat
}

/// Recursive helper for `const_to_pat`; invoke that (instead of calling this directly).
fn const_to_pat_inner(
&self,
instance: ty::Instance<'tcx>,
cv: &'tcx ty::Const<'tcx>,
id: hir::HirId,
span: Span,
// This tracks if we signal some hard error for a given const
// value, so that we will not subsequently issue an irrelevant
// lint for the same const value.
saw_const_match_error: &mut bool,
) -> Pattern<'tcx> {

let mut adt_subpattern = |i, variant_opt| {
let field = Field::new(i);
let val = crate::const_eval::const_field(
self.tcx, self.param_env, variant_opt, field, cv
);
self.const_to_pat(instance, val, id, span)
self.const_to_pat_inner(instance, val, id, span, saw_const_match_error)
};
let adt_subpatterns = |n, variant_opt| {
let mut adt_subpatterns = |n, variant_opt| {
(0..n).map(|i| {
let field = Field::new(i);
FieldPattern {
Expand All @@ -967,7 +1048,8 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
}
}).collect::<Vec<_>>()
};
debug!("const_to_pat: cv.ty={:?} span={:?}", cv.ty, span);


let kind = match cv.ty.sty {
ty::Float(_) => {
self.tcx.lint_hir(
Expand All @@ -982,9 +1064,11 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
}
ty::Adt(adt_def, _) if adt_def.is_union() => {
// Matching on union fields is unsafe, we can't hide it in constants
*saw_const_match_error = true;
self.tcx.sess.span_err(span, "cannot use unions in constant patterns");
PatternKind::Wild
}
// keep old code until future-compat upgraded to errors.
ty::Adt(adt_def, _) if !self.tcx.has_attr(adt_def.did, sym::structural_match) => {
let path = self.tcx.def_path_str(adt_def.did);
let msg = format!(
Expand All @@ -993,9 +1077,11 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
path,
path,
);
*saw_const_match_error = true;
self.tcx.sess.span_err(span, &msg);
PatternKind::Wild
}
// keep old code until future-compat upgraded to errors.
ty::Ref(_, ty::TyS { sty: ty::Adt(adt_def, _), .. }, _)
if !self.tcx.has_attr(adt_def.did, sym::structural_match) => {
// HACK(estebank): Side-step ICE #53708, but anything other than erroring here
Expand All @@ -1007,6 +1093,7 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
path,
path,
);
*saw_const_match_error = true;
self.tcx.sess.span_err(span, &msg);
PatternKind::Wild
}
Expand Down Expand Up @@ -1058,6 +1145,120 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
}
}

/// This method traverses the structure of `ty`, trying to find an
/// instance of an ADT (i.e. struct or enum) that was declared without
/// the `#[structural_match]` attribute.
///
/// The "structure of a type" includes all components that would be
/// considered when doing a pattern match on a constant of that
/// type.
///
/// * This means this method descends into fields of structs/enums,
/// and also descends into the inner type `T` of `&T` and `&mut T`
///
/// * The traversal doesn't dereference unsafe pointers (`*const T`,
/// `*mut T`), and it does not visit the type arguments of an
/// instantiated generic like `PhantomData<T>`.
///
/// The reason we do this search is Rust currently require all ADT's
/// reachable from a constant's type to be annotated with
/// `#[structural_match]`, an attribute which essentially says that
/// the implementation of `PartialEq::eq` behaves *equivalently* to a
/// comparison against the unfolded structure.
///
/// For more background on why Rust has this requirement, and issues
/// that arose when the requirement was not enforced completely, see
/// Rust RFC 1445, rust-lang/rust#61188, and rust-lang/rust#62307.
fn search_for_adt_without_structural_match<'tcx>(tcx: TyCtxt<'tcx>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is weird here, should be:

fn search_for_adt_without_structural_match<'tcx>(
    tcx: TyCtxt<'tcx>,
    ty: Ty<'tcx>,
) -> Option<&'tcx AdtDef> {
    ...

ty: Ty<'tcx>)
-> Option<&'tcx AdtDef>
{
// Import here (not mod level), because `TypeFoldable::fold_with`
// conflicts with `PatternFoldable::fold_with`
use crate::rustc::ty::fold::TypeVisitor;
use crate::rustc::ty::TypeFoldable;

let mut search = Search { tcx, found: None, seen: FxHashSet::default() };
ty.visit_with(&mut search);
return search.found;

struct Search<'tcx> {
tcx: TyCtxt<'tcx>,

// records the first ADT we find without `#[structural_match`
found: Option<&'tcx AdtDef>,

// tracks ADT's previously encountered during search, so that
// we will not recur on them again.
seen: FxHashSet<&'tcx AdtDef>,
}

impl<'tcx> TypeVisitor<'tcx> for Search<'tcx> {
fn visit_ty(&mut self, ty: Ty<'tcx>) -> bool {
debug!("Search visiting ty: {:?}", ty);

let (adt_def, substs) = match ty.sty {
ty::Adt(adt_def, substs) => (adt_def, substs),
ty::RawPtr(..) => {
// `#[structural_match]` ignores substructure of
// `*const _`/`*mut _`, so skip super_visit_with

// (But still tell caller to continue search.)
return false;
}
ty::Array(_, n) if n.assert_usize(self.tcx) == Some(0) => {
// rust-lang/rust#62336: ignore type of contents
// for empty array.
return false;
}
_ => {
ty.super_visit_with(self);
return false;
}
};

if !self.tcx.has_attr(adt_def.did, sym::structural_match) {
self.found = Some(&adt_def);
debug!("Search found adt_def: {:?}", adt_def);
return true // Halt visiting!
}

if self.seen.contains(adt_def) {
debug!("Search already seen adt_def: {:?}", adt_def);
// let caller continue its search
return false;
}

self.seen.insert(adt_def);

// `#[structural_match]` does not care about the
// instantiation of the generics in an ADT (it
// instead looks directly at its fields outside
// this match), so we skip super_visit_with.
//
// (Must not recur on substs for `PhantomData<T>` cf
// rust-lang/rust#55028 and rust-lang/rust#55837; but also
// want to skip substs when only uses of generic are
// behind unsafe pointers `*const T`/`*mut T`.)

// even though we skip super_visit_with, we must recur on
// fields of ADT.
let tcx = self.tcx;
for field_ty in adt_def.all_fields().map(|field| field.ty(tcx, substs)) {
if field_ty.visit_with(self) {
// found an ADT without `#[structural_match]`; halt visiting!
assert!(self.found.is_some());
return true;
}
}

// Even though we do not want to recur on substs, we do
// want our caller to continue its own search.
false
}
}
}

impl UserAnnotatedTyHelpers<'tcx> for PatternContext<'_, 'tcx> {
fn tcx(&self) -> TyCtxt<'tcx> {
self.tcx
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/issues/issue-55511.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ fn main() {
//~^ ERROR `a` does not live long enough [E0597]
match b {
<() as Foo<'static>>::C => { }
//~^ WARN must be annotated with `#[derive(PartialEq, Eq)]`
//~| WARN will become a hard error in a future release
_ => { }
}
}
10 changes: 10 additions & 0 deletions src/test/ui/issues/issue-55511.stderr
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
warning: to use a constant of type `std::cell::Cell` in a pattern, `std::cell::Cell` must be annotated with `#[derive(PartialEq, Eq)]`
--> $DIR/issue-55511.rs:16:9
|
LL | <() as Foo<'static>>::C => { }
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= note: #[warn(indirect_structural_match)] on by default
= 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 #62411 <https://github.com/rust-lang/rust/issues/62411>

error[E0597]: `a` does not live long enough
--> $DIR/issue-55511.rs:13:28
|
Expand Down
Loading