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

Tracking issue for ? operator and try blocks (RFC 243, question_mark & try_blocks features) #31436

Open
10 of 12 tasks
nikomatsakis opened this issue Feb 5, 2016 · 450 comments
Open
10 of 12 tasks
Labels
A-error-handling Area: Error handling B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-try_blocks `#![feature(try_blocks)]` Libs-Tracked Libs issues that are tracked on the team's project board. S-tracking-design-concerns Status: There are blocking design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 5, 2016

Tracking issue for rust-lang/rfcs#243 and rust-lang/rfcs#1859.

Implementation concerns:

@nikomatsakis nikomatsakis added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. A-const-fn labels Feb 5, 2016
@reem
Copy link
Contributor

reem commented Feb 5, 2016

The accompanying RFC discusses a desugaring based on labeled return/break, are we getting that too or will there just be special treatment for ? and catch in the compiler?

EDIT: I think labeled return/break is an excellent idea separate from ? and catch, so if the answer is no I will probably open a separate RFC for it.

@nikomatsakis
Copy link
Contributor Author

Labeled return/break is purely for explanatory purposes.

On Fri, Feb 5, 2016 at 3:56 PM, Jonathan Reem notifications@github.com
wrote:

The accompanying RFC discusses a desugaring based on labeled return/break,
are we getting that too or will there just be special treatment for ? and
catch in the compiler?


Reply to this email directly or view it on GitHub
#31436 (comment).

@glaebhoerl
Copy link
Contributor

Another unresolved question we have to resolve before stabilizing is what the contract which impls of Into have to obey should be -- or whether Into is even the right trait to use for the error-upcasting here. (Perhaps this should be another checklist item?)

@petrochenkov
Copy link
Contributor

@reem

I think labeled return/break is an excellent idea ... I will probably open a separate RFC for it.

Please do!

@thepowersgang
Copy link
Contributor

On the subject of the Carrier trait, here is a gist example of such a trait I wrote back early in the RFC process.
https://gist.github.com/thepowersgang/f0de63db1746114266d3

@petrochenkov
Copy link
Contributor

How this is treated during parsing?

struct catch {
    a: u8
}

fn main() {

    let b = 10;
    catch { a: b } // struct literal or catch expression with type ascription inside?

}

@eddyb
Copy link
Member

eddyb commented Feb 6, 2016

@petrochenkov Well, the definition couldn't affect parsing, but I think we still have a lookahead rule, based on the second token after {, : in this case, so it should still be parsed as a struct literal.

@petrochenkov
Copy link
Contributor

Also

let c1 = catch { a: 10 };
let c2 = catch { ..c1 }; // <--

struct catch {}
let c3 = catch {}; // <--

+ rust-lang/rfcs#306 if (when!) implemented.
It seems like there are no conflicts besides struct literals.

Given the examples above I'm for the simplest solution (as usual) - always treat catch { in expression positions as start of a catch block. Nobody calls their structures catch anyway.

@durka
Copy link
Contributor

durka commented Feb 6, 2016

It would be easier if a keyword was used instead of catch.

@bluss
Copy link
Member

bluss commented Feb 6, 2016

This is the keywords list: http://doc.rust-lang.org/nightly/grammar.html#keywords

@durka
Copy link
Contributor

durka commented Feb 6, 2016

@bluss yeah, I admit none of them are great... override seems like the only one that is close. Or we could use do, heh. Or a combination, though I don't see any great ones immediately. do catch?

@bluss
Copy link
Member

bluss commented Feb 6, 2016

do is the only one that seems to be close IMO. A keyword soup with do as prefix is a bit irregular, not similar to any other part of the language? Is while let a keyword soup as well? That one feels ok now, when you are used to it.

@est31
Copy link
Member

est31 commented Feb 7, 2016

port try! to use ?

Can't ? be ported to use try! instead? This would allow for the use case where you want to get a Result return path, e.g. when debugging. With try! this is fairly easy, you just override the macro at the beginning of the file (or in lib/main.rs):

macro_rules! try {
    ($expr:expr) => (match $expr {
        Result::Ok(val) => val,
        Result::Err(err) => {
            panic!("Error occured: {:?}", err)
        }
    })
}

You will get a panic stack trace starting from the first occurrence of try! in the Result return path. In fact, if you do try!(Err(sth)) if you discover an error instead of return Err(sth), you even get the full stack trace.

But when debugging foreign libraries written by people who haven't implemented that trick, one relies on try! usage somewhere higher in the chain. And now, if the library uses the ? operator with hardcoded behaviour, getting a stacktrace gets almost impossible.

It would be cool if overriding try! would affect the ? operator as well.

Later on when the macro system gets more features you can even panic! only for specific types.

If this proposal requires an RFC please let me know.

@rpjohnst
Copy link
Contributor

rpjohnst commented Feb 7, 2016

Ideally ? could just be extended to provide stack trace support directly, rather than relying on the ability to override try!. Then it would work everywhere.

@durka
Copy link
Contributor

durka commented Feb 7, 2016

Stack traces are just one example (though a very useful one, seems to me).
If the Carrier trait is made to work, maybe that can cover such extensions?

On Sun, Feb 7, 2016 at 4:14 PM, Russell Johnston notifications@github.com
wrote:

Ideally ? could just be extended to provide stack trace support directly,
rather than relying on the ability to override try!. Then it would work
everywhere.


Reply to this email directly or view it on GitHub
#31436 (comment).

@est31
Copy link
Member

est31 commented Feb 8, 2016

Without wanting to speculate, I think that it could work, albeit with some issues.

Consider the usual case where one has code returning some Result<V,E> value. Now we would need to allow for multiple implementations of the Carrier trait to coexist. In order to not run into E0119, one has to make all implementations out of scope (possibly through different traits which are per default not imported), and when using the ? operator, the user is required to import the wished implementation.

This would require everybody, even those who don't want to debug, to import their wished trait implementation when using ?, there would be no option for a predefined default.

Possibly E0117 can be an issue too if wanting to do custom Carrier implementations for Result<V,E>, where all types are outside bounds, so libstd should provide already provide a set of implementations of the Carrier trait with the most used use cases (trivial implementation, and panic! ing implementation, perhaps more).

Having the possibility to override via a macro would provide a greater flexibility without the additional burden on the original implementor (they don't have to import their wished implementation). But I also see that rust never had a macro based operator before, and that implementing ? via a macro isn't possible if catch { ... } is supposed to work, at least not without additional language items (return_to_catch, throw, labeled break with param as used in RFC 243).

I am okay with any setup which enables one to get Result stacktraces with an Err return path, while having only to modify a very small amount of code, prefferably at the top of the file. The solution should also work unrelated to how and where the Err type is implemented.

@rphmeier
Copy link
Contributor

rphmeier commented Feb 8, 2016

Just to chime in on bikeshedding: catch in { ... } flows pretty nicely.

@durka
Copy link
Contributor

durka commented Feb 8, 2016

catch! { ... } is another backcompat choice.

@durka
Copy link
Contributor

durka commented Feb 8, 2016

Also, not that I expect this to change anything, but a note that this is going to break multi-arm macros that were accepting $i:ident ?, in the same way that type ascription broke $i:ident : $t:ty.

@dgrunwald
Copy link
Contributor

Don't overdo the backwards compatibility, just treat catch as a keyword when followed by { (possibly only in expression position, but I'm not sure if that changes much compatibility-wise).

I can also imagine some possible problems that don't involve struct literals (e.g. let catch = true; if catch {}); but I prefer a minor breaking change over a more ugly syntax.

Didn't we have a for adding new keywords, anyways? We could offer some kind of from __future__ opt-in for new syntax; or specify a rust language version number on the command-line / in Cargo.toml.
I highly doubt that in the long term, we'll be able to work with only those keywords that are already reserved. We don't want our keywords to have three different meanings each, depending on context.

@glaebhoerl
Copy link
Contributor

I agree. This isn't even the first RFC where this has come up (rust-lang/rfcs#1444 is another example). I expect it won't be the last. (Also default from rust-lang/rfcs#1210, although it's not an RFC I'm in favor of.) I think we need to find a way to add honest-to-god keywords instead of trying to figure out how to ad-hoc hack the grammar for every new case.

@rkjnsn
Copy link
Contributor

rkjnsn commented Feb 26, 2016

Wasn't the whole argument for not reserving several keywords prior to 1.0 that we'd definitely be introducing a way to add new keywords to the language backward compatibly (possibly by explicitly opting in), so there was no point? Seems like now would be a good time.

@aturon
Copy link
Member

aturon commented Feb 26, 2016

@japaric Are you interested in reviving your old PR and taking this on?

@japaric
Copy link
Member

japaric commented Feb 26, 2016

@aturon My implementation simply desugared foo? in the same way as try!(foo). It also only worked on method and function calls, i.e. foo.bar()? and baz()? work but quux? and (quux)? don't. Would that be okay for an initial implementation?

@eddyb
Copy link
Member

eddyb commented Feb 26, 2016

@japaric What was the reason for restricting it to methods and function calls? Wouldn't parsing it be easier as a general postfix operator?

@Kixunil
Copy link
Contributor

Kixunil commented Jan 3, 2024

@clarfonthey oh, yeah, that works too. An alternative could be added later anyway.

@rakshith-ravi
Copy link
Contributor

@clarfonthey I completely agree. Not only does let x: Result<A, B> = try { ... } seem simpler, it also resembles the same thing for futures (let x: impl Future<Output = A> = async { ... }). This will make it consistent across the language and makes the syntax easier for people, especially new-comers, to understand

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jan 8, 2024

Is all that is required here a stabilization PR proposing that the assignment notation for inferring the type is consistent with other block types such as async, and is preferable? Or are there still other blockers?

Edit: here is basically the same question asked in September 2022: #31436 (comment)

@dev-ardi
Copy link
Contributor

I also believe that bad inference shouldn't be a blocker, we have already merged RPITIT with improving the iffy parts later on, so there is a precedent.

This is also a very wanted feature by many people in the community and if this was stabilized just by having people using it they will notice the most common problems with type inference and will likely work on it, give solutions, ideas etc.

Every good work of software starts by scratching a developer's personal itch. – Eric Raymond

@afetisov
Copy link

afetisov commented Feb 1, 2024

Suggestion: what if the ? in try-blocks by default performs only the early return, without any error conversion? In my code, most commonly I find that I don't really need the conversion functionality, and it just gets in the way of type inference. I.e. the suggestion is, if the return error type of a try-block isn't explicitly annotated, then all error types in ? operations must be the same, and that is the error type returned by the try-block.

The obvious downside is that it's obviously inconsistent with the behaviour of ? in closures and functions. On the other hand, it removes the type inference ambiguity in the quite common case of the precise error type, and it's also a bit faster for compilation, since we aren't dumping redundant conversions into the optimizer.

@HactarCE
Copy link
Contributor

HactarCE commented Feb 1, 2024

This is consistent with ? in functions (since the return type is always annotated) but inconsistent with its use in closures. I've often had type annotation issues with closures as well for the same reason.

@ahicks92
Copy link
Contributor

ahicks92 commented Feb 1, 2024

So, that'd render it useless for most code I work on, because ours goes the other way. We almost always rely on the implicit conversion of ?, and when working cross-crate or cross-module you almost never have the same error type. E.g. call aws sdk, do disk i/o, call whatever other thing, and so on--all of those are going to use a different error. Eventually we "gave up" for lack of a better word and adopted anyhow, which solves the microservices connecting lots of things together problem quite nicely. If you already design so that operations are idempotent as it is, then error handling becomes primarily about knowing if something failed and being able to log it with a side of not punishing people for raising errors with lots of details, and less about introspection of the errors, save in specific limited cases. Connect enough libraries and combinatorial explosion kills any hope, plus one must design for server failure/process gets killed/etc anyway, so it ends up just being easier to design for "might stop and explode at any time" and that turns into some sort of dynamic error container pretty quick.

But once you have, ? really becomes more of a monadic lift to the dynamic container (in our case anyhow) and the conversion becomes essential.

Maybe it's worth stabilizing something that requires unification, but it wouldn't get me to use the feature. I do see value in the other error handling pattern wherein someone does actually make sure to use the same error type, though it is hard for me to think of many complex cases that would only use one error type, simply because of dependencies all bringing their own. It'd also be surprising behavior anyway: the ? operator would then have slight differences depending on if it's in a try {}. For the extreme case, imagine how "fun" this is for macros.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 1, 2024
…h-arm, r=compiler-errors

add test for try-block-in-match-arm

This is noted as an implementation concern under the tracking issue for `?` and `try` blocks. (Issue 31436)

Refs: rust-lang#31436
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 1, 2024
…h-arm, r=compiler-errors

add test for try-block-in-match-arm

This is noted as an implementation concern under the tracking issue for `?` and `try` blocks. (Issue 31436)

Refs: rust-lang#31436
@HactarCE
Copy link
Contributor

HactarCE commented Feb 1, 2024

To be clear, @afetisov was suggesting that the conversation still happens when the return type on the try block is annotated.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Feb 2, 2024

I feel like that when try { } blocks are nested, the inference from the inner ones should be that of the outer ones.

That is the inference solver could maybe forward the inference constraints from inner try blocks to outer try blocks (if they are the direct parent in the AST). I think it's fine to error and ask for annotations if that does not lead to a inference solution.

More concretely, given:

#![feature(try_blocks)]

fn main() {
    match try { 
        let r: Result::<(), ()> = try {
            Err(())?;
        };
        r?;
    } {
        Err(()) => (),
        Ok(()) => (),
    }
}

It seems very reasonable that r (even if no variable is made) should attempt to instead see if a solution can be found in the match try { }, and ask for annotations if that does not solve.

Are there cases where this would obviously not work? And, are there inference problems which are not related to try block nesting?

Edit: Ok I suppose this is really asking why .into().into() doesn't work even for cases where an intermediary type would be irrelevant or non-existent, which is probably it's own problem.

Edit 2: It seems the inference problem can be summed up by this code which was linked to from the try-trait-v2 rfc:

if self.sess.target.is_like_osx {
// Write a plain, newline-separated list of symbols
let res: io::Result<()> = try {
let mut f = BufWriter::new(File::create(&path)?);
for sym in self.info.exports[&crate_type].iter() {
debug!(" _{}", sym);
writeln!(f, "_{}", sym)?;
}
};
if let Err(e) = res {
self.sess.fatal(&format!("failed to write lib.def file: {}", e));
}
} else if is_windows {
let res: io::Result<()> = try {
let mut f = BufWriter::new(File::create(&path)?);
// .def file similar to MSVC one but without LIBRARY section
// because LD doesn't like when it's empty
writeln!(f, "EXPORTS")?;
for symbol in self.info.exports[&crate_type].iter() {
debug!(" _{}", symbol);
writeln!(f, " {}", symbol)?;
}
};
if let Err(e) = res {
self.sess.fatal(&format!("failed to write list.def file: {}", e));
}
} else {
// Write an LD version script
let res: io::Result<()> = try {
let mut f = BufWriter::new(File::create(&path)?);
writeln!(f, "{{")?;
if !self.info.exports[&crate_type].is_empty() {
writeln!(f, " global:")?;
for sym in self.info.exports[&crate_type].iter() {
debug!(" {};", sym);
writeln!(f, " {};", sym)?;
}
}
writeln!(f, "\n local:\n *;\n}};")?;
};
if let Err(e) = res {
self.sess.fatal(&format!("failed to write version script: {}", e));
}
}

@ahicks92
Copy link
Contributor

ahicks92 commented Feb 2, 2024

Isn't the core part of this discussion blocking at least in part on syntax to annotate the blocks, though? I did slightly misread, to be fair--if annotating causes the conversion to happen then yeah that works for me and the codebases I work on. But I think that "pruning" ideas might be the better thing to do, and nothing says that Rust can't just shrug and go "always annotate" and land. There's definitely precedent for that by now, e.g. rpitit.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Feb 2, 2024

So after reading the small pile of RFCs which are in one way or other related to this feature (but not well linked from the OP) (probably because the OP outdates some of these):

There doesn't seem to be any comprehensive reference for try { } blocks. The original RFC is very light on specifics regarding catch { }, the try expressions RFC only deals with the naming, and the try trait v2 RFC leaves it to another RFC.

From the try trait v2 RFC:

So a future RFC could define a way (syntax, code inspection, heuristics, who knows) to pick which of the desugarings would be best. (As a strawman, one could say that try { ... } uses the "same family" desugaring whereas try as anyhow::Result<_> { ... } uses the contextual desugaring.) This RFC declines to debate those possibilities, however.

So it seems to me that the best way to move forward here would really be to make a dedicated try blocks RFC where a reference can be made for what the implementation should conform to, and try to make a direction for how to handle the inference problems, as well as documenting which concerns actually need to be figured out before stabilization and which can be left for future possibilities.

Maybe that's a bit heavy handed, and perhaps people don't think that's necessary but that's probably the direction I would opt to take before wanting to make PRs to adjust the compiler inference for try blocks in specific... if no one directs otherwise an RFC is what I plan to do...

Edit: It seems I missed the try-blocks Zulip discussion from 2022:

In the top of that thread, scottmcm says:

The problem is that I think the vanilla form (of try blocks inference) should do something different, which isn't a backwards-compatible change.

I've got an RFC started; ...

For those who don't want to dig into the thread, there was a draft at https://github.com/scottmcm/rfcs/blob/try-again/text/0000-resolving-try-annotations.md#summary

@Kobzol
Copy link
Contributor

Kobzol commented Feb 3, 2024

I think that before another RFC, a useful step forward would be to summarize all the existing RFCs and discussions from both GitHub and Zulip in a single document, so that it is easier to see the status quo, and the unresolved questions and blockers.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 4, 2024
…h-arm, r=compiler-errors

add test for try-block-in-match-arm

This is noted as an implementation concern under the tracking issue for `?` and `try` blocks. (Issue 31436)

Refs: rust-lang#31436
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 5, 2024
Rollup merge of rust-lang#120540 - Fishrock123:test-try-block-in-match-arm, r=compiler-errors

add test for try-block-in-match-arm

This is noted as an implementation concern under the tracking issue for `?` and `try` blocks. (Issue 31436)

Refs: rust-lang#31436
@dev-ardi
Copy link
Contributor

dev-ardi commented Mar 2, 2024

I think that before another RFC, a useful step forward would be to summarize all the existing RFCs and discussions from both GitHub and Zulip in a single document, so that it is easier to see the status quo, and the unresolved questions and blockers.

I don't agree. All of the discussion should happen here and all of the relevant points should be raised here. If anyone has heard of anything they might point to it but I don't think that it makes sense to require a document because nobody wants to go over all threads compiling information.

It is my understanding that the only question left is about the inference, and based on the general sentiment that's not a blocking issue.

I propose to stabilize!

@Kobzol
Copy link
Contributor

Kobzol commented Mar 2, 2024

Of course that no one wants to do it, it's a lot of work, but that's what it takes to move these big features forwars :) For a stabilization PR, we'd need to do that anyway, to summarize the current state and precisely specify what are the current unresolved questions, potential blockers and alternatives.

@CAD97
Copy link
Contributor

CAD97 commented Mar 3, 2024

@dev-ardi All of the discussion should happen here

Actually, no. Discussion should be linked into the tracking issue, but tracking issues are not intended to be where discussion takes place. From the current tracking issue template:

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.


It was my understanding that try blocks without a "return type annotation" were weakly intended to move to "residual preserving" semantics before stabilization. But still separately from that, I expect stabilization of Try itself is blocked on improving documentation around the Residual type. (Personally, I wish "stem inter-conversion" hadn't been used for Poll and poll? had been ready!(poll) instead. Unfortunately, that's quite unlikely to be changed even over an edition.)

@Fishrock123
Copy link
Contributor

I asked @scottmcm if he still wanted to pursue his RFC in the Zulip thread, but with no reply https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/try.20blocks/near/419566066

I think that before another RFC, a useful step forward would be to summarize all the existing RFCs and discussions from both GitHub and Zulip in a single document, so that it is easier to see the status quo, and the unresolved questions and blockers.

I'm pretty sure I already did what you are referring by summarizing the discussion in the comment above that comment?

Like I said, there isn't a clear design for what the feature should actually be; so honestly an RFC (or pre-rfc, or similar) - a "document", as you describe it, which I don't see why would be anything other than a RFC-like document. RFCs are for defining features with discussion.

@Sytten
Copy link

Sytten commented May 6, 2024

What are the blockers for stabilization right now? Does this need another RFC or can the initial implementation be added to stable?

@Houtamelo
Copy link
Contributor

What are the blockers for stabilization right now? Does this need another RFC or can the initial implementation be added to stable?

I would assume all issues would have to be fixed.

For example, I'm still running into this one:

Address issues with type inference (try { expr? }? currently requires an explicit type annotation somewhere).

@Kobzol
Copy link
Contributor

Kobzol commented Dec 9, 2024

rust-lang/rfcs#3721 is the latest update, IIRC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Area: Error handling B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-try_blocks `#![feature(try_blocks)]` Libs-Tracked Libs issues that are tracked on the team's project board. S-tracking-design-concerns Status: There are blocking design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests