Propagate trait and the ability to propagate "success" (for some definition of success) (yeah we're back at not calling it Try)

The Propagate trait, in particular the Propagate V2 trait, would allow the introduction of return? foo semantics, equivalent to the following:

let x = return? foo;
// same as:
let x = match foo_is_option {
  Some(v) => return Ok(v),
  None => (),
};
let x = match foo_is_result {
  Ok(v) => return Some(v),
  Err(e) => e,
}

We have at least 3 crates that would benefit from such operator. It'd be useful if the Propagate trait had the variants to PropagateReturn and PropagateNonreturn, that match to return? foo and foo? respectively.

The use-case is simple. Take the following function:

    /// Returns the number of commits removed and the number of added between
    /// from and to, respectively.
    pub fn get_counts(&self, from: &str, to: &str)
        -> Result<(u64, u64), GitError>
    {
        if self.sha256 {
            assert!(NamePurpose::Commit64.is_fit(from));
            assert!(NamePurpose::Commit64.is_fit(to));
        } else {
            assert!(NamePurpose::Commit.is_fit(from));
            assert!(NamePurpose::Commit.is_fit(to));
        }
        let mut output = self.cmd(|args| {
            args.arg("rev-list");
            args.arg("--left-right");
            args.arg("--count");
            args.arg(format!("{}...{}", from, to));
        })?;
        // perf: Vec::default doesn't allocate.
        let stdout = std::mem::take(&mut output.stdout);
        let stdout = String::from_utf8(stdout);
        match stdout.as_ref().ok().map(|x| x.trim()).filter(|x| {
            x.trim_start_matches(|x| {
                char::is_ascii_digit(&x)
            }).trim_end_matches(|x| {
                char::is_ascii_digit(&x)
            }) == "\t"
        }).and_then(|x| {
            let (y, z) = x.split_once("\t")?;
            Some((y.parse::<u64>().ok()?, z.parse::<u64>().ok()?))
        }) {
            Some(v) => {
                return Ok(v)
            },
            _ => (),
        }
        output.stdout = match stdout {
            Ok(e) => e.into_bytes(),
            Err(e) => e.into_bytes(),
        };
        let v = vec![
            OsString::from("git"),
            "rev-list".into(),
            "--left-right".into(),
            "--count".into(),
            format!("{}...{}", from, to).into(),
        ];
        Err(GitError::new(output, v))
    }

It's designed to avoid indentation, because indentation is bad. However, it's currently required to use this weird match [stuff] { Some(v) => return Ok(v), _ => (), } approach, which would be better served with return?. Lacking return? the only way to avoid the indentation is to do it like this (or to use if let but then you run into column limit). Note also that you can't just throw or_else/etc at it, because it messes with ownership in a way that or_else/etc don't like.

Propagate is a better name because it's about simple, unbiased, early-returns/value propagation. The name Try adds a bias to it and discourages ppl from even considering this "propagate-return" operator! But if at least 3 crates would benefit from this operator, why not have it? It's analogous to or_else, just like ? is analogous to and_then, and we do have or operators! As for the slight weirdness of try { return? foo; }... eh, it's just syntax, the semantics would work fine. (Specifically, this snippet would convert a Result<T, T> into a Result<T, !>, assuming try{} uses PropagateReturn/Ok-wrapping.)

Also, if you need a tagline for the operator: "Please try to return this value. If returning this value doesn't work, give us the error." ^^

This could easily be a user land crate once TryV2 is released

2 Likes

Yeah in the same way we could bring back try! and deprecate ? once TryV2 is released. But who does that benefit?

You'd need two different macros, one for the function and one for try{} blocks. That's not the intended semantics.

Not even a macro, @scottmcm demenstrated an fn bubbleSuccess<T, E>(res: Result<T, E>) -> ... function that could return a type that implement TryV2 and do exactly what you want. Return Ok(t) or Some(t) and leave Err(e) or None as the residual in scope.

4 Likes

Wouldn't that, at least for ergonomics, require that Option<T>: FromResidual<Option<T>> (which I assume would be the Residual there), and Result<T,E>: FromResidual<Result<T,!>>? In which case, it would require a standard library change at the least, even if not a Core Language change.

Side note, what would <Success<Option<T>> as Try>::Continue even be (where Success<T> is the return type of bubbleSuccess<T>(opt: Option<T>)?

Other than that, I'm strongly in favour of this. I have a decent amount of code, in for loops, that's like

if let Ok(x) = <some operation>{
    return Ok(x)
}

Where I want to try everything, until I succeed or run out of things to try (a good example would be resolving something like which::which, which has to iterate over each element of PATH, something that wants to find the first of a long list of potential programs (such as looking up the C Compiler), or feature detection.

I just want to note that "try" semantics work for both the bail-fail and early-success case; it's just a matter of flipping what's being tried and what's the failure, and whether success or failure is the continue case. In pseudo prose/Rust hybrid:

// yeet fail case
let info = try to_read_from_server();
let _ = try to_resond_to_server(info);

// yeet success case
let fail1 = try just(to_connect_on_port(21));
let fail2 = try just(to_connect_on_port(22));
yeet FailedToConnectOnPorts([fail1, fail2]);

We're (or at least I am) to consider straightline the success case. Even writing this in pseudo prose, I was very tempted to stick an // else in there to make it clearer, even though my point is that "try" as an action works for yeet-left or yeet-right.

return is currently always ! typed, and return? would necessarily be ReturnResidual typed, and that also messes with my intuition in ways that aren't the most comfortable.

Especially since this can be a (non-macro!) library helper with the TryV2 trait, I think it should be a (provided) library item to start with. If it turns out to be widely used adapter, we can consider giving it special syntax, the same way that try! was originally upgraded.

(And just to judge my weirdness budget, I was positive on both ? and .await being syntax in those forms from the start. I'm generally on the more favorable side for giving syntax to things.)

If try blocks are potential (labeled) break targets (whether or not break does "ok wrapping"), then the existence of return? should imply break? as well. Honestly, even without breakable try block, it still probably should; return is semantically just a label-break-value from the entire fn after all (even if break isn't actually usable for this purpose).

It would just be called return? tho. Yeah it'd bind to the nearest try but be called return? instead of break? but that's okay and unlikely to cause confusion. (try Ok-wraps anyway, so bare return doesn't make sense for it, but return? re-wraps so it makes sense)

We do challenge you to try rewriting the code in OP using your solution, without introducing extra indentation.

Here's some code that can compile in the current nightly, only requiring you to implement an extension trait:

pub fn test_fn<T, E>(a: Result<T, i32>, b: Result<i32, E>, c: T) -> Result<T, E> {
    let _a: i32 = a.raise_ok()?;
    let _b: i32 = b?;
    Ok(c)
}

Full playground is here. This is just as concise as your proposed syntax, and has the advantage of being just a method rather than requiring new syntax.

4 Likes

It also has the disadvantage of going at the end and thus being confusing.

Nit pick: Line 40 of the playground should be Err(e) => match e{} instead of unreachable!().

Postfix expressions aren't that confusing. If there's a problem, you can move them to a new line.

4 Likes

Good nitpick! In fact, in the impl from std that I adapted this from, that line is omitted completely; I imagine std is using some additional feature to detect uninhabited variants.

They look like error propagation, rather than error recovery. The separate syntax is a win for readability.

I don't find it more readable, and I don't think most Rust users would find it more readable either. And for people who don't know what it means yet, it's more difficult to look up. (If a method like raise_ok was part of std, you could look it up on the docs page for Result, and you could also do a web search for it. On the other hand, return? has no obvious docs page and is not searchable.) Proposals for new syntax must be able to justify their costs (in learnability and maintenance) by being so much better than the nearest alternative that the costs are justified. This proposal isn't.

3 Likes
pub fn test_fn<T, E>(a: Result<T, i32>, b: Result<i32, E>, c: T) -> Result<T, E> {
    let _a: i32 = a.raise_ok()?;
    let _b: i32 = b?;
    Ok(c)
}

vs

pub fn test_fn<T, E>(a: Result<T, i32>, b: Result<i32, E>, c: T) -> Result<T, E> {
    let _a: i32 = return? a;
    let _b: i32 = b?;
    Ok(c)
}

the latter is more readable if you read return? as "Please try to return this value. If returning this value doesn't work, give us the error."

and return? works with anything Try, whereas .raise_ok (or a better name .try_return) would only work with Result (and maybe Option).

bonus points if you can also make these possible:

fn foo(x: Result<i32, ()>) -> i32 {
  let _x = return? x;
  ...
}
fn bar(x: Option<i32>) -> i32 {
  let _x = return? x;
  ...
}
fn baz(x: Option<i32>) -> Result<i32, ()> {
  let _x = return? x;
  ...
}
fn qux(x: Result<i32, ()>) -> Option<i32> {
  let _x = return? x;
  ...
}

Correct; it's using exhaustive_patterns - The Rust Unstable Book

Without that I agree that match x {} is the way to go -- here's what I submitted for Rocket:

1 Like

This seems confusing to me.

  1. In general, the ? operator is used to return on "bad" values (None, Err(T)), and this has it return on "good" values.
  2. It makes return? foo an expression instead of a statement, which is not what I expect to see when I see return. I think this would be especially hard for people new to the language.

I also think there are alternatives. As others have pointed out, this could be trivially replaced by already existing rust code.

let x = if let Err(e) = foo {e} else { foo };
// and
if foo.is_some() { return foo; }

I think these are clearer. If concern is indentation, I would prioritize getting rustfmt to accept if condition { statement; } on a single line.

I think there could be some value in having something more elaborate than if .. return, but I'd like to see some real code demonstrating the need for it.

For instance, I have some code in mind as a 'worst case scenario' which I'll reproduce here to demonstrate, but as you can see, the implementation I have is really not that bad using current code:

Example code
fn parse_token<'text, Cm>(
    &mut self, 
    source: &'text str,
    base: Pos,
    metrics: Cm)
    -> Option<(Token, Pos)>
    where Cm: ColumnMetrics,
{
    use Token::*;
    match self.open.take() {
        Some(OpenLineComment) => {
            // Line comment cannot fail, so no other parse could be returned
            // here.
            self.parse_line_comment_text(source, base, metrics)
        },
        Some(OpenBlockComment) => {
            if let Some(parse) = self
                .parse_str(source, base, metrics, "*/", CloseBlockComment)
            {
                self.depth = 0;
                return Some(parse);
            }
            
            self.open = Some(OpenBlockComment);
            // Block comment cannot fail, so no other parse could be
            // returned here.
            self.parse_block_comment_text(source, base, metrics)
        },


        Some(RawStringText) => {
            // Because it is necessary to recognize the RawStringClose to
            // finish parsing RawStringText, we should never get here unless
            // we know the next part of the source is the appropriately sized
            // RawStringClose token. So instead of explicitely parsing it,
            // we can just jump forward.
            let byte: usize = (self.depth + 1)
                .try_into()
                .expect("Pos overflow");
            Some((RawStringClose, Pos::new(byte, 0, byte)))
        },
        Some(RawStringOpen) => {
            if let Some(parse) = self
                .parse_raw_string_close(source, base, metrics)
            {
                self.depth = 0;
                return Some(parse);
            }
            return_if_some!(self
                .parse_raw_string_text(source, base, metrics));
            None
        },

        Some(StringOpenSingle) => {
            return_if_some!(self
                .parse_str(source, base, metrics, "\'", StringCloseSingle));
            if let Some(parse) = self
                .parse_string_text(source, base, metrics, StringOpenSingle)
            {
                // Keep this open to prioritize the close.
                self.open = Some(StringOpenSingle);
                return Some(parse);
            }
            panic!("invalid scanner state");
        },
        Some(StringOpenDouble) => {
            return_if_some!(self
                .parse_str(source, base, metrics, "\"", StringCloseDouble));
            if let Some(parse) = self
                .parse_string_text(source, base, metrics, StringOpenDouble)
            {
                // Keep this open to prioritize the close.
                self.open = Some(StringOpenDouble);
                return Some(parse);
            }
            panic!("invalid scanner state");
        },

        Some(Hash) => {
            // HexDigits can only come after Hash.
            return_if_some!(self.parse_hex_digits(source, base, metrics));
            self.scan(source, base, metrics)
        },

        Some(Colon) => {
            // Colon will make Position parts a priority until something
            // else is seen. It is important to have uint before float to
            // avoid swallowing them up with decimals.
            self.open = Some(Colon);
            return_if_some!(self.parse_uint(source, base, metrics));

            return_if_some!(self
                .parse_str(source, base, metrics, ".", Decimal));

            return_if_some!(self
                .parse_str(source, base, metrics, "*", Mult));

            return_if_some!(self
                .parse_str(source, base, metrics, "+", Plus));

            return_if_some!(self
                .parse_str(source, base, metrics, "-", Minus));


            self.open = None;
            self.scan(source, base, metrics)
        },

        None => {
            return_if_some!(self.parse_whitespace(source, base, metrics));

            return_if_some!(self
                .parse_str(source, base, metrics, "(", OpenParen));

            return_if_some!(self
                .parse_str(source, base, metrics, ")", CloseParen));

            return_if_some!(self
                .parse_str(source, base, metrics, "[", OpenBracket));

            return_if_some!(self
                .parse_str(source, base, metrics, "]", CloseBracket));

            return_if_some!(self
                .parse_str(source, base, metrics, "{", OpenBrace));

            return_if_some!(self
                .parse_str(source, base, metrics, "}", CloseBrace));



            if let Some(parse) = self
                .parse_str(source, base, metrics, "//", OpenLineComment)
            {
                self.open = Some(OpenLineComment);
                return Some(parse);
            }
            if let Some(parse) = self
                .parse_str(source, base, metrics, "/*", OpenBlockComment)
            {
                self.open = Some(OpenBlockComment);
                self.depth = 1;
                return Some(parse);
            }

            // RawStringOpen must be parsed before Hash.
            if let Some(parse) = self
                .parse_raw_string_open(source, base, metrics)
            {
                self.open = Some(RawStringOpen);
                return Some(parse);
            }
            if let Some(parse) = self
                .parse_str(source, base, metrics, "\'", StringOpenSingle)
            {
                self.open = Some(StringOpenSingle);
                return Some(parse);
            }
            if let Some(parse) = self
                .parse_str(source, base, metrics, "\"", StringOpenDouble)
            {
                self.open = Some(StringOpenDouble);
                return Some(parse);
            }

            return_if_some!(self
                .parse_str(source, base, metrics, ";", Semicolon));
            
            if let Some(parse) = self
                .parse_str(source, base, metrics, ":", Colon)
            {
                self.open = Some(Colon);
                return Some(parse);
            }

            return_if_some!(self
                .parse_str(source, base, metrics, ",", Comma));
            
            if let Some(parse) = self
                .parse_str(source, base, metrics, "#", Hash)
            {
                self.open = Some(Hash);
                return Some(parse);
            }

            return_if_some!(self
                .parse_str(source, base, metrics, "*", Mult));
            return_if_some!(self
                .parse_str(source, base, metrics, "+", Plus));
            return_if_some!(self
                .parse_str(source, base, metrics, "-", Minus));
            
            // Float must be parsed before Uint and Decimal.
            return_if_some!(self.parse_float(source, base, metrics));

            return_if_some!(self
                .parse_str(source, base, metrics, ".", Decimal));
            return_if_some!(self.parse_uint(source, base, metrics));

            // Ident must be parsed before Underscore.
            return_if_some!(self.parse_ident(source, base, metrics));
            
            return_if_some!(self
                .parse_str(source, base, metrics, "_", Underscore));
            
            None
        },

        Some(s) => panic!(
            "invalid lexer state Some({:?}) continuing at {:?}", s, source),
    }
}

Addendum: Outside of this exact function, I've personally had very little need for a 'return if Some/Ok' sugar.

Lol we knew it was gonna be about parsing before we even opened it :‌p

So yeah. The reason is parsing. The whole entire reason is just parsing. That's the one and only reason to have this feature.

Literally all the parsing code we have, across multiple crates, would benefit from this feature. But outside of said parsing code we can't think of an use for it. But maybe that says more about parsing than it does about this proposal.

Then I suggest trying, on nightly, making a custom type and using try_trait_v2 to have it support ? using the semantics you want for the parsing code.

Experience reports like that are always helpful as input to stabilization decisions.

2 Likes

Not necessarily, as I mentioned previously. My feature detection code would benefit from this sugar.

1 Like