Make ExitStatus implement Default by ijackson · Pull Request #106425 · rust-lang/rust

ijackson · 2023-01-03T21:01:49Z

And, necessarily, make it inhabited even on platforms without processes.

I noticed while preparing rust-lang/rfcs#3362 that there was no way for anyone to construct an ExitStatus.

This would be insta-stable so needs an FCP.

Even where actually running processes is not supported. Needed for the next commit. The manual trait implementations now belong on ExitStatusError, which still can't exist.

rustbot · 2023-01-03T21:01:56Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-01-03T21:01:58Z

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 @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. 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

ijackson · 2023-01-03T21:02:14Z

@rustbot label +T-libs-api -T-libs

thomcc

Impl looks fine, but will need API review.

thomcc · 2023-01-04T02:15:52Z

This should probably have ACP, tbh

ijackson · 2023-01-04T10:36:26Z

This should probably have ACP, tbh

Ah, sorry. Now done in rust-lang/libs-team#158

The rustbot comment says (emphasis added):

If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already.

Maybe that isn't quite right? I'm not sure what it should say precisely (or where to edit it).

m-ou-se · 2023-01-10T11:16:41Z

@rfcbot merge

(See rust-lang/libs-team#158 for motivation/details.)

rfcbot · 2023-01-10T11:16:43Z

Team member @m-ou-se 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 · 2023-01-10T15:22:58Z

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

scottmcm · 2023-01-14T22:41:01Z

I see that there's nothing in the PR that documents what the default is.

I think it should at least say somewhere what that default is. Having it come from a derive(Default) on a c_int feels indirect to me as well, so perhaps it would make sense to define it manually in terms of ExitCode::SUCCESS instead, say -- that would also allow putting a doc-comment on the impl.

scottmcm · 2023-01-14T22:42:15Z

It also feels like it make make sense to make ExitCode also be Default at the same time, since it feels weird for the more-complicated type to be Default when the simple one isn't.

ijackson · 2023-01-19T12:35:19Z

@scottmcm I generally agree with your comments.

In rust-lang/libs-team#158 @m-ou-se suggested impl From<ExitCode> for ExitStatus, which I also agree with.

In summary, I think we should

impl Default for ExitCode (simply SUCCESS)
impl From<ExitCode> for ExitStatus (with the obvious semantics, but might be worth some additional docs)
Document that Default for ExitStatus is "process reported successful termination", ie the result of ExitCode::success().into(), and, implement it that way.

I have put this on my personal todo list. Items 1 and 2 are insta-stable. Should I make an ACP, or can we do the FCP in the MR? It seems to me that this is all relatively uncontroversial so maybe going straight to MR is best.

I don't think any of those are ABI breaks from where this MR gets us, so this MR can go through as-is, even though it has room for improvement.

ijackson · 2023-08-07T15:34:41Z

Oh. I see I lost the review race there. I will rewind and do the followup as its own MR.

As suggested here rust-lang#106425 (comment)

ijackson · 2023-08-08T10:23:33Z

@workingjubilee I think my push and rewind have caused the bots to become confused. @bors said "this is now in the queue" but clicking through to the queue it isn't showing as Approved. Would you mind re-approving 1f1d49a ? Sorry for the inconvenience.

dtolnay · 2023-08-08T14:49:05Z

@bors r+

bors · 2023-08-08T14:49:07Z

📌 Commit 1f1d49a has been approved by dtolnay

It is now in the queue for this repository.

…olnay Make ExitStatus implement Default And, necessarily, make it inhabited even on platforms without processes. I noticed while preparing rust-lang/rfcs#3362 that there was no way for anyone to construct an `ExitStatus`. This would be insta-stable so needs an FCP.

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#106425 (Make ExitStatus implement Default) - rust-lang#113480 (add aarch64-unknown-teeos target) - rust-lang#113586 (Mention style for new syntax in tracking issue template) - rust-lang#113593 (CFI: Fix error compiling core with LLVM CFI enabled) - rust-lang#114612 (update llvm-wrapper include to silence deprecation warning) - rust-lang#114613 (Prevent constant rebuilds of `rustc-main` (and thus everything else)) - rust-lang#114615 (interpret: remove incomplete protection against invalid where clauses) - rust-lang#114628 (Allowing re-implementation of mir_drops_elaborated query) - rust-lang#114629 (tests: Uncomment now valid GAT code behind FIXME) - rust-lang#114630 (Migrate GUI colors test to original CSS color format) - rust-lang#114631 (add provisional cache test for new solver) r? `@ghost` `@rustbot` modify labels: rollup

…m-ou-se Improve docs for impl Default for ExitStatus This addresses a review comment in rust-lang#106425 (which is on the way to being merged I think). Some of the other followup work is more complicated so I'm going to do individual MRs. ~~Note this branch is on top of rust-lang#106425~~

Language -------- - [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.] (rust-lang/rust#111717) - [Make `noop_method_call` warn by default.] (rust-lang/rust#111916) - [Support interpolated block for `try` and `async` in macros.] (rust-lang/rust#112953) - [Make `unconditional_recursion` lint detect recursive drops.] (rust-lang/rust#113902) - [Future compatibility warning for some impls being incorrectly considered not overlapping.] (rust-lang/rust#114023) - [The `invalid_reference_casting` lint is now **deny-by-default** (instead of allow-by-default)] (rust-lang/rust#112431) Compiler -------- - [Write version information in a `.comment` section like GCC/Clang.] (rust-lang/rust#97550) - [Add documentation on v0 symbol mangling.] (rust-lang/rust#97571) - [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.] (rust-lang/rust#114562) - [Only check outlives goals on impl compared to trait.] (rust-lang/rust#109356) - [Infer type in irrefutable slice patterns with fixed length as array.] (rust-lang/rust#113199) - [Discard default auto trait impls if explicit ones exist.] (rust-lang/rust#113312) - Add several new tier 3 targets: - [`aarch64-unknown-teeos`] (rust-lang/rust#113480) - [`csky-unknown-linux-gnuabiv2`] (rust-lang/rust#113658) - [`riscv64-linux-android`] (rust-lang/rust#112858) - [`riscv64gc-unknown-hermit`] (rust-lang/rust#114004) - [`x86_64-unikraft-linux-musl`] (rust-lang/rust#113411) - [`x86_64-unknown-linux-ohos`] (rust-lang/rust#113061) - [Add `wasm32-wasi-preview1-threads` as a tier 2 target.] (rust-lang/rust#112922) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.] (rust-lang/rust#94748) - [Merge functionality of `io::Sink` into `io::Empty`.] (rust-lang/rust#98154) - [Implement `RefUnwindSafe` for `Backtrace`] (rust-lang/rust#100455) - [Make `ExitStatus` implement `Default`] (rust-lang/rust#106425) - [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`] (rust-lang/rust#111081) - [Change default panic handler message format.] (rust-lang/rust#112849) - [Cleaner `assert_eq!` & `assert_ne!` panic messages.] (rust-lang/rust#111071) - [Correct the (deprecated) Android `stat` struct definitions.] (rust-lang/rust#113130) Stabilized APIs --------------- - [Unsigned `{integer}::div_ceil`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.div_ceil) - [Unsigned `{integer}::next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of) - [Unsigned `{integer}::checked_next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of) - [`std::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html) - [`std::os::unix::fs::chown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html) - [`std::os::unix::fs::fchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html) - [`std::os::unix::fs::lfchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html) - [`LocalKey::<Cell<T>>::get`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get) - [`LocalKey::<Cell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set) - [`LocalKey::<Cell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take) - [`LocalKey::<Cell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace) - [`LocalKey::<RefCell<T>>::with_borrow`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow) - [`LocalKey::<RefCell<T>>::with_borrow_mut`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut) - [`LocalKey::<RefCell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1) - [`LocalKey::<RefCell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1) - [`LocalKey::<RefCell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1) These APIs are now stable in const contexts: - [`rc::Weak::new`] (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new) - [`sync::Weak::new`] (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new) - [`NonNull::as_ref`] (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref) Cargo ----- - [Encode URL params correctly for `SourceId` in `Cargo.lock`.] (rust-lang/cargo#12280) - [Bail out an error when using `cargo::` in custom build script.] (rust-lang/cargo#12332) Compatibility Notes ------------------- - [Update the minimum external LLVM to 15.] (rust-lang/rust#114148) - [Check for non-defining uses of return position `impl Trait`.] (rust-lang/rust#112842) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Remove LLVM pointee types, supporting only opaque pointers.] (rust-lang/rust#105545) - [Port PGO/LTO/BOLT optimized build pipeline to Rust.] (rust-lang/rust#112235) - [Replace in-tree `rustc_apfloat` with the new version of the crate.] (rust-lang/rust#113843) - [Update to LLVM 17.] (rust-lang/rust#114048) - [Add `internal_features` lint for internal unstable features.] (rust-lang/rust#108955) - [Mention style for new syntax in tracking issue template.] (rust-lang/rust#113586)

impl Default for ExitCode As suggested here rust-lang#106425 (comment) Needs FCP since this is an insta-stable impl. Ideally we would have `impl From<ExitCode> for ExitStatus` and implement the default `ExitStatus` using that. That is sadly not so easy because of the various strange confusions about `ExitCode` (unix: exit status) vs `ExitStatus` (unix: wait status) in the not-really-unix platforms in `library//src/sys/unix/process`. I'll try to follow that up.

As suggested here rust-lang#106425 (comment)

impl Default for ExitCode As suggested here rust-lang#106425 (comment) Needs FCP since this is an insta-stable impl. Ideally we would have `impl From<ExitCode> for ExitStatus` and implement the default `ExitStatus` using that. That is sadly not so easy because of the various strange confusions about `ExitCode` (unix: exit status) vs `ExitStatus` (unix: wait status) in the not-really-unix platforms in `library//src/sys/unix/process`. I'll try to follow that up.

Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * For an external LLVM, set dependency of llvm >= 15, in accordance with the upstream changes. * Add a patch with a backport from LLVM 17.0.3 fixing codegen for PPC, ref. rust-lang/rust#116845 Upstream changes: Version 1.73.0 (2023-10-05) ========================== Language -------- - [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.] (rust-lang/rust#111717) - [Make `noop_method_call` warn by default.] (rust-lang/rust#111916) - [Support interpolated block for `try` and `async` in macros.] (rust-lang/rust#112953) - [Make `unconditional_recursion` lint detect recursive drops.] (rust-lang/rust#113902) - [Future compatibility warning for some impls being incorrectly considered not overlapping.] (rust-lang/rust#114023) - [The `invalid_reference_casting` lint is now **deny-by-default** (instead of allow-by-default)] (rust-lang/rust#112431 Compiler -------- - [Write version information in a `.comment` section like GCC/Clang.] (rust-lang/rust#97550) - [Add documentation on v0 symbol mangling.] (rust-lang/rust#97571) - [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.] (rust-lang/rust#114562) - [Only check outlives goals on impl compared to trait.] (rust-lang/rust#109356) - [Infer type in irrefutable slice patterns with fixed length as array.] (rust-lang/rust#113199) - [Discard default auto trait impls if explicit ones exist.] (rust-lang/rust#113312) - Add several new tier 3 targets: - [`aarch64-unknown-teeos`] (rust-lang/rust#113480) - [`csky-unknown-linux-gnuabiv2`] (rust-lang/rust#113658) - [`riscv64-linux-android`] (rust-lang/rust#112858) - [`riscv64gc-unknown-hermit`] (rust-lang/rust#114004) - [`x86_64-unikraft-linux-musl`] (rust-lang/rust#113411) - [`x86_64-unknown-linux-ohos`] (rust-lang/rust#113061) - [Add `wasm32-wasi-preview1-threads` as a tier 2 target.] (rust-lang/rust#112922) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.] (rust-lang/rust#94748) - [Merge functionality of `io::Sink` into `io::Empty`.] (rust-lang/rust#98154) - [Implement `RefUnwindSafe` for `Backtrace`] (rust-lang/rust#100455) - [Make `ExitStatus` implement `Default`] (rust-lang/rust#106425) - [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`] (rust-lang/rust#111081) - [Change default panic handler message format.] (rust-lang/rust#112849) - [Cleaner `assert_eq!` & `assert_ne!` panic messages.] (rust-lang/rust#111071) - [Correct the (deprecated) Android `stat` struct definitions.] (rust-lang/rust#113130) Stabilized APIs --------------- - [Unsigned `{integer}::div_ceil`] (https://doc.rust-lang.org/stable/std/primitiv e.u32.html#method.div_ceil) - [Unsigned `{integer}::next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of) - [Unsigned `{integer}::checked_next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of) - [`std::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html) - [`std::os::unix::fs::chown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html) - [`std::os::unix::fs::fchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html) - [`std::os::unix::fs::lfchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html) - [`LocalKey::<Cell<T>>::get`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get) - [`LocalKey::<Cell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set) - [`LocalKey::<Cell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take) - [`LocalKey::<Cell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace) - [`LocalKey::<RefCell<T>>::with_borrow`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow) - [`LocalKey::<RefCell<T>>::with_borrow_mut`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut) - [`LocalKey::<RefCell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1) - [`LocalKey::<RefCell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1) - [`LocalKey::<RefCell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1) These APIs are now stable in const contexts: - [`rc::Weak::new`] (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new) - [`sync::Weak::new`] (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new) - [`NonNull::as_ref`] (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref) Cargo ----- - [Encode URL params correctly for `SourceId` in `Cargo.lock`.] (rust-lang/cargo#12280) - [Bail out an error when using `cargo::` in custom build script.] (rust-lang/cargo#12332) Misc ---- Compatibility Notes ------------------- - [Update the minimum external LLVM to 15.] (rust-lang/rust#114148) - [Check for non-defining uses of return position `impl Trait`.] (rust-lang/rust#112842) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Remove LLVM pointee types, supporting only opaque pointers.] (rust-lang/rust#105545) - [Port PGO/LTO/BOLT optimized build pipeline to Rust.] (rust-lang/rust#112235) - [Replace in-tree `rustc_apfloat` with the new version of the crate.] (rust-lang/rust#113843) - [Update to LLVM 17.] (rust-lang/rust#114048) - [Add `internal_features` lint for internal unstable features.] (rust-lang/rust#108955) - [Mention style for new syntax in tracking issue template.] (rust-lang/rust#113586)

Make ExitStatus an inhabited type on all platforms

2d213f7

Even where actually running processes is not supported. Needed for the next commit. The manual trait implementations now belong on ExitStatusError, which still can't exist.

rustbot assigned thomcc Jan 3, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 3, 2023

rustbot 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 Jan 3, 2023

This comment was marked as resolved.

Sign in to view

rustbot added the needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. label Jan 4, 2023

thomcc approved these changes Jan 4, 2023

View reviewed changes

thomcc added S-waiting-on-team and removed S-waiting-on-team labels Jan 4, 2023

ijackson mentioned this pull request Jan 4, 2023

impl Default for ExitStatus rust-lang/libs-team#158

Closed

thomcc added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2023

m-ou-se removed the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label Jan 10, 2023

ijackson force-pushed the exit-status-default branch from dcbd04d to 1f1d49a Compare August 7, 2023 15:36

ijackson mentioned this pull request Aug 7, 2023

Improve docs for impl Default for ExitStatus #114588

Merged

ijackson added a commit to ijackson/rust that referenced this pull request Aug 7, 2023

impl Default for ExitCode

01494da

As suggested here rust-lang#106425 (comment)

This was referenced Aug 7, 2023

impl Default for ExitCode #114589

Merged

non-Unix cfg(unix) ExitStatus is broken #114593

Closed

dtolnay approved these changes Aug 8, 2023

View reviewed changes

matthiaskrgr mentioned this pull request Aug 8, 2023

Rollup of 8 pull requests #114632

Closed

matthiaskrgr mentioned this pull request Aug 8, 2023

Rollup of 11 pull requests #114637

Merged

bors merged commit b355089 into rust-lang:master Aug 9, 2023

rustbot added this to the 1.73.0 milestone Aug 9, 2023

ijackson deleted the exit-status-default branch August 22, 2023 14:38

bors pushed a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2023

impl Default for ExitCode

b8f6d48

As suggested here rust-lang#106425 (comment)

dtolnay self-assigned this Mar 24, 2024

Uh oh!

Conversation

ijackson commented Jan 3, 2023

Uh oh!

rustbot commented Jan 3, 2023

Uh oh!

rustbot commented Jan 3, 2023

Uh oh!

ijackson commented Jan 3, 2023

Uh oh!

This comment was marked as resolved.

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

thomcc commented Jan 4, 2023

Uh oh!

ijackson commented Jan 4, 2023

Uh oh!

m-ou-se commented Jan 10, 2023

Uh oh!

rfcbot commented Jan 10, 2023 • edited by joshtriplett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfcbot commented Jan 10, 2023

Uh oh!

scottmcm commented Jan 14, 2023

Uh oh!

scottmcm commented Jan 14, 2023

Uh oh!

ijackson commented Jan 19, 2023

Uh oh!

ijackson commented Aug 7, 2023

Uh oh!

ijackson commented Aug 8, 2023

Uh oh!

dtolnay commented Aug 8, 2023

Uh oh!

bors commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

rfcbot commented Jan 10, 2023 •

edited by joshtriplett

Loading