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

Implement multidispatch and conditional dispatch. #17669

Merged
merged 4 commits into from Oct 10, 2014

Conversation

nikomatsakis
Copy link
Contributor

Implement multidispatch and conditional dispatch. Because we do not attempt to preserve crate concatenation, this is a backwards compatible change. This is not yet fully integrated into method dispatch, so "UFCS"-style wrappers must be used to take advantage of the new features (see the run-pass tests).

cc #17307 (multidispatch)
cc #5527 (trait reform -- conditional dispatch)

Because we no longer preserve crate concatenability, this deviates slightly from what was specified in the RFC. The motivation for this change is described in this blog post. I will post an amendment to the RFC in due course but do not anticipate great controversy on this point -- particularly as the RFCs more important features (e.g., conditional dispatch) just don't work without the change.

@nikomatsakis
Copy link
Contributor Author

cc @aturon @nick29581 @pcwalton

@nikomatsakis
Copy link
Contributor Author

These rules should also make it possible to enable unification of type variables with unsized types.

@comex
Copy link
Contributor

comex commented Oct 1, 2014

I guess it's way too late for this, but for me having the choice of method implementation potentially be determined by the type of some random parameter, or even the time-traveling type of some other function the result gets used in, is moving into confusing, C++-like territory, and I don't like it. It's just too indirect and nonlocal.

edit: I realize you can basically do this already with a generic function that forwards to a trait, but you have to really work at it.

@pcwalton
Copy link
Contributor

pcwalton commented Oct 1, 2014

It's needed for lots of stuff in the standard library.

@pcwalton
Copy link
Contributor

pcwalton commented Oct 1, 2014

Method dispatch is also not determined by the type of some random parameter: that would be C++-style overloading, which this patch does not implement. Rather the method dispatch is only determined by the type parameters in the trait. It is true that you can't just look at the LHS of . to determine which method gets called, but without multidispatch operator overloading (to name one major use case) is too limited to be useful.

I do agree that programmers should avoid adding type parameters to traits unless they know it's needed. This should be a pretty niche feature.

@reem
Copy link
Contributor

reem commented Oct 1, 2014

I'm extremely excited to use this!

@@ -194,10 +193,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// Tests whether an obligation can be selected or whether an impl can be
// applied to particular types. It skips the "confirmation" step and
// hence completely ignores output type parameters.
//
// The result is "true" if the obliation *may* hold and "false" if
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "obligation"

@jroesch
Copy link
Member

jroesch commented Oct 2, 2014

I'm really excited to see these land as well.

@japaric
Copy link
Member

japaric commented Oct 4, 2014

This is awesome! Multidispatch is one the features (the other being HKT) that I've been so eagerly waiting for!

I gave this PR (rebased on top of e434aa1) a test by implementing the IntoIterator trait proposed in the collection reform RFC (modulo the AI stuff), and I ran into some compile errors, but I think my code should work.

Here's the code:

use std::vec::MoveItems;

trait IntoIter<I> {
    fn into_iter_(self) -> I;
}

impl<T, I: Iterator<T>> IntoIter<I> for I {
    fn into_iter_(self) -> I {
        self
    }
}

impl<T> IntoIter<MoveItems<T>> for Vec<T> {
    fn into_iter_(self) -> MoveItems<T> {
        self.into_iter()
    }
}

fn into_iter<I, S: IntoIter<I>>(iterable: S) -> I {
    iterable.into_iter_()
}

fn main() {
    let v = vec![0u8, 1, 2];
    // Pick one of these three lines
    let i: MoveItems<u8> = into_iter(v);  // Works
    //let i = v.into_iter_();  // ~ Err1
    //let i = into_iter(v);  // ~ Err2
}

And these are the errors:

into_iter.rs:27:13: 27:27 error: multiple applicable methods in scope [E0034]
into_iter.rs:27     let i = v.into_iter_();  // ~ Err1
                            ^~~~~~~~~~~~~~
into_iter.rs:8:5: 10:6 note: candidate #1 is `I.IntoIter<I>::into_iter_`
into_iter.rs:8     fn into_iter_(self) -> I {
into_iter.rs:9         self
into_iter.rs:10     }
into_iter.rs:14:5: 16:6 note: candidate #2 is `Vec<T>.IntoIter<MoveItems<T>>::into_iter_`
into_iter.rs:14     fn into_iter_(self) -> MoveItems<T> {
into_iter.rs:15         self.into_iter()
into_iter.rs:16     }
into_iter.rs:27:13: 27:27 error: the trait `core::iter::Iterator<<generic #185>>` is not implemented for the type `collections::vec::Vec<u8>`
into_iter.rs:27     let i = v.into_iter_();  // ~ Err1
                            ^~~~~~~~~~~~~~
error: aborting due to 2 previous errors

I think this should work, v is not eligible for the I: Iterator<T> blanket impl, so it should use the for Vec<T> impl. (Furthermore let i: MoveItems<u8> = into_iter(v) did work!) (On a second thought, I think this may be #16949)

into_iter.rs:28:9: 28:10 error: unable to infer enough type information to locate the impl of the trait `core::kinds::Sized` for the type `<generic #5>`; type annotations required
into_iter.rs:28     let i = into_iter(v);  // ~ Err2
                        ^
into_iter.rs:28:9: 28:10 note: all local variables must have a statically known size
into_iter.rs:28     let i = into_iter(v);  // ~ Err2
                        ^
error: aborting due to previous error

Inference failed. The error message seems to hint DST stuff.

@alexcrichton alexcrichton mentioned this pull request Oct 7, 2014
26 tasks
@nikomatsakis
Copy link
Contributor Author

@japaric note that this PR does not yet integrate with method notation. That rules out the first case. I am not sure about the second, I agree it should probably work.

@nikomatsakis
Copy link
Contributor Author

OK I looked a bit into the example. I see why the code is refusing to infer. You could argue it's a bug but I'm not sure. So what happens is basically that when asked to infer MoveItems<?0> for Vec<u8>, the compiler considers two candidates:

  • The one for all I:Iterator
  • The one just for Vec

You might think that it would rule out the I:Iterator<T> because Vec<u8> does not implement Iterator<?T>, where ?T is some type variable it creates to represent T. But in fact it does not, because it would be legal for another crate X to come along with implement Iterator<Foo> for Vec<u8> where Foo is defined within X. Hence there is in fact ambiguity here.

This basically just shows why its still useful to have associated types as designated output only types. Inference works better but under conditional dispatch the compiler must still be somewhat conservative. If T were an output type of Iterator, this would work, because when we consider the where clause we'd be looking for Iterator for Vec<u8>, not Iterator<?T> for Vec<u8>.

Sorry if this is confusingly phrased. I'll try to explain it better later. TL;DR -- this patch is working as intended afaict.

@nikomatsakis
Copy link
Contributor Author

Actually I might be wrong about it working as intended. Let me look at it a bit more. I'm trying to decide if this ought to be a coherence violation. =)

@nikomatsakis
Copy link
Contributor Author

Ah, of course, not a coherence violation, because I must be the same in the first impl, and Vec<T> is not unifiable with MoveItems<T>

@japaric
Copy link
Member

japaric commented Oct 9, 2014

But in fact it does not, because it would be legal for another crate X to come along with implement Iterator for Vec where Foo is defined within X. Hence there is in fact ambiguity here.

That makes sense.

This basically just shows why its still useful to have associated types as designated output only types.

Indeed!

Sorry if this is confusingly phrased. I'll try to explain it better later. TL;DR -- this patch is working as intended afaict.

I think it was a good explanation. 👍


note that this PR does not yet integrate with method notation

I was suspecting that.

@aturon
Copy link
Member

aturon commented Oct 9, 2014

A small thought experiment:

// Crate A

trait Foo<T> {
    fn foo(&self) -> &T;
}

fn use_foo<T, S: Foo<T>>(s: &S) -> &T {
    s.foo()
}

// Crate B, uses crate A

struct B;
impl Foo<B> for u8 { ... }

use_foo(0u8) // compiles, has type B

// Crate C, uses crate A

struct C;
impl Foo<C> for u8 { ... }

use_foo(0u8) // compiles, has type C

// Crate D, which uses A, B and C

use_foo(0u8) // does not compile, ambiguity 

The expression use_foo(0u8), with use_foo bound to the same function in all cases, has different meanings (and even fails to compile in some cases) depending on the crate in which it appears.

Of course, this doesn't actually violate coherence (as we think of it), but I think it is a new property of the trait system with these changes.

@nikomatsakis
Copy link
Contributor Author

@aturon yes. this is the impact of not requiring crate concatenability.

@nikomatsakis
Copy link
Contributor Author

key point with @aturon's example: the way to think of it is that we consider the visible impls when elaborating out the type parameters, but coherence applies to the fully elaborated source. That means that the various calls to use_foo appear identical before elaboration, but after elaboration they are quite different, since all type parameters are spelled out -- and once that is done, the choice of impl follows.

@nikomatsakis
Copy link
Contributor Author

Thinking a bit more, I realize that I think we could successfully infer the example that @japaric gave. Basically the principle of @aturon's example is that we will infer the only types the user could have written at each point. On that principle, @japaric's example also has exactly one set of types the user could actually write -- despite the fact that a downstream crate could implement Iterator<Foo> for Vec<u8> and hence introduce ambiguity in that crate.

Nonetheless, the reason that the code fails to infer is reasonable and valid, and I think that to adjust the code would require a bit of effort. In particular we have to be careful around the interaction of this case and coherence, since coherence has to be more cautious about what could happen in downstream crates. So I'm roughly content with the behavior as is but I do think we could improve the inference in the future with some effort. Might be worth opening a follow-up bug.

@nikomatsakis
Copy link
Contributor Author

(Note in particular that this would be a backwards compatible extension)

@nikomatsakis
Copy link
Contributor Author

Opened an issue. I also noted that the precise rule in question has rather crucial and subtle interactions, and changing it would be tricky.

bors added a commit that referenced this pull request Oct 10, 2014
Implement multidispatch and conditional dispatch. Because we do not attempt to preserve crate concatenation, this is a backwards compatible change. This is not yet fully integrated into method dispatch, so "UFCS"-style wrappers must be used to take advantage of the new features (see the run-pass tests).

cc #17307 (multidispatch)
cc #5527 (trait reform -- conditional dispatch)

Because we no longer preserve crate concatenability, this deviates slightly from what was specified in the RFC. The motivation for this change is described in [this blog post](http://smallcultfollowing.com/babysteps/blog/2014/09/30/multi-and-conditional-dispatch-in-traits/). I will post an amendment to the RFC in due course but do not anticipate great controversy on this point -- particularly as the RFCs more important features (e.g., conditional dispatch) just don't work without the change.
@bors bors closed this Oct 10, 2014
@bors bors merged commit 7a07f2a into rust-lang:master Oct 10, 2014
@nikomatsakis nikomatsakis deleted the multidispatch branch March 30, 2016 16:16
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.

None yet

9 participants