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

suggest casting between i/u32 and char #91245

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

cameron1024
Copy link
Contributor

As discussed in #91063 , this adds a suggestion for converting between i32/u32 <-> char with as, and a short explanation for why this is safe

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nagisa (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2021
|
help: you can cast a `u32` to a `char`, since a `char` always occupies 4 bytes
|
LL | foo::<char>(0u32 as char);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alas, casting integers to chars this way is not allowed, because not every value representable by an integer is a valid char.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, an appropriate cast here might be char::try_from(0u32).expect("not a valid char") if we want to give such a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah didn't realise that it was fallible, I'll change this PR to only suggest the safe char -> {int} conversion 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth leaving the foo::<char>(0u32) case in the UI test to capture the fact that it doesn't suggest 0u32 as char?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Negative test cases are always great to have.

@cameron1024 cameron1024 force-pushed the suggest-i32-u32-char-cast branch 2 times, most recently from d15b4d5 to bd4966b Compare November 28, 2021 01:23
Comment on lines 1176 to 1177
(&ty::Uint(ref exp), &ty::Char) => {
let cast_infallible = exp.bit_width().map(|w| w >= 32).unwrap_or(false);
Copy link
Member

@nagisa nagisa Nov 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably better written as below to allow fallthrough to the other cases if added in the future

(&ty::Uint(ref exp), &ty::Char) if exp.bit_width() >= Some(32) => {...}

One other thing that is quite unfortunate is the duplication between match arms, but I don't see an easy way to avoid it outside of listing all of the variants. Perhaps that would be better either way, e.g. something like the snippet below doesn't seem all that verbose:

( ty::Uint(ty::UintTy::U32 | ty::UintTy::U64 | ty::UintTy::U128) 
  | ty::Int(ty::IntTy::I32, ty::IntTy::I64, ty::IntTy::I128), &ty::Char)

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 1, 2021
@nagisa
Copy link
Member

nagisa commented Dec 8, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 8, 2021

📌 Commit 37ca2eb has been approved by nagisa

@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 Dec 8, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 8, 2021
…st, r=nagisa

suggest casting between i/u32 and char

As discussed in rust-lang#91063 , this adds a suggestion for converting between i32/u32 <-> char with `as`, and a short explanation for why this is safe
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2021
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#91245 (suggest casting between i/u32 and char)
 - rust-lang#91337 (Add a suggestion if `macro_rules` is misspelled)
 - rust-lang#91534 (Make rustdoc headings black, and markdown blue)
 - rust-lang#91637 (Add test for packed drops in generators)
 - rust-lang#91667 (Fix indent of itemTypes in search.js)

Failed merges:

 - rust-lang#91568 (Pretty print break and continue without redundant space)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2411cd7 into rust-lang:master Dec 9, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants