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

Suggestion for type mismatch when we need a u8 but the programmer wrote a char literal #106859

Merged
merged 1 commit into from
Jan 15, 2023

Conversation

tialaramex
Copy link
Contributor

Today Rust just points out that we have a char and we need a u8, but if I wrote 'A' then I could fix this by just writing b'A' instead. This code should detect the case where we're about to report a type mismatch of this kind, and the programmer wrote a char literal, and the char they wrote is ASCII, so therefore just prefixing b to make a byte literal will do what they meant.

I have definitely written this mistake more than once, it's not difficult to figure out what to do, but the compiler might as well tell us anyway.

I provided a test with two simple examples where the suggestion is appropriate, and one where it is not because the char literal is not ASCII, showing that the suggestion is only triggered in the former cases.

I have contributed only a small typo doc fix before, so this is my first substantive rustc change.

@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2023

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 14, 2023
@tialaramex
Copy link
Contributor Author

Aha, I need to update the tests as well, working on that now.

@Noratrieb
Copy link
Member

r? Nilstrieb
Thanks, that looks great! One last thing: could you squash your commits into one? I'll approve it afterwards. If you need help doing that, feel free to ask here

@rustbot rustbot assigned Noratrieb and unassigned eholk Jan 14, 2023
suggested by Nilstrieb

Co-authored-by: nils <48135649+Nilstrieb@users.noreply.github.com>
@Noratrieb
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 14, 2023

📌 Commit 130d02b has been approved by Nilstrieb

It is now in the queue for this repository.

@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 Jan 14, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 15, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#106072 (fix: misleading "add dyn keyword before derive macro" suggestion)
 - rust-lang#106859 (Suggestion for type mismatch when we need a u8 but the programmer wrote a char literal)
 - rust-lang#106863 (Remove various double spaces in compiler source comments.)
 - rust-lang#106865 (Add explanation comment for GUI test)
 - rust-lang#106867 (Fix the stability attributes for `std::os::fd`.)
 - rust-lang#106878 (Add regression test for rust-lang#92157)
 - rust-lang#106879 (Add regression test for rust-lang#42114)
 - rust-lang#106880 (doc: fix typo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 980bf19 into rust-lang:master Jan 15, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 15, 2023
@ayosec
Copy link
Contributor

ayosec commented Feb 1, 2023

The suggestion is broken if the character is non-ASCII, but the sequence starts with \.

For example, the following code (playground):

fn f(_c: u8) {}

fn main() {
    f('\u{1234}');
}

Produces this message:

help: if you meant to write a byte literal, prefix with `b`
  |
4 |     f(b'\u{1234}');
  |       ~~~~~~~~~~~

But the proposed change does not compile:

  |
4 |     f(b'\u{1234}');
  |         ^^^^^^^^ unicode escape in byte string
  |

Sorry for writing this in a closed merge-request (I just saw this patch in a HNews thread). I can open a new issue if necessary.

@tialaramex
Copy link
Contributor Author

Rats. I will look at whether I can improve this code but likely not before this weekend. Knew I should have checked an escaped literal like this :/

@tialaramex
Copy link
Contributor Author

#107709 proposed, thanks again ayosec for spotting the problem

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2023
Fix problem noticed in PR106859 with char -> u8 suggestion

HN reader `@ayosec` noticed that my rust-lang#106859 a few weeks back, malfunctions if you have a Unicode escape, the code suggested b'\u{0}' if you tried to use '\u{0}' where a byte should be, when of course b'\u{0}' is not a byte literal, regardless of the codepoint you can't write Unicode escapes in a byte literal at all.

My proposed fix here just checks that the "character" you wrote is fewer than 5 bytes, thus allowing \x7F and similar escapes but conveniently forbidding even the smallest Unicode escape \u{0} before offering the suggestion as before.

I have provided an updated test which includes examples which do and don't work because of this additional rule.
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.

6 participants