Github try_trait_v2: A new design for the ? desugaring by scottmcm · Pull Reques...

Member

joshtriplett commented 5 days ago

This looks great.

Nominating for the next design meeting.

dureuill commented 5 days ago

Wow! I'm unsure if I'm qualified to comment on a RFC, but for the record, I believe this covers everything discussed here and then some.

This looks great!

Diggsey commented 5 days ago •

This seems like a good design, but I have a few concerns:

• The ? operator is already known as the Try operator, so in: let a: T = ...; a? the trait that T must implement should be the Try trait. Given that the RFC proposes using the Bubble trait for this, it should also propose to change the name of the operator (and consider the fallout that will cause).

• Holder means very little to me. Payload seems slightly better, but the design doesn't make it easy to choose good names. (just saw the note about bikeshedding, the prior point is not bikeshedding imo)

• The design as a whole seems confusing if you're not deeply familiar with the limitations of the Rust type system. For example, the Holder type will generally be defined with a ! parameter and exists mostly to workaround the lack of HKTs. It also serves two separate purposes:

  1. Carry the error value.
  2. Specify the "class" of preferred return types, ie. Option<!> is used as a proxy for Option<*>.

  To explain further, you could split the Holder type into two:

  trait Bubble {
      type Continue;
      type Break;
      type TypeClass: TypeClass<Self::Continue, Self::Break>;
      fn continue_with(c: Self::Continue) -> Self;
      fn branch(self) -> ControlFlow<Self::Break, Self::Continue>;
  }
  
  trait TypeClass<C, B> {
      type Output;
  }
  
  struct OptionClass;
  
  impl<T> TypeClass<T, ()> for OptionClass {
      type Output = Option<T>;
  }

  With this solution you don't have to rely on the ! type being optimised correctly.

  I'm not sure if this is better, but it's worth acknowledging that Holder is being overloaded here to achieve two things, and it could cause problems if those two ever result in conflicting requirements. For example, a type which is not generic over the "Continue" type cannot follow this pattern when defining its Holder.

• Don't RFCs normally include a "how do we document this" section?

• How confident are we that this is the simplest solution that achieves the stated goals?

scottmcm commented 5 days ago

Thanks for the reply! Splitting out the TypeClass marker like that is interesting.

Don't RFCs normally include a "how do we document this" section?

That section is no longer in the template: https://github.com/rust-lang/rfcs/blob/master/0000-template.md

I'm not sure if this is better, but it's worth acknowledging that Holder is being overloaded here to achieve two things, and it could cause problems if those two ever result in conflicting requirements. For example, a type which is not generic over the "Continue" type cannot follow this pattern when defining its Holder.

Yeah, I discuss holder types for types that aren't generic in Continue a bit in https://github.com/scottmcm/rfcs/blob/do-or-do-not/text/0000-try-trait-v2.md#why-a-holder-type-is-better-than-an-error-type

I think what pushed me to using the same type for both things is that a "residual" type (to use Niko's word) of some sort is needed for just about any type -- Result<T, E> could potentially just put an E in the Break, but just about everything else needs to pick something other than one of its generic parameters. Various conversations have talked about just using the Self type to avoid that, but I'm not a fan of that approach because it means handling a should-be-impossible variant in from_error/from_holder methods.

Suppose we wanted ? on Ordering (as it's a nice simple example, though we may well not choose to actually have it in std). Taking the definition that was proposed in a PR, Ordering::Equal should result in Continue(_), and Ordering::{Less, Greater} should result in Break(_). But what type should that payload be in the Break? I'm not a fan of having it be Ordering, as previously mentioned. The laziest one would be bool, since we only need two states. But then we'd end up with some form of Ordering: WhateverTheBikeshed<bool>, which seems unclear in what the semantics would be. I'd rather a newtype like struct OrderingHolder(bool); that doesn't expose its contents to the world or an additional enum like enum Unequal { Less = -1, Greater = 1 }. But once that exists, I don't know that also making a struct OrderingClass; as well is providing much value.

Even for Result, I don't know that I'd want a separate struct ResultClass;. I'd be awfully tempted to just do type TypeClass = Result<(), ()>; or something, even though that's not a ZST, because it's just a type-level marker so why not? (Or maybe Result<!, !>? Is it expected that one could have values of these types?)

mbartlett21 commented 5 days ago

This would be useful to have something like NonCoercingResult or something

Maybe I'm not qualified to speak on this, but I find this RFC really confusing. Perhaps it's because I'm a non-native english speaker, but I fail to follow its whole reasoning about the Holder type in the Guide-level explanation. I'm just not sure I understand how it can be used in a way that's not just another form of Result's Holder.

tmccombs commented 5 days ago

I am I native english speaker, and I do have to agree with @castroed47 a little. I did find the description for Holder in the guide level explanation a little confusing, and had to read through it multiple times. I also didn't understand the necessity of the BreakHolder trait at all after reading the Guide Level explanation. And didn't really get the point until reading the implementation of try_find. At the very least I think the purpose of that trait, and how it would be used should be better explained in the guide.

clarfonthey commented 4 days ago •

I'm also a native english speaker and I also had a really hard time trying (heh) to understand the way all of these traits were set up. In hindsight my explanation is super rambly but maybe it'll help clarify what I think are the biggest issues with this setup. Most of them aren't about the spirit of the implementation (which, after thinking about it, I am fine with), but rather the actual implementation itself and the naming.

Like, I don't actually have a big problem with the general idea of this, and especially I feel like the ControlFlow enum is a very good idea. However, the way the traits are set up almost feels a bit embarrassed about the implementation and they don't really have enough good conceptual cohesion to hold their own weight. As someone implementing the traits for their own types, I should have a strong grasp of what each of the traits is and what all the methods and associated types do, and right now, I really don't have that. It solves the problem of relying on Result at the cost of making the control flow harder to understand.

I know you didn't mention any bikeshedding and I don't think this should be considered as a suggestion of names, but I do want to point out how the flow is very confusing with the existing methods:

1. Bubble::branch is called when we enter the try flow to make a control flow decision.
2. Try::from_holder is called when we exit the flow due to a break.
3. Bubble::continue_with is called when we exit the flow due to a continue.

The names do not have any consistency at all, and don't represent at all what's happening during the flow. Although the implementation mentions control flow decisions, it doesn't at all mention what flow these decisions are controlling. The term "bubble" implies that they're part of a "bubble" flow but we've already decided to call the flow a try flow, so it seems to imply that we're talking about an entirely different flow even though we're not.

Now, let's look at the types:

1. Branching gives us a ControlFlow<B, C>. Seems okay so far.
2. Except we have a "holder" and a continue. Given the control flow struct, this should be called "break" and continue. It doesn't matter if the break is wrapped in some other type; it is a break.
3. Inside the flow, we use continues to continue the flow. That's fine.
4. Once we exit the flow due to a break, after going through multiple traits, we end up back at the original type that managed the control flow. That also seems fine.
5. Once we exit the flow after our final continue, we pass a continue to continue_with, which returns the type that managed the control flow. That also seems fine.
6. The entire flow, ultimately, would make sense as having a T -> T signature, right? Wrong. We have T -> <T::Holder as ops::BreakHolder<S>>::Output. That's because continues can vary throughout the entire flow, and we need some way of demonstrating this difference when we get to the end. To do this, we just generally assume that our "holder" is generic over multiple continues, and that each of these continues ultimately brings us back to a similar type with a different continue. (Note, the T -> T thing is obviously not the case when you consider the purpose of the original flow, but I'm choosing to just interpret it based upon the methods for now.)

Basically… the entire flow is designed to sidestep the fact that we don't have GATs. The whole purpose of the holder is to have a single type that represents what can happen in the control flow, and then get the associated type associated with the right continue to finish things off. After thinking about it, going away from GATs actually is a good idea (your ExitStatus example convinced me) and it is better to just have specific implementations on other traits narrow down what types of breaks and continues we can have, but really, we should at least arrange those other traits properly.

So… here's my general description of what we're trying to do with this implementation. I'm going to use Result as the example but it will apply in either case.

1. It has an "enter flow" method that returns ControlFlow<B, C>. Where B represents the "break" type and C represents the "continue" type.
2. It has an "exit with break" method that takes in B and returns Result<T, E>.
3. It has an "exit with continue" method that takes in C and returns Result<T, E>.

Now, there are a few caveats:

1. During the whole flow, T may change into S, and the flow should return S instead.
2. During the whole flow, E may change into F, but the flow should still return E.

Note that in both cases, we have a conversion, but the direction of the conversions is switched.

The way that this implementation solves the second problem is with a BreakHolder -- encapsulate the concept of Result<!, E> and allow us to convert Result<!, F> into Result<!, E>. However, we also use BreakHolder to encapsulate the first bit, which I think is a mistake: because Result<!, E> can be turned into both Result<T, E> or Result<S, E>, we can also add an associated type that will let us "convert" it back into a different continue.

Really, what we want is our "try" trait to have "exit with break" have a signature like B<F> -> Result<T, E> and "exit with continue" have a signature like C<S> -> Result<S, E>. I think that having BreakHolder be the one that makes the decision about continues and Try be the one that makes the decision about breaks to be extremely unintuitive. I'm not convinced we can't make it work more symmetrically.

This comment was marked as off-topic.

scottmcm commented 4 days ago

Reminder: Please hold off on bikesheds for the first week. Let's focus on getting the structure correct before worrying about names.

clarfonthey commented 4 days ago

(Not sure if what I said counts as bikeshedding; I mention names as a way of discussing issues with structure but tried not to strictly discuss how the trait should specifically be named.)

I think @clarfonthey's comment really clarifies thing a lot, at least for me. If I'm understanding this correctly, Bubble turns our type into ops::ControlFlow, and, in the case of a Break, our Holder type holds information about just the Break. The BreakHolder trait has then the job to put type information about the Continue case back into our Holder type, and the Try trait for a given type allows it to represent the end of a control flow, no matter if it ends with a continue or with a break, while a Bubble type can only represent an ongoing control flow and must be converted into a Try type at the end. Am I correct, close, totally off..?

QuineDot commented 4 days ago

Constructive criticism after my second or third pass.

The section "Avoiding interconversion with a custom Holder" is very hard to follow. Especially if you come back to it and aren't continuing from the previous section. This is a shame, because for me at least, it was a key section for understanding how everything ties together. Some specific points:

• The interconversion in question is never defined. The only mention of interconversion before this section is that it was some sort of oops in the current implementation. After the first paragraph of this section, there's 2-3 pages of "how to make a custom Holder", and then the last sentence is "As expected, the mixing in bar no longer compiles". Ahh, that must have been the point.
  • bar was introduced in the last section, not this section; I had to hunt it down
  • Naming it interconverting_example instead of bar would help
• Early on is a sentence, "Thus for us it'll depend only on U, never on T". This is the first mention of T and U in this section.
  • In retrospect I see they are short for Terrific and Unfortunate, but they also happen to be the most common non-descriptive generic trait parameter names in the ecosystem. Longer names would help.
• There's a sentence near the end, "With that we can still use ? in both directions as in foo previously."
  • I'm still not sure what this means, really. What directions? You have two versions of foo in the prior sections... are we talking about both? If not, which one?

The point of continue_with could be made more directly. It shows up in the unstable try_find example and the future possibilities for try{}. From the try {} example (another unstable feature), I now recognize it was implicit in the try_fold example as well. And after writing that out, I now actually wonder... is it even needed for this RFC?

Relatedly, changing the Continue type is mentioned in passing during the Guide-level explanation, but isn't really explored. I guess this is the discussion that needs to happen around scope, as you mention in the unresolved questions. Probably what an alternative without BreakHolder would look like should be sketched out, along with a rationale of why BreakHolder should be included. There seems to be a lot of machinery here motivated by unstable features and future possibilities.

ControlFlow is unstable, though it recently had a MCP. I'll definitely go read up on it myself, but it feels like that should be stabilized or at least formalized first? I don't yet have a feel for how far reaching that is, but wonder if it's another iceberg like scenario, with a small visible surface area here pulling a much larger unstable and future possibility mass behind it.

I'm still assimilating the technical aspects, but those are my initial thoughts on (mostly) the presentation. Hopefully they're useful. I'm looking forward to the possibility of custom Try types and early successful returns.

scottmcm commented 3 days ago •

I'm just not sure I understand how it can be used in a way that's not just another form of Result's Holder.

I also didn't understand the necessity of the BreakHolder trait at all after reading the Guide Level explanation. And didn't really get the point until reading the implementation of try_find.

The point of continue_with could be made more directly.

Relatedly, changing the Continue type is mentioned in passing during the Guide-level explanation, but isn't really explored.

Thanks, these are important gaps. I've pushed some new guide sections in the hopes that they'll help, although they're still somewhat sparse.

There's been a bunch of good pedagogical feedback. to things like clarifying what I mean by "interconversion" using names less bad than "foo", etc. I'll get to these, but possibly not for a few days yet.

One thing I'll note is that the subsections of the guide are definitely not independent. For example, everything about MyResult is based on the same type definition from the top of the section.

The entire flow, ultimately, would make sense as having a T -> T signature, right? Wrong. We have T -> <T::Holder as ops::BreakHolder<S>>::Output.

Note that there's also the possibility that it's T -> impl Try<T::Holder, Ok = S>, which doesn't need to be the same as the BreakHolder::Output type -- notably that's the case in any situation where one is using error-conversion in Result.

And after writing that out, I now actually wonder... is [continue_with] even needed for this RFC?

Yes, because it's essential to implementing try_fold. Imagine try_folding over an iter::empty(): there needs to be some way to wrap the argument accumulator into a Result/Option/etc.

Probably what an alternative without BreakHolder would look like should be sketched out

It can be pretty much just straight-up deleted, actually. That'd still work for everything but the try_find scenario. The trouble is that it can't be added later as a requirement on Bubble, so it needs to be at least somewhat considered before stabilizing that.

it feels like [ControlFlow] should be stabilized or at least formalized first?

The complication here is that it has a Try implementation, so if we ever want to move to this RFC's formulation of the traits, ControlFlow shouldn't stabilize before this RFC is in nightly as it would stabilize additional interconversions (using ControlFlow? in a -> Result method) that are undesirable.

Note that the MCP, while awesome, is just about changing the return type of a thing from bool to ControlFlow<Self::Break> -- the compiler could use its own internal definition of the type if it needs to. (In fact it had one before the change that moved the internal-only iter::LoopState to `ops::ControlFlow.)

Agree with the previous comments pointing out that the traits are overall confusing, even with the new non-generic section.

In particular, I felt confused about what the difference between Bubble and Try was, and why BreakHolder was needed.

And, it's hard to communicate, but I think I felt confused in a way additional explanations probably wouldn't help much. I don't know, I feel the traits need some structural changes to be easier to understand.

(the ControlFlow part made perfect sense to me though)

liigo commented 2 days ago •

In real world, bubbles only continue bubble on normal state, and stop bubbling when anyone break them. But in this RFC, bubbles start bubbling on ControlFlow::Break instead of ControlFlow::Continue, which makes great confusing.

impl<T, E> ops::Bubble for Result<T, E> {
    fn branch(self) -> ControlFlow<Self::Holder, T> {
        match self {
            Ok(c) => ControlFlow::Continue(c),
            Err(e) => ControlFlow::Break(Err(e)),
        }
    }
}

Edit: suggest: replace ControlFlow::{Continue,Break} with Wind::{Down,Up}.

scottmcm commented 2 days ago •

Status update: The feedback here, in the design meeting, and in the followup zulip conversation have convinced me that this needs substantial reworking. A few specifics:

• The guide is doing a bad job of giving people intuition for what's going on here.
• Try and Bubble are coming across as having a meaningful distinction other than their generic/nongeneric-ness, which I think is inducing confusion.
• The BreakHolder bound on Bubble::Holder seems to be unhelpful even in the cases that need to use the BreakHolder trait, so it should just be dropped. That'll help simplify the structure and mean that a useful guide example can be made without mentioning it at all.
• It can thus move the whole HKT discussion to "future possibilities". Hopefully that will help by focusing this on the flexibility and interconversion motivations, allowing a simpler guide, and communicate that only the BreakHolder type "is designed to sidestep the fact that we don't have GATs" -- the rest of the design makes sense on its own.

But none of this will happen today, so I've moved it to draft.