Skip to content

Commit

Permalink
Auto merge of #61812 - jonas-schievink:assoc-ty-defaults, r=nikomatsakis
Browse files Browse the repository at this point in the history
Implement RFC 2532 – Associated Type Defaults

This is a partial implementation that is still missing the changes to object types, since I ran into some trouble while implementing that. I'm opening this part already to get feedback on the implementation and the unexpected test fallout (see my comments below). The remaining changes can be done in a later PR.

Blockers before this can land:
* [x] Resolve unsoundness around interaction with specialization (#61812 (comment)) - #64564

cc #29661
Fixes #53907
Fixes #54182
Fixes #62211
Fixes #41868
Fixes #63593
Fixes #47385
Fixes #43924
Fixes #32350
Fixes #26681
Fixes #67187
  • Loading branch information
bors committed Feb 26, 2020
2 parents 46f5aa9 + 6cc268b commit 3a0d106
Show file tree
Hide file tree
Showing 53 changed files with 2,048 additions and 154 deletions.
4 changes: 3 additions & 1 deletion src/librustc_error_codes/error_codes/E0399.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
#### Note: this error code is no longer emitted by the compiler

You implemented a trait, overriding one or more of its associated types but did
not reimplement its default methods.

Example of erroneous code:

```compile_fail,E0399
```
#![feature(associated_type_defaults)]
pub trait Foo {
Expand Down
43 changes: 29 additions & 14 deletions src/librustc_infer/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1054,25 +1054,40 @@ fn assemble_candidates_from_impls<'cx, 'tcx>(
// an error when we confirm the candidate
// (which will ultimately lead to `normalize_to_error`
// being invoked).
node_item.item.defaultness.has_value()
false
} else {
// If we're looking at a trait *impl*, the item is
// specializable if the impl or the item are marked
// `default`.
node_item.item.defaultness.is_default()
|| super::util::impl_is_default(selcx.tcx(), node_item.node.def_id())
};

// Only reveal a specializable default if we're past type-checking
// and the obligations is monomorphic, otherwise passes such as
// transmute checking and polymorphic MIR optimizations could
// get a result which isn't correct for all monomorphizations.
if !is_default {
true
} else if obligation.param_env.reveal == Reveal::All {
// NOTE(eddyb) inference variables can resolve to parameters, so
// assume `poly_trait_ref` isn't monomorphic, if it contains any.
let poly_trait_ref = selcx.infcx().resolve_vars_if_possible(&poly_trait_ref);
!poly_trait_ref.needs_infer() && !poly_trait_ref.needs_subst()
} else {
false
match is_default {
// Non-specializable items are always projectable
false => true,

// Only reveal a specializable default if we're past type-checking
// and the obligation is monomorphic, otherwise passes such as
// transmute checking and polymorphic MIR optimizations could
// get a result which isn't correct for all monomorphizations.
true if obligation.param_env.reveal == Reveal::All => {
// NOTE(eddyb) inference variables can resolve to parameters, so
// assume `poly_trait_ref` isn't monomorphic, if it contains any.
let poly_trait_ref =
selcx.infcx().resolve_vars_if_possible(&poly_trait_ref);
!poly_trait_ref.needs_infer() && !poly_trait_ref.needs_subst()
}

true => {
debug!(
"assemble_candidates_from_impls: not eligible due to default: \
assoc_ty={} predicate={}",
selcx.tcx().def_path_str(node_item.item.def_id),
obligation.predicate,
);
false
}
}
}
super::VtableParam(..) => {
Expand Down
21 changes: 0 additions & 21 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1964,7 +1964,6 @@ fn check_impl_items_against_trait<'tcx>(

// Locate trait definition and items
let trait_def = tcx.trait_def(impl_trait_ref.def_id);
let mut overridden_associated_type = None;

let impl_items = || impl_item_refs.iter().map(|iiref| tcx.hir().impl_item(iiref.id));

Expand Down Expand Up @@ -2046,9 +2045,6 @@ fn check_impl_items_against_trait<'tcx>(
hir::ImplItemKind::OpaqueTy(..) | hir::ImplItemKind::TyAlias(_) => {
let opt_trait_span = tcx.hir().span_if_local(ty_trait_item.def_id);
if ty_trait_item.kind == ty::AssocKind::Type {
if ty_trait_item.defaultness.has_value() {
overridden_associated_type = Some(impl_item);
}
compare_ty_impl(
tcx,
&ty_impl_item,
Expand Down Expand Up @@ -2082,8 +2078,6 @@ fn check_impl_items_against_trait<'tcx>(

// Check for missing items from trait
let mut missing_items = Vec::new();
let mut invalidated_items = Vec::new();
let associated_type_overridden = overridden_associated_type.is_some();
for trait_item in tcx.associated_items(impl_trait_ref.def_id).in_definition_order() {
let is_implemented = trait_def
.ancestors(tcx, impl_id)
Expand All @@ -2094,28 +2088,13 @@ fn check_impl_items_against_trait<'tcx>(
if !is_implemented && !traits::impl_is_default(tcx, impl_id) {
if !trait_item.defaultness.has_value() {
missing_items.push(*trait_item);
} else if associated_type_overridden {
invalidated_items.push(trait_item.ident);
}
}
}

if !missing_items.is_empty() {
missing_items_err(tcx, impl_span, &missing_items, full_impl_span);
}

if !invalidated_items.is_empty() {
let invalidator = overridden_associated_type.unwrap();
struct_span_err!(
tcx.sess,
invalidator.span,
E0399,
"the following trait items need to be reimplemented as `{}` was overridden: `{}`",
invalidator.ident,
invalidated_items.iter().map(|name| name.to_string()).collect::<Vec<_>>().join("`, `")
)
.emit();
}
}

fn missing_items_err(
Expand Down
99 changes: 99 additions & 0 deletions src/librustc_typeck/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,10 +425,109 @@ fn check_trait(tcx: TyCtxt<'_>, item: &hir::Item<'_>) {

for_item(tcx, item).with_fcx(|fcx, _| {
check_where_clauses(tcx, fcx, item.span, trait_def_id, None);
check_associated_type_defaults(fcx, trait_def_id);

vec![]
});
}

/// Checks all associated type defaults of trait `trait_def_id`.
///
/// Assuming the defaults are used, check that all predicates (bounds on the
/// assoc type and where clauses on the trait) hold.
fn check_associated_type_defaults(fcx: &FnCtxt<'_, '_>, trait_def_id: DefId) {
let tcx = fcx.tcx;
let substs = InternalSubsts::identity_for_item(tcx, trait_def_id);

// For all assoc. types with defaults, build a map from
// `<Self as Trait<...>>::Assoc` to the default type.
let map = tcx
.associated_items(trait_def_id)
.in_definition_order()
.filter_map(|item| {
if item.kind == ty::AssocKind::Type && item.defaultness.has_value() {
// `<Self as Trait<...>>::Assoc`
let proj = ty::ProjectionTy { substs, item_def_id: item.def_id };
let default_ty = tcx.type_of(item.def_id);
debug!("assoc. type default mapping: {} -> {}", proj, default_ty);
Some((proj, default_ty))
} else {
None
}
})
.collect::<FxHashMap<_, _>>();

/// Replaces projections of associated types with their default types.
///
/// This does a "shallow substitution", meaning that defaults that refer to
/// other defaulted assoc. types will still refer to the projection
/// afterwards, not to the other default. For example:
///
/// ```compile_fail
/// trait Tr {
/// type A: Clone = Vec<Self::B>;
/// type B = u8;
/// }
/// ```
///
/// This will end up replacing the bound `Self::A: Clone` with
/// `Vec<Self::B>: Clone`, not with `Vec<u8>: Clone`. If we did a deep
/// substitution and ended up with the latter, the trait would be accepted.
/// If an `impl` then replaced `B` with something that isn't `Clone`,
/// suddenly the default for `A` is no longer valid. The shallow
/// substitution forces the trait to add a `B: Clone` bound to be accepted,
/// which means that an `impl` can replace any default without breaking
/// others.
///
/// Note that this isn't needed for soundness: The defaults would still be
/// checked in any impl that doesn't override them.
struct DefaultNormalizer<'tcx> {
tcx: TyCtxt<'tcx>,
map: FxHashMap<ty::ProjectionTy<'tcx>, Ty<'tcx>>,
}

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

fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> {
match t.kind {
ty::Projection(proj_ty) => {
if let Some(default) = self.map.get(&proj_ty) {
default
} else {
t.super_fold_with(self)
}
}
_ => t.super_fold_with(self),
}
}
}

// Now take all predicates defined on the trait, replace any mention of
// the assoc. types with their default, and prove them.
// We only consider predicates that directly mention the assoc. type.
let mut norm = DefaultNormalizer { tcx, map };
let predicates = fcx.tcx.predicates_of(trait_def_id);
for &(orig_pred, span) in predicates.predicates.iter() {
let pred = orig_pred.fold_with(&mut norm);
if pred != orig_pred {
// Mentions one of the defaulted assoc. types
debug!("default suitability check: proving predicate: {} -> {}", orig_pred, pred);
let pred = fcx.normalize_associated_types_in(span, &pred);
let cause = traits::ObligationCause::new(
span,
fcx.body_id,
traits::ItemObligation(trait_def_id),
);
let obligation = traits::Obligation::new(cause, fcx.param_env, pred);

fcx.register_predicate(obligation);
}
}
}

fn check_item_fn(tcx: TyCtxt<'_>, item: &hir::Item<'_>) {
for_item(tcx, item).with_fcx(|fcx, tcx| {
let def_id = fcx.tcx.hir().local_def_id(item.hir_id);
Expand Down
17 changes: 17 additions & 0 deletions src/test/ui/associated-const/defaults-cyclic-fail.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// build-fail

// Cyclic assoc. const defaults don't error unless *used*
trait Tr {
const A: u8 = Self::B;
//~^ ERROR cycle detected when const-evaluating + checking `Tr::A`

const B: u8 = Self::A;
}

// This impl is *allowed* unless its assoc. consts are used
impl Tr for () {}

fn main() {
// This triggers the cycle error
assert_eq!(<() as Tr>::A, 0);
}
31 changes: 31 additions & 0 deletions src/test/ui/associated-const/defaults-cyclic-fail.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
error[E0391]: cycle detected when const-evaluating + checking `Tr::A`
--> $DIR/defaults-cyclic-fail.rs:5:5
|
LL | const A: u8 = Self::B;
| ^^^^^^^^^^^^^^^^^^^^^^
|
note: ...which requires const-evaluating `Tr::A`...
--> $DIR/defaults-cyclic-fail.rs:5:19
|
LL | const A: u8 = Self::B;
| ^^^^^^^
note: ...which requires const-evaluating + checking `Tr::B`...
--> $DIR/defaults-cyclic-fail.rs:8:5
|
LL | const B: u8 = Self::A;
| ^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating `Tr::B`...
--> $DIR/defaults-cyclic-fail.rs:8:19
|
LL | const B: u8 = Self::A;
| ^^^^^^^
= note: ...which again requires const-evaluating + checking `Tr::A`, completing the cycle
note: cycle used when const-evaluating `main`
--> $DIR/defaults-cyclic-fail.rs:16:16
|
LL | assert_eq!(<() as Tr>::A, 0);
| ^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0391`.
36 changes: 36 additions & 0 deletions src/test/ui/associated-const/defaults-cyclic-pass.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// run-pass

// Cyclic assoc. const defaults don't error unless *used*
trait Tr {
const A: u8 = Self::B;
const B: u8 = Self::A;
}

// This impl is *allowed* unless its assoc. consts are used, matching the
// behavior without defaults.
impl Tr for () {}

// Overriding either constant breaks the cycle
impl Tr for u8 {
const A: u8 = 42;
}

impl Tr for u16 {
const B: u8 = 0;
}

impl Tr for u32 {
const A: u8 = 100;
const B: u8 = 123;
}

fn main() {
assert_eq!(<u8 as Tr>::A, 42);
assert_eq!(<u8 as Tr>::B, 42);

assert_eq!(<u16 as Tr>::A, 0);
assert_eq!(<u16 as Tr>::B, 0);

assert_eq!(<u32 as Tr>::A, 100);
assert_eq!(<u32 as Tr>::B, 123);
}
45 changes: 45 additions & 0 deletions src/test/ui/associated-const/defaults-not-assumed-fail.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// build-fail

trait Tr {
const A: u8 = 255;

// This should not be a constant evaluation error (overflow). The value of
// `Self::A` must not be assumed to hold inside the trait.
const B: u8 = Self::A + 1;
//~^ ERROR any use of this value will cause an error
}

// An impl that doesn't override any constant will NOT cause a const eval error
// just because it's defined, but only if the bad constant is used anywhere.
// This matches the behavior without defaults.
impl Tr for () {}

// An impl that overrides either constant with a suitable value will be fine.
impl Tr for u8 {
const A: u8 = 254;
}

impl Tr for u16 {
const B: u8 = 0;
}

impl Tr for u32 {
const A: u8 = 254;
const B: u8 = 0;
}

fn main() {
assert_eq!(<() as Tr>::A, 255);
assert_eq!(<() as Tr>::B, 0); // causes the error above
//~^ ERROR evaluation of constant expression failed
//~| ERROR erroneous constant used

assert_eq!(<u8 as Tr>::A, 254);
assert_eq!(<u8 as Tr>::B, 255);

assert_eq!(<u16 as Tr>::A, 255);
assert_eq!(<u16 as Tr>::B, 0);

assert_eq!(<u32 as Tr>::A, 254);
assert_eq!(<u32 as Tr>::B, 0);
}
31 changes: 31 additions & 0 deletions src/test/ui/associated-const/defaults-not-assumed-fail.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
error: any use of this value will cause an error
--> $DIR/defaults-not-assumed-fail.rs:8:19
|
LL | const B: u8 = Self::A + 1;
| --------------^^^^^^^^^^^-
| |
| attempt to add with overflow
|
= note: `#[deny(const_err)]` on by default

error[E0080]: evaluation of constant expression failed
--> $DIR/defaults-not-assumed-fail.rs:33:5
|
LL | assert_eq!(<() as Tr>::B, 0); // causes the error above
| ^^^^^^^^^^^-------------^^^^^
| |
| referenced constant has errors
|
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: erroneous constant used
--> $DIR/defaults-not-assumed-fail.rs:33:5
|
LL | assert_eq!(<() as Tr>::B, 0); // causes the error above
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ referenced constant has errors
|
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0080`.
Loading

0 comments on commit 3a0d106

Please sign in to comment.