PoC: Integer range analysis

[RunDevelopment]

This PR is a proof of concept for using integer range analysis to improve existing rules. I focused on the lints cast_possible_truncation, cast_possible_wrap, and cast_sign_loss for now, but range analysis is potentially useful in other existing rules as well.

The range analysis itself is rather simplistic right now. The possible values of an expression are represented using a closed interval with the IInterval type. This representation is applicable to all integer expression and variables, even those we know nothing about except for the type. E.g. a function parameter x: u8 will have the range 0..=255, and the expression x / 3 + 1 will have the range 1..=86.

The interval arithmetic necessary to implement operations like addition, subtractions, and many std integer methods is currently implemented by the Arithmetic type. I already implemented most methods that return integers or Option<int_type>.

Example

While only a limited proof of concept, it is already enough to fix false positives in cast_possible_truncation such as #7486. In the following, I annotated the computed interval ranges for the code of the issue:

fn issue7486(number: u64, other: u32) -> u32 {
    let other_u64 = other as u64;
    // ^ range: 0..=u32::MAX
    let result = number % other_u64;
    // ^ range: 0..=u32::MAX-1
    result as u32
}

As we can see, the range analysis is capable enough to determine that the result variable can only hold values in the range 0..=u32::MAX-1 despite being of type u64. This is enough to eliminate the false positive.

Here's a slightly more complex example where I annotated all expressions:

fn unorm16_to_unorm8(x: u16) -> u8 {
    //               ^ 0..=65535
    ((x as u32 * 255 + 32895) >> 16) as u8
    //^~~~~~~~ 0..=65535
    //^~~~~~~~~~~~~~ 0..=16711425
    //^~~~~~~~~~~~~~~~~~~~~~ 32895..=16744320
    //^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 0..=255
}

A few details

I want to mention a few details to hopefully make what I implemented more understandable for potential reviewers.

1. IInterval doesn't just represent an integer interval, but a typed interval. I omitted types in the above annotations, but all IIntervals know the underlying integer type.

2. IIntervals can be empty. This is important for operations that can panic. E.g. 1_u32.strict_sub(2_u32) will always panic, so it will be assigned the empty range (u32).

3. All interval operations guarantee that they return a superset of the actual result. E.g. for 1 + 1, the actual result interval is 1..=1, but the add implementation is allowed to return any superset of it, e.g. 0..=255. This is both necessary and useful.

   It's necessary because some operations don't output clean intervals. E.g. u8::wrapping_add(value, 1) where value is 254 or 255 will result in the outputs 255 or 0 respectively. However, the smallest interval that contains the set $\set{0,255}$ is 0..=255 — a superset.

   It's useful, because it makes operations easier to implement. Some operations are very complex to implement, so allowing any superset makes it possible to start with a very simple implementation that returns a large superset and then later improve it.

4. Arithmetic has both methods and static functions. This is because the data Arithmetic carries represents compiler flags/options. If an operation depends on any flags/options, it's a method, and a static function otherwise. E.g. abs() may panic or wrap depending on whether overflow checks are enabled, so it's a method, while strict_abs() always panics, so it's a static function.

5. IntervalCtxt is the type that implements recursively evaluating expression intervals (inspired by ConstEvalCtxt).

---

I want to stress that this is a proof of concept. This PR is in no state to be merged right now. Before I invest more time into this, I would like to ask (1) whether there is interest in clippy doing this type of range analysis in the first place and (2) whether there are people that could help me with the clippy side of things?

The main thing I need help with is testing (I think). Specifically:

• I used property tests and snapshot tests to ensure the correctness of Arithmetic and IInterval, but I'm not sure how to best integrate them. I say "used", because I initially worked on IInterval and Arithmetic to be their own crate, but then I just copy-pasted the code for the sake of this PR. However, I didn't copy the tests.

  I don't care whether this code ends up inside clippy or as its own crate, I just want well-tested working code to make clippy smarter. So I would like to ask (1) whether rinterval should be its own crate (probably not), and if not (2) what's the best way to add the snapshot tests I had?

• I want to test the recursive expression evaluation too. I imagine snapshot tests similar to uitest, where I can just define a few source files and it will output a snapshot whether the ranges of all expressions are annotated, similar to what I did for the above examples. Is there any existing infrastructure I could use?

Of course, other feedback and suggestions are also appreciated.

Before this PR is ready, some work still needs to be done:

1. Lints need to actually integer ranges to reject false positives. Right now, I just made the 3 lints I mentioned at the start evaluate the intervals for all reported expressions and report it as a note, because I had no other way of seeing the ranges.
2. Casting UI tests need to be re-written, because a lot of existing case are false positives. E.g. 1u64 as isize -> u64::MAX as isize.
3. Caching. Right now, the expression evaluation is entirely uncached. For the sake of performance, this should obviously be changed.
4. usize. I currently use rustc_lint::context::LateContext::data_layout().ptr_sized_integer() to map usize to a fixed-width integer type. Same for isize. This works, but also means that the interval analysis can't be used to determine the intervals for 32-bit AND 64-bit system, only one is possible. This can potentially lead to false positives and false negatives in lints.

I also want to point out that I see what I did in this PR as an MVP. In the future, I would like to make the range analysis even smarter (e.g. control-flow-based narrowing) and add floating-point ranges. Of course, I also want to use range analysis to improve other existing rules (which may even be done in this PR, idk).

Please tell me what you think.

[github-actions]

Lintcheck changes for fe13b7d

Lint	Added	Removed	Changed
clippy::cast_possible_truncation	11	189	289
clippy::cast_possible_wrap	0	79	50
clippy::cast_sign_loss	0	26	55

This comment will be updated if you push new changes

[Jarcho]

This is something I've wanted to add for a while, but other things have been a higher priority. The analysis itself should be done as a MIR analysis rather than working on the HIR tree. See rustc_mir_dataflow for the dataflow framework.

Some notes for implementing this:

• Start with implementing Analysis. The domain type will be something like IndexVec<Local, Option<Interval>>, but wrapped in it's own type. None means the local doesn't contain a value.
• Implement JoinSemiLattice for Interval. This should produce the union of both intervals.
• Implement JointSemiLattice for the domain type. This is an element wise join of the vec.
• Implement the apply_primary_statment_effect, apply_primary_terminator_effect and apply_call_return_effect methods on Analysis. StatementKind::Assign is where most things of interest happen for this analysis. A MIR visitor might help with implementing this.
• Once the analysis is finished a ResultsCursor can be used to get the value at the points you need them.

[RunDevelopment]

The analysis itself should be done as a MIR analysis rather than working on the HIR tree.

@Jarcho Is that even possible?

Note that I am very new to rustc and clippy development, so what I'm about to say stems from me looking into MIR (mostly reading this and this and playing around with it) and only really knowing about the parts of clippy and the compiler that I have touched so far.

I currently don't see how this will work for the following reasons:

1. MIR seems to fold constants in release mode. This is a problem, because it will fold expressions that the lints I'm interested in are supposed to check. E.g. (3 + 255) as u8 is represented as the constant 2_u8 in MIR in release mode. How is it supposed to analyze expressions that are optimized away in MIR?
2. MIR seems to be affected by whether the overflow-checks flag is set. I noticed that e.g. a + b will produce Add(copy _1, copy _2) on release and AddWithOverflow(copy _1, copy _2) + assert on debug builds. This is a problem, because it will result in different intervals for the result of a + b and I don't know which MIR clippy uses. For correctness, the analysis has to be done assuming overflow checks disabled (aka operations wrap).
3. MIR seems to inline small functions in release mode. This is a problem, because the analysis is supposed to stop at function boundaries and not "peek inside" so to say. The contract of a function is its signature, and the analysis should hold it to that. (This obviously isn't true for core/std functions. Their behavior is specified by the language, so the analysis is allowed to make more assumptions.)
4. assert! and debug_assert! seem to lower to the same representation when debug assertions are enabled. I plan to add control-flow-based narrowing, and I don't want to trust debug_assert!s for that. E.g. debug_assert!(c > 0); c as uN should trigger cast_sign_loss, while assert!(c > 0); c as uN should not. (Ideally, it would be user configurable whether debug asserts are trusted.)

Do we have enough control over how the MIR is generated to address these issues? Especially issue 2. If the MIR has overflow checks, then the results of the analysis will be plain wrong for release builds. This would kill the idea.

Start with implementing Analysis.

I have a question about loops. From my (admittedly shaky) understanding, Analysis::iterate_to_fixpoint iterates until the state converges, mainly for loops. If my understanding is correct, then I'm not sure how an implementation is supposed to handle the following case:

fn foo() -> u8 {
    let mut x: u32 = 1;
    while x != 0 {
        x += 1;
    }
    x as u8
}

This function either returns 0 after x overflows or crashes (overflow checks). In either case, each iteration only increases the range of x by 1, meaning that it would take around 2^32 iterations until the full range for x has been determined.

Obviously, this is completely unusable. My first thought for fixing this was to do a small fixed number of iterations (maybe 3) and set any locals that haven't converged to the full range of their type, but I'm not sure if this is the best idea.

My question is (1) whether my understanding is broadly correct and (2) whether there are better ways of ensuring quick convergence?

[Jarcho]

MIR seems to fold constants in release mode. This is a problem, because it will fold expressions that the lints I'm interested in are supposed to check. E.g. (3 + 255) as u8 is represented as the constant 2_u8 in MIR in release mode. How is it supposed to analyze expressions that are optimized away in MIR?

I'm not sure where in the compiler is done. I think it's a MIR optimization and we disable those when running clippy. At some point this will change to running before optimizations.

MIR seems to be affected by whether the overflow-checks flag is set. I noticed that e.g. a + b will produce Add(copy _1, copy _2) on release and AddWithOverflow(copy _1, copy _2) + assert on debug builds. This is a problem, because it will result in different intervals for the result of a + b and I don't know which MIR clippy uses. For correctness, the analysis has to be done assuming overflow checks disabled (aka operations wrap).

We run before without MIR inlining so you can treat them as the same. The only way they'll appear is if someone calls an intrinsic directly which can be not a well supported thing.

MIR seems to inline small functions in release mode. This is a problem, because the analysis is supposed to stop at function boundaries and not "peek inside" so to say. The contract of a function is its signature, and the analysis should hold it to that. (This obviously isn't true for core/std functions. Their behavior is specified by the language, so the analysis is allowed to make more assumptions.)

See previous responses. We don't run with optimizations.

assert! and debug_assert! seem to lower to the same representation when debug assertions are enabled. I plan to add control-flow-based narrowing, and I don't want to trust debug_assert!s for that. E.g. debug_assert!(c > 0); c as uN should trigger cast_sign_loss, while assert!(c > 0); c as uN should not. (Ideally, it would be user configurable whether debug asserts are trusted.)

MIR has some span information so you can detect if the SwitchInt terminator is coming from debug_assert. Note debug assertions also have a minor issue where they won't be in the MIR or HIR if they're disabled. Not really a problem since cargo clippy --release isn't a thing people do.

This function either returns 0 after x overflows or crashes (overflow checks). In either case, each iteration only increases the range of x by 1, meaning that it would take around 2^32 iterations until the full range for x has been determined.

Obviously, this is completely unusable. My first thought for fixing this was to do a small fixed number of iterations (maybe 3) and set any locals that haven't converged to the full range of their type, but I'm not sure if this is the best idea.

My question is (1) whether my understanding is broadly correct and (2) whether there are better ways of ensuring quick convergence?

That is correct. A naive implementation would indeed take a long time to run. You can use JoinSemiLattice to put a limit on the number of times a range can change. This would make your domain Option<(Inverval, ChangeCount)>. There are more intelligent things you could do once something #14683 is merged, but that's not going to be for a while.

[RunDevelopment]

Alrighty, then this seems pretty doable to me now. I should be able to get something working in a few days.

MIR seems to be affected by whether the overflow-checks flag is set. I noticed that e.g. a + b will produce Add(copy _1, copy _2) on release and AddWithOverflow(copy _1, copy _2) + assert on debug builds. This is a problem, because it will result in different intervals for the result of a + b and I don't know which MIR clippy uses. For correctness, the analysis has to be done assuming overflow checks disabled (aka operations wrap).

We run before without MIR inlining so you can treat them as the same. The only way they'll appear is if someone calls an intrinsic directly which can be not a well supported thing.

This kind of conflicts with what docs are saying and what I see on rust playground, but maybe even debug builds do some amount of MIR inlining. I'll figure it out.

That said, clippy doing no amount of inlining and representing operators as calls to their trait impls in MIR would be my ideal scenario.

[Jarcho]

Operators (as in the actual operators, not calls to the trait methods) are not lowered to function calls for primitive types. That's different from inlining.

[Jarcho]

When I say treat them as the same I mean you can treat the WithOverflow variants as though they are both wrapping and just ignore the boolean value. You'll need to handle the projection when reading the result which will complicate things a little, but that's minor.