Check inferred integer literals for overflows, closes #4220

[fhahn]

I've started working on this issue and pushed a small commit, which adds a range check for integer literals in middle::const_eval (no uint at the moment)
At the moment, this patch is just a proof of concept, I'm not sure if there is a better function for the checks in middle::const_eval. This patch does not check for overflows after constant folding, eg:

let x: i8 = 99 + 99;

[huonw]

)| → ) |

[fhahn]

I've updated the style issues.

If this is a reasonable approach I could complete this patch in the next days.

I think I should mention the original issue as well, #4220

[brson]

@nikomatsakis Do you have an opinion about this? It's not a lint check, and I'm not sure why graydon suggested doing it in const_eval instead of in lint in #4220, but this looks pretty straightforward.

[nikomatsakis]

I'm still of the opinion that the proper way to do this is a lint.

[nikomatsakis]

Thinking a bit more on this, I don't see how adding things into const_eval will work. Isn't that only used for constant expressions (i.e., static decls and enum discriminants and that sort of thing?). And the compile-fail tests don't include expected error messages, so they won't pass. It shouldn't be hard to adapt this code to run at lint time instead though -- basically just visit all literal expressions, examine the type assigned to them, and check whether the literal fits. For extra credit, you would track whether the literal is enclosed within a unary minus so as to distinguish the fun edge case:

let x: i8 = 128; // ideally, would warn
let x: i8 = -128; // should not
let x: i8 = -(128); // and this shouldn't either
let x: i8 = -{128}; // but you've got to draw the line somewhere, so it'd be ok if this did :)

[fhahn]

I moved to code to the linter and fixed the test.

As it turns out, checking

 let x: i8 = -128; // should not 

is a tricky case. It's represented as ExprUnary(_, NegOp, ExprLit(lit)) but lit holds the positive value (128) instead of the negative value, so there is some extra work needed to check if the ExprLit follows a NegOp. A quick and dirty solution would be to pass the information through the walk calls (fhahn@d468bbf#diff-1e2a5771a9e8a30cc0802838a4a9da9dR529 ), but there's probably a much better solution (with my solution let x:i8 = --128; does not warn and there must be special handling for cases like let x: i8 = -(128);). The ExprLit inside ExprBinary has a negative value, maybe all ExprLit should be folded and contain the literal with a correct sign before calling the linter.

Another thing I noticed: the overflow check for typed literals does handle edge cases correctly:

let x = 128_i8; // does not fail
let x = 129_i8; // does fail

But I guess once the check for overflows is handled by the linter, we could remove the check in check_const.rs? https://github.com/mozilla/rust/blob/master/src/librustc/middle/check_const.rs#L206

[nikomatsakis]

Your point about overflow was precisely why I suggested the example. There is probably no great way to do this right now; I had in mind passing the information down via walk calls, or perhaps building up a (temporary) set of node-ids that are negated (i.e., the unary minus would insert its operand's id into the map, and ExprParen would propagate to its operand if it itself is in the map) -- this way you could just check whether the literal is in the map.

As far as code in check_const.rs, I'd remove it, yes. It seems to be superseded by the lint check.

[fhahn]

My patch should pass down the information via walk calls, I hope I understood your intention correctly. Your test cases pass and I've removed the code from check_const.rs, but it seems like there were no tests for this. Should I add some to the tests in test/compile-fail/lint-type-overflow.rs ?

[huonw]

Indentation.

[huonw]

I'm not 100% sure we want to be adding a non-() context to a Visitor rather than just adding a field to the type that Visitor is being implemented upon. (@pnkfelix might have an opinion on this.)

(Needs a rebase, btw.)

[huonw]

(Also, in this case, a bare bool is very opaque: one essentially has to read the entire impl to find the place that indicates what its purpose is. I'd much prefer a outer_expr_is_neg: bool field on Context (don't feel obliged to use that name).)

[nikomatsakis]

On Tue, Oct 29, 2013 at 03:19:09PM -0700, Florian Hahn wrote:

My patch should pass down the information via walk calls, I hope I
understood your intention correctly.

Cool, I'll take a look.

Your test cases pass and I've removed the code from check_const.rs,
but it seems like there were no tests for this. Should I add some to
the tests in test/compile-fail/lint-type-overflow.rs ?

Definitely we need some tests -- particularly for edge cases!

[nikomatsakis]

Ordinarily I'd say that this is exactly the kind of case where a bool parameter is appropriate, except that this visitor appears to be used for many different lints. Therefore I agree with @huonw that a field is more appropriate. However, a field has the disadvantage that it's easily forgotten -- for example, if visit_stmt or some such failed to clear the field, then let x = - { let y = 128u8; 22 } would fail to warn. In general, I always get afraid when writing "context passing" visitors that some new intermediate level will be added and screw up my code anyhow.

My suggestion (which may well be overkill) is to add a field, but rather than making it a boolean, have its type be a NodeId. For example, negated_expr: ast::NodeId. Then the unary negative expr can set negated_expr = operand.id, and ExprParen can do if self.negated_expr == expr.id { self.negated_expr = parenthesized_expr.id; }, and finally literals can compare if self.negated_expr == self.id. Everybody else can just ignore the field altogether.

In general though this looks great, exactly what I had in mind.

[nikomatsakis]

Oh, and I think that -(-128) should not warn.

[fhahn]

Wouldn't that evalute to 128, which overflows an i8?

[nikomatsakis]

Yes. but so would:

let x = -128;
-x

and I don't intend to warn about that. In other words, we're doing a best effort thing, and I feel like if someone wrote something wacky like -(-128) there is likely a reason for it. But I don't really care, if you'd rather warn for this, that's fine too.

[huonw]

(Needs a rebase.)

[fhahn]

I've rebased, implemented the checks for uint (although maybe some code could be reused better, I'll look into that tomorrow) and added a couple of tests.

[alexcrichton]

ping @nikomatsakis

[nikomatsakis]

One question -- @fhahn do you know if we have code anywhere (lexer maybe?) that checks for integer literals that don't fit within a u64?

[huonw]

@nikomatsakis yes in the lexer: https://github.com/mozilla/rust/blob/46100c06223f737a7604b46667800ed42cf20d3b/src/libsyntax/parse/lexer.rs#L520-L524

[fhahn]

After my patch, the compile-fail/lint-type-limits.rs test fails, because in addition to the warning, that the comparission is useless it displays a warning that the literal is out of range for its type, eg:

fn bar() -> i8 {
    return 123;
}

fn baz() -> bool {
    128 > bar() //~ ERROR comparison is useless due to type limits
}

It warns that the comparision is useless for 128 > bar(), but it also warns, that the literal 128 does not fit into it's type (i8). How should this case be handled?

My guess would be, to only warn about the comparison and silence the warning about the literal, which would require passing the information from the comparison check to the overflow check. Should this be done by another field?

[nikomatsakis]

I would be inclined to issue both warnings. You can annotate this by adding

//~^ ERROR your error message here

underneath the line in question (the //~^ indicates that an error is expected on the line above).

[fhahn]

I've added the warnings to the two failing tests, they should pass now.

But I was wondering if merging the type-limits and the type-overflow linter message would make sense.

fn bar() -> i8 {
    return 123;
}
fn baz() -> bool {
    128 > bar() //~ ERROR comparison is useless due to type limits
}

In this case, the comparison is useless, because 128 does not fit into an i8 and there is a overflow. With "literal out of range for its type" as the only warning, you could easily draw the same conclusion.
Getting rid of the " comparison is useless due to type limits" in favor of the more general "literal out of range for its type" would also be more consistent with other cases. Consider

fn bar() -> i8 {
     return 123;
}

fn baz() -> i8{
    128 + bar()
}

In this case, the warning would be "literal out of range for its type" instead of something like "addition is useless due to type limits" .

Are logical operators special enough to have a separate warning?

[emberian]

The idea for that link, I think, is usually for cases like foo() < 0 or when you change the type to something smaller. I think the lint is useful. If anything it should be expanded to the other operators (though I would think that lint would be off by default)

[fhahn]

Well, I see, there are other use cases for the type-limit lint.

Compiling the tests for the std lib yields a few overflow warnings, I'l try to fix them tonight

[fhahn]

There was another error, when runnning the tests on gthe mac 32 bit buildbot. In an example in the docs, uint was used instead of u64 for a big number, I guess uint is 32 bit on the mac-32 plattform, so there was an overflow warning.