Skip to content

Implementation of Result::swap and tests #81323

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

Closed
wants to merge 3 commits into from
Closed

Implementation of Result::swap and tests #81323

wants to merge 3 commits into from

Conversation

gymore-io
Copy link

This adds the swap method on Result as well as a test function for it.

Tracking issue: #81332

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2021
@gymore-io gymore-io mentioned this pull request Jan 24, 2021
4 tasks
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking rand v0.7.3
    Checking alloc v0.0.0 (/checkout/library/alloc)
    Checking core v0.0.0 (/checkout/library/core)
    Checking std v0.0.0 (/checkout/library/std)
error[E0658]: use of unstable library feature 'result_swap'
    |
    |
364 |     assert_eq!(Ok("err").swap(), Err("err"));
    |
    |
    = help: add `#![feature(result_swap)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'result_swap'
    |
    |
365 |     assert_eq!(Err("ok").swap(), Ok("ok"));
    |
    |
    = help: add `#![feature(result_swap)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'result_swap'
    |
    |
366 |     assert_eq!(Ok("ok").swap().swap(), Ok("ok"));
    |
    |
    = help: add `#![feature(result_swap)]` to the crate attributes to enable

error[E0658]: use of unstable library feature 'result_swap'
    |
    |
366 |     assert_eq!(Ok("ok").swap().swap(), Ok("ok"));
    |
    |
    = help: add `#![feature(result_swap)]` to the crate attributes to enable
error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0658`.
error: could not compile `core`
error: could not compile `core`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--all-targets" "-p" "test" "-p" "std" "-p" "unwind" "-p" "alloc" "-p" "term" "-p" "proc_macro" "-p" "panic_abort" "-p" "panic_unwind" "-p" "core" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:00:49

@tesuji
Copy link
Contributor

tesuji commented Jan 24, 2021

What's the practical use case for this method?

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking alloc v0.0.0 (/checkout/library/alloc)
error[E0282]: type annotations needed
   --> library/core/tests/result.rs:364:16
    |
364 |     assert_eq!(Ok("err").swap(), Err("err"));
    |                ^^--------------
    |                |
    |                this method call resolves to `std::result::Result<E, T>`
    |                cannot infer type for type parameter `E` declared on the enum `Result`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0282`.
error: could not compile `core`
error: could not compile `core`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--all-targets" "-p" "test" "-p" "std" "-p" "alloc" "-p" "unwind" "-p" "panic_unwind" "-p" "proc_macro" "-p" "core" "-p" "panic_abort" "-p" "term" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:00:40

@gymore-io
Copy link
Author

@lzutao I described two use cases I could think of in the tracking issue.

I intend this method to be primarily used when the success case of a function is actually an not-so-happy path for the caller. With this function, it would be easy to propagate that success result as an error.

let ok = not_so_good_success().swap()?;

This is basically a case I had in one of my projects and I thought it would be a nice addition because currently, the idiomatic way of doing the same thing would be something like

let ok = match not_so_good_success() {
    Ok(err) => return Err(err),
    Err(ok) = ok,
};

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.................................................................................................... 9100/11283
.................................................................................................... 9200/11283
...............................................................................i......i............. 9300/11283
.................................................................................................... 9400/11283
.................iiiiii..iiiiii..i.................................................................. 9500/11283
.................................................................................................... 9700/11283
.................................................................................................... 9800/11283
.................................................................................................... 9900/11283
.................................................................................................... 10000/11283
---
Suite("src/test/assembly") not skipped for "bootstrap::test::Assembly" -- not in ["src/tools/tidy"]
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 29 tests
iiiiiiiiiiiiiiiiiiiiiiiiiiiii

 finished in 0.075 seconds
Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 116 tests
iiiiiiiiii.i.i..i..i..ii....i.i....ii..........iiii.........i.....i...i.......ii.i.ii.....iiii.....i 100/116
test result: ok. 78 passed; 0 failed; 38 ignored; 0 measured; 0 filtered out; finished in 2.26s

 finished in 2.327 seconds
Suite("src/test/ui-fulldeps") not skipped for "bootstrap::test::UiFullDeps" -- not in ["src/tools/tidy"]
---
---- src/result.rs - result::Result<T, E>::swap (line 602) stdout ----
error[E0282]: type annotations needed
 --> src/result.rs:605:12
  |
6 | assert_eq!(Ok("Error :(").swap(), Err("Error :("));
  |            ^^-------------------
  |            |
  |            this method call resolves to `std::result::Result<E, T>`
  |            cannot infer type for type parameter `E` declared on the enum `Result`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0282`.
Couldn't compile the test.
---

error: test failed, to rerun pass '--doc'


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "-p" "core" "--" "--quiet"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --exclude src/tools/tidy
Build completed unsuccessfully in 0:20:21

@kennytm
Copy link
Member

kennytm commented Jan 24, 2021

@gymore-io

I don't think Result::swap has much use alone beyond binary_search(). Most of the time when we need a result it is of the form Result<T, Error>. Swapping it makes it Result<Error, T> which cannot be fit into the rest of ecosystem easily. That is, the following won't work:

fn do_something() -> Result<(), Error> {
    let e1 = attempt("1").swap()?;
    let e2 = attempt("2").swap()?;
    let e3 = attempt("3").swap()?;
    Err(Error::all(vec![e1, e2, e3]))
}

you need to do double-swap in a try block instead, which looks quite unnatural.

fn do_something() -> Result<(), Error> {
    (|| {
        let e1 = attempt("1").swap()?;
        let e2 = attempt("2").swap()?;
        let e3 = attempt("3").swap()?;
        Ok(Error::all(vec![e1, e2, e3]))
    }()).swap()
}

@kennytm
Copy link
Member

kennytm commented Jan 24, 2021

When it comes to early return from the Ok variant, I'm much more interested in the approach via try_trait_v2 (rust-lang/rfcs#3058) than introducing swap. (Playground)

We define a .yeet_ok() method to wrap the result in a type that early returns on Ok:

struct YeetSuccess<R>(R);

impl<T, E> Result<T, E> {
    /// Wraps this result in a type such that, when applied with `?`, 
    /// will either `return Ok(t)` early or extract the error value out.
    pub fn yeet_ok(self) -> YeetSuccess<Self> {
        YeetSuccess(self)
    }
}

impl<T> Option<T> {
    /// Wraps this result in a type such that, when applied with `?`, 
    /// will either `return Some(t)` early or results in an empty value.
    pub fn yeet_some(self) -> YeetSuccess<Self> {
        YeetSuccess(self)
    }
}
Then we implement the try_trait_v2 traits.
impl<T, E> Bubble for YeetSuccess<Result<T, E>> {
    type Continue = E;
    type Holder = YeetSuccess<Result<T, !>>;
    fn continue_with(e: E) -> Self {
        Self(Err(e))
    }
    fn branch(self) -> ControlFlow<YeetSuccess<Result<T, !>>, E> {
        match self.0 {
            Ok(t) => ControlFlow::Break(YeetSuccess(Ok(t))),
            Err(e) => ControlFlow::Continue(e),
        }
    }
}

impl<T> Bubble for YeetSuccess<Option<T>> {
    type Continue = ();
    type Holder = YeetSuccess<Option<T>>;
    fn continue_with(_: ()) -> Self {
        Self(None)
    }
    fn branch(self) -> ControlFlow<YeetSuccess<Option<T>>, ()> {
        match self.0 {
            Some(t) => ControlFlow::Break(YeetSuccess(Some(t))),
            None => ControlFlow::Continue(()),
        }
    }
}

impl<T, E> BreakHolder<E> for YeetSuccess<Result<T, !>> {
    type Output = YeetSuccess<Result<T, E>>;
}

impl<T> BreakHolder<()> for YeetSuccess<Option<T>> {
    type Output = YeetSuccess<Option<T>>;
}

impl<T, E> Try<YeetSuccess<Result<T, !>>> for Result<T, E> {
    fn from_holder(holder: YeetSuccess<Result<T, !>>) -> Self {
        match holder.0 {
            Ok(t) => Ok(t),
            Err(e) => match e {},
        }
    }
}

impl<T> Try<YeetSuccess<Option<T>>> for Option<T> {
    fn from_holder(holder: YeetSuccess<Option<T>>) -> Self {
        holder.0
    }
}

impl<H, R> Try<YeetSuccess<H>> for YeetSuccess<R>
where
    YeetSuccess<R>: Bubble,
    R: Try<YeetSuccess<H>>,
{
    fn from_holder(holder: YeetSuccess<H>) -> Self {
        YeetSuccess(R::from_holder(holder))
    }
}

With these, we can perform the early Ok return more naturally like:

fn do_something() -> Result<(), Error> {
    let e1 = attempt("1").yeet_ok()?;
    let e2 = attempt("2").yeet_ok()?;
    let e3 = attempt("3").yeet_ok()?;
    Err(Error::All(vec![e1, e2, e3]))
}

fn do_something_option() -> Option<&'static str> {
    attempt_option("1").yeet_some()?;
    attempt_option("2").yeet_some()?;
    attempt_option("3").yeet_some()?;
    None
}

@gymore-io
Copy link
Author

gymore-io commented Jan 24, 2021

@kennytm I see how try_trait_v2 would fix the problem as a whole and I must admit that it makes the Result::swap less useful and less clear regarding of what is actually short-circuiting the function. Should I close the tracking issue and this PR ?

@kennytm
Copy link
Member

kennytm commented Jan 24, 2021

Yes. Thanks for the contribution.

In the future you could also first raise a thread on the internals forum or #t-libs zulip stream to see if there's general interest to a new library feature.

@gymore-io
Copy link
Author

@kennytm I will keep that in mind!

@gymore-io gymore-io closed this Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants