search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.35k stars 12.59k forks source link

Tracking issue for `impl Trait` (RFC 1522, RFC 1951, RFC 2071) #34511

Closed aturon closed 5 years ago

aturon commented 8 years ago

NEW TRACKING ISSUE = https://github.com/rust-lang/rust/issues/63066

Implementation status

The basic feature as specified in RFC 1522 is implemented, however there have been revisions that are still in need of work:

RFCs

There have been a number of RFCs regarding impl trait, all of which are tracked by this central tracking issue.

Unresolved questions

The implementation has raised a number of interesting questions as well:

alexreg commented 6 years ago

After this stabilization, it will be possible to use impl Trait in argument position and return position of non-trait functions. However, the use of impl Trait anywhere in Fn syntax is still disallowed in order to allow for future design iteration. Additionally, manually specifying the type parameters of functions which use impl Trait in argument position is not allowed.

What is the status of using impl Trait in argument/return positions in trait functions, or in Fn syntax, for that matter?

cramertj commented 6 years ago

@alexreg Return-position impl Trait in traits is blocked on an RFC, although RFC 2071 will allow similar functionality once implemented. Argument-position impl Trait in traits isn't blocked on any technical features that I'm aware of, but it wasn't explicitly permitted in the RFC so it has been omitted for the time being.

impl Trait in the argument position of Fn syntax is blocked on type-level HRTB, since some folks think that T: Fn(impl Trait) should desugar to T: for<X: Trait> Fn(X). impl Trait in the return position of Fn syntax isn't blocked for any technical reason that I'm aware of, but it was disallowed in the RFC pending further design work-- I would expect to see another RFC or at least a separate FCP before stabilizing this.

alexreg commented 6 years ago

@cramertj Okay, thanks for the update. Hopefully we can see these two features that aren't blocked on anything get the go-ahead soon, after some discussion. The desugaring makes sense, in the argument position, an argument foo: T where T: Trait is equivalent to foo: impl Trait, unless I'm mistaken.

kennytm commented 6 years ago

Concern: https://github.com/rust-lang/rust/issues/34511#issuecomment-322340401 is still the same. Is it possible to allow the following?

fn do_it_later_but_cannot() -> impl Iterator<Item=u8> { //~ ERROR E0227
    unimplemented!()
}
cramertj commented 6 years ago

@kennytm No, that's not possible at the moment. That function returns !, which does not implement the trait you provided, nor do we have a mechanism to convert it to an appropriate type. This is unfortunate, but there isn't an easy way to fix it right now (aside from implementing more traits for !). It's also backwards-compatible to fix in the future, as making it work would allow strictly more code to compile.

leoyvens commented 6 years ago

The turbofish question has only been halfway resolved. We should at least warn on impl Trait in arguments of effectively public functions, considering impl Trait in arguments to be a private type for the new private in public check.

The motivation is to prevent libs from breaking users' turbofishes by changing an argument from explicit generic to impl Trait. We don't yet have a good reference guide for libs to know what is and is not a breaking change and it's very unlikely that tests will catch this. This issue was not sufficiently discussed, if we wish to stabilize before fully deciding we should at least point the gun away from the foot of lib authors.

petrochenkov commented 6 years ago

The motivation is to prevent libs from breaking users' turbofishes by changing an argument from explicit generic to impl Trait.

I hope when this starts happening and people start complaining lang-team people who are in currently doubt will be convinced that impl Trait should support explicitly supplying type arguments with turbofish.

cramertj commented 6 years ago

@leodasvacas

The turbofish question has only been halfway resolved. We should at least warn on impl Trait in arguments of effectively public functions, considering impl Trait in arguments to be a private type for the new private in public check.

I disagree- this has been resolved. We are disallowing turbofish for these functions completely for the time being. Changing the signature of a public function to use impl Trait instead of explicit generic parameters is a breaking change.

If we do allow turbofish for these functions in the future, it will likely only allow specifying non-impl Trait type parameters.

rfcbot commented 6 years ago

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

nikomatsakis commented 6 years ago

I should add that I don't want to stabilize until https://github.com/rust-lang/rust/pull/49041 lands. (But hopefully that'll be soon.)

nikomatsakis commented 6 years ago

So #49041 contains a fix for #46541, but that fix has more impact than I anticipated -- e.g., the compiler doesn't bootstrap now -- and it's giving me a measure of pause about the right course here. The problem in #49041 is that we could accidentally allow lifetimes to leak out that we were not supposed to. Here is how this manifests in the compiler. We might have a method like this:

impl TyCtxt<'cx, 'gcx, 'tcx>
where 'gcx: 'tcx, 'tcx: 'cx
{
    fn foos(self) -> impl Iterator<Item = &'tcx Foo> + 'cx {
        /* returns some type `Baz<'cx, 'gcx, 'tcx>` that captures self */
    }
}

The key thing here is that TyCtxt is invariant w/r/t 'tcx and 'gcx, so they must appear in the return type. And yet only 'cx and 'tcx appear in the impl trait bounds, so only those two lifetimes are supposed to be "captured". The old compiler was accepting this because 'gcx: 'cx, but that's not really correct if you think about the desugaring we have in mind. That desugaring would create an abstract type like this:

abstract type Foos<'cx, 'tcx>: Iterator<Item = &'tcx Foo> + 'cx;

and yet the value for this abstract type would be Baz<'cx, 'gcx, 'tcx> -- but 'gcx is not in scope!

The workaround here is that we have to name 'gcx in the bounds. This is kind of annoying to do; we can't use 'cx + 'gcx. We can I suppose make a dummy trait:

trait Captures<'a> { }
impl<T: ?Sized> Captures<'a> for T { }

and then return something like this impl Iterator<Item = &'tcx Foo> + Captures<'gcx> + Captures<'cx>.

nikomatsakis commented 6 years ago

Something I forgot to note: if the declared return type were dyn Iterator<Item = &'tcx Foo> + 'cx, that would be ok, because dyn types are expected to erase lifetimes. Therefore i don't believe any unsoundness is possible here, presuming you can't do anything problematic with an impl Trait that would not be possible with a dyn Trait.

One could vaguely imagine the idea that the value of the abstract type is a similar existential: exists<'gcx> Baz<'cx, 'gcx, 'tcx>.

It seems to me however ok to stabilize a conservative subset (which rules out the fns above) and revisit this as a possible expansion later, once we've decided how we want to think about it.

UPDATE: To clarify my meaning about dyn traits: I am saying that they can "hide" a lifetime like 'gcx so long as the bound ('cx, here) ensures that 'gcx will still be live wherever the dyn Trait is used.

aturon commented 6 years ago

@nikomatsakis That is an interesting example, but I don't feel it changes the basic calculus here, i.e. that we want all relevant lifetimes to be clear from the return type alone.

The Captures trait seems like a good, lightweight approach for this situation. It seems like it could go into std::marker as unstable for the moment?

aturon commented 6 years ago

@nikomatsakis Your follow-up comment made me realize I hadn't quite put all the pieces together here in understanding why you might expect to elide the 'gcx in this case, i.e. 'gcx is not a "relevant lifetime" from the client's point of view. In any case, starting conservative seems fine.

cramertj commented 6 years ago

My personal opinion is that https://github.com/rust-lang/rust/issues/46541 isn't really a bug-- it's the behavior that I would expect, I don't see how it could be made unsound, and it's a pain to work around. IMO it should be possible to return a type which implements Trait and outlives lifetime 'a as impl Trait + 'a, no matter what other lifetimes it contains. However, I'm fine with stabilizing a more conservative approach to start if that's what @rust-lang/lang prefers.

nikomatsakis commented 6 years ago

(One other thing to clarify: the only time you will get errors with the fix in #49041 is when the hidden type is invariant with respect to the missing lifetime 'gcx, so this probably strikes relatively rarely.)

nikomatsakis commented 6 years ago

@cramertj

My personal opinion is that #46541 isn't really a bug-- it's the behavior that I would expect, I don't see how it could be made unsound, and it's a pain to work around.

I am sympathetic to that POV, but I'm reluctant to stabilize something for which we don't understand how to desugar it (e.g., because it seems to rely on some vague notion of existential lifetimes).

nikomatsakis commented 6 years ago

@rfcbot concern multiple-return-sites

I would like to register one final concern with existential impl trait. A substantial fraction of the times that I want to use impl trait, I actually want to return more than one type. For example:

fn foo(empty: bool) -> impl Iterator<Item = u32> {
    if empty { None.into_iter() } else { &[1, 2, 3].cloned() }
}

Of course, this doesn't work today, and it's definitely out of scope to make it work. However, the way that impl trait works now, we are effectively closing the door for it to ever work (with that syntax). This is because -- currently -- you can accumulate constraints from multiple return sites:

fn foo(empty: bool) -> (impl Debug, impl Debug) {
    if empty { return (22, Default::default()); }
    return (Default::default(), false);
}

Here, the inferred type is (i32, bool), where first return constrains the i32 part, and the second return constrains bool part.

This implies that we could never support cases where the two return statements do not unify (as in my first example) -- or else it would be very annoying to do so.

I wonder if we should put in a constraint that requires that each return (in general, each source of a constraint) must be independently fully specified? (And we unify them after the fact?)

This would make my second example illegal, and leave room for us to potentially support the first case at some point in the future.

nikomatsakis commented 6 years ago

@rfcbot resolve multiple-return-sites

So I talked to @cramertj on #rust-lang a fair bit. We were discussing the idea of making "early return" unstable for impl Trait, so that we might eventually change it.

They made the case that it would be better to have an explicit opt-in to this sort of syntax, specifically because there are other cases (e.g., let x: impl Trait = if { ... } else { ... }) where one would want it, and we can't expect to handle them all implicitly (definitely not).

I find this pretty persuasive. Prior to this, I was assuming we would have some opt-in syntax here anyway, but I just wanted to be sure we didn't close any doors prematurely. After all, explaining when you need to insert the "dynamic shim" is kind of tricky.

Enet4 commented 6 years ago

@nikomatsakis Just my possibly less-informed opinion: While enabling a function to return one of multiple types possible at run-time can be useful, I would be reluctant to have the same syntax for both static return type inferrence to a single type, and allowing situations where some run-time decision is required internally (what you just called the "dynamic shim").

That first foo example, as far as I understood the problem, could either resolve to (1) a boxed + type-erased Iterator<Item = u32>, or (2) a sum type of std::option::Iter or std::slice::Iter, which in turn would derive an Iterator implementation. Trying to keep it short, since there were some updates on the discussion (namely, I read the IRC logs now) and it's getting harder to pick up: I would certainly agree with a dyn-like syntax for the dynamic shim, although I also understand that calling it dyn might not be ideal.

Centril commented 6 years ago

Shameless plug and a small note just for the record: You can get "anonymous" sum types and products easily with:

cramertj commented 6 years ago

@Centril Yeah, that stuff from frunk is super cool. However, note that in order for CoprodInjector::inject to work, the resulting type has to be inferrable, which is usually impossible without naming the resulting type (e.g. -> Coprod!(A, B, C)). It's often the case that you're working w/ unnameable types, so you'd need -> Coprod!(impl Trait, impl Trait, impl Trait), which will fail inference because it doesn't know which variant should contain which impl Trait type.

Centril commented 6 years ago

@cramertj Very true (sidenote: each "variant" may not be entirely unnameable, but only partially, e.g: Map<Namable, Unnameable>).

kennytm commented 6 years ago

The enum impl Trait idea has been discussed before in https://internals.rust-lang.org/t/pre-rfc-anonymous-enums/5695

cramertj commented 6 years ago

@Centril Yeah, that's true. I'm specifically thinking of futures, where I often write things like

fn foo(x: Foo) -> impl Future<Item = (), Error = Never> {
    match x {
        Foo::Bar => do_request().and_then(|res| ...).left().left(),
        Foo::Baz => do_other_thing().and_then(|res| ...).left().right(),
        Foo::Boo => do_third_thing().and_then(|res| ...).right(),
    }
}
cramertj commented 6 years ago

@kennytm Yup, there's also https://github.com/rust-lang/rfcs/pull/1154 and https://github.com/rust-lang/rfcs/issues/294.

kennytm commented 6 years ago

@cramertj I wouldn't say anonymous enum is similar to enum impl Trait, because we cannot conclude X: Tr && Y: Tr ⇒ (X|Y): Tr (counter example: Default trait). So library authors will need to manually impl Future for (X|Y|Z|...).

cramertj commented 6 years ago

@kennytm Presumably we'd want to autogenerate some trait impls for anonymous enums, so it seems like basically the same feature.

kennytm commented 6 years ago

@cramertj Since an anonymous enum can be named (heh), if a Default impl is generated for (i32|String), we would be able to write <(i32|String)>::default(). OTOH <enum impl Default>::default() simply won't compile, so no matter what we auto-generate, it would still be safe since it can't be invoked at all.

Nevertheless, there are some cases where auto-generation can still cause problem with enum impl Trait. Consider

pub trait Rng {
    fn next_u32(&mut self) -> u32;
    fn gen<T: Rand>(&mut self) -> T where Self: Sized;
    fn gen_iter<'a, T: Rand>(&'a mut self) -> Generator<'a, T, Self> where Self: Sized;
}

It is perfectly normal that, if we've got an mut rng: (XorShiftRng|IsaacRng) we could compute rng.next_u32() or rng.gen::<u64>(). However, rng.gen_iter::<u16>() cannot be constructed because auto-generation can only produce (Generator<'a, u16, XorShiftRng>|Generator<'a, u16, IsaacRng>), while what we actually want is Generator<'a, u16, (XorShiftRng|IsaacRng)>.

(Maybe the compiler can automatically reject a delegation-unsafe call just like the Sized check.)

glaebhoerl commented 6 years ago

FWIW this feature strikes me as being closer in spirit to closures than to tuples (which are, of course, the anonymous struct counterpart to the hypothetical anonymous enums). The ways in which these things are "anonymous" is different.

For anonymous structs and enums (tuples and "disjoins"), the "anonymous" is in the sense of "structural" (as opposed to "nominal") types -- they're built-in, fully generic over their component types, and aren't a named declaration in any source file. But the programmer still writes them out and uses them like any other type, trait implementations for them are written down explicitly as usual, and they aren't particularly magical (apart from having built-in syntax and being 'variadic', which other types can't yet be). In some sense, they have a name, but instead of being alphanumeric, their 'name' is the syntax used to write them (parentheses and commas).

Closures, on the other hand, are anonymous in the sense that their name is secret. The compiler generates a fresh type with a fresh name each time you write one, and there is no way to find out what that name is or refer to it even if you wanted to. The compiler implements a trait or two for this secret type, and the only way you can interact with it is through these traits.

Being able to return different types from different branches of an if, behind an impl Trait, seems closer to the latter -- the compiler implicitly generates a type to hold the different branches, implements the requested trait on it to dispatch to the appropriate one, and the programmer never writes down or sees what that type is, and can't refer to it nor has any real reason to want to.

glaebhoerl commented 6 years ago

(In fact, this feature feels kind of related to the hypothetical "object literals" -- which would be for other traits what the existing closure syntax is for Fn. That is, instead of a single lambda expression, you'd implement each method of the given trait (with self being implicit) using the variables in scope, and the compiler would generate an anonymous type to hold the upvars, and implement the given trait for it, it'd have an optional move mode in the same way, and so on. Anyway, I suspect a different way of expressing if foo() { (some future) } else { (other future) } would be object Future { fn poll() { if foo() { (some future).poll() } else { (other future).poll() } } } (well, you'd also need to lift the result of foo() out into a let so it'd only be run once). That's rather less ergonomic and probably shouldn't be thought of as an actual alternative* to the other feature, but it suggests there's a relationship. Maybe the former could desugar into the latter, or something.)

Centril commented 6 years ago

@glaebhoerl that's a very interesting idea! There's also some prior art from Java here.

Some thoughts off the top of my head (so not very baked):

  1. [bikeshed] the prefix object suggests that this is a trait object rather than just existential -- but it isn't.

A possible alternative syntax:

impl Future { fn poll() { if foo() { a.poll() } else { b.poll() } } }
// ^ --
// this conflicts with inherent impls for types, so you have to delay
// things until you know whether `Future` is a type or a trait.
// This might be __very__ problematic.

// and perhaps (but probably not...):
dyn Future { fn poll() { if foo() { a.poll() } else { b.poll() } } }
  1. [macros/sugar] you can provide some trivial syntactic sugar so that you get: 
    future!(if foo() { a.poll() } else { b.poll() })
glaebhoerl commented 6 years ago

Yeah the syntax question is a mess because it's not clear whether you want to draw inspiration from struct literals, closures, or impl blocks :) I just picked one off the top of my head for example's sake. (Anyway, my main point was not that we should go and add object literals [though we should] but that I think anonymous enums are a red herring here [though we should add them too].)

nikomatsakis commented 6 years ago

Being able to return different types from different branches of an if, behind an impl Trait, seems closer to the latter -- the compiler implicitly generates a type to hold the different branches, implements the requested trait on it to dispatch to the appropriate one, and the programmer never writes down or sees what that type is, and can't refer to it nor has any real reason to want to.

Hmm. So I had assumed that rather than generate "fresh names" for enum types, we would instead leverage | types, corresponding to impls like this:

impl<A: IntoIterator, B: IntoIterator> IntoIterator for (A|B)  { /* dispatch appropriately */ }

Obviously there would be coherence issues with this, in the sense that multiple functions will generate identical impls. But even leaving those aside, I realize now that this idea may not work for other reasons -- e.g., if there are multiple associated types, in some contexts they might have to be the same, but in others be allowed to be different. For example maybe we return:

-> impl IntoIterator<Item = Y>

but somewhere else we do

-> impl IntoIterator<IntoIter = X, Item = Y>

These would be two overlapping impls I guess that can't be "coallesced"; well, maybe with specialization.

Anyway, the notion of "secret enums" seems cleaner all around I suppose.

stuhood commented 6 years ago

I would like to register one final concern with existential impl trait. A substantial fraction of the times that I want to use impl trait, I actually want to return more than one type.

@nikomatsakis: Is it fair to say that in this case what is being returned is closer to a dyn Trait than to impl Trait, because the synthetic/anonymous return value implements something akin to dynamic dispatch?

cramertj commented 6 years ago

cc https://github.com/rust-lang/rust/issues/49288, an issue I've been hitting a lot lately working with Futures and Future-returning trait methods.

daboross commented 6 years ago

Since this is the last chance before FCP closes, I'd like to make one last argument against automatic auto traits. I realize this is a bit last minute, so at most I'd like to formally address this issue before we commit to the current implementation.

To clarify for anyone who hasn't been following impl Trait, this is the issue I'm presenting. A type represented by impl X types currently automatically implement auto traits if and only if the concrete type behind them implements said auto traits. Concretely, if the following code change is made, the function will continue to compile, but any usages of the function relying on the fact that the type it returns implements Send will fail.

 fn does_some_operation() -> impl Future<Item=(), Error=()> {
-    let data_stored = Arc::new("hello");
+    let data_stored = Rc::new("hello");

     return some_long_operation.and_then(|other_stuff| {
         do_other_calculation_with(data_stored)
     });
}

(simpler example: working, internal changes causes failure)

This issue is not clear cut. There was a very delibrate decision to have auto traits "leak": if we didn't, we'd have to put + !Send + !Sync on every function which returns something non-Send or non-Sync, and we'd have an unclear story with potential other custom auto-traits which could simply not be implementable on the concrete type the function is returning. These are two problems I will touch on later.

First, I'd like to simply state my objection to the issue: this allows changing a function body to change the public facing API. This directly reduces the maintainability of code.

Throughout the development of rust, decisions have been made which err on the side of verbosity over usability. When newcomers see these, they think it's verbosity for the sake of verbosity, but this is not the case. Each decision, whether it's to not have structures automatically implement Copy, or to have all types explicit at function signatures, is for the sake of maintainability.

When I introduce people to Rust, sure, I can show them speed, productivity, memory safety. But go has speed. Ada has memory safety. Python has productivity. What Rust has trumps all of these, it has maintainability. When a library author wants to change an algorithm to be more efficient, or when they want to redo the structure of a crate, they have a strong guarantee from the compiler that it will tell them when they make mistakes. In rust, I can be assured that my code will continue to function not just in terms of memory safety, but logic and interface as well. Every function interface in Rust is fully representable by the function's type declaration.

Stabilizing impl Trait as is has a large chance of going against this belief. Sure, it's extremely nice for the sake of writing code quickly, but if I want to prototype I'll use python. Rust is the language of choice when one needs long-term maintainability, not short-term write-only code.

I say there's only a "large chance" of this being bad here because again, the issue isn't clear cut. The whole idea of 'auto traits' in the first place is non-explicit. Send and Sync are implemented based on what the contents of a struct, not the public declaration. Since this decision worked out for rust, impl Trait acting similarly could also work out well.

However, functions and structures are used differently in a codebase, and these aren't the same issues.

When modifying the fields of a structure, even private fields, it's immediately clear one's changing the real contents of it. Structures with non-Send or non-Sync fields made that choice, and library maintainers know to double check when a PR changes a structure's fields.

When modifying the internals of a function, it's definitely clear one can affect both performance and correctness. However, in Rust, we don't need to check that we're returning the right type. Function declarations are a hard contract we must uphold, and rustc watches our back. It's a thin line between auto traits on structs and in function returns, but changing the internals of a function is much more routine. Once we have full generator-powered Futures, it will be even more routine to modify functions returning -> impl Future. These will all be changes authors need to screen for modified Send/Sync implementations if the compiler doesn't catch it.

To resolve this, we could decide that this is an acceptable maintenance burden, as the original RFC discussion did. This section in the conservative impl trait RFC lays out the biggest arguments for leaking auto traits ("OIBIT"s being the old name for auto traits).

I've already laid out my main response to this, but here's one last note. Changing a structure's layout is not that common to do; it can be guarded against. The maintenance burden in ensuring functions continue to implement the same auto traits is greater than that of structures simply because functions change a lot more.

As a final note, I'd like to say that automatic auto traits is not the only option. It's the option we chose, but the alternative of opt-out auto traits is still an alternative.

We could require functions returning non-Send / non-Sync items to either state + !Send + !Sync or to return a trait (alias possibly?) which has those bounds. This wouldn't be a good decision, but it might be better than the one we're currently choosing.

As for the concern regarding custom auto traits, I would argue that any new auto traits should be not-implemented only for new types introduced after the auto trait. This might provide more of an issue than I can address now, but it's not one we can't address with more design.

This is very late, and very long winded, and I'm certain I've raised these objections before. I'm glad just to be able to comment one last time, and ensure we're fully OK with the decision we're making.

Thank you for reading, and I hope that the final decision sets Rust in the best direction it can go in.

Centril commented 6 years ago

Expanding on @daboross's review wrt. trait aliases, one could improve ergonomics of non-leaking auto traits like so:

trait FutureNSS<T, E> = Future<Item = T, Error= E> + !Send + !Sync;

fn does_some_operation() -> impl FutureNSS<(), ()> {
     let data_stored = Rc::new("hello");
     some_long_operation.and_then(|other_stuff| {
         do_other_calculation_with(data_stored)
     });
}

This is not so bad -- you'd have to come up with a good name (which FutureNSS is not). The main benefit is that it reduces the paper cut incurred by repetition of the bounds.

Robbepop commented 6 years ago

Wouldn't it be possible to stabilize this feature with the requirements to state auto-traits explicitely and later maybe remove those requirements once we found a suitable solution to that maintenance problem or once we are certain enough that there are in fact no maintenance burdens by the decision to lift the requirements?

vi commented 6 years ago

What about requiring Send unless it is marked as !Send, but not providing Sync unless marked as Sync? Isn't Send supposed to be more common compared to Sync?

Like this:

fn provides_send_only1() -> impl Trait {  compatible_with_Send_and_Sync }
fn provides_send_only2() -> impl Trait {  compatible_with_Send_only }
fn fails_to_complile1() -> impl Trait {  not_compatible_with_Send }
fn provides_nothing1() -> !Send + impl Trait { compatible_with_Send}
fn provides_nothing2() -> !Send + impl Trait { not_compatible_with_Send }
fn provides_send_and_sync() -> Sync + impl Trait {  compatible_with_Send_and_Sync }
fn fails_to_compile2() -> Sync + impl Trait { compatible_with_Send_only }
Centril commented 6 years ago

Is there an inconsistency between impl Trait in argument position and return position wrt. auto traits?

fn foo(x: impl ImportantTrait) {
    // Can't use Send cause we have not required it...
}

This makes sense for argument position cause if you were allowed to assume Send here, you would get post monomorphization errors. Of course, the rules for return position and argument position are not required to coincide here, but it presents a problem in terms of learnability.

ExpHP commented 6 years ago

As for the concern regarding custom auto traits, I would argue that any new auto traits should be not-implemented only for new types introduced after the auto trait.

Well, this is true for the upcoming auto trait Unpin (only not-implmented for self-referential generators), but... that seems to be dumb luck? Is this a limitation we can really live with? I can't believe that there won't be something in the future that would need to be disabled for e.g. &mut or Rc...

rpjohnst commented 6 years ago

I believe this has been discussed, and this is of course very late, but I'm still unsatisfied with impl Trait in argument position.

The abilities to both a) work with closures/futures by value, and b) treat some types as "outputs" and thus implementation details, are idiomatic and have been since before 1.0, because they directly support Rust's core values of performance, stability, and safety.

-> impl Trait is thus merely fulfilling a promise made by 1.0, or removing an edge case, or generalizing existing features: it adds output types to functions, taking the same mechanism that has always been used to handle anonymous types and applying it in more cases. It may have been more principled to start with abstract type, i.e. output types for modules, but given that Rust doesn't have an ML module system the order isn't a big deal.

fn f(t: impl Trait) instead feels like it was added "just because we can," making the language bigger and stranger without giving enough back in return. I've struggled and failed to find some existing framework to fit it into. I understand the argument around the conciseness of fn f(f: impl Fn(...) -> ...), and the justification that bounds can already be in both <T: Trait> and where clauses, but those feel hollow. They don't negate the downsides:

So what it comes down to for me is that impl Trait in argument position adds edge cases, uses more of the strangeness budget, makes the language feel bigger, etc. while impl Trait in return position unifies, simplifies, and makes the language hang together more tightly.

vorner commented 6 years ago

What about requiring Send unless it is marked as !Send, but not providing Sync unless marked as Sync? Isn't Send supposed to be more common compared to Sync?

That feels very… arbitrary and ad-hoc. Maybe it's less typing, but more remembering and more chaos.

rpjohnst commented 6 years ago

Bike shed-y idea here so as not to distract from my points above: instead of impl, use type? That's the keyword used for associated types, it's likely (one of) the keyword(s) used for abstract type, it's still fairly natural, and it hints more at the idea of "output types for functions":

// keeping the same basic structure, just replacing the keyword:
fn f() -> type Trait

// trying to lean further into the concept:
fn f() -> type R: Trait
fn f() -> type R where R: Trait
fn f() -> (i32, type R) where R: Trait
// or perhaps:
fn f() -> type R: Trait in R
// or maybe just:
fn f() -> type: Trait
nikomatsakis commented 6 years ago

Thank you for reading, and I hope that the final decision sets Rust in the best direction it can go in.

I appreciate the well authored objection. As you pointed out, auto traits have always been a deliberate choice to "expose" some implementation details that one might have expected to remain hidden. I think that -- thus far -- that choice has actually worked out quite well, but I confess that I am constantly nervous about it.

It seems to me that the important question is the extent to which functions really are different from structs:

Changing a structure's layout is not that common to do; it can be guarded against. The maintenance burden in ensuring functions continue to implement the same auto traits is greater than that of structures simply because functions change a lot more.

It's really hard to know how true this will be. It seems like the general rule is going to be that introducing Rc is something to be done with caution -- it's not so much a question of where you store it. (Actually, the case I really work about is not Rc but rather introducing dyn Trait, since that can be less obvious.)

I strongly suspect that in code that is returning futures, working with non-thread-safe types and so forth will be rare. You will tend to avoid those sorts of libraries. (Also, of course, it always pays to have tests exercising your code in realistic scenarios.)

In any case, this is frustrating because it is the sort of thing that is hard to know in advance, no matter how long of a stabilization period we give it.

As a final note, I'd like to say that automatic auto traits is not the only option. It's the option we chose, but the alternative of opt-out auto traits is still an alternative.

True, though I definitely feel nervous about the idea of "singling out" specific auto traits like Send. It's also keeping in mind that there are other use cases for impl trait besides futures. For example, returning iterators or closures -- and in those cases, it's not obvious that you would want send or sync by default. In any case, what you would really want, and what we are trying to defer =), is a kind of "conditional" bound (Send if T is Send). This is precisely what auto traits give you.

nikomatsakis commented 6 years ago

@rpjohnst

I believe this has been discussed

Indeed, it has :) ever since the first impl Trait RFC lo these many years ago. (Woah, 2014. I feel old.)

I've struggled and failed to find some existing framework to fit it into.

I feel quite the opposite. To me, without impl Trait in argument position, impl Trait in return position stands out all the more. The unifying thread that I see is:

There are also plans to expand the set of places where impl Trait can appear, building on that intuition. For example, https://github.com/rust-lang/rfcs/pull/2071 permits

let x: impl Trait = ...;

The same principle applies: the choice of type is known statically. Similarly, the same RFC introduces abstract type (for which impl Trait can be understood as a kind of syntacti sugar), which can appear in trait impls and even as members in modules.

Bike shed-y idea here so as not to distract from my points above: instead of impl, use type?

Personally, I'm not inclined to reinstigate a bikeshed here. We spent quite some time discussing syntax in https://github.com/rust-lang/rfcs/pull/2071 and elsewhere. There doesn't seem to be a "perfect keyword", but reading impl as "some type that implements" works pretty well imo.

nikomatsakis commented 6 years ago

Let me add a bit more about auto trait leakage:

First off, ultimately I think that auto trait leakage is actually the right thing to do here, precisely because it is consistent with the rest of the language. Auto traits were -- as I said earlier -- always a gamble, but they seem to have been one that has basically paid off. I just don't see impl Trait being so very different.

But also, I am pretty nervous about delaying here. I agree that there are other interesting points in the design space and I am not 100% confident we have hit the right spot, but I don't know that we will ever be sure of that. I am pretty worried if we delay now we will have a hard time delivering on our roadmap for the year.

Finally, let's consider the implications if I am wrong: What we are basically talking about here is that semver becomes even more subtle to judge. This is a concern, I think, but one that can be mitigated in various ways. For example, we can use lints that warn when !Send or !Sync types are introduced. We have long talked about introducing a semver checker that helps you to prevent accidental semver violations -- this seems like another case where that would help. In short, a problem, but not I think a critical one.

So -- at least as of this moment -- I still feel inclined to continue the current path.

rpjohnst commented 6 years ago

Personally, I'm not inclined to reinstigate a bikeshed here.

I'm not terribly invested in it either; it was an afterthought based on my impression that impl Trait in argument position seem to be motivated by "filling in holes" syntactically rather than semantically, which seems to be correct given your response. :)

To me, without impl Trait in argument position, impl Trait in return position stands out all the more.

Given the analogy to associated types, this comes across very much like "without type T in argument position, associated types stand out all the more." I suspect that particular objection hasn't come up because the syntax we've chosen makes it feel nonsensical- the existing syntax there is good enough that nobody feels the need for syntactic sugar like trait Trait<type SomeAssociatedType>.

We already have syntax for "some monomorphized type that implements Trait." In the case of traits, we have both "caller" and "callee"-specified variants. In the case of functions, we only have the caller-specified variant, so we need the new syntax for the callee-specified variant.

Expanding this new syntax to local variables might be justified, because that is also very much an associated-type-like situation- it's a way to hide+name an expression's output type, and is useful for forwarding callee functions' output types.

Like I mentioned in my previous comment, I am also a fan of abstract type. It is, again, simply an expansion of the "output type" concept to modules. And applying -> impl Trait, let x: impl Trait, and abstract type's use of inference to trait impls' associated types is also great.

It's specifically the concept of adding this new syntax for function arguments that I dislike. It doesn't do the same thing as any of the other features it's being pulled in with. It does do the same thing as the syntax we already have, just with more edge cases and less applicability. :/

Centril commented 6 years ago

@nikomatsakis

It's really hard to know how true this will be.

It seems to me that we should err on the side of being conservative then? Can we gain more confidence in the design with more time (by letting auto trait leakage be under a separate feature gate and only in nightly while we stabilize the rest of impl Trait)? We can always add support for auto trait leakage later on if we don't leak now..

But also, I am pretty nervous about delaying here. [..] I am pretty worried if we delay now we will have a hard time delivering on our roadmap for the year.

Understandable! However, and as I'm sure you have considered, the decisions here will live with us for many years to come.

For example, we can use lints that warn when !Send or !Sync types are introduced. We have long talked about introducing a semver checker that helps you to prevent accidental semver violations -- this seems like another case where that would help. In short, a problem, but not I think a critical one.

This is good to hear! 🎉 And I think this mostly mollifies my concerns.

True, though I definitely feel nervous about the idea of "singling out" specific auto traits like Send.

I very much agree with this sentiment 👍.

In any case, what you would really want, and what we are trying to defer =), is a kind of "conditional" bound (Send if T is Send). This is precisely what auto traits give you.

I feel as tho T: Send => Foo<T>: Send would be better understood if the code explicitly stated this.

fn foo<T: Extra, trait Extra = Send>(x: T) -> impl Bar + Extra {..}

Tho, as we discussed in WG-Traits, you might not get inference at all here, so you always have to specify Extra if you want something other than Send, which would be a total bummer.

@rpjohnst

The analogy with dyn Trait is, honestly, a false one:

With respect to impl Trait in argument position it is false, but not so with -> impl Trait as both are existential types.

  • Now what should be a function's implementation detail (how it declares its type parameters) becomes part of its interface, because you can't write its type!

I'd like to note that the order of type parameters has never been an implementation detail due to turbofish, and in this respect, I think that impl Trait can help since it allows you to leave certain type arguments unspecified in turbofish.

[..] the existing syntax there is good enough that nobody feels the need for syntactic sugar like trait Trait.

Never say never? https://github.com/rust-lang/rfcs/issues/2274