-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 From<bool> for int types #50554
Add From<bool> for int types #50554
Conversation
r? @TimNN (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Actually, apparently this causes inference issues. That should probably be investigated before this is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn try_result_ok() -> Result<u8, u8> {
let val = Ok(1)?;
Ok(val)
}
Ok(x)?
for Result<_, u8>
(as well as variations like return Err(u8::from(unimplemented!()));
) sound like an edge case for me - I don't think realistically anyone would write code like this. This can be written as x
, and I think this inference failure is not a practical issue. The test here could be changed to use a type other than u8
, for example fn try_result_ok() -> Result<u8, bool>
.
Additionally, according to https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md, this would be a minor change.
cc @rust-lang/libs: This has the potential to be inference-breaking. What do you think? Does this need a crater run? |
This comment has been minimized.
This comment has been minimized.
@SimonSapin The motivation for this is automatically generated code by |
Also, the main purpose of the lint is that in general, |
Yeah I think it wouldn't hurt to get a crater run for this. cc @rust-lang/infra, mind scheduling a crater run here? |
@bors try |
⌛ Trying commit a882197034b9b3eafb5e35c3f010d21fca0e0df4 with merge 4b647638d12b7679c1eee120c4c064bc749eb782... |
☀️ Test successful - status-travis |
Check-only crater run started. |
Hi @alexcrichton (crater requester), @TimNN (PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-50554/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file. (interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious) |
And both of the regressions are spurious |
This looks good to go, @clarcharr can you fix the travis failure?
|
Since adding impls is a stabilization, doesn't this need an FCP? |
🔒 Merge conflict |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
I rebased but there weren't actually any merge conflicts. |
@bors r+ |
📌 Commit b1797d5 has been approved by |
Add From<bool> for int types Fixes #46109.
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Add From<bool> for int types Fixes #46109.
☀️ Test successful - status-appveyor, status-travis |
Fixes #46109.