rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.97k stars 1.57k forks source link

Add `homogeneous_try_blocks` RFC #3721

Open scottmcm opened 3 weeks ago

scottmcm commented 3 weeks ago

Tweak the behaviour of ? inside try{} blocks to not depend on context, in order to work better with methods and need type annotations less often.

The stable behaviour of ? when not in a try{} block is untouched.

Rendered

Nadrieril commented 3 weeks ago

Big +1 on "the common case should just work". We could just have method for people who want the type-changing case, like foo().bikeshed_change_try_type()?, right?

joshtriplett commented 3 weeks ago

@scottmcm Can you clarify why this is a T-libs-api RFC in addition to a T-lang one? The implementation of this might involve changes to the traits underlying Try, but it seems like the key purpose of this RFC is "how should try and ? interact", rather than the implementation of that.

Nadrieril commented 3 weeks ago

Should we wait a few weeks before nominating for T-lang? A few recent RFCs felt rushed to me because they were FCPed very quickly. I'd like to see more comments from the community before, especially when this is pushed by a lang member

Lokathor commented 3 weeks ago

Nominating only makes it show up on the generated meeting agendas for T-lang triage, but given the pace of those meetings in recent years it doesn't really mean that this RFC will necessarily be discussed at the next meeting, or even in the next 3 meetings. And the RFC theoretically could be discussed at the meeting even without it being nominated because people can just bring up topics they feel need to be discussed. There's basically no reason to not nominate this if 2 lang members already take this proposal seriously.

dev-ardi commented 3 weeks ago

Could it be possible to request this as a fallback for the closure (and async blocks) case too? Adding the try {...}? block as a workaround is not that much different from specifying the type of the Err, it's still an annoying papercut.

scottmcm commented 3 weeks ago

@Nadrieril Good point! The majority case there is just .map_err(Into::into) -- I've added a section. We could probably do something similar for residual-to-residual conversion too, though it's unclear how commonly needed that would be.

@dev-ardi Unfortunately "fallback" in the type system is extremely complicated, especially as things get nested. If you have a try block inside another try block, for example, deciding which one falls back is non-obvious. See also multiple years of discussion about wishing we could have "fallback to usize" for indexing, for example, in order to allow indexing by more types without changing how existing code infers.

My thought here is that the try marker is particularly tolerable for the closure case, because adding the try{} also allows removing the Ok() from the end. So in the unit case, using try actually means fewer characters than a fallback rule.

scottmcm commented 3 weeks ago

@joshtriplett I originally just put lang, but then thought "well but I did put traits in here", so added libs-api too in case.

You're absolutely right that my intent here is about the interaction, rather than the detail. So if libs-api wishes to leave this to lang, that's fine by me. I just didn't want to assume that.

dev-ardi commented 3 weeks ago

What would be the downside of adding this desugaring (make_try_type()) to everything? That should solve the issue with closures and async bocks too, right?

afetisov commented 2 weeks ago

@scottmcm I think it's a very nice property of this RFC: it also solves the issue of error type inference for closures and async blocks in a backwards-compatible and syntactically light way. It's kind of obvious, but could you add it as an explicit supporting use case in the RFC? My first thought was that the default error type mechanism could be extended to those cases, but this RFC obviates such additions.

purplesyringa commented 2 weeks ago

I'm a bit concerned about this change. Applications and libraries often use crates like thiserror to automatically group errors. For example, I often write something like

#[derive(Error)]
enum MyError {
    #[error("Failed to parse config: {0}")]
    InvalidConfig(#[from] serde::Error),
    #[error("Failed to connect to server: {0}")]
    ServerConnectionFailed(#[from] io::Error),
    ...
}

which I then use as

fn example() -> Result<(), MyError> {
    let config = parse_config()?; // ? promotes serde::Error to MyError
    let server = connect_to_server(server.url)?; // ? promotes io::Error to MyError
    // ...
}

With this change, this approach would stop working in try blocks.

newpavlov commented 12 hours ago

I think that changing behavior of ? in try blocks is a really big drawback of this proposal. It introduces a very visible inconsistency to the language, which arguably will trip beginners and sometimes even experienced developers. The error conversion case mentioned by @purplesyringa is also quite common in application-level code.

I think we need a more consistent solution which will work for both closures and try blocks. I don't know if it was discussed previously, but can't we collect all error types used with ? and if all error types inside a block are concrete and the same, then use this type? If at least one type is different, then an explicit type annotation will be required.

For example:

fn foo() -> Result<(), E1> { ... }
fn bar() -> Result<(), E1> { ... }
fn baz() -> Result<(), E2> { ... }
fn zoo<E: Error>() -> Result<(), E> { ... }

// Works. Evaluates to `Result<u32, E1>`
let val = try {
    foo()?;
    bar()?;
    42u32
};

// This also works. Because we "bubble" `Result`, this block evaluates to `Result<(), E1>`
let val = try {
    foo()?;
    bar()?;
};

// Same for closures. The closure returns `Result<u32, E1>`.
let f = || {
    foo()?;
    bar()?;
    Ok(42u32)
}; 

// Compilation error. Encountered two different "break" types (`E1` and `E2`),
// additional type annotations are needed
let val = try {
    foo()?;
    baz()?;
    42u32
};

// Compilation error. Generics also require explicit annotations.
// Same for mixing `Option` and `Result` in one block.
let val = try {
    foo()?;
    zoo()?;
    42u32
};

In the case of nested try blocks the same principle will apply recursively starting from "leaf" blocks.

purplesyringa commented 10 hours ago

can't we collect all error types used with ? and if all error types inside a block are concrete and the same, then use this type

I admit I have no idea how rustc handles this, but I believe this is complicated due to type inference. Generics might be parametric in the return type, so there's no "type" you can "collect" before settling on the type, at which point it's too late to reconsider the choice.

I think what you want is a fallback in type inference: try to union all return types (as in HM; I'm not sure if that's how rustc handles this), and only consider Into conversions if this fails. I can imagine that implementing fallbacks would require major changes in the compiler.