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

Add Unsupported to std::io::ErrorKind #78880

Merged
merged 8 commits into from
Apr 18, 2021
Merged

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Nov 8, 2020

I noticed a significant portion of the uses of ErrorKind::Other in std is for unsupported operations.
The notion that a specific operation is not available on a target (and will thus never succeed) seems semantically distinct enough from just "an unspecified error occurred", which is why I am proposing to add the variant Unsupported to std::io::ErrorKind.

Implementation:

The following variant will be added to std::io::ErrorKind:

/// This operation is unsupported on this platform.
Unsupported

std::io::ErrorKind::Unsupported is an error returned when a given operation is not supported on a platform, and will thus never succeed; there is no way for the software to recover. It will be used instead of Other where appropriate, e.g. on wasm for file and network operations.

decode_error_kind will be updated to decode operating system errors to Unsupported:

  • Unix and VxWorks: libc::ENOSYS
  • Windows: c::ERROR_CALL_NOT_IMPLEMENTED
  • WASI: wasi::ERRNO_NOSYS

Stability:
This changes the kind of error returned by some functions on some platforms, which I think is not covered by the stability guarantees of the std? User code could depend on this behavior, expecting ErrorKind::Other, however the docs already mention:

Errors that are Other now may move to a different or a new ErrorKind variant in the future. It is not recommended to match an error against Other and to expect any additional characteristics, e.g., a specific Error::raw_os_error return value.

The most recent variant added to ErrorKind was UnexpectedEof in 1.6.0 (almost 5 years ago), but ErrorKind is marked as #[non_exhaustive] and the docs warn about exhaustively matching on it, so adding a new variant per se should not be a breaking change.

The variant Unsupported itself could be marked as #[unstable], however, because this PR also immediately uses this new variant and changes the errors returned by functions I'm inclined to agree with the others in this thread that the variant should be insta-stabilized.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(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 Nov 8, 2020
@jyn514 jyn514 added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 8, 2020
@scottmcm
Copy link
Member

scottmcm commented Nov 8, 2020

cc @rust-lang/libs explicitly for this -- this might need to go in insta-stable, since while the variant can be marked unstable, the change to stop emitting the previous one would not be feature-gated.

@the8472
Copy link
Member

the8472 commented Nov 8, 2020

Should decode_error_kind be extended to translate any of libc::{ENOSYS,ENOTSUP,EOPNOTSUPP} to this ErrorKind?

@CDirkx
Copy link
Contributor Author

CDirkx commented Nov 8, 2020

I updated decode_error_kind to support NotSupported on various platforms:

  • Unix and VxWorks: libc::{ENOSYS, ENOTSUP, EOPNOTSUPP}
  • Windows: c::{ERROR_CALL_NOT_IMPLEMENTED, E_NOTIMPL}
  • CloudABI: abi::errno::{NOSYS, NOTSUP} (has no OPNOTSUPP)
  • WASI: wasi::{ERRNO_NOSYS, ERRNO_NOSUP} (has no OPNOTSUPP)
  • Hermit: (has hard coded error numbers, I could not find a relevant error number)
  • SGX: (has no equivalent error in fortanix_sgx_abi::Error)

I didn't include errors like EAFNOSUPPORT (address family not supported) and EPROTONOSUPPORT (protocol not supported) for now.

The implementation for unsupported targets is currently:

pub fn decode_error_kind(_code: i32) -> crate::io::ErrorKind {
    crate::io::ErrorKind::Other
}

which still seems fine to me.

@m-ou-se
Copy link
Member

m-ou-se commented Nov 9, 2020

user code could depend on this behavior, expecting ErrorKind::Other

The documentation fortunately already notes that these kind of changes might happen in the future:

Errors that are Other now may move to a different or a new ErrorKind variant in the future. It is not recommended to match an error against Other and to expect any additional characteristics, e.g., a specific Error::raw_os_error return value.

(In hindsight it might have been better if matching 'other' errors against ErrorKind::Other was not possible. E.g. by using an unstable/doc(hidden) variant for those. But I guess it's too late for that now.)

I'm not sure if adding an #[unstable] variant to this stable enum is a good idea. Especially as these errors used a #[stable] variant before, like @scottmcm mentioned.

CloudABI: abi::errno::{NOSYS, NOTSUP} (has no OPNOTSUPP)

Feel free to ignore CloudABI, as that platform itself will be NOTSUP soon: #78439

@KodrAus
Copy link
Contributor

KodrAus commented Nov 9, 2020

I can see the value in wanting to check whether or not an IO operation is supported so you can fallback to alternatives.

I also think we should consider marking this as insta-stable if we want to add it. In either case I think an FCP would probably be a good idea before we land it.

@CDirkx
Copy link
Contributor Author

CDirkx commented Nov 13, 2020

I have updated the PR to immediately stabilize NotSupported and updated the description to include the information inthis thread.

@shepmaster
Copy link
Member

r? @KodrAus

@rust-highfive rust-highfive assigned KodrAus and unassigned shepmaster Nov 17, 2020
@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2020
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2021
@KodrAus
Copy link
Contributor

KodrAus commented Jan 14, 2021

@rfcbot fcp merge

This proposes adding a NotSupported variant to io::ErrorKind that's used when an operation isn't supported. Callers may want to try falling back to alternatives or bubble up.

@rfcbot
Copy link

rfcbot commented Jan 14, 2021

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

Concerns:

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 Jan 14, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 18, 2021

Rebased and fixed the clippy error, should be ready to try merging again.

@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Apr 18, 2021

📌 Commit 5b5afae has been approved by joshtriplett

@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 18, 2021
Add `Unsupported` to `std::io::ErrorKind`

I noticed a significant portion of the uses of `ErrorKind::Other` in std is for unsupported operations.
The notion that a specific operation is not available on a target (and will thus never succeed) seems semantically distinct enough from just "an unspecified error occurred", which is why I am proposing to add the variant `Unsupported` to `std::io::ErrorKind`.

**Implementation**:

The following variant will be added to `std::io::ErrorKind`:

```rust
/// This operation is unsupported on this platform.
Unsupported
```
`std::io::ErrorKind::Unsupported` is an error returned when a given operation is not supported on a platform, and will thus never succeed; there is no way for the software to recover. It will be used instead of `Other` where appropriate, e.g. on wasm for file and network operations.

`decode_error_kind` will be updated  to decode operating system errors to `Unsupported`:
- Unix and VxWorks: `libc::ENOSYS`
- Windows: `c::ERROR_CALL_NOT_IMPLEMENTED`
- WASI: `wasi::ERRNO_NOSYS`

**Stability**:
This changes the kind of error returned by some functions on some platforms, which I think is not covered by the stability guarantees of the std? User code could depend on this behavior, expecting `ErrorKind::Other`, however the docs already mention:

> Errors that are `Other` now may move to a different or a new `ErrorKind` variant in the future. It is not recommended to match an error against `Other` and to expect any additional characteristics, e.g., a specific `Error::raw_os_error` return value.

The most recent variant added to `ErrorKind` was `UnexpectedEof` in `1.6.0` (almost 5 years ago), but `ErrorKind` is marked as `#[non_exhaustive]` and the docs warn about exhaustively matching on it, so adding a new variant per se should not be a breaking change.

The variant `Unsupported` itself could be marked as `#[unstable]`, however, because this PR also immediately uses this new variant and changes the errors returned by functions I'm inclined to agree with the others in this thread that the variant should be insta-stabilized.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 18, 2021
Add `Unsupported` to `std::io::ErrorKind`

I noticed a significant portion of the uses of `ErrorKind::Other` in std is for unsupported operations.
The notion that a specific operation is not available on a target (and will thus never succeed) seems semantically distinct enough from just "an unspecified error occurred", which is why I am proposing to add the variant `Unsupported` to `std::io::ErrorKind`.

**Implementation**:

The following variant will be added to `std::io::ErrorKind`:

```rust
/// This operation is unsupported on this platform.
Unsupported
```
`std::io::ErrorKind::Unsupported` is an error returned when a given operation is not supported on a platform, and will thus never succeed; there is no way for the software to recover. It will be used instead of `Other` where appropriate, e.g. on wasm for file and network operations.

`decode_error_kind` will be updated  to decode operating system errors to `Unsupported`:
- Unix and VxWorks: `libc::ENOSYS`
- Windows: `c::ERROR_CALL_NOT_IMPLEMENTED`
- WASI: `wasi::ERRNO_NOSYS`

**Stability**:
This changes the kind of error returned by some functions on some platforms, which I think is not covered by the stability guarantees of the std? User code could depend on this behavior, expecting `ErrorKind::Other`, however the docs already mention:

> Errors that are `Other` now may move to a different or a new `ErrorKind` variant in the future. It is not recommended to match an error against `Other` and to expect any additional characteristics, e.g., a specific `Error::raw_os_error` return value.

The most recent variant added to `ErrorKind` was `UnexpectedEof` in `1.6.0` (almost 5 years ago), but `ErrorKind` is marked as `#[non_exhaustive]` and the docs warn about exhaustively matching on it, so adding a new variant per se should not be a breaking change.

The variant `Unsupported` itself could be marked as `#[unstable]`, however, because this PR also immediately uses this new variant and changes the errors returned by functions I'm inclined to agree with the others in this thread that the variant should be insta-stabilized.
@bors
Copy link
Contributor

bors commented Apr 18, 2021

⌛ Testing commit 5b5afae with merge 0052f444a7935147d31baa80099dbdfb1140255d...

@rust-log-analyzer
Copy link
Collaborator

The job dist-x86_64-apple-alt failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
      Memory: 14 GB
      Boot ROM Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VMdSnO8Wr7RX

hw.ncpu: 3
hw.byteorder: 1234
hw.memsize: 15032385536

@bors
Copy link
Contributor

bors commented Apr 18, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 18, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 18, 2021

error: failed to download from `https://crates.io/api/v1/crates/minifier/0.0.39/download`
failed to run: /Users/runner/work/rust/rust/build/bootstrap/debug/bootstrap dist

Caused by:
  failed to get 200 response from `https://crates.io/api/v1/crates/minifier/0.0.39/download`, got 503

@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2021
@bors
Copy link
Contributor

bors commented Apr 18, 2021

⌛ Testing commit 5b5afae with merge 5a4ab26...

@bors
Copy link
Contributor

bors commented Apr 18, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 5a4ab26 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 18, 2021
@bors bors merged commit 5a4ab26 into rust-lang:master Apr 18, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 18, 2021
@CDirkx CDirkx deleted the not_supported branch April 18, 2021 22:46
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 22, 2021
Add `Unsupported` to `std::io::ErrorKind`

I noticed a significant portion of the uses of `ErrorKind::Other` in std is for unsupported operations.
The notion that a specific operation is not available on a target (and will thus never succeed) seems semantically distinct enough from just "an unspecified error occurred", which is why I am proposing to add the variant `Unsupported` to `std::io::ErrorKind`.

**Implementation**:

The following variant will be added to `std::io::ErrorKind`:

```rust
/// This operation is unsupported on this platform.
Unsupported
```
`std::io::ErrorKind::Unsupported` is an error returned when a given operation is not supported on a platform, and will thus never succeed; there is no way for the software to recover. It will be used instead of `Other` where appropriate, e.g. on wasm for file and network operations.

`decode_error_kind` will be updated  to decode operating system errors to `Unsupported`:
- Unix and VxWorks: `libc::ENOSYS`
- Windows: `c::ERROR_CALL_NOT_IMPLEMENTED`
- WASI: `wasi::ERRNO_NOSYS`

**Stability**:
This changes the kind of error returned by some functions on some platforms, which I think is not covered by the stability guarantees of the std? User code could depend on this behavior, expecting `ErrorKind::Other`, however the docs already mention:

> Errors that are `Other` now may move to a different or a new `ErrorKind` variant in the future. It is not recommended to match an error against `Other` and to expect any additional characteristics, e.g., a specific `Error::raw_os_error` return value.

The most recent variant added to `ErrorKind` was `UnexpectedEof` in `1.6.0` (almost 5 years ago), but `ErrorKind` is marked as `#[non_exhaustive]` and the docs warn about exhaustively matching on it, so adding a new variant per se should not be a breaking change.

The variant `Unsupported` itself could be marked as `#[unstable]`, however, because this PR also immediately uses this new variant and changes the errors returned by functions I'm inclined to agree with the others in this thread that the variant should be insta-stabilized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` 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. merged-by-bors This PR was explicitly merged by bors. 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.