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

Solution to trait alias bug #65673 #66813

Closed
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
12 changes: 7 additions & 5 deletions src/librustc/traits/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,19 +358,21 @@ impl<'tcx> TraitAliasExpander<'tcx> {
debug!("expand_trait_aliases: trait_ref={:?}", trait_ref);

// Don't recurse if this bound is not a trait alias.
let is_alias = tcx.is_trait_alias(trait_ref.def_id());
if !is_alias {
if !tcx.is_trait_alias(trait_ref.def_id()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I believe that there are no functional changes to this file? (Just checking)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can remove this change if you like. I think I just forgot. Makes sense to inline this, but it's not important.

return true;
}

// Don't recurse if this trait alias is already on the stack for the DFS search.
let anon_pred = anonymize_predicate(tcx, &pred);
if item.path.iter().rev().skip(1)
.any(|(tr, _)| anonymize_predicate(tcx, &tr.to_predicate()) == anon_pred) {
if item.path.iter()
.rev()
.skip(1)
.any(|(tr, _)| anonymize_predicate(tcx, &tr.to_predicate()) == anon_pred)
{
return false;
}

// Get components of trait alias.
// Get the components of this trait alias.
let predicates = tcx.super_predicates_of(trait_ref.def_id());

let items = predicates.predicates
Expand Down
98 changes: 52 additions & 46 deletions src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
//! The main routine here is `ast_ty_to_ty()`; each use is parameterized by an
//! instance of `AstConv`.

use errors::{Applicability, DiagnosticId};
use crate::hir::{self, GenericArg, GenericArgs, ExprKind};
use crate::hir::def::{CtorOf, Res, DefKind};
use crate::hir::def_id::DefId;
Expand All @@ -12,32 +11,34 @@ use crate::lint;
use crate::middle::lang_items::SizedTraitLangItem;
use crate::middle::resolve_lifetime as rl;
use crate::namespace::Namespace;
use crate::require_c_abi_if_c_variadic;
use crate::util::common::ErrorReported;
use crate::util::nodemap::FxHashMap;

use rustc::lint::builtin::AMBIGUOUS_ASSOCIATED_ITEMS;
use rustc::traits;
use rustc::ty::{self, DefIdTree, Ty, TyCtxt, Const, ToPredicate, TypeFoldable};
use rustc::ty::{GenericParamDef, GenericParamDefKind};
use rustc::ty::subst::{self, Subst, InternalSubsts, SubstsRef};
use rustc::ty::wf::object_region_bounds;
use rustc_target::spec::abi;
use crate::require_c_abi_if_c_variadic;
use smallvec::SmallVec;

use syntax::ast;
use syntax::errors::pluralize;
use syntax::feature_gate::{GateIssue, emit_feature_err};
use syntax::util::lev_distance::find_best_match_for_name;
use syntax::symbol::sym;
use syntax_pos::{DUMMY_SP, Span, MultiSpan};
use crate::util::common::ErrorReported;
use crate::util::nodemap::FxHashMap;

use errors::{Applicability, DiagnosticId};
use rustc_data_structures::fx::FxHashSet;
use rustc_error_codes::*;
use smallvec::SmallVec;

use std::collections::BTreeSet;
use std::iter;
use std::slice;

use rustc_data_structures::fx::FxHashSet;

use rustc_error_codes::*;

#[derive(Debug)]
pub struct PathSeg(pub DefId, pub usize);

Expand All @@ -64,8 +65,7 @@ pub trait AstConv<'tcx> {
&self,
param: Option<&ty::GenericParamDef>,
span: Span,
)
-> Option<ty::Region<'tcx>>;
) -> Option<ty::Region<'tcx>>;

/// Returns the type to use when a type is omitted.
fn ty_infer(&self, param: Option<&ty::GenericParamDef>, span: Span) -> Ty<'tcx>;
Expand Down Expand Up @@ -1247,8 +1247,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
// Expand trait aliases recursively and check that only one regular (non-auto) trait
// is used and no 'maybe' bounds are used.
let expanded_traits =
traits::expand_trait_aliases(tcx, bounds.trait_bounds.iter().cloned());
let (mut auto_traits, regular_traits): (Vec<_>, Vec<_>) =
traits::expand_trait_aliases(tcx, bounds.trait_bounds.iter().cloned())
// Ensure that trait ref is to self type and not some type param.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems pretty subtle and not obviously correct. One thing I noticed is that the original ICE was based on something like trait Foo<Ix, OnSet> = where OnSet: Callback0, but I don't see any test that covers that particular scenario? It seems like the effect of this change is to going to be to wind up with an empty list here, which I guess means that you will get an error?

However, something like this would work ok:

trait Foo = where Self: Display;

which I think would be equivalent to trait Foo = Display, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit worried here about an example like this:

#![feature(trait_alias)]

trait Foo<T> = where T: PartialEq<Self>;

fn main() {
    let _: &dyn Foo<()> = &();
}

When I test this, I currently get some wacky ICE =) But it seems like we would screen out the T: PartialEq<Self> from the list of regular_traits here. Then, later, when we check for object safety, we will never see that T: PartialEq<Self> is not object-safe.

My intuition is that trait Foo<T> = ... should work exactly as if you created a trait and implemented it, for the most part. And if we do that with this example we get object safety errors. I don't quite see where they are supposed to crop up here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key thing is to avoid trying to create existential predicates for predicates where the self ty is not the dummy self ty, which is of course impossible. This filter might be blocking out too much (projections on Self too?), but all the tests seem to be working anyway...

The relevant test is issue-65673.rs, I believe (pre-existing, slightly modified).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trait Foo = where Self: Display; should (and I think is) equivalent to trait Foo = Display; in most circumstances, except as a trait object. Since technically no principal trait is specified, the compiler will complain. Perhaps we should do that, but perhaps we should instead check that there's at least one predicate instead of trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for object safety, I guess we need to modify that code explicitly? Given such predicates on T definitely cannot be included in the trait object itself (they're simply not existential).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand your response here, @alexreg.

What is the behavior on the example I gave, for example? I don't think that issue-65673.rs is equivalent. For one thing, it's run-pass, but I expect an error, but also the trait alias in issue-65673.rs is:

 trait Alias<T> = Any where T: Trait; 

whereas the trait alias in my example was trait Foo<T> = where T: PartialEq<Self>;, which (notably) references Self (unlike issue-65673.rs).

Also, regarding the difference between trait Foo = Display and trait Foo where Self: Display, I guess I'd have to consult the RFC. It's a touch surprising to me since trait Foo: Display and trait Foo where Self: Display are equivalent, for better or worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikomatsakis Okay, I've gotten your expected behaviour implemented. (And I managed to remove the Any from issue-65673.rs.) Now, the question is, do we want to allow just dyn or dyn 'static as a type?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know! I think I'm inclined to preserve the status quo, whatever it is, though if/when we generalize to dyn A + B I'd also be inclined to accept zero bounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikomatsakis Okay, sounds fair. I'll push shortly and this PR will be ready for review.

.filter(|info| info.trait_ref().self_ty() == dummy_self);
let (auto_traits, regular_traits): (Vec<_>, Vec<_>) =
expanded_traits.partition(|i| tcx.trait_is_auto(i.trait_ref().def_id()));
if regular_traits.len() > 1 {
let first_trait = &regular_traits[0];
Expand Down Expand Up @@ -1289,7 +1291,8 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
let mut associated_types = BTreeSet::default();

let regular_traits_refs = bounds.trait_bounds
.into_iter()
.iter()
.cloned()
.filter(|(trait_ref, _)| !tcx.trait_is_auto(trait_ref.def_id()))
.map(|(trait_ref, _)| trait_ref);
for trait_ref in traits::elaborate_trait_refs(tcx, regular_traits_refs) {
Expand Down Expand Up @@ -1398,50 +1401,53 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
err.emit();
}

let (mut auto_traits, regular_traits): (Vec<_>, Vec<_>) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't love the way these variables shadow the variables above. In general, this function seems to be awfully big, and these shadows are quite far apart. I had to expand the diff fully just to be sure that shadowing was even happening. I think we should extract a helper function for the logic above, perhaps, and then a separate helper for the logic below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this fn was already very big, and that’s just gotten worse now. I’ll see what I can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, complain_about_missing_associated_types has already been factored out now, which makes this fn size much more reasonable!

bounds.trait_bounds
.into_iter()
.map(|(trait_ref, _)| trait_ref)
.partition(|i| tcx.trait_is_auto(i.def_id()));

// De-duplicate auto traits so that, e.g., `dyn Trait + Send + Send` is the same as
// `dyn Trait + Send`.
auto_traits.sort_by_key(|i| i.trait_ref().def_id());
auto_traits.dedup_by_key(|i| i.trait_ref().def_id());
debug!("regular_traits: {:?}", regular_traits);
debug!("auto_traits: {:?}", auto_traits);
auto_traits.sort_by_key(|i| i.def_id());
auto_traits.dedup_by_key(|i| i.def_id());
debug!(
"conv_object_ty_poly_trait_ref: regular_traits={:?} auto_traits={:?}",
regular_traits, auto_traits
);

// Transform a `PolyTraitRef` into a `PolyExistentialTraitRef` by
// removing the dummy `Self` type (`trait_object_dummy_self`).
let trait_ref_to_existential = |trait_ref: ty::TraitRef<'tcx>| {
if trait_ref.self_ty() != dummy_self {
// FIXME: There appears to be a missing filter on top of `expand_trait_aliases`,
// which picks up non-supertraits where clauses - but also, the object safety
// completely ignores trait aliases, which could be object safety hazards. We
// `delay_span_bug` here to avoid an ICE in stable even when the feature is
// disabled. (#66420)
tcx.sess.delay_span_bug(DUMMY_SP, &format!(
"trait_ref_to_existential called on {:?} with non-dummy Self",
trait_ref,
));
bug!("trait_ref_to_existential called on {:?} with non-dummy Self", trait_ref);
}
ty::ExistentialTraitRef::erase_self_ty(tcx, trait_ref)
};

// Erase the `dummy_self` (`trait_object_dummy_self`) used above.
let existential_trait_refs = regular_traits.iter().map(|i| {
i.trait_ref().map_bound(|trait_ref| trait_ref_to_existential(trait_ref))
});
let existential_projections = bounds.projection_bounds.iter().map(|(bound, _)| {
bound.map_bound(|b| {
let trait_ref = trait_ref_to_existential(b.projection_ty.trait_ref(tcx));
ty::ExistentialProjection {
ty: b.ty,
item_def_id: b.projection_ty.item_def_id,
substs: trait_ref.substs,
}
})
});
let existential_trait_refs = regular_traits
.iter()
.map(|i| i.map_bound(|trait_ref| trait_ref_to_existential(trait_ref)));
let existential_projections = bounds.projection_bounds
.iter()
.map(|(bound, _)| {
bound.map_bound(|b| {
let trait_ref = trait_ref_to_existential(b.projection_ty.trait_ref(tcx));
ty::ExistentialProjection {
ty: b.ty,
item_def_id: b.projection_ty.item_def_id,
substs: trait_ref.substs,
}
})
});

// Calling `skip_binder` is okay because the predicates are re-bound.
let regular_trait_predicates = existential_trait_refs.map(
|trait_ref| ty::ExistentialPredicate::Trait(*trait_ref.skip_binder()));
let auto_trait_predicates = auto_traits.into_iter().map(
|trait_ref| ty::ExistentialPredicate::AutoTrait(trait_ref.trait_ref().def_id()));
let regular_trait_predicates = existential_trait_refs
.map(|trait_ref| ty::ExistentialPredicate::Trait(*trait_ref.skip_binder()));
let auto_trait_predicates = auto_traits
.into_iter()
.map(|trait_ref| ty::ExistentialPredicate::AutoTrait(trait_ref.def_id()));
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, also no functional changes here, correct? ( I didn't spot any )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, just made it more readable since I was editing this fn anyway.

let mut v =
regular_trait_predicates
.chain(auto_trait_predicates)
Expand Down Expand Up @@ -1469,10 +1475,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
}
})
};
debug!("region_bound: {:?}", region_bound);
debug!("conv_object_ty_poly_trait_ref: region_bound={:?}", region_bound);

let ty = tcx.mk_dynamic(existential_predicates, region_bound);
debug!("trait_object_type: {:?}", ty);
debug!("conv_object_ty_poly_trait_ref: trait_object_type={:?}", ty);
ty
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error[E0271]: type mismatch resolving `<std::vec::IntoIter<u32> as std::iter::It
LL | let _: &dyn I32Iterator<Item = u32> = &vec![42].into_iter();
| ^^^^^^^^^^^^^^^^^^^^^ expected `i32`, found `u32`
|
= note: required for the cast to the object type `dyn std::iter::Iterator<Item = u32, Item = i32>`
= note: required for the cast to the object type `dyn I32Iterator<Item = u32, Item = i32>`

error: aborting due to previous error

Expand Down
10 changes: 7 additions & 3 deletions src/test/ui/bad/bad-sized.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
// ignore-tidy-linelength

trait Trait {}

pub fn main() {
let x: Vec<dyn Trait + Sized> = Vec::new();
//~^ ERROR only auto traits can be used as additional traits in a trait object
//~| ERROR the size for values of type
//~| ERROR the size for values of type
//~^ ERROR only auto traits can be used as additional traits in a trait object [E0225]
//~| ERROR the size for values of type `dyn std::marker::Sized` cannot be known at compilation time [E0277]
//~| the trait `std::marker::Sized` cannot be made into an object [E0038]
//~| ERROR the size for values of type `dyn std::marker::Sized` cannot be known at compilation time [E0277]
//~| the trait `std::marker::Sized` cannot be made into an object [E0038]
}
36 changes: 26 additions & 10 deletions src/test/ui/bad/bad-sized.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0225]: only auto traits can be used as additional traits in a trait object
--> $DIR/bad-sized.rs:4:28
--> $DIR/bad-sized.rs:6:28
|
LL | let x: Vec<dyn Trait + Sized> = Vec::new();
| ----- ^^^^^
Expand All @@ -9,27 +9,43 @@ LL | let x: Vec<dyn Trait + Sized> = Vec::new();
| first non-auto trait
| trait alias used in trait object type (first use)

error[E0277]: the size for values of type `dyn Trait` cannot be known at compilation time
--> $DIR/bad-sized.rs:4:12
error[E0277]: the size for values of type `dyn std::marker::Sized` cannot be known at compilation time
--> $DIR/bad-sized.rs:6:12
|
LL | let x: Vec<dyn Trait + Sized> = Vec::new();
| ^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `std::marker::Sized` is not implemented for `dyn Trait`
= help: the trait `std::marker::Sized` is not implemented for `dyn std::marker::Sized`
= note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
= note: required by `std::vec::Vec`

error[E0277]: the size for values of type `dyn Trait` cannot be known at compilation time
--> $DIR/bad-sized.rs:4:37
error[E0038]: the trait `std::marker::Sized` cannot be made into an object
--> $DIR/bad-sized.rs:6:12
|
LL | let x: Vec<dyn Trait + Sized> = Vec::new();
| ^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Sized` cannot be made into an object
|
= note: the trait cannot require that `Self : Sized`

error[E0277]: the size for values of type `dyn std::marker::Sized` cannot be known at compilation time
--> $DIR/bad-sized.rs:6:37
|
LL | let x: Vec<dyn Trait + Sized> = Vec::new();
| ^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `std::marker::Sized` is not implemented for `dyn Trait`
= help: the trait `std::marker::Sized` is not implemented for `dyn std::marker::Sized`
= note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
= note: required by `std::vec::Vec::<T>::new`

error: aborting due to 3 previous errors
error[E0038]: the trait `std::marker::Sized` cannot be made into an object
--> $DIR/bad-sized.rs:6:37
|
LL | let x: Vec<dyn Trait + Sized> = Vec::new();
| ^^^^^^^^ the trait `std::marker::Sized` cannot be made into an object
|
= note: the trait cannot require that `Self : Sized`

error: aborting due to 5 previous errors

Some errors have detailed explanations: E0225, E0277.
For more information about an error, try `rustc --explain E0225`.
Some errors have detailed explanations: E0038, E0225, E0277.
For more information about an error, try `rustc --explain E0038`.
7 changes: 4 additions & 3 deletions src/test/ui/issues/issue-32963.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ use std::mem;

trait Misc {}

fn size_of_copy<T: Copy+?Sized>() -> usize { mem::size_of::<T>() }
fn size_of_copy<T: Copy + ?Sized>() -> usize { mem::size_of::<T>() }

fn main() {
size_of_copy::<dyn Misc + Copy>();
//~^ ERROR only auto traits can be used as additional traits in a trait object
//~| ERROR the trait bound `dyn Misc: std::marker::Copy` is not satisfied
//~^ ERROR only auto traits can be used as additional traits in a trait object [E0225]
//~| ERROR the trait bound `dyn std::marker::Copy: std::marker::Copy` is not satisfied [E0277]
//~| ERROR the trait `std::marker::Copy` cannot be made into an object [E0038]
}
20 changes: 14 additions & 6 deletions src/test/ui/issues/issue-32963.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,24 @@ LL | size_of_copy::<dyn Misc + Copy>();
| first non-auto trait
| trait alias used in trait object type (first use)

error[E0277]: the trait bound `dyn Misc: std::marker::Copy` is not satisfied
error[E0277]: the trait bound `dyn std::marker::Copy: std::marker::Copy` is not satisfied
--> $DIR/issue-32963.rs:8:5
|
LL | fn size_of_copy<T: Copy+?Sized>() -> usize { mem::size_of::<T>() }
LL | fn size_of_copy<T: Copy + ?Sized>() -> usize { mem::size_of::<T>() }
| ------------ ---- required by this bound in `size_of_copy`
...
LL | size_of_copy::<dyn Misc + Copy>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `dyn Misc`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `dyn std::marker::Copy`

error: aborting due to 2 previous errors
error[E0038]: the trait `std::marker::Copy` cannot be made into an object
--> $DIR/issue-32963.rs:8:20
|
LL | size_of_copy::<dyn Misc + Copy>();
| ^^^^^^^^^^^^^^^ the trait `std::marker::Copy` cannot be made into an object
|
= note: the trait cannot require that `Self : Sized`

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0225, E0277.
For more information about an error, try `rustc --explain E0225`.
Some errors have detailed explanations: E0038, E0225, E0277.
For more information about an error, try `rustc --explain E0038`.
12 changes: 0 additions & 12 deletions src/test/ui/issues/issue-65673.rs

This file was deleted.

17 changes: 0 additions & 17 deletions src/test/ui/issues/issue-65673.stderr

This file was deleted.

13 changes: 0 additions & 13 deletions src/test/ui/traits/auxiliary/trait_alias.rs

This file was deleted.

14 changes: 0 additions & 14 deletions src/test/ui/traits/trait-alias-import-cross-crate.rs

This file was deleted.

Loading