Skip to content

small coherence cleanup #74454

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.

Sign up for GitHub

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
merged 4 commits into from
Jul 22, 2020
Merged

small coherence cleanup #74454

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5300ca3
cleanup ty_is_~non~_local_constructor
lcnr Jul 17, 2020
1ac3713
refactor ty_is_non_local
lcnr Jul 17, 2020
c71b196
review
lcnr Jul 20, 2020
cfcbca6
update coherence docs
lcnr Jul 20, 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
74 changes: 30 additions & 44 deletions src/librustc_trait_selection/traits/coherence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ fn orphan_check_trait_ref<'tcx>(
) -> Vec<Ty<'tcx>> {
// FIXME(eddyb) figure out if this is redundant with `ty_is_non_local`,
// or maybe if this should be calling `ty_is_non_local_constructor`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This FIXME is interesting.

Will have to do something about this before we merge this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into the current impl it seems somewhat overly complicated but can't quite think of a clear solution here.

I do feel like using a TypeWalker in fn orphan_check_trait_ref might actually be the cleanest approach here, but iirc we wanted to slowly phase out uses of this struct.

What's the current status here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will have to do something about this before we merge this PR

Is there an interaction, or you would just like to clean this up as part of the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really would like to clean this up before merging this.

If we merge it without fixing this, we should at least update the method names 😅

if ty_is_non_local(tcx, ty, in_crate).is_some() {
if !contained_non_local_types(tcx, ty, in_crate).is_empty() {
if let Some(inner_tys) = fundamental_ty_inner_tys(tcx, ty) {
return inner_tys
.flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate))
Expand All @@ -408,8 +408,8 @@ fn orphan_check_trait_ref<'tcx>(
.enumerate()
{
debug!("orphan_check_trait_ref: check ty `{:?}`", input_ty);
let non_local_tys = ty_is_non_local(tcx, input_ty, in_crate);
if non_local_tys.is_none() {
let non_local_tys = contained_non_local_types(tcx, input_ty, in_crate);
if non_local_tys.is_empty() {
debug!("orphan_check_trait_ref: ty_is_local `{:?}`", input_ty);
return Ok(());
} else if let ty::Param(_) = input_ty.kind {
Expand All @@ -418,37 +418,36 @@ fn orphan_check_trait_ref<'tcx>(
.substs
.types()
.flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate))
.find(|ty| ty_is_non_local_constructor(ty, in_crate).is_none());
.find(|ty| ty_is_local_constructor(ty, in_crate));

debug!("orphan_check_trait_ref: uncovered ty local_type: `{:?}`", local_type);

return Err(OrphanCheckErr::UncoveredTy(input_ty, local_type));
}
if let Some(non_local_tys) = non_local_tys {
for input_ty in non_local_tys {
non_local_spans.push((input_ty, i == 0));
}

for input_ty in non_local_tys {
non_local_spans.push((input_ty, i == 0));
}
}
// If we exit above loop, never found a local type.
debug!("orphan_check_trait_ref: no local type");
Err(OrphanCheckErr::NonLocalInputType(non_local_spans))
}

fn ty_is_non_local(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, in_crate: InCrate) -> Option<Vec<Ty<'tcx>>> {
match ty_is_non_local_constructor(ty, in_crate) {
Some(ty) => {
if let Some(inner_tys) = fundamental_ty_inner_tys(tcx, ty) {
let tys: Vec<_> = inner_tys
.filter_map(|ty| ty_is_non_local(tcx, ty, in_crate))
.flatten()
.collect();
if tys.is_empty() { None } else { Some(tys) }
} else {
Some(vec![ty])
/// Returns a list of relevant non-local types for `ty`.
///
/// This is just `ty` itself unless `ty` is `#[fundamental]`,
/// in which case we recursively look into this type.
fn contained_non_local_types(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, in_crate: InCrate) -> Vec<Ty<'tcx>> {
if ty_is_local_constructor(ty, in_crate) {
Vec::new()
} else {
match fundamental_ty_inner_tys(tcx, ty) {
Some(inner_tys) => {
inner_tys.flat_map(|ty| contained_non_local_types(tcx, ty, in_crate)).collect()
}
None => vec![ty],
}
None => None,
}
}

Expand Down Expand Up @@ -493,8 +492,7 @@ fn def_id_is_local(def_id: DefId, in_crate: InCrate) -> bool {
}
}

// FIXME(eddyb) this can just return `bool` as it always returns `Some(ty)` or `None`.
fn ty_is_non_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> Option<Ty<'_>> {
fn ty_is_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> bool {
debug!("ty_is_non_local_constructor({:?})", ty);

match ty.kind {
Expand All @@ -513,29 +511,17 @@ fn ty_is_non_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> Option<Ty<'_>>
| ty::Never
| ty::Tuple(..)
| ty::Param(..)
| ty::Projection(..) => Some(ty),
| ty::Projection(..) => false,

ty::Placeholder(..) | ty::Bound(..) | ty::Infer(..) => match in_crate {
InCrate::Local => Some(ty),
InCrate::Local => false,
// The inference variable might be unified with a local
// type in that remote crate.
InCrate::Remote => None,
InCrate::Remote => true,
},

ty::Adt(def, _) => {
if def_id_is_local(def.did, in_crate) {
None
} else {
Some(ty)
}
}
ty::Foreign(did) => {
if def_id_is_local(did, in_crate) {
None
} else {
Some(ty)
}
}
ty::Adt(def, _) => def_id_is_local(def.did, in_crate),
ty::Foreign(did) => def_id_is_local(did, in_crate),
ty::Opaque(..) => {
// This merits some explanation.
// Normally, opaque types are not involed when performing
Expand All @@ -553,7 +539,7 @@ fn ty_is_non_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> Option<Ty<'_>>
// the underlying type *within the same crate*. When an
// opaque type is used from outside the module
// where it is declared, it should be impossible to observe
// anyything about it other than the traits that it implements.
// anything about it other than the traits that it implements.
//
// The alternative would be to look at the underlying type
// to determine whether or not the opaque type itself should
Expand All @@ -562,18 +548,18 @@ fn ty_is_non_local_constructor(ty: Ty<'_>, in_crate: InCrate) -> Option<Ty<'_>>
// to a remote type. This would violate the rule that opaque
// types should be completely opaque apart from the traits
// that they implement, so we don't use this behavior.
Some(ty)
false
}

ty::Dynamic(ref tt, ..) => {
if let Some(principal) = tt.principal() {
if def_id_is_local(principal.def_id(), in_crate) { None } else { Some(ty) }
def_id_is_local(principal.def_id(), in_crate)
} else {
Some(ty)
false
}
}

ty::Error(_) => None,
ty::Error(_) => true,

ty::Closure(..) | ty::Generator(..) | ty::GeneratorWitness(..) => {
bug!("ty_is_local invoked on unexpected type: {:?}", ty)
Expand Down