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

Resolving Ok-wrapping for try blocks #70941

Closed
joshtriplett opened this issue Apr 9, 2020 · 26 comments
Closed

Resolving Ok-wrapping for try blocks #70941

joshtriplett opened this issue Apr 9, 2020 · 26 comments
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-try_blocks `#![feature(try_blocks)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Apr 9, 2020

Please read this issue all the way through before commenting; this issue carefully pulls out one particular bit of a previously contentious topic for evaluation in isolation.

There's been a long-standing dispute that blocks the stabilization of try blocks: whether try should wrap its final value in Ok or not, referred to by the shorthand of "Ok-wrapping".

There's been extensive discussion on both approaches. Within the language team, that discussion has remained unresolved for several years, and people have been somewhat hesitant to reopen that discussion. When last we left it, there were a couple of proponents of Ok-wrapping, several people who didn't seem to have a strong opinion one way or another, and I was the last remaining holdout arguing that try should not do Ok-wrapping.

However, two days ago, I read a comment which got me thinking about Ok-wrapping again; in particular, the elegant observation that it makes try and ? symmetric, such that try { x }? == x and try { x? } == x. I also thought about much of the Rust code I've written recently, including its error handling. This got me to do some introspection, which led to the realization that I don't actually object to Ok-wrapping specifically, and in fact I think Ok-wrapping is the right answer.

What I realized I actually object to is some of the proposed syntax for function-level try, which hides the Result<T, E> type behind something like T throws E. (I am not going to go into any of my objections to such syntax, as it's off-topic for this issue.) I'd so commonly seen that and Ok-wrapping presented together as a unit that it hadn't occurred to me to separate the two and evaluate them independently. Once I did, I realized I didn't actually have any objection to Ok-wrapping.

And, as it turns out, we can separate those two, because try blocks don't depend on any syntax for function-level try. We can evaluate and stabilize try blocks independently, and the only disagreement about that was over Ok-wrapping.

So, with that in mind, I'm filing this issue to separate out and gauge language-team consensus on the specific question of Ok-wrapping in try blocks. I'd to focus only on that point within this issue, so that we can determine if the lang team now has a consensus on that topic with my objections now withdrawn.

In particular, this issue is not about function-level try or any syntax for it, nor about the Try trait and any issues that may need resolving with that trait before we can stabilize try blocks. Finding consensus here does not mean that try blocks are completely ready to ship; I'd just like to evaluate this one issue in isolation and confirm that it's no longer an issue.

@joshtriplett joshtriplett added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. F-try_blocks `#![feature(try_blocks)]` labels Apr 9, 2020
@joshtriplett
Copy link
Member Author

Do we have language team consensus on the specific position that try should Ok-wrap its expression?

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 9, 2020

Team member @joshtriplett 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!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 9, 2020
@scottmcm
Copy link
Member

scottmcm commented Apr 9, 2020

Well I'm obviously in favour 🙂

@rfcbot reviewed

Definitely good to get this separated out from the other, more complicated questions.

I think consistency with async/await also helps here, since that has the same

       try { x? } == x == (try{ x })?
async { x.await } == x == (async{ x }).await

@mark-i-m
Copy link
Member

mark-i-m commented Apr 9, 2020

I still dislike the implicit control flow, but I do have to admit that this is a pretty elegant argument, and I also have to admit that ?'s implicit control flow hasn't caused me undue suffering either. So I guess that puts me in the neutral group...

@joshtriplett
Copy link
Member Author

@mark-i-m This issue isn't about any other property of try blocks; this issue is only about whether try blocks should Ok-wrap their final expression. To the best of my knowledge, that doesn't have any impact on control flow.

@mark-i-m
Copy link
Member

mark-i-m commented Apr 9, 2020

@joshtriplett Sorry, perhaps it's a poor choice of words on my part, but I mean that we are calling the Ok(...) constructor implicitly; a function call is control flow, no?

(But either way, I don't wish to create noise on this thread, so I'm content to just drop it)

@nox
Copy link
Contributor

nox commented Apr 9, 2020

Does that stabilise and sets in stone that try blocks do Ok wrapping if it gets accepted, or is it just to experiment in nightly with try blocks doing Ok wrapping?

@Centril
Copy link
Contributor

Centril commented Apr 9, 2020

@nox point of information: try { ... } blocks already do Try::from_ok-wrapping on nightly and have for some time.

@nox
Copy link
Contributor

nox commented Apr 9, 2020

So that sets in stone that if try blocks get stabilised, they will do so with Ok wrapping and that cannot be discussed anymore, is that it?

@nikomatsakis
Copy link
Contributor

@nox Correct. That is what we are deciding on.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 9, 2020

Forgive me for belaboring the point here: Am I right that when everyone in this issue (and in the linked discussion on #31436) says Ok-wrapping, that is meant as a short-hand for Try::from_ok wrapping?

That is, I'm trying to understand if @joshtriplett is proposing that we (simply) commit to the current behavior, where, according to @Centril 's note above, we already have Try::from_ok-wrapping in try blocks on nightly.

Or if instead, @joshtriplett is proposing that we wrap the result expression of a try block with Result::Ok. (Which would put less burden on the type system to infer the intended instance of the Try trait.)

I would be in favor of the former.

I would need to see more arguments explaining why the latter is better. (I can imagine why one might choose this option, namely there may be scenarios where type inference is unable to determine the desired instance of the Try trait that is necessary. But I still would want that spelled out.)

@joshtriplett
Copy link
Member Author

This issue isn't about whether we wrap using Ok or Try::from_ok; this is about whether we wrap at all. How we do that wrapping may depend on what the Try trait and the try desugaring ends up looking like.

In this issue I'm just seeking consensus on whether we should do wrapping at all.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 9, 2020

Thank you for the clarification @joshtriplett

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 9, 2020
@rfcbot
Copy link

rfcbot commented Apr 9, 2020

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Apr 9, 2020
@joshtriplett
Copy link
Member Author

Note: despite rfcbot's handling, I'd still like to get responses from @cramertj and @withoutboats as well, to confirm that we have full consensus here.

@nikomatsakis
Copy link
Contributor

So I started a comment some time ago but it seems like I never finished it. I just wanted to note my full thoughts.

I'm in favor of ok-wrapping. I do have some mild other concerns about try block -- in particular it is frequently necessary in practice to give hints as to the try of a try-block. This is for a few reasons:

  • If you don't have any ? inside, then with ok-wrapping there is no real information about what the "wrapper type" should be (i.e., should try { 3 } have the type Option<i32> or Result<i32, _>, and what should _ be?)
  • Further, the current Try trait makes no connection between the type of the enclosing function and the "carrier" type to which ? is applied, so try { foo()?.bar } also in no particular way constrains the result type. Some earlier designs would have made it so that ? could only be applied to a Result if the enclosing try block also yielded a Result, for example, but that still would leave the error type unconstrained, due to the into() conversion.

Therefore, I think we should consider some syntax that makes it either mandatory or at minimum convenient to annotate the type -- something like try as Result<_, _> { ... } or what have you. But I've not given too much serious thought into what such a type would be at present.

@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

Therefore, I think we should consider some syntax that makes it either mandatory or at minimum convenient to annotate the type -- something like try as Result<_, _> { ... } or what have you. But I've not given too much serious thought into what such a type would be at present.

I'm not sure how much you'd like to continue this discussion here @nikomatsakis. However, like @scottmcm (as they elaborate on in rust-lang/rfcs#2388 (comment)), I'm also not in favor of a strict requirement that the type be included.

As for try as Result<_, _> { ... }, the syntax "try" ":" Type Block was initially proposed as part of my type ascription RFC (rust-lang/rfcs#2522) but was dropped, primarily because it wasn't deemed necessary at the time, I believe (although it was some time ago, so it could have been for other reasons). If we're going to support a type-ascription syntax on try, I would make that use : and not as, since the former is ascription, and the latter is a cast.

@leo60228
Copy link
Contributor

What reasonable code would cause ambiguity that only exists with Ok-wrapping? Try doesn't coerce Option to Result or Result to Option, so whether it's Option<T> or Result<T, E> shouldn't be ambiguous if the try block uses ? anywhere. Why would you use a try block without ??

@nikomatsakis
Copy link
Contributor

I'd prefer not to continue the discussion here. I just wanted to note that this "rfcbot reviewed" on my part is not saying "let's stabilize try as is", but just on the ok-wrapping aspect.

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Apr 19, 2020
@rfcbot
Copy link

rfcbot commented Apr 19, 2020

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.

The RFC will be merged soon.

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 19, 2020
@CAD97
Copy link
Contributor

CAD97 commented Apr 19, 2020

ping @withoutboats; we still would like to see full consensus here.

Though I suppose it's a pretty fair bet you're in favor of try blocks doing ok wrapping.

@jD91mZM2
Copy link
Contributor

Pinging @withoutboats again... I'm sorry if this is not OK, just saw that this has been inactive for some time :)

@withoutboats
Copy link
Contributor

Sorry for never replying on this thread. I think it's well known that I am in favor of try blocks ok wrapping. But along the lines of @nikomatsakis's comment, I think someone needs to thoroughly review the state of try blocks other than ok wrapping before we stabilize them (this issue has drowned out any other issues that probably exist with try blocks).

@nikomatsakis
Copy link
Contributor

I'm going to close this as resolved. Thanks all.

@AlbertMarashi

This comment was marked as off-topic.

@fulara
Copy link

fulara commented May 25, 2022

I am sure this has been already discussed over many times,
but we just identified bug in the codebase due to Ok wrapping:

fn returns_result() -> Result<_,_>;

let result = try {
	returns_result()?;
	returns_result()
};

the result is Result<Result<_,_>,_> which is probably almost always unexpected.
Probably needs a clippy lint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-try_blocks `#![feature(try_blocks)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests