Skip to content

impl PartialEq<ErrorKind> for io::Error #93100

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

Conversation

ChrisDenton
Copy link
Member

This is an ergonomic improvement that allows directly testing io::Error against io::ErrorKind instead of needing to call kind(). Inspired by #93090.

r? @joshtriplett

This is a minor ergonomic improvement that allows directly testing for equality instead of needing to call `kind()`.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 20, 2022
@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Jan 20, 2022
@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 21, 2022

Team member @joshtriplett 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 21, 2022
@joshtriplett joshtriplett added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 21, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 21, 2022

I'm not convinced it makes sense to consider an error and the wider category it falls into as 'equal'. This seems similar to adding PartialEq<FpCategory> for f64 to allow 1.0 == FpCategory::Normal.

Being able to skip .kind() doesn't seem that useful, and skipping that method call hides that an io::Error contains more information than just the io::ErrorKind.

(The PartialEq documentation even warns against "a comparison like the one above, which ignores some fields of the struct".)

@rfcbot concern eq-to-wider-category

@joshtriplett
Copy link
Member

@m-ou-se True, but those additional fields (like the string) are for human consumption and printing, not typically for machine consumption. That seems like a notable difference.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 21, 2022

those additional fields (like the string) are for human consumption and printing, not typically for machine consumption.

That's not true for .raw_os_error() and also not for downcasting the dyn Error from .get_ref()/.into_inner().

@ChrisDenton
Copy link
Member Author

Hm, the way I thought of it is that io::Error is an ErrorKind with the next error in the chain being either an OS error code or a dyn Error. Not sure if that's the right mental model though.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 21, 2022

Hm, the way I thought of it is that io::Error is an ErrorKind with the next error in the chain being either an OS error code or a dyn Error. Not sure if that's the right mental model though.

The wrapped error (whether an error code from the operating system or a custom dyn Error) is not used as the 'source' of the io::Error. The io::Error directly represents that error. .source() forwards to .source() on the dyn Error, not to the dyn Error itself:

fn source(&self) -> Option<&(dyn error::Error + 'static)> {
match self.repr {
Repr::Os(..) => None,
Repr::Simple(..) => None,
Repr::SimpleMessage(..) => None,
Repr::Custom(ref c) => c.error.source(),
}
}

@joshtriplett
Copy link
Member

those additional fields (like the string) are for human consumption and printing, not typically for machine consumption.

That's not true for .raw_os_error()

That's quite convincing, thank you.

This would be convenient, but I agree that it may not be the right solution. In particular, if we ever added a PartialEq between io::Error instances, err == some_kind might not match err == io::Error::from(some_kind).

@joshtriplett
Copy link
Member

@rfcbot cancel

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 21, 2022

@joshtriplett proposal cancelled.

@rfcbot rfcbot removed 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 21, 2022
@ChrisDenton
Copy link
Member Author

Yeah I'll close this now, thanks!

@ChrisDenton ChrisDenton deleted the io-error-eq-errorkind branch January 21, 2022 17:30
@joshtriplett
Copy link
Member

@ChrisDenton Thank you for trying the experiment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

5 participants