Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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, or significant enough to need a team 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!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
needs-fcp This change is insta-stable, or significant enough to need a team 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