Suggest type for overflowing bin/hex-literals #48432

flip1995 · 2018-02-22T15:17:09Z

For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this comment.

Additionally it will suggest a type (X < Y):

iX: if literal fits in uX => uX, else => iY
-iX => iY
uX => uY

Exceptions: isize, usize. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case.

r? @oli-obk

rust-highfive · 2018-02-22T15:17:24Z

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

oli-obk

The final error message output looks good to me

oli-obk · 2018-02-22T15:38:17Z

src/librustc_lint/types.rs

+                                        if bits < 128 {
+                                            // v & 0b0..01..1, |1| = bits
+                                            let trimmed = v & ((1 << bits) - 1);
+                                            actually = if v & (1 << (bits - 1)) == 0 {


You can simplify these conversions by first casting to i128 and then bitshifting by 128 - bits to the left and then to the right.

oli-obk · 2018-02-22T15:40:37Z

src/librustc_lint/types.rs

-                                    ast::FloatTy::F64 => v.as_str().parse().map(f64::is_infinite),
-                                }
-                            }
+                            ast::LitKind::Float(v, _) | ast::LitKind::FloatUnsuffixed(v) => match t


unrelated formatting change

oli-obk · 2018-02-22T15:40:50Z

src/librustc_lint/types.rs

-                            cx.span_lint(OVERFLOWING_LITERALS,
-                                         e.span,
-                                         &format!("literal out of range for {:?}", t));
+                            cx.span_lint(


unrelated formatting change

oli-obk · 2018-02-22T15:41:59Z

src/librustc_lint/types.rs

@@ -340,6 +421,69 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TypeLimits {
                _ => false,
            }
        }
+
+        fn get_bin_hex_repr(cx: &LateContext, lit: &ast::Lit) -> Option<String> {
+            if let Some(src) = cx.sess().codemap().span_to_snippet(lit.span).ok() {


you can rewrite this entire function with the ? operator instead of if let

oli-obk · 2018-02-22T15:44:30Z

src/librustc_lint/types.rs

+                                err.emit();
+                                return;
+                            }
+                            cx.span_lint(


It feels like there's a lot of duplication between the error reporting of signed and unsigned integers. Is it possible to merge them into a function?

I think changing the function get_fitting_type(), that it returns the suggestion string directly is the best approach here.
~~An extra function for the err.note() part is hard, because we need the actually value as i128 in the singed case and as u128 in the unsigned case. Same problem with the Type.~~

Edit: Actually, I will rewrite the error reporting while adding the span suggestion @estebank requested.

mattico · 2018-02-22T16:11:06Z

src/librustc_lint/types.rs

+                ($ty:expr, $val:expr, $negative:expr,
+                 $($type:ident => [$($utypes:expr),*] => [$($itypes:expr),*]),+) => {
+                    {
+                        let _neg = if negative { 1 } else { 0 };


nit: preceding underscores suppress dead code warnings, so I'd avoid them if that is not intended.

Also the PR has a succint description for what this function does, which you could copy into a comment.

The underscore is intended: for finding a fitting unsigned type, we don't look at integer types so the expanded macro never uses _neg in this case. So the line

if let &ty::TyUint(u) = t { return find_fit!(u, val, negative ...}

gives a warning. If there is a better way to do this, it would be nice to know :)

And yeah maybe I should have wrote a few more comments. Will do this, while implementing the other suggestions

estebank

Looks great!

Could you address @oli-obk's and my comments?

estebank · 2018-02-23T00:08:26Z

src/librustc_lint/types.rs

+        //  - `uX` => `uY`
+        //
+        // No suggestion for: `isize`, `usize`.
+        fn get_type_suggestion<'a>(t: &ty::TypeVariants, val: u128, negative: bool) -> String {


Could get_type_suggestion return an Option<String> instead? That way you communicate intent that the empty String doesn't.

estebank · 2018-02-23T00:10:27Z

src/librustc_lint/types.rs

+                        match $ty {
+                            $($type => {
+                                $(if !negative && val <= uint_ty_range($utypes).1 {
+                                    return format!("Consider using `{:?}`", $utypes)


use leading lowercase here and below

"consider using {:?} instead"

estebank · 2018-02-23T00:11:44Z

src/librustc_lint/types.rs

+                                    false,
+                                );
+                                if !sugg_ty.is_empty() {
+                                    err.help(&sugg_ty);


It'd be nice if this were a span_suggestion instead if it is a numeric literal that sets the type, otherwise provide the help (ideally I would check the source of the type constraint and point at it, but it is not necessary to go down that rabbit hole in this PR):

warning: literal out of range for i8 --> $DIR/type-overflow.rs:34:17 | 34 | let fail = -0b1111_1111i8; | ^^^^^^^^^^^^^ help: consider using `i16` instead: `-0b1111_1111i16` | = note: the literal `0b1111_1111i8` (decimal `255`) does not fit into an `i8` and will become `-1i8`

This looks better. I will add this, this evening.

estebank · 2018-02-23T00:12:07Z

src/librustc_lint/types.rs

+                                );
+                                err.note(&format!(
+                                    "the literal `{}` (decimal `{}`) does not fit into \
+                                     an `{:?}` and will become `{}{:?}`.",


remove trailing .

flip1995 · 2018-02-24T16:01:11Z

src/test/ui/lint/type-overflow.stderr

+34 |     let fail = -0b1111_1111i8; //~WARNING literal out of range for i8
+   |                 ^^^^^^^^^^^^^ help: consider using `i16` instead: `0b1111_1111i16`
+   |
+   = note: the literal `0b1111_1111i8` (decimal `255`) does not fit into an `i8` and will become `-1i8`


The note says that the literal will become -1i8 (which is technically correct), while fail will have the value 1i8 due to the negation. That is, because I ignore whether the literal is negated in the generation of the note.

I'm not sure if I should change this because it is correct for the given span.

I think that this is fine for now. Filed #48535 to follow up.

estebank · 2018-02-25T21:23:34Z

@bors r+

bors · 2018-02-25T21:23:35Z

📌 Commit de7d375 has been approved by estebank

estebank · 2018-02-28T04:03:07Z

@bors rollup

estebank · 2018-02-28T04:04:58Z

@bors r-

@flip1995 you'll have to rebase and fix the ui test output to replace the line numbers in he margins with LL because of #48449.

flip1995 · 2018-02-28T23:35:54Z

Whoops. I failed hard on the rebase. Sorry... :(
Should I open a new PR for this?

estebank · 2018-03-01T00:07:45Z

If you can fix the rebase, just do a git push --force.

flip1995 · 2018-03-01T00:50:49Z

Fixed it somehow. That should be good now.

estebank · 2018-03-01T03:18:40Z

@bors r+ rollup

bors · 2018-03-01T03:18:41Z

📌 Commit f45f760 has been approved by estebank

estebank · 2018-03-01T17:37:57Z

Completely missed this pitfall, @Manishearth.

@flip1995, don't test the i/u size cases, as they're platform dependent and are prone to work locally and in travis, but break in the full run on all targets.

flip1995 · 2018-03-02T12:35:18Z

Should I rebase again?

oli-obk

No need to rebase if Travis passes

oli-obk · 2018-02-23T07:39:51Z

src/librustc_lint/types.rs

+            let src = cx.sess().codemap().span_to_snippet(lit.span).ok()?;
+            let firstch = src.chars().next()?;
+
+            if let Some(0) = char::to_digit(firstch, 10) {


why don't you just check firstch == '0'?

r=me after @oli-obk's comment is addressed. Sorry for the back and forth, @flip1995, I didn't catch this.

Don't know why I made this so complicated.

No problem. I'm happy for the great support on my first PR to the rust compiler! :)

flip1995 · 2018-03-03T21:06:32Z

r? @estebank

oli-obk · 2018-03-04T07:01:40Z

@bors r+

bors · 2018-03-04T07:01:41Z

📌 Commit fc33b25 has been approved by oli-obk

@oli-obk

Suggest type for overflowing bin/hex-literals Fixes rust-lang#48073 For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)). Additionally it will suggest a type (`X < Y`): - `iX`: if literal fits in `uX` => `uX`, else => `iY` - `-iX` => `iY` - `uX` => `uY` Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case. r? @oli-obk

@oli-obk

Suggest type for overflowing bin/hex-literals Fixes rust-lang#48073 For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)). Additionally it will suggest a type (`X < Y`): - `iX`: if literal fits in `uX` => `uX`, else => `iY` - `-iX` => `iY` - `uX` => `uY` Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case. r? @oli-obk

@oli-obk

Suggest type for overflowing bin/hex-literals Fixes rust-lang#48073 For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)). Additionally it will suggest a type (`X < Y`): - `iX`: if literal fits in `uX` => `uX`, else => `iY` - `-iX` => `iY` - `uX` => `uY` Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case. r? @oli-obk

@oli-obk

Suggest type for overflowing bin/hex-literals Fixes rust-lang#48073 For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)). Additionally it will suggest a type (`X < Y`): - `iX`: if literal fits in `uX` => `uX`, else => `iY` - `-iX` => `iY` - `uX` => `uY` Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case. r? @oli-obk

Rollup of 14 pull requests - Successful merges: #48403, #48432, #48546, #48573, #48590, #48657, #48727, #48732, #48753, #48754, #48761, #48474, #48507, #47463 - Failed merges:

rust-highfive assigned oli-obk Feb 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2018

oli-obk requested changes Feb 22, 2018

View reviewed changes

mattico reviewed Feb 22, 2018

View reviewed changes

estebank requested changes Feb 23, 2018

View reviewed changes

flip1995 commented Feb 24, 2018

View reviewed changes

estebank mentioned this pull request Feb 25, 2018

Handle negative literals in cast overflow warning #48535

Closed

estebank approved these changes Feb 25, 2018

View reviewed changes

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 Feb 25, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 28, 2018

flip1995 added 3 commits March 1, 2018 01:34

Suggest type for overflowing bin/hex-literals

e822e62

Implementing requested changes

19c4771

Rewrite error reporting as requested

5c70619

flip1995 force-pushed the lit_diag branch from bb52e60 to 5c70619 Compare March 1, 2018 00:43

Adapt stderr of UI test to PR rust-lang#48449

f45f760

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 Mar 1, 2018

Manishearth mentioned this pull request Mar 1, 2018

Rollup of 9 pull requests #48632

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2018

Remove isize test

15ecf06

oli-obk reviewed Mar 2, 2018

View reviewed changes

Improve getting literal representation

fc33b25

rust-highfive assigned estebank and unassigned oli-obk Mar 3, 2018

oli-obk approved these changes Mar 4, 2018

View reviewed changes

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 Mar 4, 2018

Manishearth mentioned this pull request Mar 5, 2018

Rollup of 7 pull requests #48744

Closed

kennytm mentioned this pull request Mar 6, 2018

Rollup of 14 pull requests #48768

Merged

GuillaumeGomez mentioned this pull request Mar 6, 2018

Rollup of 11 pull requests #48774

Closed

GuillaumeGomez mentioned this pull request Mar 6, 2018

Rollup of 13 pull requests #48776

Closed

alexcrichton merged commit fc33b25 into rust-lang:master Mar 6, 2018

flip1995 deleted the lit_diag branch May 6, 2018 13:07

estebank mentioned this pull request Aug 15, 2018

Literal overflow warnings should only apply to decimal. #24361

Closed

Suggest type for overflowing bin/hex-literals #48432

Suggest type for overflowing bin/hex-literals #48432

Conversation

flip1995 commented Feb 22, 2018

rust-highfive commented Feb 22, 2018

oli-obk 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

Choose a reason for hiding this comment

flip1995 Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Feb 25, 2018

bors commented Feb 25, 2018

estebank commented Feb 28, 2018

estebank commented Feb 28, 2018 • edited Loading

flip1995 commented Feb 28, 2018

estebank commented Mar 1, 2018

flip1995 commented Mar 1, 2018

estebank commented Mar 1, 2018

bors commented Mar 1, 2018

estebank commented Mar 1, 2018

flip1995 commented Mar 2, 2018

oli-obk 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

flip1995 commented Mar 3, 2018

oli-obk commented Mar 4, 2018

bors commented Mar 4, 2018

flip1995 Feb 22, 2018 •

edited

Loading

estebank commented Feb 28, 2018 •

edited

Loading