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

Implement RFC 2532 – Associated Type Defaults #61812

Merged
merged 31 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a323ff2
Implement RFC 2532 – Associated Type Defaults
jonas-schievink Jun 13, 2019
a549bbd
Add regression test for #54182
jonas-schievink Jun 13, 2019
37686ed
Add comments and assertions to tests
jonas-schievink Jun 13, 2019
de447eb
Add tests for assoc. const defaults
jonas-schievink Jun 13, 2019
1e3c020
Test interactions with specialization
jonas-schievink Jun 14, 2019
d3be26d
Improve test
jonas-schievink Jun 14, 2019
c73ee98
Check suitability of the provided default
jonas-schievink Jun 15, 2019
f403882
Add a `Self: Sized` bound
jonas-schievink Jun 15, 2019
5e9317a
Put the check into its own function
jonas-schievink Jun 15, 2019
ff5d11e
Add comments and tests explaining the shallow substitution rule
jonas-schievink Jun 16, 2019
07ad64f
Add tests for #62211
jonas-schievink Jun 28, 2019
fead458
Fix tests after rebase
jonas-schievink Aug 29, 2019
485111c
Add regression tests for issues
jonas-schievink Aug 29, 2019
1f61f36
Add comment about the shallow subst rule
jonas-schievink Aug 29, 2019
fd28614
Add regression test for #26681
jonas-schievink Sep 14, 2019
c964520
Update tests after compiletest changes
jonas-schievink Sep 15, 2019
3f03d95
Improve associated-types-overridden-default.rs
jonas-schievink Sep 15, 2019
f408794
Improve defaults-in-other-trait-items-pass
jonas-schievink Sep 15, 2019
fbcd136
Improve the cycle tests
jonas-schievink Sep 14, 2019
c8da9ee
Improve specialization test
jonas-schievink Sep 15, 2019
24ec364
Test mixed default and non-default
jonas-schievink Sep 15, 2019
f94eaea
Fix rebase damage
jonas-schievink Oct 7, 2019
708f053
Add test for #65774
jonas-schievink Nov 24, 2019
a01846f
appease tidy
jonas-schievink Nov 24, 2019
ec50190
Bless test output
jonas-schievink Dec 8, 2019
232c1f3
Format code
jonas-schievink Jan 5, 2020
9930e1f
Fix tests that fail with `--emit metadata`
jonas-schievink Jan 5, 2020
4d4da92
Fix rebase damage
jonas-schievink Feb 15, 2020
af2931b
Mark E0399 test as obsolete
jonas-schievink Feb 21, 2020
c605831
Fix rebase fallout
jonas-schievink Feb 21, 2020
6cc268b
Mark E0399.md as obsolete
jonas-schievink Feb 21, 2020
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
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this file be left alone with a note mentioning that it is no longer emitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that. Not sure what the proper process is here, but the code examples probably need to be ignored then, if that even works on the error index files. e01ad6a also removed the file directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the common action is to have a title "Note: this error code is no longer emitted by the compiler." at the top and ignore the code samples: https://doc.rust-lang.org/error-index.html

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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit subtle, I'll like to think about it a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

First, this check is not necessary for soundness - the trait predicates are always checked in the implementing impl. Might be worth commenting on that.

Second, I find it hard to come with an interesting invariant this check guarantees. Might be worth thinking about that?

Copy link
Contributor

@Centril Centril Jul 6, 2019

Choose a reason for hiding this comment

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

Might be worth commenting on that.

👍

Second, I find it hard to come with an interesting invariant this check guarantees. Might be worth thinking about that?

The options are laid out briefly in https://github.com/rust-lang/rfcs/blob/master/text/2532-associated-type-defaults.md#1-when-do-suitability-of-defaults-need-to-be-proven but the pros and cons aren't really discussed. In #61812 (comment) the rationale for a shallow substitution is discussed and @jonas-schievink makes a compelling point from a semver perspective. In any case, this check is more conservative so it is a good starting point that we can relax later if we want to.

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
Centril marked this conversation as resolved.
Show resolved Hide resolved

// 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;
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved
//~^ 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