rust-lang / rfcs

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

Lazy formatting equivalent of [Result, Option]::expect() #1952

Closed abonander closed 7 years ago

abonander commented 7 years ago

I've oft-encountered a want for a version of Result::expect() which performs lazy formatting so I can provide dynamic information in panic messages, without paying the cost of formatting up-front if the unwrap operation succeeds. Currently, it's either necessary to format the error message first, regardless of whether the operation failed or not, or match manually and call panic!().

A naive search shows a number of occurrences of the former pattern in-tree (ignoring the false positives from asm.rs which use an entirely different API):

rust $ grep -r --exclude-dir=parse --include \*.rs "expect(&"
src/libsyntax_ext/format_foreign.rs:                Num::Arg(arg.parse().expect(&format!("invalid format arg `{:?}`", arg)))
src/libsyntax_ext/format_foreign.rs:                Num::Num(s.parse().expect(&format!("invalid format num `{:?}`", s)))
src/libsyntax_ext/format.rs:            panictry!(p.expect(&token::Eq));
src/librustc_trans/back/rpath.rs:        .expect(&format!("couldn't create relative path from {:?} to {:?}", output, lib));
src/vendor/toml/src/parser.rs:    fn expect(&mut self, ch: char) -> bool {
src/grammar/verify.rs:    let proto_tok = tokens.get(toknum).expect(&not_found[..]);
src/tools/compiletest/src/header.rs:    let major: isize = version_string.parse().ok().expect(&error_string);
src/tools/compiletest/src/runtest.rs:                    .expect(&format!("failed to exec `{:?}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:                    .expect(&format!("failed to exec `{:?}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:                    .expect(&format!("failed to exec `{:?}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:                    .expect(&format!("failed to exec `{:?}`", gdb_path));
src/tools/compiletest/src/runtest.rs:                         input).expect(&format!("failed to exec `{}`", prog));
src/tools/compiletest/src/runtest.rs:            .expect(&format!("failed to exec `{}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:            .expect(&format!("failed to exec `{}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:            .expect(&format!("failed to exec `{}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:            .expect(&format!("failed to exec `{}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:            .expect(&format!("failed to exec `{}`", self.config.adb_path));
src/tools/compiletest/src/runtest.rs:                    .expect(&format!("failed to exec `{}`", self.config.adb_path));

I'd like to propose one of three APIs:

nagisa commented 7 years ago

I just use unwrap_or_else for this. I think it is still pretty elegant.

On Mar 15, 2017 12:48 PM, "Austin Bonander" notifications@github.com wrote:

I've oft-encountered a want for a version of Result::expect() which performs lazy formatting so I can provide dynamic information in panic messages, without paying the cost of formatting up-front if the unwrap operation succeeds. Currently, it's either necessary to format the error message first, regardless of whether the operation failed or not, or match manually and call panic!().

A naive search shows a number of occurrences of the former pattern in-tree:

rust $ grep -r --exclude-dir=parse --include *.rs "expect(&" src/libsyntax_ext/asm.rs: panictry!(p.expect(&token::OpenDelim(token::Paren))); src/libsyntax_ext/asm.rs: panictry!(p.expect(&token::CloseDelim(token::Paren))); src/libsyntax_ext/asm.rs: panictry!(p.expect(&token::OpenDelim(token::Paren))); src/libsyntax_ext/asm.rs: panictry!(p.expect(&token::CloseDelim(token::Paren))); src/libsyntax_ext/format_foreign.rs: Num::Arg(arg.parse().expect(&format!("invalid format arg {:?}", arg))) src/libsyntax_ext/format_foreign.rs: Num::Num(s.parse().expect(&format!("invalid format num {:?}", s))) src/libsyntax_ext/format.rs: panictry!(p.expect(&token::Eq)); src/librustc_trans/back/rpath.rs: .expect(&format!("couldn't create relative path from {:?} to {:?}", output, lib)); src/vendor/toml/src/parser.rs: fn expect(&mut self, ch: char) -> bool { src/grammar/verify.rs: let proto_tok = tokens.get(toknum).expect(&not_found[..]); src/tools/compiletest/src/header.rs: let major: isize = version_string.parse().ok().expect(&error_string); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {:?}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {:?}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {:?}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {:?}", gdb_path)); src/tools/compiletest/src/runtest.rs: input).expect(&format!("failed to exec {}", prog)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {}", self.config.adb_path)); src/tools/compiletest/src/runtest.rs: .expect(&format!("failed to exec {}", self.config.adb_path));

I'd like to propose one of three APIs:

-

[Result, Option]::expect_fmt(args: std::fmt::Arguments) which is roughly equivalent to .expect(&args.to_string())

  • Pros: No code bloat due to monomorphization
    • Cons: proliferates fmt::Arguments (which I've always considered to be more of an implementation detail of formatting which still comes in handy sometimes) into more APIs; expect_fmt(format_args! is more verbose than expect(&format!, though negligibly.
  • Changing [Result, Option]::expect() to take some T: Display.

  • Pros: Works with more things (including fmt::Arguments), doesn't introduce a new API
    • Cons: more code bloat due to monomorphization (or it could take a trait object), potentially a breaking change (though I doubt it)
  • Adding a new method which takes some T: Display and prints like expect()

  • Pros: no potential for breakage
    • Cons: more API surface, same monomorphization bloat

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rfcs/issues/1952, or mute the thread https://github.com/notifications/unsubscribe-auth/AApc0qlWyi_i08rQUEad4Ja5kHSIXuglks5rl8ILgaJpZM4MdxnN .

clarfonthey commented 7 years ago

We could technically change expect to take something Displayable because it wouldn't be breaking. I'd personally rather do that.

clarfonthey commented 7 years ago

Also, this is a dupe of #1922.

abonander commented 7 years ago

I thought I searched well enough, I guess I didn't.