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 #3193

Closed
wants to merge 3 commits into from

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Nov 10, 2021

This RFC describes a minimal version of return position impl trait in traits. It is a building block for async function support.

It was developed as part of the impl trait initiative.

Rendered

@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the RFC. label Nov 10, 2021
@Gankra
Copy link
Contributor

Gankra commented Nov 10, 2021

From an API-design perspective, the example of IntoIterator is quite significant! (To be clear, I know you're not proposing replacing IntoIterator, but I am considering how NewIntoIterator limits the usefulness of IntoIterator, which generalizes to future consumers of this feature.)

Although it is only required that type IntoIter: Iterator we have a zoo of very important traits which refine Iterator (DoubleEndedIterator, ExactSizeIterator). So using impl Iterator instead would prevent e.g. vec.into_iter().rev() from working.

In addition, many iterators expose extra inherent methods like std::vec::IntoIter::as_slice() which are niche but extremely useful when they come up.

Both of these issues can be worked around in the concrete case by every implementer of NewIntoIterator (that wants these benefits) providing an inherent into_iter method that shadows the trait version in concrete contexts. This is not an uncommon pattern (especially for types where the trait method is "core functionality" that they want to be in scope even when the trait is not), but it is annoying. By default folks will not do this extra work, limiting the usefulness of all the machinery built around DoubleEndedIterator and ExactSizeIterator.

Now of course all of these issues exist from the original impl Trait feature, and I am an avid user of -> impl Iterator! But -> impl Iterator I regard as more for... "whatever" iterators. It's a slam dunk for internal APIs where I know the Iterator Zoo doesn' t matter. I do also use it for public APIs though, for iterators where I wouldn't put in the effort to forward all the Iterator Zoo methods on my newtype wrapper anyway. So at that point the only real regression is being able to name the type.

God -> impl Trait is a great feature. 😻

So: does shifting impl Trait from something that can be used at your discretion to something mandated by the Trait itself significantly change the calculus? A little bit! It breaks the ability to write generic Iterator code that requires Iterator Zoo refinements. For instance, I believe it would become impossible to write:

fn is_palindrome<Iter, T>(iterable: Iter) -> bool
  where Iter: NewIntoIterator<Item=T>,
   Iter::IntoIter: DoubleEndedIterator,
   T: Eq;

Instead, you would have to avoid accepting anything that was IntoIter and instead directly accept a DoubleEndedIterator. From a usability perspective this means requiring users to run is_palindrome(slice.into_iter()) vs the usual is_palindrome(slice).

A minor papercut perhaps, but it defeats the entire purpose of having an IntoIterator trait if it can't be used generically! You would need to introduce IntoDoubleEndedIterator, IntoExactSizeIterator, etc, and force everyone to implement all the applicable ones.

Now of course, the Iterator traits are basically the most extreme usage of traits you can find. They're part of the language (insofar as for implicitly invokes it). There's tons of implementations (and you wouldn't bat an eye at a random crate implementing it!). It's performance sensitive. It has a Zoo of refining traits. It has a million subtle little power-user features.

There are plenty of traits where these restrictions are basically fine. Is that the case for the async traits this feature is intended for? I would love to have more motivating details included so we know we're actually solving the problem we intend to.

text/0000-return-position-impl-trait-in-traits.md Outdated Show resolved Hide resolved
text/0000-return-position-impl-trait-in-traits.md Outdated Show resolved Hide resolved
}
```

## Impl trait must be used in both trait 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.

is it required that the impl be exactly the same as the trait?

impl NewIntoIterator for Vec<u32> {
    type Item = u32;
    fn into_iter(self) -> impl Iterator<Item = u32> + 'static {
//                                                  ~~~~~~~~~ ok or not?
        self.into_iter()
    }
}

Choose a reason for hiding this comment

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

If you write the "desugered" version today, you can specify stricter requirements than the trait.

#![feature(type_alias_impl_trait)]

trait MyIntoIterator {
    type Item;
    type Iterator: Iterator<Item = Self::Item>;

    fn my_into_iter(self) -> Self::Iterator;
}

impl<T> MyIntoIterator for Vec<T> {
    type Item = T;
    type Iterator = impl DoubleEndedIterator<Item = T> + ExactSizeIterator;

    fn my_into_iter(self) -> Self::Iterator {
        IntoIterator::into_iter(self)
    }
}

@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 14, 2021

LGTM, although I have the same impression as @Gankra that due to limited usability this will be used for private stuff, or for public but unstable interface boundaries (e.g. everything in rustc_* crates), and stable public interfaces will use the desugared form with explicit associated types anyway.
But private and unstable interfaces actually represent a lot of code, so the sugar will still be useful.

@nrc
Copy link
Member

nrc commented Nov 15, 2021

How does this relate to RFC 2071? Part of the motivation for that RFC was also impl Trait in trait return types. Do we still intend to keep existential types or will they be removed? If kept, will there be any overlap with the anonymous types proposed here?

Never mind, I see 2071 has mutated into TAITs, which are what are used for the desugaring in this RFC.

@tmandry
Copy link
Member

tmandry commented Nov 15, 2021

@Gankra all good points, let me attempt to address some of them here..

Why use return impl Trait?

So there are basically three reasons that someone will want to use an "output" impl Trait (that I know of):

  1. Convenience – no need to write out the full type (or make an associated type, in the case of traits)
  2. Opaqueness – we want to hide details of the underlying type so outside code doesn't depend on them
  3. Nameability – it gives us the ability to use types that we can't ordinarily name (the async fn use case)

Nameability is in some sense a special case of Opaqueness: Because we can't name a type, we can only talk about it in terms of what the type can do. The type's identity is already hidden from us, so we can only express it in a form that also hides it from outside code.

Mitigating the downsides of return impl Trait

These are all good reasons, the problem is they can come with unwanted constraints:

  1. You can't migrate away from return impl Trait once you've chosen it without breaking semver
  2. Opaqueness isn't always what you want; it limits what downstream code can do

Smoothing the convenience off-ramp

IMO we should definitely pursue proposals which make this possible (like the inference proposal mentioned in the RFC).

Leaky existentials and you

A caveat about Opaqueness is that we are not hiding all details of the type; namely, we allow auto traits of the underlying type (like Send and Sync) to leak through the existential veneer. It is conceivable that we would want to expand this to other traits if an impl opts in to it.

For example, we could allow you to write an impl like this:

impl NewIntoIterator for Range<u32> {
    type Item = u32;
    fn into_iter(self) -> impl Iterator<Item = Self::Item> + DoubleEndedIterator { ... }
}

when the trait definition only guarantees impl Iterator<Item = Self::Item>. This doesn't seem fundamentally any harder than auto trait leakage; both require knowing the impl statically. The implementer has to opt in, but it can be added later without breakage (and this is no different than other uses of impl Trait; it's inherent in the Opaqueness utility).

@nikomatsakis
Copy link
Contributor Author

I pushed up some edits which I think describe @Gankra's comment, and I wanted to raise what is in my mind the one open question right now:

Should we require the impl to match the trait exactly? What if we allowed it to specify a precise type, or additional bounds?

The RFC requires the impl to use impl Trait, but is that truly necessary? What if we permitted any sort of type? Then you might have an impl like the following, for example:

trait GimmeIterator {
    fn gimme_iter(self) -> impl Iterator<Item = u32>;
}

struct MyIterator { .. }

impl GimmeIterator for MyType {
    fn gimme_iter(self) -> MyIterator {
        MyIterator { ... }
    }
}

As a result, clients of <MyType as GimmeIterator>::gimme_iter would be able to resolve the return type to MyIterator precisely.

Why?

  • Fits the general direction we've discussed, where the impl is permitted to be more specific than the trait.
    • This potentially fits quite well with specialization, which permits one to add intermediate impls that can tighten guarantees or loosen requirements for all things that specialize them.
      • e.g., default fn gimme_iter(self) -> impl Iterator + DoubleEndedIterator
    • Effectively lets you model OO-like hierarchies.
  • Permits people to give names to the return type, especially where those names already exist.
    • This in turn allows e.g. inherent methods to be added to the resulting type.
  • Could also use this to write e.g. -> impl Iterator + DoubleEndedIterator in the impl, so as to give additional guarantees.
  • These semantics just "fall out" from the desugaring, so trying to not make these guarantees will require us to modify the trait system to permit "further opaqueness".

Why not?

  • Changing the return type of the impl then becomes a breaking change.
    • Same as any other function, of course.

@Gankra
Copy link
Contributor

Gankra commented Nov 16, 2021

Ooh yes, if you could still name a concrete type that would make the concrete case totally equivalent! You still wouldn't be able to use the refinements generically (still can't write is_palindrome), but vec.into_iter().rev() and vec.into_iter().as_slice() would work fine.

Being able to name -> impl Iterator + DoubledEndedIterator would also be a really nice convenience, although just stating the perhaps-obvious for the sake of stating it: this cannot express conditional conformance (e.g. Map<I>: DoubleEndedIterator where I: DoubleEndedIterator). So if you are returning a wrapper type that conditionally implements traits based on its contents, you'll still need to name the type to expose those conditional conformances to concrete code.

So actually in the concrete -> impl traits would be strictly more flexible, I think? In that implementors of the trait can return anonymous types, but you don't have to deal with that possibility unless the specific implementation you're using takes advantage of that capability.

Although I guess you lose the minor convenience of being able to use the MyType::IntoIter alias and have to actually find what the concrete type is called and what module path it can be found on.

@CAD97
Copy link

CAD97 commented Nov 18, 2021

I definitely agree that trait impls should be able to overconstrain and provide more than the trait requires. (Similar to the accepted RFC that allows implementing an unsafe fn with a safe fn.)

Although I guess you lose the minor convenience of being able to use the MyType::IntoIter alias

I think at some later point we're going to need to solve the "name the output type of this function" and that will resolve both this and the ability for a generic function to constrain the return type further.

This RFC does not need to and shouldn't discuss such functionality, though, imo. Even without the ability to do so RPIT in traits is extremely valuable. The future feature (if it comes) will then just make it even more functional.

Comment on lines +269 to +270
# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives
Copy link
Contributor

Choose a reason for hiding this comment

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

Why -> impl Trait before type Foo = impl Trait?

From a users perspective, it sounds like an impl associated type covers a superset of use cases compared to impl in return position. I am not familiar with the language design or compiler implementation concerns but it sounds like an impl associated type needs to be implemented along the way to impl in return position.

Copy link
Member

Choose a reason for hiding this comment

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

type Foo = impl Trait is already implemented under RFC 2515.

* Introducing a name by converting to camel-case feels surprising and inelegant.
* Return position impl Trait in other kinds of functions doesn't introduce any sort of name for the return type, so it is not analogous.

There is a need to introduce a mechanism for naming the return type for functions that use `-> impl Trait`; we plan to introduce a second RFC addressing this need uniformly across all kinds of functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: unsure what all is being discussed in the lead up to this RFC but something I've been playing with in my mind is functions also being types. This would allow Trait::func::ReturnType.

No idea what has been discussed for a desugared version of -> dyn Trait but the above idea came up as part my "how would I implement it" brainstorming. a -> dyn Trait would desugar to Trait::func::alloc_requirements (size, alignment) and Trait::func::func_into (wrapper around the user's function, moving into the provided memory)

@nikomatsakis
Copy link
Contributor Author

I'm going to close this RFC -- we've been doing some thinking and I plan to open a revised version that's a bit more ambitious at laying out some of the overall plans.

@tmandry
Copy link
Member

tmandry commented Mar 23, 2022

I've posted an RFC to address the specific question that came up in this thread about whether impls need to match the trait.

@huntc
Copy link

huntc commented Jun 6, 2022

I'm going to close this RFC -- we've been doing some thinking and I plan to open a revised version that's a bit more ambitious at laying out some of the overall plans.

Thanks for everything here. Was a revised RFC started? I’ve spent some time reading through this RFC issue because I wanted to understand why I couldn’t use impl Stream as a return type to my trait methods, but could do from a struct method. Avoiding an allocation when returning Stream and Future from trait methods would be great; particularly where I don’t have an allocator such as with nostd. :-)

Not supporting impl for return types within traits is surprising, particularly given that I can subsequently return a pinned-boxed return value from a function that returns the impl.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=78911e4881308eeffa9af54a20ac8097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.