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

Allow impl Fn() -> impl Trait in return position #93582

Merged
merged 7 commits into from
Oct 30, 2022

Conversation

WaffleLapkin
Copy link
Member

This was originally proposed as part of #93082 which was closed due to allowing impl Fn() -> impl Trait in argument position.

This allows writing the following function signatures:

fn f0() -> impl Fn() -> impl Trait;
fn f3() -> &'static dyn Fn() -> impl Trait;

These signatures were already allowed for common traits and associated types, there is no reason why Fn* traits should be special in this regard.

impl Trait in both f0 and f3 means "new existential type", just like with -> impl Iterator<Item = impl Trait> and such.

Arrow in impl Fn() -> is right-associative and binds from right to left, it's tested by this test.

There even is a test that f0 compiles:

fn allowed_in_ret_type() -> impl Fn() -> impl Into<u32> {
//~^ `impl Trait` not allowed
|| 5
}

But it was changed in PR 48084 (lines) to test the opposite, probably unintentionally given PR 48084 (lines).

r? @nikomatsakis


This limitation is especially annoying with async code, since it forces one to write this:

trait AsyncFn3<A, B, C>: Fn(A, B, C) -> <Self as AsyncFn3<A, B, C>>::Future {
    type Future: Future<Output = Self::Out>;

    type Out;
}

impl<A, B, C, Fut, F> AsyncFn3<A, B, C> for F
where
    F: Fn(A, B, C) -> Fut,
    Fut: Future,
{
    type Future = Fut;

    type Out = Fut::Output;
}

fn async_closure() -> impl AsyncFn3<i32, i32, i32, Out = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}

Instead of:

fn async_closure() -> impl Fn(i32, i32, i32) -> impl Future<Output = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 2, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 2, 2022
@WaffleLapkin WaffleLapkin changed the title Allow impl Fn() -> impl Trait in return possition Allow impl Fn() -> impl Trait in return position Feb 2, 2022
@bors
Copy link
Contributor

bors commented Feb 8, 2022

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

@bors
Copy link
Contributor

bors commented Feb 11, 2022

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

@bors
Copy link
Contributor

bors commented Feb 19, 2022

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

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Hi @WaffleLapkin, sorry for the very slow review. Here are some tests that would be good!

Come to think of it, @WaffleLapkin, here are more tests related to '_: ---

we should show that fn foo() -> impl Fn() -> (impl Debug + '_) is an error

and that impl for<'a> Fn(&'a u8) -> (impl Debug + '_) correctly matches the '_ to 'a

related: impl Fn(&'a u8) -> (impl Debug + '_) correctly matches the '_ to some pre-existing 'a

|| ()
}

fn ff_debug() -> impl Fn() -> impl Fn() -> impl Debug {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some tests about + bounds:

// This should error because the returned closure is capturing
// its argument, which was not declared as part of the signature.
fn f_debug() -> impl Fn(&u8) -> impl Debug {
    |x| x
}
// This should (maybe) error because or parsing ambiguities
fn f_debug() -> impl Fn(&u8) -> impl Debug + '_ {
    |x| x
}
// This should (maybe) error because or parsing ambiguities
fn f_debug() -> impl Fn() -> impl Debug + Send {
    || ()
}
// This should work
fn f_debug() -> impl Fn(&u8) -> (impl Debug + '_) {
    |x| x
}
// This should work
fn f_debug() -> impl Fn() -> (impl Debug + Send) {
    || ()
}

@nikomatsakis nikomatsakis added the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 14, 2022
@nikomatsakis
Copy link
Contributor

Nominating for @rust-lang/lang meeting. We've discussed a predecessor of this PR before, and this is the follow-up. It extends -> impl Trait so that it works in "RPITIRPIT" position:

fn foo() -> impl Fn() -> impl Debug { }

There are a few interesting questions. On the prior PR, @cramertj pointed out that parsing has some syntactical ambiguities here:

fn foo() -> impl Fn() -> impl Debug + Send { }

// Is this `-> impl Fn() -> (impl Debug) + Send` or `impl Fn() -> (impl Debug + Send)` ?

The same above also implies the '_ bound, which ought to capture lifetimes from the arguments:

fn foo() -> impl Fn(&u8) -> (impl Debug + '_)

@nikomatsakis
Copy link
Contributor

@rustbot team

@bors
Copy link
Contributor

bors commented Mar 30, 2022

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

@oli-obk oli-obk added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2022
@nikomatsakis
Copy link
Contributor

Hmm, we've been slow in getting to this! I won't be at the next triage meeting, but I'll try to ensure we discuss it soon! Apologies @WaffleLapkin

@WaffleLapkin
Copy link
Member Author

No worries, this was blocked on #94081 anyway (without it -> impl for<'a> Fn(&'a u8) -> (impl Debug + 'a) crushed the compiler). I'll try adding suggested tests now that #94081 was merged :)

@joshtriplett joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label May 3, 2022
@nikomatsakis
Copy link
Contributor

@WaffleLapkin -- this is still blocked on you making some adjustments, right? We are going to remove the nomination in the meantime.

@nikomatsakis
Copy link
Contributor

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 3, 2022
@nikomatsakis nikomatsakis removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 3, 2022
@WaffleLapkin
Copy link
Member Author

@nikomatsakis the problem is not necessarily with this feature by itself, but more with the general HRTB+RPIT issues like #95647 which stop me from adding proposed tests (because they ICE).

@JohnCSimon JohnCSimon removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 22, 2022
@jackh726 jackh726 removed the I-types-nominated Nominated for discussion during a types team meeting. label Oct 21, 2022
This allows writing the following function signatures:
```rust
fn f0() -> impl Fn() -> impl Trait;
fn f3() -> &'static dyn Fn() -> impl Trait;
```

These signatures were already allowed for common traits and associated
types, there is no reason why `Fn*` traits should be special in this
regard.
@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Oct 25, 2022

@compiler-errors rebased ;)

@compiler-errors
Copy link
Member

@bors r+

See my comment #93582 (comment) -- this may be something we want to revisit before stabilizing this, but it shouldn't block landing the feature gate IMO.

@bors
Copy link
Contributor

bors commented Oct 27, 2022

📌 Commit e93982a has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 28, 2022
…errors

Allow `impl Fn() -> impl Trait` in return position

_This was originally proposed as part of rust-lang#93082 which was [closed](rust-lang#93082 (comment)) due to allowing `impl Fn() -> impl Trait` in argument position._

This allows writing the following function signatures:
```rust
fn f0() -> impl Fn() -> impl Trait;
fn f3() -> &'static dyn Fn() -> impl Trait;
```

These signatures were already allowed for common traits and associated types, there is no reason why `Fn*` traits should be special in this regard.

`impl Trait` in both `f0` and `f3` means "new existential type", just like with `-> impl Iterator<Item = impl Trait>` and such.

Arrow in `impl Fn() ->` is right-associative and binds from right to left, it's tested by [this test](https://github.com/WaffleLapkin/rust/blob/a819fecb8dea438fc70488ddec30a61e52942672/src/test/ui/impl-trait/impl_fn_associativity.rs).

There even is a test that `f0` compiles:
https://github.com/rust-lang/rust/blob/2f004d2d401682e553af3984ebd9a3976885e752/src/test/ui/impl-trait/nested_impl_trait.rs#L25-L28

But it was changed in [PR 48084 (lines)](https://github.com/rust-lang/rust/pull/48084/files#diff-ccecca938872d65ffe8cd1c3ef1956e309fac83bcda547d8b16b89257e53a437R37)  to test the opposite, probably unintentionally given [PR 48084 (lines)](https://github.com/rust-lang/rust/pull/48084/files#diff-5a02f1ed43debed1fd24f7aad72490064f795b9420f15d847bac822aa4621a1cR476-R477).

r? `@nikomatsakis`

----

This limitation is especially annoying with async code, since it forces one to write this:
```rust
trait AsyncFn3<A, B, C>: Fn(A, B, C) -> <Self as AsyncFn3<A, B, C>>::Future {
    type Future: Future<Output = Self::Out>;

    type Out;
}

impl<A, B, C, Fut, F> AsyncFn3<A, B, C> for F
where
    F: Fn(A, B, C) -> Fut,
    Fut: Future,
{
    type Future = Fut;

    type Out = Fut::Output;
}

fn async_closure() -> impl AsyncFn3<i32, i32, i32, Out = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}
```
Instead of:
```rust
fn async_closure() -> impl Fn(i32, i32, i32) -> impl Future<Output = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}
```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 28, 2022
…errors

Allow `impl Fn() -> impl Trait` in return position

_This was originally proposed as part of rust-lang#93082 which was [closed](rust-lang#93082 (comment)) due to allowing `impl Fn() -> impl Trait` in argument position._

This allows writing the following function signatures:
```rust
fn f0() -> impl Fn() -> impl Trait;
fn f3() -> &'static dyn Fn() -> impl Trait;
```

These signatures were already allowed for common traits and associated types, there is no reason why `Fn*` traits should be special in this regard.

`impl Trait` in both `f0` and `f3` means "new existential type", just like with `-> impl Iterator<Item = impl Trait>` and such.

Arrow in `impl Fn() ->` is right-associative and binds from right to left, it's tested by [this test](https://github.com/WaffleLapkin/rust/blob/a819fecb8dea438fc70488ddec30a61e52942672/src/test/ui/impl-trait/impl_fn_associativity.rs).

There even is a test that `f0` compiles:
https://github.com/rust-lang/rust/blob/2f004d2d401682e553af3984ebd9a3976885e752/src/test/ui/impl-trait/nested_impl_trait.rs#L25-L28

But it was changed in [PR 48084 (lines)](https://github.com/rust-lang/rust/pull/48084/files#diff-ccecca938872d65ffe8cd1c3ef1956e309fac83bcda547d8b16b89257e53a437R37)  to test the opposite, probably unintentionally given [PR 48084 (lines)](https://github.com/rust-lang/rust/pull/48084/files#diff-5a02f1ed43debed1fd24f7aad72490064f795b9420f15d847bac822aa4621a1cR476-R477).

r? ``@nikomatsakis``

----

This limitation is especially annoying with async code, since it forces one to write this:
```rust
trait AsyncFn3<A, B, C>: Fn(A, B, C) -> <Self as AsyncFn3<A, B, C>>::Future {
    type Future: Future<Output = Self::Out>;

    type Out;
}

impl<A, B, C, Fut, F> AsyncFn3<A, B, C> for F
where
    F: Fn(A, B, C) -> Fut,
    Fut: Future,
{
    type Future = Fut;

    type Out = Fut::Output;
}

fn async_closure() -> impl AsyncFn3<i32, i32, i32, Out = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}
```
Instead of:
```rust
fn async_closure() -> impl Fn(i32, i32, i32) -> impl Future<Output = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93582 (Allow `impl Fn() -> impl Trait` in return position)
 - rust-lang#103560 (Point only to the identifiers in the typo suggestions of shadowed names instead of the entire struct)
 - rust-lang#103588 (rustdoc: add missing URL redirect)
 - rust-lang#103689 (Do fewer passes and generally be more efficient when filtering tests)
 - rust-lang#103740 (rustdoc: remove unnecessary CSS `.search-results { padding-bottom }`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b4cf523 into rust-lang:master Oct 30, 2022
@rustbot rustbot added this to the 1.67.0 milestone Oct 30, 2022
@WaffleLapkin WaffleLapkin deleted the rpitirpit branch October 31, 2022 11:26
@WaffleLapkin
Copy link
Member Author

Yay!

@bombless
Copy link
Contributor

Mention related tracking issue #34511 #63066

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…errors

Allow `impl Fn() -> impl Trait` in return position

_This was originally proposed as part of rust-lang#93082 which was [closed](rust-lang#93082 (comment)) due to allowing `impl Fn() -> impl Trait` in argument position._

This allows writing the following function signatures:
```rust
fn f0() -> impl Fn() -> impl Trait;
fn f3() -> &'static dyn Fn() -> impl Trait;
```

These signatures were already allowed for common traits and associated types, there is no reason why `Fn*` traits should be special in this regard.

`impl Trait` in both `f0` and `f3` means "new existential type", just like with `-> impl Iterator<Item = impl Trait>` and such.

Arrow in `impl Fn() ->` is right-associative and binds from right to left, it's tested by [this test](https://github.com/WaffleLapkin/rust/blob/a819fecb8dea438fc70488ddec30a61e52942672/src/test/ui/impl-trait/impl_fn_associativity.rs).

There even is a test that `f0` compiles:
https://github.com/rust-lang/rust/blob/2f004d2d401682e553af3984ebd9a3976885e752/src/test/ui/impl-trait/nested_impl_trait.rs#L25-L28

But it was changed in [PR 48084 (lines)](https://github.com/rust-lang/rust/pull/48084/files#diff-ccecca938872d65ffe8cd1c3ef1956e309fac83bcda547d8b16b89257e53a437R37)  to test the opposite, probably unintentionally given [PR 48084 (lines)](https://github.com/rust-lang/rust/pull/48084/files#diff-5a02f1ed43debed1fd24f7aad72490064f795b9420f15d847bac822aa4621a1cR476-R477).

r? `@nikomatsakis`

----

This limitation is especially annoying with async code, since it forces one to write this:
```rust
trait AsyncFn3<A, B, C>: Fn(A, B, C) -> <Self as AsyncFn3<A, B, C>>::Future {
    type Future: Future<Output = Self::Out>;

    type Out;
}

impl<A, B, C, Fut, F> AsyncFn3<A, B, C> for F
where
    F: Fn(A, B, C) -> Fut,
    Fut: Future,
{
    type Future = Fut;

    type Out = Fut::Output;
}

fn async_closure() -> impl AsyncFn3<i32, i32, i32, Out = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}
```
Instead of:
```rust
fn async_closure() -> impl Fn(i32, i32, i32) -> impl Future<Output = u32> {
    |a, b, c| async move { (a + b + c) as u32 }
}
```
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93582 (Allow `impl Fn() -> impl Trait` in return position)
 - rust-lang#103560 (Point only to the identifiers in the typo suggestions of shadowed names instead of the entire struct)
 - rust-lang#103588 (rustdoc: add missing URL redirect)
 - rust-lang#103689 (Do fewer passes and generally be more efficient when filtering tests)
 - rust-lang#103740 (rustdoc: remove unnecessary CSS `.search-results { padding-bottom }`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.