coercion: use a generalization of the target to specialize the source to coerce instead of propagating the wrong type.

[eddyb]

The issue solved here involved generic "constructor" traits and unsizing coercions.
A real usecase is box desugaring, which is being demonstrated as a test-case.

Given such a trait with more than one implementation, e.g. for Box<T> and Rc<T>:

trait Make<T> {
    fn make(T) -> Self;
}
impl<T> Make<T> for Box<T> {
    fn make(x: T) -> Box<T> { Box::new(x) }
}
impl<T> Make<T> for Rc<T> {
    fn make(x: T) -> Rc<T> { Rc::new(x) }
}

An direct call would be too unconstrained for a coercion, so the expected type ends up propagated:

fn make_boxed_trait<E: Error>(x: E) -> Box<Error> {
    // error: Make<E> not implemented for Box<Error>.
    Make::make(x)
}

The workaround was to manually specify enough about the produced type to allow resolving it:

fn make_boxed_trait<E: Error>(x: T) -> Box<Error> {
    let boxed: Box<_> = Make::make(x);
    // <Box<_> as Make<E>> gets selected to the appropriate impl, and so
    // boxed has the type Box<E> at this point, which can coerce to Box<Error>.
   boxed
}

That workaround is implemented in this PR, for all coercions where unsizing was ambiguous.
A generalized type is picked based on the CoerceUnsize impl that could be used for coercing.

For example, CoerceUnsized<Box<Error>> is implemented by Box<T> forall T, so the generalized
type is Box<_>, which matches both Box<E> and Box<Error>.
Such generalization allows constraining the source of the potential coercion while still allowing
it to be any type that could coerce to the target, or the target type itself.

[rust-highfive]

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

[eddyb]

r? @pnkfelix

[pnkfelix]

@eddyb I'm confused by the description you have in this issue.

When you write "That workaround is implemented in this PR", I now infer that you mean: "This PR makes it unnecessary to explicitly write such workarounds in one's code, because the compiler will now effectively do the work for you."

So, before I jump into the review itself, is that a correct inference ?

(And if so, maybe rephrase the description so that the commit of the code to the repo will have a somewhat clearer statement of its purpose?)

---

update: (maybe part of my issue is that I jumped straight to reading the description of this PR without reading its title. This is a common "mistake" I make, though I'm not really sure its a mistake since bors still does not include the titles of PR's in its merge commit messages. I.e. you should not assume that people can see the title; the PR's description should be able to stand on its own. I often deal with this by just cut-and-pasting my title into my description's first line, to be honest.)

[pnkfelix]

@eddyb oh, no, wait, my inference above was incorrect, I guess? That is, you do still need to employ the described workaround in the user's code. (I only realized this reading the comments and then skipping ahead to the test case.)

So now I'm back to trying to work out what this patch is accomplishing in practice.

[eddyb]

@pnkfelix I did expect to need changes to the explanations, as the feature itself is a bit convoluted.
Your first interpretation is correct, let me try to come up with a better phrasing.
How about "This PR enables the compiler to generate the same coercion, without the above workaround being employed in the code"?

[pnkfelix]

@eddyb yes, that sounds better. I still need to read the test case more carefully, I'll have more feedback shortly.

[pnkfelix]

It may be even nicer to have a test that shows the kind of case that we really expect to come up, namely where no type annotation is present at all on any of the local variables, and instead the constraints like Box<_> or Rc<_> on the type arise from where the value flows to, e.g. return position for the demo function, or argument position for some subroutine that the demo function invokes.

Having said that, at least I grok the test at this point.

[pnkfelix]

so, in the code before this PR, the control flow that reached this point (line 159) corresponds to the comment above "Otherwise, just use subtyping rules."

Clearly you are no longer "just using the subtyping rules" in the code that follows that point in the control flow here. :)

Still, for some reason I wish there was a comment line or two here of the same pithy nature as "Otherwise just use subtyping rules" before jumping straight into the discussion of "generalization in hopes of an unsizing coercion" ... something that helped the reader re-establish what case we are in in the coercion analysis? Or is that just a pipe dream?

---

Update: I might be thinking of a comment along the lines of:

"Attempt to select all CoerceUnsized and Unsized obligations (as well as applying subtyping rules). Other obligations will be gathered and applied later."

(Basically restating/summarizing the comment above CoerceUnsizedResult)

[eddyb]

Something along the lines of "All attempts at coercion have failed, must use subtyping (or try harder)."?

[eddyb]

I have found a relatively simple example that will be mishandled by the current implementation:

impl CoerceUnsized<Pointer<Trait>> for Pointer<Trait> {}
impl<T: Trait> CoerceUnsized<Pointer<Trait>> for Pointer<T> {}

Selecting <Pointer<Trait> as CoerceUnsized<Pointer<Trait>>> won't be ambiguous, and it would result in the first impl.
It's harmless, because it will result in the same behavior as before this patch, but it would be better to detect this ambiguity (by manually looking at impls, instead of using trait selection) and maybe even find the common supertype of the Self types of all applicable impls (Pointer<_> in this case).

[pnkfelix]

I've skimmed this but I really want @nikomatsakis to look at it too, since I think has expressed concern over the growing complexity of the coercion logic.

r? @nikomatsakis

[nikomatsakis]

Sorry, I'm being slow again. I plan to examine this this week. I have to admit I am pretty wary here, though, I sort of want to tread lightly on inference until we have a feeling for the full set of use cases that we aim to support. Modifying the unsizing coercion seems like an interesting approach but not clearly the right one -- and these sorts of changes are very hard to "contain" in terms of stabilization.

[bors]

☔ The latest upstream changes (presumably #28138) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

ping? needs a review?

[nikomatsakis]

Yes, I've been very overwhelmed. I'm working on clearing my backlog right now, including this PR.

[nikomatsakis]

Nit: this type is getting complex enough that adding a struct to represent the Ok case might be a good idea, or else using a different enum altogether.

[eddyb]

I believe I had tried that at some point and it got unwieldy, but maybe just a struct would work.

[nikomatsakis]

/me starts reading this PR. Sorry for the delay.

[nikomatsakis]

(OK, still reading, sorry, this is dense code I'm having to bring it back into cache)

[eddyb]

@nikomatsakis Any progress on this? I almost feel like we should move to https://reviewable.io/ so we can keep track of reviews.

[nikomatsakis]

A meta-comment: I think that changes like this ought to be feature-gated in some way, like any other. This one seems particularly amenable to such treatment.

[nikomatsakis]

@eddyb not sure if you saw my comments on this; sorry it took me so amazingly long to read it. The main reason for the delay is that I feel like we need to get a better handle on coercions before we go and expand them, it's hard for me to tell if this approach is good or is something that will box us into a corner. A feature gate would make me feel a lot better about that. All that said, there is only so long we need to stall here-- I admit this patch is a lot cleaner and more elegant than I initially anticipated, and this may well be a part of the way forward.

Perhaps a good idea would be to schedule a time to brainstorm and chat about this as a video chat. I'd also like to sketch out how this affects other scenarios that seem strongly related; I have a few in mind but have to go scouring logs to find the specific examples.

[alexcrichton]

(ping)

Should this be rebased? Closed?

(just clearing out the queue)

[nikomatsakis]

@alexcrichton Heh. This is an old PR by now. @eddyb and I still have not caught up as I described in my comment -- but maybe we can make time for it. I've forgotten the details of this PR now but I remember it was nicer than I feared. :)

This part of the codebase hasn't really changed much in the meantime though, I imagine rebasing would not be so hard.

[eddyb]

@nikomatsakis Actually, I've abandoned this PR as there are other issues, and there is a much more principled approach to the whole thing.
In retrospect, I should've mentioned you in that precise comment.

[nikomatsakis]

No worries, I'll take a look now:)