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.

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

[perf] Skip attempting to run coerce_unsized on an inference variable #69530

Merged
merged 2 commits into from
May 10, 2020
Merged

[perf] Skip attempting to run coerce_unsized on an inference variable #69530

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32c360b
[perf] Skip attempting to run coerce_unsized on an inference variable
Aaron1011 Feb 27, 2020
ff6e4ee
Use resolved types in body of coerce_unsized
Aaron1011 Feb 28, 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
36 changes: 35 additions & 1 deletion src/librustc_typeck/check/coercion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,9 +452,43 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
// &[T; n] or &mut [T; n] -> &[T]
// or &mut [T; n] -> &mut [T]
// or &Concrete -> &Trait, etc.
fn coerce_unsized(&self, source: Ty<'tcx>, target: Ty<'tcx>) -> CoerceResult<'tcx> {
fn coerce_unsized(&self, mut source: Ty<'tcx>, mut target: Ty<'tcx>) -> CoerceResult<'tcx> {
debug!("coerce_unsized(source={:?}, target={:?})", source, target);

source = self.shallow_resolve(source);
target = self.shallow_resolve(target);
debug!("coerce_unsized: resolved source={:?} target={:?}", source, target);

// These 'if' statements require some explanation.
// The `CoerceUnsized` trait is special - it is only
// possible to write `impl CoerceUnsized<B> for A` where
// A and B have 'matching' fields. This rules out the following
// two types of blanket impls:
//
// `impl<T> CoerceUnsized<T> for SomeType`
// `impl<T> CoerceUnsized<SomeType> for T`
//
// Both of these trigger a special `CoerceUnsized`-related error (E0376)
//
// We can take advantage of this fact to avoid performing unecessary work.
// If either `source` or `target` is a type variable, then any applicable impl
// would need to be generic over the self-type (`impl<T> CoerceUnsized<SomeType> for T`)
// or generic over the `CoerceUnsized` type parameter (`impl<T> CoerceUnsized<T> for
// SomeType`).
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph feels a bit misleading, although the conclusion is likely correct. In particular, if (say) the "source" type is an unbound inference variable, then it's not that the other, more narrow impls don't apply, right? It's more that any impl could apply, and hence we wind up with an ambiguous result?

That said, the trait selection code refuses to proceed (always yields ambiguous) if the source type is unknown, so this short-circuit here has no effect I imagine. I do feel like this could change behavior if the target type is unknown, since there may be at most one impl that could apply, though I feel like we probably don't want to be coercing then -- I sort of expect this coerce to only trigger if we are coercing to a "fat-pointer type", but I don't see any code that forces this to be the case.

In general, though, we don't try to coerce from cases where the source or target types are unknown anyhow, right? Well, I think that's true,but it doesn't seem to be enforced by any kind of "blanket check" in the coerce_to routine.

Copy link
Member Author

Choose a reason for hiding this comment

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

then it's not that the other, more narrow impls don't apply, right? It's more that any impl could apply, and hence we wind up with an ambiguous result?

When I was writing this, my concern was blanket impls - I wanted to be sure that we wouldn't bail out when we could have actually picked a blanket impl.

I do feel like this could change behavior if the target type is unknown, since there may be at most one impl that could apply

That's a good point. Since unsizing coercions happen fairly 'early' during typeck, it's possible that we might skip performing an unsizing coercion that turns out to be 'reasonable' (e.g. we later unify the target variable with the expected type).

In general, though, we don't try to coerce from cases where the source or target types are unknown anyhow, right?

I'm not quite sure what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I think of coercions, they trigger only when we know the source/target well enough to see that the coercion applies (otherwise we just unify). But I think that is not an 'obvious property' of the code as written.

//
// However, these are exactly the kinds of impls which are forbidden by
// the compiler! Therefore, we can be sure that coercion will always fail
// when either the source or target type is a type variable. This allows us
// to skip performing any trait selection, and immediately bail out.
if source.is_ty_var() {
debug!("coerce_unsized: source is a TyVar, bailing out");
return Err(TypeError::Mismatch);
}
if target.is_ty_var() {
debug!("coerce_unsized: target is a TyVar, bailing out");
return Err(TypeError::Mismatch);
}

let traits =
(self.tcx.lang_items().unsize_trait(), self.tcx.lang_items().coerce_unsized_trait());
let (unsize_did, coerce_unsized_did) = if let (Some(u), Some(cu)) = traits {
Expand Down