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

Show better warning for trying to cast non-u8 scalar to char #48033

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

GuillaumeGomez
Copy link
Member

Fixes #44201.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

r=me after addressing comments.

OVERFLOWING_LITERALS,
e.span,
"only u8 can be casted into char");
err.help(&format!("try with `'\\u{:X}' as char`", lit_val));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a span_suggestion instead? Specially so that the help doesn't appear after the "lint defined here" span. Also, that way it can be easily applied by RLS enabled tools.

You don't need to cast (as char) when you have a literal char (\uEF8888).

I would probably word this as: "instead of casting this numeric literal, use a char literal".

#![deny(overflowing_literals)]

fn main() {
const XYZ: char = 0xEF8888 as char;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add test line using a non-hex, non-u8 numeric literal.

@estebank estebank added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 6, 2018
@GuillaumeGomez
Copy link
Member Author

Updated.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Love the PR!

I left a few more changes that I'd like to see before merging. By doing the suggested changes, the output would be:

14 |     const XYZ: char = 0x1F888 as char;
   |                       ^^^^^^^-------- help: use a char literal instead: `'/u{1F888}'`

err.span_suggestion(e.span,
&format!("try with `'\\u{{{:X}}}'`",
lit_val),
"".to_owned());
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be format!("'\\u{{{:X}}}'", lit_val). Otherwise when tools try to apply this will remove the code from the users file.

OVERFLOWING_LITERALS,
e.span,
"only u8 can be casted into char");
err.span_suggestion(e.span,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a span enclosing all of lit as char (probably the parent_expr's span). If you don't want to have overlapping spans, use the same span in the DiagnosticBuilder and for the suggestion.

e.span,
"only u8 can be casted into char");
err.span_suggestion(e.span,
&format!("try with `'\\u{{{:X}}}'`",
Copy link
Contributor

Choose a reason for hiding this comment

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

The text could be something along the lines of: "use a char literal instead".

#![deny(overflowing_literals)]

fn main() {
const XYZ: char = 0x1F888 as char;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add another case where the numeric literal is not formatted in hexadecimal. That way we can verify that the suggestion will be correct, even if the literal was originally decimal.

@GuillaumeGomez
Copy link
Member Author

Updated.

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 10, 2018

📌 Commit 0cccd9a has been approved by estebank

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Feb 12, 2018
…sage, r=estebank

Show better warning for trying to cast non-u8 scalar to char

Fixes rust-lang#44201.
bors added a commit that referenced this pull request Feb 12, 2018
Rollup of 8 pull requests

- Successful merges: #47846, #48033, #48087, #48114, #48126, #48130, #48133, #48151
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 13, 2018
…sage, r=estebank

Show better warning for trying to cast non-u8 scalar to char

Fixes rust-lang#44201.
bors added a commit that referenced this pull request Feb 13, 2018
Rollup of 14 pull requests

- Successful merges: #47784, #47846, #48033, #48083, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48163, #48165, #48167
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018
…sage, r=estebank

Show better warning for trying to cast non-u8 scalar to char

Fixes rust-lang#44201.
bors added a commit that referenced this pull request Feb 14, 2018
bors added a commit that referenced this pull request Feb 15, 2018
bors added a commit that referenced this pull request Feb 15, 2018
@kennytm kennytm merged commit 0cccd9a into rust-lang:master Feb 15, 2018
@GuillaumeGomez GuillaumeGomez deleted the better-char-cast-message branch February 15, 2018 17:43
let mut err = cx.struct_span_lint(
OVERFLOWING_LITERALS,
parent_expr.span,
"only u8 can be casted into char");
Copy link
Contributor

Choose a reason for hiding this comment

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

It should read "can be cast into char", not "casted"

Copy link
Member Author

Choose a reason for hiding this comment

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

Please open an issue for it or fix the spelling directly. :) (good catch!)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants