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

[Gankra]

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

[rust-highfive]

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]

r? @m-ou-se

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

[yaahc]

r? @yaahc

[Gankra]

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]

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]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

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.

[Gankra]

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

[Gankra]

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

[yaahc]

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.

[dtolnay]

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.

[Gankra]

Definitely a diagnostic regression but idk how to do that stuff

[yaahc]

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]

Seems reasonable to me!

@rfcbot reviewed

[rfcbot]

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

[Gankra]

New test result pushed

[bors]

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

[rfcbot]

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.

[dtolnay]

Needs rebase.
@rustbot author

[Gankra]

@dtolnay rebased

[dtolnay]

Thanks!

[dtolnay]

@bors r+

[bors]

📌 Commit 0894660 has been approved by dtolnay

[WaffleLapkin]

This error now is a bit off 🤔