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

Return position impl Trait in traits #3425

Merged
merged 12 commits into from
Jun 13, 2023
Merged

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Apr 27, 2023

  • Permit impl Trait in fn return position within traits and trait impls.
  • Allow async fn in traits and trait impls to be used interchangeably with its equivalent impl Trait desugaring.
  • Allow trait impls to #[refine] an impl Trait return type with added bounds or a concrete type.

Background

This RFC is a collaboration between myself and @compiler-errors, and is based on an earlier RFC by @nikomatsakis.

The primary changes from that RFC are:

  • async fn is now allowed to be used interchangeably with its equivalent impl Trait desugaring.
  • #[refine] is now included as a way to add more information about the returned type in an impl, either by adding bounds to the impl Trait type, or by using a concrete type. Refined trait implementations #3245 did not exist at the time of the previous RFC (return position impl trait in traits #3193), and was added partly in response to discussion there.
  • You are now allowed to use dyn Trait on a trait with a method returning impl Trait, as long as that method has a where Self: Sized bound.

Rendered

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Apr 27, 2023
@lcnr lcnr added the T-types Relevant to the types team, which will review and decide on the RFC. label May 2, 2023
@nikomatsakis

This comment was marked as outdated.

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 2, 2023
@nikomatsakis

This comment was marked as off-topic.

@rustbot

This comment was marked as off-topic.

@rfcbot

This comment was marked as off-topic.

@rfcbot rfcbot removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 2, 2023
@nikomatsakis
Copy link
Contributor

I don't think this should have a T-types FCP for the feature (perhaps for stabilization).

@rustbot label -T-types

@rustbot

This comment was marked as off-topic.

@nikomatsakis nikomatsakis removed the T-types Relevant to the types team, which will review and decide on the RFC. label May 2, 2023
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

This feature has been a long time coming and feels like a no brainer to me. I'm going to start the merge proceedings.

@rfcbot
Copy link
Collaborator

rfcbot commented May 2, 2023

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 2, 2023
# Motivation
[motivation]: #motivation

The `impl Trait` syntax is currently accepted in a variety of places within the Rust language to mean "some type that implements `Trait`" (for an overview, see the [explainer] from the impl trait initiative). For function arguments, `impl Trait` is [equivalent to a generic parameter][apit] and it is accepted in all kinds of functions (free functions, inherent impls, traits, and trait impls).
Copy link
Member

Choose a reason for hiding this comment

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

There's this ...

XXX document:

    - you can capture type parameters
    - but not lifetimes, unless they appear in the bounds

in the explainer. It would be nice to explain a bit better that + what happens if all the lifetimes are captured as we do with types and consts too.

@lcnr
Copy link
Contributor

lcnr commented May 3, 2023

Wrote this down after midnight so it's not as clear and concise as I would have liked. I only really thought about this in-depth after Niko proposed the FCP.

tl;dr: RPITIT is not a trivial extension of TAIT or RPIT and does not simply fall out of the desire for a consistent design. As I stated separately, I don't think we should add return type notation RTN. This causes RPITIT to be a major footgun for library authors without a significant benefit.

I very much agree that we should add support to async fn in traits but am not yet sold that RPITIT is the right approach for that.

RPITITs are not simple opaque types

moved to a hackmd as it is somewhat pedantic

I don't agree that RPITIT will result in a more consistent language given that the semantics of RPITIT are significantly different from RPIT. It is easy to provide context to the error message when trying to use it, recommending an associated type with TAIT.

RPITIT can result in worse library design

To keep this short, I prefer https://github.com/tmandry/rfcs/blob/rpitit/text/0000-return-position-impl-trait-in-traits.md#what-about-using-an-explicit-associated-type over RPITIT and don't think we should add RPITIT (at least for now).

The RFC uses IntoIterator as a good example for where RPITIT would be applicable. Searching for ::IntoIter: on github results in a non-negligible amount of crates which use bounds on IntoIter. Using RPITIT without a way to name the anonymous projection would have prevented this code. Given that I fairly strongly dislike RTN, this is a major issue with RPITIT imo.

Going through the list of cases where an explicit associated type should be used, we can reverse it to get requirements to correctly use RPITIT:

  • it is irrelevant whether users provide concrete types as you can't expect them to always add #[refine]
  • no user will ever need to constrain the impl Trait to a concrete value: also not the case for IntoIterator
  • no user will ever need to add additional bounds for the impl Trait

We have proof that the second and third point do not hold for IntoIterator and I am certain that the first point also does not hold. I don't think RPITIT would be sensible for IntoIterator.

More generally, I cannot think of a trait where RPITIT would be appropriate given these requirements, unless the trait is local to your library or sealed. I assume that given the ability, library users will sometimes end up using RPITIT simply because it is possible, which will then require breaking changes to fix later on.

RPITIT is not necessary for async functions in traits

By going with the "explicit associated type" route, we can instead add some attribute to express the concept of "this named associated type may be implicitly defined by this function", something like

trait IntoIterator {
    type Item;
    type IntoIter: Iterator<Item = Self::Item>;
    #[may_implicitly_define(IntoIter)]
    fn into_iter(self) -> Self::IntoIter;
}

impl<T: Iterator> IntoIterator for T {
    type Item = T::Item;
    // does not explicitly define `IntoIter`,
    fn into_iter(self) -> Self::IntoIter {
        self
    }
    // or
    fn into_iter(self) -> impl Iterator<Item = T::Item> {
        self
    }
} 

A similar approach can then be used for async functions as well. I don't yet know how exactly these attributes should look but I do prefer such attributes over RPITIT.

RPITIT is at most a linear ergonomics improvement

As stated in the RFC, RPITIT should be equivalent to its desugaring using TAIT and associated types. This desugaring has some repetition but it should pretty much never significantly impact development speed or readability.

Given the fairly mechanical desugaring, we should be able to emulate RPITIT using TAIT and a set of macros for trait definitions and impls. If the manual desugaring poses a major issue, users should be drawn to such a set of macros, at which point we can reconsider adding RPITIT.

@comex
Copy link

comex commented May 4, 2023

I agree with @lcnr that this feature effectively requires return type notation. In some hypothetical future where Rust decided against adding return type notation, RPITIT would clearly not hold its weight. Therefore it does seem strange to be landing it without at least talking about return type notation.

Unlike @lcnr, I have no complaints about return type notation; I think it sounds great. But I am confused about the details. Is it going to be () or (..)? (The implementation PR suggests it might switch over to (..) in the future, but today's blog post uses ().) And since neither of those syntaxes actually lists the arguments, how does that interact with generic functions, where the return type might conditionally implement a trait depending on the argument types?

@dhardy
Copy link
Contributor

dhardy commented May 4, 2023

We expect to introduce a mechanism for naming the result of -> impl Trait return types in a follow-up RFC.

Is deferring this acceptable? It seems like it should be a core part of the proposal. Then again, we still have no syntax for naming the return type of a closure or a regular impl Trait method, so perhaps not.

Should we at least have somewhere to discuss relevant issues:

  • Syntax for naming the return type of RPITIT (or something more general like return_type_of<F>)
  • Associated type aliases (which are not associated type defaults)

@dhardy
Copy link
Contributor

dhardy commented May 4, 2023

An alternative approach:

trait T {
    auto type Fut = return_type_of<Self::fun>;

    async fn fun(&self) -> i32;
}

auto type defines an associated type, except it cannot be defined at the impl site. (Optionally, we could allow impl-site definitions that correlate exactly with the inferred type in order to allow migration from assoc. types to assoc. auto types, and just warn that the definition is redundant.)

async fn fun (and any other usage of RPIT in a trait method) could be legal only given a corresponding auto type. By making a corresponding auto type required, we (a) always have a named type for usage in bounds, field types of generic structs, etc., and (b) are more transparent about why dyn Trait is not valid (at least not without constraining the assoc. auto type, if that is even possible).

Putting both of these together would allow e.g. migrating ImplIterator::IntoIter into an auto type and into_iter into an RPITIT method.


Addendum: but is an auto type just a type alias? The key question is whether a single auto type can be used by multiple RPITIT methods, and with the above syntax the answer is clearly no (it's also likely not useful).

So the above is just:

  • associated type aliases (a new feature, which could be more general than this)
  • a suggestion that we require an assoc. type alias for RPITIT
  • an optional compatibility rule allowing migration to RPITIT
  • return_type_of syntax or something (specific to this use-case or more generally applicable)

@kpreid
Copy link
Contributor

kpreid commented May 5, 2023

As a user of stable Rust, I'd like to see effort put first into stabilization of TAIT, which would enable many of the use cases that RPITIT would, and also provide more information about what benefits RPITIT would or would not provide and how it should be designed, since it results in a smaller delta (“write this trait using an associated type” vs. “can't practically write this trait at all”).

[summary]: #summary

* Permit `impl Trait` in fn return position within traits and trait impls.
* Allow `async fn` in traits and trait impls to be used interchangeably with its equivalent `impl Trait` desugaring.
Copy link
Contributor

Choose a reason for hiding this comment

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

One consequence of this is that it basically stabilizes the impl Future desugaring. I don't imagine we'll want to change it, and enough things depend on the impl Future desugaring now that we probably can't change it practically already, but it still means we're committing to what in some ways feel like internal implementation details to me.


### async fn desugaring

`async fn` always desugars to a regular function returning `-> impl Future`. When used in a trait, the `async fn` syntax can be used interchangeably with the equivalent desugaring in the trait and trait impl:
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably the reason for doing this is so that we can use #[refine] to make stricter promises on the impl? For example, adding a Send bound at the impl level.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's one (another example of an added promise would be that you don't capture self). A whole other reason would be to include code that runs at call time instead of during poll.

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label May 23, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented May 23, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

Copy link
Member

@aliemjay aliemjay left a comment

Choose a reason for hiding this comment

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

I'm concerned about an inconsistency with inherent impls regarding the capture rules for lifetimes. It can be shown in this example:

struct Foo<'a>(&'a str);

// ERROR: `impl Sized` doesn't capture 'a
impl<'a> Foo<'a> {
    fn into(self) -> impl Sized { self }
}

trait IntoImpl {
    fn into(self) -> impl Sized;
}

// OK! Allowed by the RFC.
impl<'a> IntoImpl for Foo<'a> {
    fn into(self) -> impl Sized { self }
}

We say that a generic parameter is captured if it may appear in the hidden type. These rules are the same as those for -> impl Trait in inherent impls.

Given the difference above, this statement is no longer true.

Additionally, in the capture rules, the text talks only about generic parameters of the trait implementation - it says nothing about the trait definition. It is not clear to me, for example, whether impl Sized captures the lifetime 'a here:

trait Trait<'a> {
    fn test() -> impl Sized;
}

@tmandry
Copy link
Member Author

tmandry commented Jun 2, 2023

@aliemjay Oh, I think that text should have been removed. I've reorganized the sections related to scoping rules and removed the rules that were incorrect. RPITIT should now be consistent with RPIT in inherent methods.

I also noticed there's an inconsistency in how the implementation handles this; see rust-lang/rust#112194.

Additionally, in the capture rules, the text talks only about generic parameters of the trait implementation - it says nothing about the trait definition.

We are talking about the hidden type supplied by each implementation. The type supplied by the implementation can't name the parameters of the trait itself because those aren't in scope – instead, usually you would have a generic parameter of the impl that corresponds to each trait parameter. (I realize this is a bit subtle, but in the reference-level sections it helps to be pedantic.)

trait Trait<'a> {
    fn test() -> impl Sized;
}

In this example the return type is not allowed to capture 'a because it does not appear in impl Sized. On the other hand, if there were a type parameter in scope, the concrete type returned by our impl could capture that type parameter (and therefore would refer to any lifetimes the actual type for that parameter includes).

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jun 2, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 2, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@aliemjay
Copy link
Member

aliemjay commented Jun 7, 2023

It is not clear to me, for example, whether impl Sized captures the lifetime 'a here:

trait Trait<'a> {
    fn test() -> impl Sized;
}

The decision here doesn't seem to be straightforward...

If it captures the lifetime 'a, we would have an inconsistency with inherent impls. If it doesn't, we should treat the types of <T as Trait<'a>>::test() and <T as Trait<'b>>::test() to be equal for two different lifetimes. This might seem reasonable but it contradicts with the desugaring specified in this RFC because projection types are generic over all trait inputs, i.e. <T as Trait<'a>>::Opaque#0 and <T as Trait<'b>>::Opaque#0 are not equal types unless 'a == 'b. For a concrete example, see how this example fails with the current implementation:

#![feature(return_position_impl_trait_in_trait)]

trait Trait<'a> {
    fn test() -> impl Sized;
}

fn test<'a, 'b, T: for<'x> Trait<'x>>() {
    let mut x;
    x = <T as Trait<'a>>::test();
    x = <T as Trait<'b>>::test();
    //^~ ERROR `'a` and `'b` must be the same: replace one with the other
}

Consequently we should change the desugaring to involve a supertrait that does not have the lifetime 'a in its parameters:

trait SuperTrait {
    type AnonOpaque;
}

trait Trait<'a>: SuperTrait {
    fn test() -> Self::AnonOpaque;
}

Copy link
Member

@aliemjay aliemjay left a comment

Choose a reason for hiding this comment

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

Default trait functions

impl Trait in trait functions with default body is underspecified. Is it allowed? If yes, we should specify the desugaring. It is not trivial as it depends on the feature of associated type defaults. We should also mention the drawback of not being able to apply #[refine] to it.

Legal positions for impl Trait in trait impls

The text specifies the legal positions in trait definitions but leaves trait implementations unspecified. Do we allow this?:

trait Trait {
    fn test() -> impl Sized;
}

impl Trait for () {
    fn test() -> Result<impl Sized, impl Sized> { Ok::<(), ()>(()) }
}

Forward compatiblity with named return types

Should we limit the legal positions for impl Trait to be maximally compatible with the naming schemes for return types?

fn f() -> impl Sized; // Ok.
fn f() -> Box<impl Sized>; // Not ok; cannot be named.
fn f() -> impl Iterator<Item = impl Sized>; // Ok.
fn f() -> impl Iterator<Item = Box<impl Sized>>; // Not ok; cannot name `impl Sized`.

type Item = u32;

fn into_iter(self) -> impl Iterator<Item = Self::Item> {
self.into_iter()
Copy link
Member

Choose a reason for hiding this comment

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

This is an ambiguous method resolution with candidates from IntoIterator and NewIntoIterator.
There are multiple similar issues below.


[scoping]: https://rust-lang.github.io/rfcs/1951-expand-impl-trait.html#scoping-for-type-and-lifetime-parameters

Lifetime parameters not in scope may still be indirectly named by one of the type parameters in scope.
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate further? I'm not sure if I understand this correctly. Does that mean we allow impl Sized in the implementation to capture 'a because it captures the type parameter Self in the trait definition?

trait Trait {
    fn foo(self) -> impl Sized;
}
impl<'a> Trait for &'a u8 {
    fn foo(self) -> impl Sized { self } // Is this ok?
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent is to be consistent with inherent methods, but the implementation is not currently. I believe this is captured in the unresolved question around rust-lang/rust#112194.

@tmandry
Copy link
Member Author

tmandry commented Jun 13, 2023

impl Trait in trait functions with default body is underspecified. Is it allowed? If yes, we should specify the desugaring. It is not trivial as it depends on the feature of associated type defaults. We should also mention the drawback of not being able to apply #[refine] to it.

Yes it is; I've updated the text to clarify that. I don't think there are any nonobvious interactions with associated type defaults, given that the types cannot be named in the first place.

Not being able to apply #[refine] is an existing drawback of that feature, not something specific to this RFC.

The text specifies the legal positions in trait definitions but leaves trait implementations unspecified. Do we allow this?

Elaborated what's allowed in the RFC. The short answer is no, but it might be an interesting future possibility.

Should we limit the legal positions for impl Trait to be maximally compatible with the naming schemes for return types?

My initial thought to this was no, but I think it's worth pondering. Added this as an unresolved question.

The decision here doesn't seem to be straightforward...

I think the decision is captured as an unresolved question around rust-lang/rust#112194. I updated the RFC text to point to your comment as well.

@nikomatsakis nikomatsakis merged commit 254fad9 into rust-lang:master Jun 13, 2023
@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

Tracking issue: rust-lang/rust#91611

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

@nikomatsakis
Copy link
Contributor

(@aliemjay -- my sense was that the specific points you raised were (a) important details to be resolved but not reasons not to accept the RFC and (b) either captured in unresolved questions or clarified; if you feel otherwise, please follow-up either on tracking issue or zulip)

@tmandry
Copy link
Member Author

tmandry commented Jun 14, 2023

Hm, it seems like the tracking issue is shared with async fn in traits. Should we make a separate tracking issue for RPITIT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.