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

Impl Termination for Infallible and then make the Result impls of Termination more generic #97803

Merged
merged 2 commits into from
Jun 18, 2022

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Jun 6, 2022

This allows things like Result<ExitCode, E> to 'just work'

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 6, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2022
@yaahc
Copy link
Member

yaahc commented Jun 6, 2022

r? @yaahc

@rust-highfive rust-highfive assigned yaahc and unassigned m-ou-se Jun 6, 2022
@Gankra
Copy link
Contributor Author

Gankra commented Jun 6, 2022

My actual motivating usecase is the following pattern that I am experimenting with to have a hybrid between std::process::exit and panic -- basically do the unwinding cleanup but then silently exit:

/// panic_any with this to std::process::exit but actually do cleanup
/// (also panic::set_hook to similarly match on this payload to silence printing)
struct ExitPanic(i32);

fn main() -> Result<(), eyre::Report> {
    // Wrap main up in a catch_panic so that we can use it to implement std::process::exit with
    // unwinding, allowing us to silently exit the program while still cleaning up.
    let result = std::panic::catch_unwind(real_main);
    match result {
        Ok(main_result) => main_result,
        Err(e) => {
            if let Some(ExitPanic(code)) = e.downcast_ref::<ExitPanic>() {
                // Exit panic, just silently exit with this status
                std::process::exit(*code);
            } else {
                // Normal panic, let it ride
                std::panic::resume_unwind(e);
            }
        }
    }
}

Here I explicitly std::process::exit but it's not clear if that's actually desirable if I want a "fully normal cleanup". Instead it would be nice if I could do something like this, where I simply nest Terminations for the different paths:

fn main() -> Result<ExitCode, eyre::Report> {
    // Wrap main up in a catch_panic so that we can use it to implement std::process::exit with
    // unwinding, allowing us to silently exit the program while still cleaning up.
    let result = std::panic::catch_unwind(real_main);
    match result {
        Ok(main_result) => main_result,
        Err(e) => {
            if let Some(code) = e.downcast_ref::<ExitCode>() {
                // Exit panic, just silently exit with this status
                Ok(code)
            } else {
                // Normal panic, let it ride
                std::panic::resume_unwind(e);
            }
        }
    }
}

For more complex cases you could also return Result<Result<Result<.....>>> to chain together different kinds of errors.

@yaahc yaahc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 6, 2022
@yaahc
Copy link
Member

yaahc commented Jun 6, 2022

This change is insta-stable and thus requires an FCP, @Gankra has already provided a summary above of the intended use case for nesting termination types via Result to provide multiple modes of failure.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jun 6, 2022

Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 6, 2022
Comment on lines 2150 to 2154
#[stable(feature = "termination_trait_lib", since = "1.61.0")]
impl<E: fmt::Debug> Termination for Result<!, E> {
impl Termination for Infallible {
fn report(self) -> ExitCode {
let Err(err) = self;
eprintln!("Error: {err:?}");
ExitCode::FAILURE
// Doesn't matter, this is unreachable
ExitCode::SUCCESS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: this impl is completely new and so maybe wants to be since 1.63.0 but idk how this works with trait impls anymore

#[stable(feature = "termination_trait_lib", since = "1.61.0")]
impl Termination for ExitCode {
#[inline]
impl<T: Termination, E: fmt::Debug> Termination for Result<T, E> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly this impl is technically new but is backfilling removed impls that were since 1.61.0 so..?

Copy link
Member

@yaahc yaahc Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo backdating them both seems correct since they're both representing existing impls from that release even if they both add new impls as well. We don't have a rule for this AFAIK.

I guess the most pertinent question is "who are these versions for and why?". The only time I end up using these is when I'm curious how long a certain API has been stable or when a decision was made. I also thought that people might be using these versions for MSRV checks but my experience is that I usually just actually test against the MSRV I am aiming for supporting, so I don't think we should consider the MSRV user experience in these attributes, which will likely be inaccurate either way and would be too time consuming to determine manually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for the stable_features diagnostic.

warning: the feature `pin` has been stable since 1.33.0 and no longer requires an attribute to enable
 --> src/main.rs:1:12
  |
1 | #![feature(pin)]
  |            ^^^
  |
  = note: `#[warn(stable_features)]` on by default

which means it is pretty much meaningless for impls, since you don't get impls by enabling a feature.

@rust-log-analyzer

This comment has been minimized.

@Gankra
Copy link
Contributor Author

Gankra commented Jun 6, 2022

Definitely a diagnostic regression but idk how to do that stuff

@rust-log-analyzer

This comment has been minimized.

@yaahc
Copy link
Member

yaahc commented Jun 6, 2022

Definitely a diagnostic regression but idk how to do that stuff

I'm guessing that's a bug in the diagnostic impl in the compiler and it's passing in the wrong ID when reporting issues with main's return type. For now I think go ahead and bless the new output and we can open an issue to fix the diagnostic separately.

@joshtriplett
Copy link
Member

Seems reasonable to me!

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 7, 2022
@rfcbot
Copy link

rfcbot commented Jun 7, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 7, 2022
@Gankra
Copy link
Contributor Author

Gankra commented Jun 7, 2022

New test result pushed

@bors
Copy link
Contributor

bors commented Jun 12, 2022

☔ The latest upstream changes (presumably #98025) made this pull request unmergeable. Please resolve the merge conflicts.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 17, 2022
@rfcbot
Copy link

rfcbot commented Jun 17, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jun 17, 2022
@dtolnay dtolnay changed the title Impl Termination for Infallible and then make the Result impls of Termination into a blanket Impl Termination for Infallible and then make the Result impls of Termination more generic Jun 17, 2022
@dtolnay
Copy link
Member

dtolnay commented Jun 17, 2022

Needs rebase.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2022
@Gankra
Copy link
Contributor Author

Gankra commented Jun 17, 2022

@dtolnay rebased

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Jun 17, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jun 17, 2022

📌 Commit 0894660 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2022
@dtolnay dtolnay assigned dtolnay and unassigned yaahc Jun 17, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 18, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97803 (Impl Termination for Infallible and then make the Result impls of Termination more generic)
 - rust-lang#97828 (Allow configuring where artifacts are downloaded from)
 - rust-lang#98150 (Emscripten target: replace -g4 with -g, and -g3 with --profiling-funcs)
 - rust-lang#98195 (Fix rustdoc json primitive handling)
 - rust-lang#98205 (Remove a possible unnecessary assignment)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fc8027f into rust-lang:master Jun 18, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 18, 2022
@@ -1,4 +1,4 @@
error[E0277]: `main` has invalid return type `Result<f32, ParseFloatError>`
error[E0277]: `main` has invalid return type `f32`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error now is a bit off 🤔

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 8, 2022
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 10, 2022
Pkgsrc changes:
 * Adjust patches as needed & checksum updates.

Upstream changes:

Version 1.63.0 (2022-08-11)
==========================

Language
--------
- [Remove migrate borrowck mode for pre-NLL errors.][95565]
- [Modify MIR building to drop repeat expressions with length zero.][95953]
- [Remove label/lifetime shadowing warnings.][96296]
- [Allow explicit generic arguments in the presence of `impl Trait` args.]
  [96868]
- [Make `cenum_impl_drop_cast` warnings deny-by-default.][97652]
- [Prevent unwinding when `-C panic=abort` is used regardless of
  declared ABI.][96959]
- [lub: don't bail out due to empty binders.][97867]

Compiler
--------
- [Stabilize the `bundle` native library modifier,][95818] also removing the
  deprecated `static-nobundle` linking kind.
- [Add Apple WatchOS compile targets\*.][95243]
- [Add a Windows application manifest to rustc-main.][96737]

\* Refer to Rust's [platform support page][platform-support-doc] for more
   information on Rust's tiered platform support.

Libraries
---------
- [Implement `Copy`, `Clone`, `PartialEq` and `Eq` for
  `core::fmt::Alignment`.][94530]
- [Extend `ptr::null` and `null_mut` to all thin (including extern)
  types.][94954]
- [`impl Read and Write for VecDeque<u8>`.][95632]
- [STD support for the Nintendo 3DS.][95897]
- [Make write/print macros eagerly drop temporaries.][96455]
- [Implement internal traits that enable `[OsStr]::join`.][96881]
- [Implement `Hash` for `core::alloc::Layout`.][97034]
- [Add capacity documentation for `OsString`.][97202]
- [Put a bound on collection misbehavior.][97316]
- [Make `std::mem::needs_drop` accept `?Sized`.][97675]
- [`impl Termination for Infallible` and then make the `Result` impls
  of `Termination` more generic.][97803]
- [Document Rust's stance on `/proc/self/mem`.][97837]

Stabilized APIs
---------------

- [`array::from_fn`]
- [`Box::into_pin`]
- [`BinaryHeap::try_reserve`]
- [`BinaryHeap::try_reserve_exact`]
- [`OsString::try_reserve`]
- [`OsString::try_reserve_exact`]
- [`PathBuf::try_reserve`]
- [`PathBuf::try_reserve_exact`]
- [`Path::try_exists`]
- [`Ref::filter_map`]
- [`RefMut::filter_map`]
- [`NonNull::<[T]>::len`][`NonNull::<slice>::len`]
- [`ToOwned::clone_into`]
- [`Ipv6Addr::to_ipv4_mapped`]
- [`unix::io::AsFd`]
- [`unix::io::BorrowedFd<'fd>`]
- [`unix::io::OwnedFd`]
- [`windows::io::AsHandle`]
- [`windows::io::BorrowedHandle<'handle>`]
- [`windows::io::OwnedHandle`]
- [`windows::io::HandleOrInvalid`]
- [`windows::io::HandleOrNull`]
- [`windows::io::InvalidHandleError`]
- [`windows::io::NullHandleError`]
- [`windows::io::AsSocket`]
- [`windows::io::BorrowedSocket<'handle>`]
- [`windows::io::OwnedSocket`]
- [`thread::scope`]
- [`thread::Scope`]
- [`thread::ScopedJoinHandle`]

These APIs are now usable in const contexts:

- [`array::from_ref`]
- [`slice::from_ref`]
- [`intrinsics::copy`]
- [`intrinsics::copy_nonoverlapping`]
- [`<*const T>::copy_to`]
- [`<*const T>::copy_to_nonoverlapping`]
- [`<*mut T>::copy_to`]
- [`<*mut T>::copy_to_nonoverlapping`]
- [`<*mut T>::copy_from`]
- [`<*mut T>::copy_from_nonoverlapping`]
- [`str::from_utf8`]
- [`Utf8Error::error_len`]
- [`Utf8Error::valid_up_to`]
- [`Condvar::new`]
- [`Mutex::new`]
- [`RwLock::new`]

Cargo
-----
- [Stabilize the `--config path` command-line argument.][cargo/10755]
- [Expose rust-version in the environment as
  `CARGO_PKG_RUST_VERSION`.][cargo/10713]

Compatibility Notes
-------------------

- [`#[link]` attributes are now checked more strictly,][96885]
  which may introduce errors for invalid attribute arguments that
  were previously ignored.

Internal Changes
----------------

These changes provide no direct user facing benefits, but represent
significant improvements to the internals and overall performance
of rustc and related tools.

- [Prepare Rust for LLVM opaque pointers.][94214]

[94214]: rust-lang/rust#94214
[94530]: rust-lang/rust#94530
[94954]: rust-lang/rust#94954
[95243]: rust-lang/rust#95243
[95565]: rust-lang/rust#95565
[95632]: rust-lang/rust#95632
[95818]: rust-lang/rust#95818
[95897]: rust-lang/rust#95897
[95953]: rust-lang/rust#95953
[96296]: rust-lang/rust#96296
[96455]: rust-lang/rust#96455
[96737]: rust-lang/rust#96737
[96868]: rust-lang/rust#96868
[96881]: rust-lang/rust#96881
[96885]: rust-lang/rust#96885
[96959]: rust-lang/rust#96959
[97034]: rust-lang/rust#97034
[97202]: rust-lang/rust#97202
[97316]: rust-lang/rust#97316
[97652]: rust-lang/rust#97652
[97675]: rust-lang/rust#97675
[97803]: rust-lang/rust#97803
[97837]: rust-lang/rust#97837
[97867]: rust-lang/rust#97867
[cargo/10713]: rust-lang/cargo#10713
[cargo/10755]: rust-lang/cargo#10755

[`array::from_fn`]: https://doc.rust-lang.org/stable/std/array/fn.from_fn.html
[`Box::into_pin`]: https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_pin
[`BinaryHeap::try_reserve_exact`]: https://doc.rust-lang.org/stable/alloc/collections/binary_heap/struct.BinaryHeap.html#method.try_reserve_exact
[`BinaryHeap::try_reserve`]: https://doc.rust-lang.org/stable/std/collections/struct.BinaryHeap.html#method.try_reserve
[`OsString::try_reserve`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve
[`OsString::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve_exact
[`PathBuf::try_reserve`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve
[`PathBuf::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve_exact
[`Path::try_exists`]: https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.try_exists
[`Ref::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.Ref.html#method.filter_map
[`RefMut::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.RefMut.html#method.filter_map
[`NonNull::<slice>::len`]: https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.len
[`ToOwned::clone_into`]: https://doc.rust-lang.org/stable/std/borrow/trait.ToOwned.html#method.clone_into
[`Ipv6Addr::to_ipv4_mapped`]: https://doc.rust-lang.org/stable/std/net/struct.Ipv6Addr.html#method.to_ipv4_mapped
[`unix::io::AsFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsFd.html
[`unix::io::BorrowedFd<'fd>`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.BorrowedFd.html
[`unix::io::OwnedFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.OwnedFd.html
[`windows::io::AsHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsHandle.html
[`windows::io::BorrowedHandle<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedHandle.html
[`windows::io::OwnedHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedHandle.html
[`windows::io::HandleOrInvalid`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrInvalid.html
[`windows::io::HandleOrNull`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrNull.html
[`windows::io::InvalidHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.InvalidHandleError.html
[`windows::io::NullHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.NullHandleError.html
[`windows::io::AsSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsSocket.html
[`windows::io::BorrowedSocket<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedSocket.html
[`windows::io::OwnedSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedSocket.html
[`thread::scope`]: https://doc.rust-lang.org/stable/std/thread/fn.scope.html
[`thread::Scope`]: https://doc.rust-lang.org/stable/std/thread/struct.Scope.html
[`thread::ScopedJoinHandle`]: https://doc.rust-lang.org/stable/std/thread/struct.ScopedJoinHandle.html

[`array::from_ref`]: https://doc.rust-lang.org/stable/std/array/fn.from_ref.html
[`slice::from_ref`]: https://doc.rust-lang.org/stable/std/slice/fn.from_ref.html
[`intrinsics::copy`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy.html
[`intrinsics::copy_nonoverlapping`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy_nonoverlapping.html
[`<*const T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to
[`<*const T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping
[`<*mut T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to-1
[`<*mut T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping-1
[`<*mut T>::copy_from`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from
[`<*mut T>::copy_from_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from_nonoverlapping
[`str::from_utf8`]: https://doc.rust-lang.org/stable/std/str/fn.from_utf8.html
[`Utf8Error::error_len`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.error_len
[`Utf8Error::valid_up_to`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.valid_up_to
[`Condvar::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.new
[`Mutex::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.new
[`RwLock::new`]: https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.new
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.