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

GuillaumeGomez · 2018-02-06T13:57:50Z

Fixes #44201.

rust-highfive · 2018-02-06T13:58:02Z

r? @pnkfelix

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

estebank

r=me after addressing comments.

estebank · 2018-02-06T23:46:06Z

src/librustc_lint/types.rs

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


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".

estebank · 2018-02-06T23:46:30Z

src/test/ui/cast_char.rs

+#![deny(overflowing_literals)]
+
+fn main() {
+    const XYZ: char = 0xEF8888 as char;


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

GuillaumeGomez · 2018-02-08T21:50:12Z

Updated.

estebank

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}'`

estebank · 2018-02-08T21:56:29Z

src/librustc_lint/types.rs

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


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

estebank · 2018-02-08T21:57:20Z

src/librustc_lint/types.rs

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


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.

estebank · 2018-02-08T21:58:48Z

src/librustc_lint/types.rs

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


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

estebank · 2018-02-08T22:01:36Z

src/test/ui/cast_char.rs

+#![deny(overflowing_literals)]
+
+fn main() {
+    const XYZ: char = 0x1F888 as char;


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 · 2018-02-10T14:36:28Z

Updated.

estebank · 2018-02-10T23:13:38Z

@bors r+ rollup

bors · 2018-02-10T23:13:39Z

📌 Commit 0cccd9a has been approved by estebank

…sage, r=estebank Show better warning for trying to cast non-u8 scalar to char Fixes rust-lang#44201.

Rollup of 8 pull requests - Successful merges: #47846, #48033, #48087, #48114, #48126, #48130, #48133, #48151 - Failed merges:

…sage, r=estebank Show better warning for trying to cast non-u8 scalar to char Fixes rust-lang#44201.

Rollup of 14 pull requests - Successful merges: #47784, #47846, #48033, #48083, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48163, #48165, #48167 - Failed merges:

…sage, r=estebank Show better warning for trying to cast non-u8 scalar to char Fixes rust-lang#44201.

Rollup of 22 pull requests - Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035 - Failed merges:

Rollup of 23 pull requests - Successful merges: #47784, #47806, #47846, #48005, #48033, #48065, #48087, #48114, #48126, #48130, #48133, #48151, #48154, #48156, #48162, #48163, #48165, #48167, #48181, #48186, #48195, #48035, #48210 - Failed merges:

lukaslueg · 2018-02-18T19:39:49Z

src/librustc_lint/types.rs

+                                        let mut err = cx.struct_span_lint(
+                                                             OVERFLOWING_LITERALS,
+                                                             parent_expr.span,
+                                                             "only u8 can be casted into char");


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

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

rust-highfive assigned pnkfelix Feb 6, 2018

estebank requested changes Feb 6, 2018

View reviewed changes

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 force-pushed the better-char-cast-message branch from 09f038d to d665ea8 Compare February 8, 2018 21:50

estebank reviewed Feb 8, 2018

View reviewed changes

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

0cccd9a

GuillaumeGomez force-pushed the better-char-cast-message branch from d665ea8 to 0cccd9a Compare February 10, 2018 14:36

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

estebank approved these changes Feb 10, 2018

View reviewed changes

kennytm added a commit to kennytm/rust that referenced this pull request Feb 12, 2018

Rollup merge of rust-lang#48033 - GuillaumeGomez:better-char-cast-mes…

c268e6f

…sage, r=estebank Show better warning for trying to cast non-u8 scalar to char Fixes rust-lang#44201.

kennytm mentioned this pull request Feb 12, 2018

Rollup of 8 pull requests #48158

Closed

bors added a commit that referenced this pull request Feb 12, 2018

Auto merge of #48158 - kennytm:rollup, r=kennytm

710e972

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

Rollup merge of rust-lang#48033 - GuillaumeGomez:better-char-cast-mes…

9cf75c2

…sage, r=estebank Show better warning for trying to cast non-u8 scalar to char Fixes rust-lang#44201.

kennytm mentioned this pull request Feb 13, 2018

Rollup of 14 pull requests #48177

Closed

kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018

Rollup merge of rust-lang#48033 - GuillaumeGomez:better-char-cast-mes…

a5c3209

…sage, r=estebank Show better warning for trying to cast non-u8 scalar to char Fixes rust-lang#44201.

kennytm mentioned this pull request Feb 14, 2018

Rollup of 23 pull requests #48203

Merged

kennytm merged commit 0cccd9a into rust-lang:master Feb 15, 2018

GuillaumeGomez deleted the better-char-cast-message branch February 15, 2018 17:43

lukaslueg reviewed Feb 18, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

GuillaumeGomez commented Feb 6, 2018

rust-highfive commented Feb 6, 2018

estebank left a comment

estebank Feb 6, 2018

estebank Feb 6, 2018

GuillaumeGomez commented Feb 8, 2018

estebank left a comment

estebank Feb 8, 2018

estebank Feb 8, 2018

estebank Feb 8, 2018

estebank Feb 8, 2018

GuillaumeGomez commented Feb 10, 2018

estebank commented Feb 10, 2018

bors commented Feb 10, 2018

lukaslueg Feb 18, 2018

GuillaumeGomez Feb 18, 2018

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

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

Conversation

GuillaumeGomez commented Feb 6, 2018

rust-highfive commented Feb 6, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 8, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 10, 2018

estebank commented Feb 10, 2018

bors commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment