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

Fix inference for projections with associated traits with multiple bounds; fixes #34792 #34912

Closed
wants to merge 1 commit into from

Conversation

rozbb
Copy link

@rozbb rozbb commented Jul 19, 2016

The problem was due to the fact that no particular trait bound was specified when a projection was matched with a trait bound, even when multiple bounds existed. The compiler consistently picked the first bound on the associated type that it found that was able to match the type's obligation (which, in the regression test, is <F as Foo> or <Self as Foo>).

The fix involved modifying the SelectionCandidate::ProjectionCandidate variant to carry a trait ref to refer to its matching bound. And instead of stopping on the first match we find, we add all matching bounds to the candidates vector. Also in the winnowing step, a ProjectionCandidate cannot trump another ProjectionCandidate unless they are identical.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@rozbb
Copy link
Author

rozbb commented Jul 19, 2016

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned pnkfelix Jul 19, 2016
@rozbb
Copy link
Author

rozbb commented Jul 19, 2016

I recall someone saying that this might require a crater test. Is that still the case?

@eddyb
Copy link
Member

eddyb commented Jul 19, 2016

@doomrobo Indeed, although you first need to fix the two compile-fail tests Travis reports - looks like the error messages changed a bit there.

@@ -1170,22 +1172,20 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
debug!("assemble_candidates_for_projected_tys: trait_def_id={:?}",
trait_def_id);

let result = self.probe(|this, snapshot| {
self.probe(|this, snapshot| {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a probe, I believe in_snapshot will do.

@rozbb
Copy link
Author

rozbb commented Jul 20, 2016

Hmm. It appears that the error messages for test/compile-fail/associated-types-project-from-hrtb-in-fn-body.rs and test/compile-fail/associated-types-subtyping-1.rs have gotten significantly worse.

Here is the gist for the first one. The second differs in the same way. I have no intuitions so far, but I'm looking into it.

@eddyb
Copy link
Member

eddyb commented Jul 20, 2016

cc @nikomatsakis @jonathandturner What could cause those errors? It looks like the Sized bound necessary on local variables, but what is it mismatching on?

@bors
Copy link
Contributor

bors commented Aug 12, 2016

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

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with a rebase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants