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

Make the return type of `Fn` trait an associated type #21019

Merged
merged 14 commits into from Jan 28, 2015

Conversation

Projects
None yet
7 participants
@nikomatsakis
Copy link
Contributor

commented Jan 12, 2015

Fixes #20871

r? @aturon (at least until we decide definitively if this is a good idea)

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-20871-ret-as-assoc-type branch from 560a3c1 to 6d2dd96 Jan 12, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jan 12, 2015

In terms of the sugar, does this limit us to the possibilities of the Trait(A, B) -> C desugaring later? It kinda semantically makes sense to me that the inputs are type parameters and the output is an associated type, but it seems like our desugaring would look like:

where F: A(B) -> C
// equivalent to
where F: A<B, FirstAssociatedType=C>

which is a little odd, but perhaps not the end of the world!

@sfackler

This comment has been minimized.

Copy link
Member

commented Jan 12, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2015

@alexcrichton that is precisely what the desugaring amounts to in this version. The fact that Fn(A) -> B desugars to Fn<(A,), Output=B> doesn't seem particularly odd to me by itself (but read on). @eddyb even had the interesting idea that we could extend the ->T sugar to always mean Output=T so that one could write Add<R> -> S instead of Add<R, Output=S>.

There is however one thing I wasn't crazy about: sometimes you won't want to constrain the output of the function. In that case, the existing desugaring doesn't give you that option, so you have to opt out of the sugar altogether.

For example, the Map adapter is written today:

struct Map<A,B,I,F>
    where I : Iterator<Item=A>, F : FnMut(A) -> B
{
    iter: I,
    fn: F
}

impl<A,B,I,F> Iterator<B> for Map<I,F>
    where I : Iterator<Item=A>, F : FnMut(A) -> B
{ ... }

Under this scheme it could be simplified to:

struct Map<I,F>
    where I : Iterator, F : FnMut<(I::Item,)>
{
    iter: I,
    fn: F
}

impl<I,F> Iterator<F::Output> for Map<I,F>
    where I : Iterator, F : FnMut<(I::Item,)>
{
}

I definitely like having fewer type parameters, but the difference between FnMut(I::Item) and FnMut<(I::Item),> feels...subtle. But at the same time having FnMut(I::Item) not bind the result type seems like a non-starter, too inconsistent with other things (and in particular it'd bite us in object type syntax: Box<FnMut()> would be an error, since the Output type is not bound).

On the other hand, if one preferred, one could never use the non-sugared form, at the cost of more type parameters:

struct Map<B,I,F>
    where I : Iterator, F : FnMut(I::Item) -> B
...

so in some sense it's no worse than today, just gives you the option to do better.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-20871-ret-as-assoc-type branch from 6d2dd96 to b7ebdcd Jan 13, 2015

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-20871-ret-as-assoc-type branch from b7ebdcd to e769d61 Jan 13, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jan 13, 2015

Whoa! That is very interesting about the difference between Fn<(Foo,)> and Fn(Foo), I had to read this a few times before it clicked.

For now we don't even necessarily have to have all the parameters on Map, we could just remove the where clause and then add the type parameters to the impl blocks I think?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2015

@alexcrichton yes, we could.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2015

I've decided to open an RFC for this. Better to err on the side of more RFCs, I figure.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2015

@alexcrichton in fact we cannot remove the parameters on map today, due to the fact that B parameter in the impl would be unconstrained, precisely because the return type of the Fn trait is not an associated type.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-20871-ret-as-assoc-type branch 2 times, most recently from 880c831 to 9a7150b Jan 27, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2015

RFC has landed now. Rebased, testing the build now. I had to add a fix for #21664 (which is now at the front of the PR series) because recent changes exposed that problem when combined with this change.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-20871-ret-as-assoc-type branch 2 times, most recently from ae86004 to 6734e63 Jan 27, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2015

r? @nick29581 (@aturon felt he wasn't familiar enough with the code in question)

@nrc

This comment has been minimized.

Copy link
Member

commented Jan 27, 2015

@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2015

☔️ Merge conflict

nikomatsakis added some commits Jan 26, 2015

Add the notion of normalizing a parameter environment and ensure that
all parameter environments are normalized. Correspondingly, stop
normalizing predicates we extract out of the environment. Fixes #21664.
Move return type an associated type of the `Fn*` traits. Mostly this …
…involves tweaking things in

the compiler that assumed two input types to assume two ouputs; we also have to teach `project.rs`
to project `Output` from the unboxed closure and fn traits.
Fix a latent bug in trait dispatch where we sometimes counted associa…
…ted types

when constructing the vtable-index. Not good.

nikomatsakis added some commits Jan 12, 2015

Update test files; mostly the problem is that they were using the
explicit form `Fn<A,B>` and now should use `Fn(A) -> B` or
`Fn<A,Output=B>`, but in some cases we get duplicate error
reports. This is mildly annoying and arises because of the main error
and another error from the projection. Might be worth squashing those,
but seems like a separate problem.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:issue-20871-ret-as-assoc-type branch from 6734e63 to aaf3df3 Jan 28, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2015

@bors r=nrc aaf3df3

bors added a commit that referenced this pull request Jan 28, 2015

Auto merge of #21019 - nikomatsakis:issue-20871-ret-as-assoc-type, r=nrc
Fixes #20871

r? @aturon (at least until we decide definitively if this is a good idea)
@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2015

⌛️ Testing commit aaf3df3 with merge f7f9c38...

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2015

@bors r=nrc 05ffdc5

@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2015

⌛️ Testing commit 05ffdc5 with merge a45e117...

bors added a commit that referenced this pull request Jan 28, 2015

Auto merge of #21019 - nikomatsakis:issue-20871-ret-as-assoc-type, r=nrc
Fixes #20871

r? @aturon (at least until we decide definitively if this is a good idea)

@bors bors merged commit 05ffdc5 into rust-lang:master Jan 28, 2015

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
homu Test successful
Details
@brson

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2015

breaking change

@nikomatsakis nikomatsakis deleted the nikomatsakis:issue-20871-ret-as-assoc-type branch Mar 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.