Skip to content

Commit

Permalink
Fix panic in lint for out of range literals
Browse files Browse the repository at this point in the history
  • Loading branch information
@petrochenkov
petrochenkov committed May 20, 2015
1 parent 6d718f2 commit 7b1916d
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/librustc_lint/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,8 @@ impl LintPass for TypeLimits {
let (min, max) = int_ty_range(int_type);
let negative = self.negated_expr_id == e.id;

if (negative && v > min.wrapping_neg() as u64) ||
(!negative && v > (max.abs() as u64)) {
if (negative && min != i64::MIN && v > -min as u64) ||
(!negative && v > max as u64) {
cx.span_lint(OVERFLOWING_LITERALS, e.span,
&*format!("literal out of range for {:?}", t));
return;
Expand Down

4 comments on commit 7b1916d

@econoplas
Copy link
Contributor

Choose a reason for hiding this comment

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

Following @pnkfelix advice to use blame rather than opening a new issue if a patch causes an undesirable change, hopefully I've done this correctly... if not, I apologize in advance.

Patch 7b1916d introduces a regression in behavior in the sense that invalid literal i64 values < -9223372036854775808i64 are no longer being properly reported as #[warn(overflowing_literals)]. The other literal checks seem to still be working.

I have been watching and thinking a lot about these few lines of code over the last few days as they have gone through various stages of being broken/fixed following the change to abs() which causes panic for i32::MIN & friends... mostly because I was experienced the same build-time panic/crash that patch 7b1916d fixes.

As a more stable and future-proof solution I would like to propose:

                                let (_, max) = int_ty_range(int_type);
                                let negative = self.negated_expr_id == e.id;

                                // Detect literal value out of range [min, max] inclusive
                                if (negative && v > max as u64 + 1) ||
                                   (!negative && v > max as u64) {
                                    cx.span_lint(OVERFLOWING_LITERALS, e.span,
                                                 &*format!("literal out of range for {:?}", t));
                                    return;
                                }

The expression (negative && v > max as u64 + 1) relies on the fact that algebraically speaking -min == max + 1 to avoid negation and removing the need for min completely. So hopefully it is more intuitive as well.

If i128 or i256 are ever added, it should also work for these types without requiring a change to min != i64::MIN && also simplifying maintenance.

Is this a worthwhile change? Or am I just wasting my time investigating stuff that doesn't matter again?

Should I submit this as a "real" issue? Or is it good enough to report this via blame (which I haven't tried before)?

Here is some test code:

fn main() {
    let (a1, b1) = (-129i8, 128i8);
    let (a2, b2) = (-32769i16, 32768i16);
    let (a3, b3) = (-2147483649i32, 2147483648i32);
    let (a4, b4) = (-9223372036854775809i64, 9223372036854775808i64);

    println!("{} {}", a1, b1);
    println!("{} {}", a2, b2);
    println!("{} {}", a3, b3);
    println!("{} {}", a4, b4);
}

Results with nightly 2015-05-19:

$ rustc invalid_literals.rs
invalid_literals.rs:3:22: 3:27 warning: literal out of range for i8, #[warn(overflowing_literals)] on by default
invalid_literals.rs:3     let (a1, b1) = (-129i8, 128i8);
                                           ^~~~~
invalid_literals.rs:3:29: 3:34 warning: literal out of range for i8, #[warn(overflowing_literals)] on by default
invalid_literals.rs:3     let (a1, b1) = (-129i8, 128i8);
                                                  ^~~~~
invalid_literals.rs:4:22: 4:30 warning: literal out of range for i16, #[warn(overflowing_literals)] on by default
invalid_literals.rs:4     let (a2, b2) = (-32769i16, 32768i16);
                                           ^~~~~~~~
invalid_literals.rs:4:32: 4:40 warning: literal out of range for i16, #[warn(overflowing_literals)] on by default
invalid_literals.rs:4     let (a2, b2) = (-32769i16, 32768i16);
                                                     ^~~~~~~~
invalid_literals.rs:5:22: 5:35 warning: literal out of range for i32, #[warn(overflowing_literals)] on by default
invalid_literals.rs:5     let (a3, b3) = (-2147483649i32, 2147483648i32);
                                           ^~~~~~~~~~~~~
invalid_literals.rs:5:37: 5:50 warning: literal out of range for i32, #[warn(overflowing_literals)] on by default
invalid_literals.rs:5     let (a3, b3) = (-2147483649i32, 2147483648i32);
                                                          ^~~~~~~~~~~~~
invalid_literals.rs:6:22: 6:44 warning: literal out of range for i64, #[warn(overflowing_literals)] on by default
invalid_literals.rs:6     let (a4, b4) = (-9223372036854775809i64, 9223372036854775808i64);
                                           ^~~~~~~~~~~~~~~~~~~~~~
invalid_literals.rs:6:46: 6:68 warning: literal out of range for i64, #[warn(overflowing_literals)] on by default
invalid_literals.rs:6     let (a4, b4) = (-9223372036854775809i64, 9223372036854775808i64);
                                                                   ^~~~~~~~~~~~~~~~~~~~~~
$ rustc --version --verbose
rustc 1.2.0-dev (built 2015-05-20)
binary: rustc
commit-hash: unknown
commit-date: unknown
build-date: 2015-05-20
host: x86_64-unknown-linux-gnu
release: 1.2.0-dev

Results with master and rustc-nightly 2015-05-24 (both after 7b1916d):

$ rustc invalid_literals.rs 
invalid_literals.rs:3:22: 3:27 warning: literal out of range for i8, #[warn(overflowing_literals)] on by default
invalid_literals.rs:3     let (a1, b1) = (-129i8, 128i8);
                                           ^~~~~
invalid_literals.rs:3:29: 3:34 warning: literal out of range for i8, #[warn(overflowing_literals)] on by default
invalid_literals.rs:3     let (a1, b1) = (-129i8, 128i8);
                                                  ^~~~~
invalid_literals.rs:4:22: 4:30 warning: literal out of range for i16, #[warn(overflowing_literals)] on by default
invalid_literals.rs:4     let (a2, b2) = (-32769i16, 32768i16);
                                           ^~~~~~~~
invalid_literals.rs:4:32: 4:40 warning: literal out of range for i16, #[warn(overflowing_literals)] on by default
invalid_literals.rs:4     let (a2, b2) = (-32769i16, 32768i16);
                                                     ^~~~~~~~
invalid_literals.rs:5:22: 5:35 warning: literal out of range for i32, #[warn(overflowing_literals)] on by default
invalid_literals.rs:5     let (a3, b3) = (-2147483649i32, 2147483648i32);
                                           ^~~~~~~~~~~~~
invalid_literals.rs:5:37: 5:50 warning: literal out of range for i32, #[warn(overflowing_literals)] on by default
invalid_literals.rs:5     let (a3, b3) = (-2147483649i32, 2147483648i32);
                                                          ^~~~~~~~~~~~~
invalid_literals.rs:6:46: 6:68 warning: literal out of range for i64, #[warn(overflowing_literals)] on by default
invalid_literals.rs:6     let (a4, b4) = (-9223372036854775809i64, 9223372036854775808i64);
                                                                   ^~~~~~~~~~~~~~~~~~~~~~
$ rustc --version --verbose
rustc 1.2.0-dev (built 2015-05-24)
binary: rustc
commit-hash: unknown
commit-date: unknown
build-date: 2015-05-24
host: x86_64-unknown-linux-gnu
release: 1.2.0-dev

Notice the missing invalid_literals.rs:6:22: 6:44 warning: literal out of range for i64, #[warn(overflowing_literals)] warning for -9223372036854775809i64.

Here is the patch to fix:

--- rustc-nightly/src/librustc_lint/builtin.rs  2015-05-23 20:57:03.000000000 -0600
+++ rustc-nightly-fix/src/librustc_lint/builtin.rs  2015-05-24 17:53:06.296001799 -0600
@@ -203,10 +203,11 @@
                                 } else {
                                     t
                                 };
-                                let (min, max) = int_ty_range(int_type);
+                                let (_, max) = int_ty_range(int_type);
                                 let negative = self.negated_expr_id == e.id;

-                                if (negative && min != i64::MIN && v > -min as u64) ||
+                                // Detect literal value out of range [min, max] inclusive
+                                if (negative && v > max as u64 + 1) ||
                                    (!negative && v > max as u64) {
                                     cx.span_lint(OVERFLOWING_LITERALS, e.span,
                                                  &*format!("literal out of range for {:?}", t));

Final results after applying proposed patch to nightly 2015-05-24:

$ rustc invalid_literals.rs
invalid_literals.rs:3:22: 3:27 warning: literal out of range for i8, #[warn(overflowing_literals)] on by default
invalid_literals.rs:3     let (a1, b1) = (-129i8, 128i8);
                                           ^~~~~
invalid_literals.rs:3:29: 3:34 warning: literal out of range for i8, #[warn(overflowing_literals)] on by default
invalid_literals.rs:3     let (a1, b1) = (-129i8, 128i8);
                                                  ^~~~~
invalid_literals.rs:4:22: 4:30 warning: literal out of range for i16, #[warn(overflowing_literals)] on by default
invalid_literals.rs:4     let (a2, b2) = (-32769i16, 32768i16);
                                           ^~~~~~~~
invalid_literals.rs:4:32: 4:40 warning: literal out of range for i16, #[warn(overflowing_literals)] on by default
invalid_literals.rs:4     let (a2, b2) = (-32769i16, 32768i16);
                                                     ^~~~~~~~
invalid_literals.rs:5:22: 5:35 warning: literal out of range for i32, #[warn(overflowing_literals)] on by default
invalid_literals.rs:5     let (a3, b3) = (-2147483649i32, 2147483648i32);
                                           ^~~~~~~~~~~~~
invalid_literals.rs:5:37: 5:50 warning: literal out of range for i32, #[warn(overflowing_literals)] on by default
invalid_literals.rs:5     let (a3, b3) = (-2147483649i32, 2147483648i32);
                                                          ^~~~~~~~~~~~~
invalid_literals.rs:6:22: 6:44 warning: literal out of range for i64, #[warn(overflowing_literals)] on by default
invalid_literals.rs:6     let (a4, b4) = (-9223372036854775809i64, 9223372036854775808i64);
                                           ^~~~~~~~~~~~~~~~~~~~~~
invalid_literals.rs:6:46: 6:68 warning: literal out of range for i64, #[warn(overflowing_literals)] on by default
invalid_literals.rs:6     let (a4, b4) = (-9223372036854775809i64, 9223372036854775808i64);
                                                                   ^~~~~~~~~~~~~~~~~~~~~~
$ rustc --version --verbose
rustc 1.2.0-dev (built 2015-05-24)
binary: rustc
commit-hash: unknown
commit-date: unknown
build-date: 2015-05-24
host: x86_64-unknown-linux-gnu
release: 1.2.0-dev

Is there a way to contribute test cases for compiler lint checks to help detect regressions? If so, point me in the right direction and I'll try to contribute some.

cc @alexcrichton @pnkfelix

@petrochenkov
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surely, it's a mistake (sorry for introducing it).
Fell free to open a pull request with your fix and test.
To test the issue you can add a new file with your test code (a bit simplified + deny the warning):

#![deny(overflowing_literals)]

fn main() {
    (-129i8, 128i8);
    (-32769i16, 32768i16);
    (-2147483649i32, 2147483648i32);
    (-9223372036854775809i64, 9223372036854775808i64);
}

to the directory src/test/compile-fail and specify the expected warning messages as described here: https://github.com/rust-lang/rust-wiki-backup/blob/master/Note-testsuite.md#specifying-the-expected-errors-and-warnings

@econoplas
Copy link
Contributor

Choose a reason for hiding this comment

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

No worries. You fixed the panic which was blocking me too.

Thanks for the feedback and advice. I discovered there was already a module providing tests for overflowing_literals but it was missing a couple of test cases for negative i64's out of range.

I am ready to create the pull request, and all tests are passing.

Would it be best to request you as the reviewer or possibly @alexcrichton or @pnkfelix since they have been involved in the discussions surrounding abs() changes and panic behavior? Or just let bors pick someone at random to review? This is my first time contributing to rust, so I wasn't sure how this works.

@petrochenkov
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no rights to approve pull requests and Alex is on vacation, so @pnkfelix or random would be a better choice.

Please sign in to comment.