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

small coherence cleanup #74454

Merged
merged 4 commits into from
Jul 22, 2020
Merged
Changes from 1 commit
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
33 changes: 18 additions & 15 deletions src/librustc_trait_selection/traits/coherence.rs
Original file line number Diff line number Diff line change
@@ -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))
@@ -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 {
@@ -424,27 +424,30 @@ fn orphan_check_trait_ref<'tcx>(

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))
}

// FIXME: Return a `Vec` without `Option` here.
fn ty_is_non_local(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, in_crate: InCrate) -> Option<Vec<Ty<'tcx>>> {
/// 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) {
None
} else 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) }
Vec::new()
} else {
Some(vec![ty])
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],
}
}
}