Deny the overflowing_literals lint for the 2018 edition

[bstrie]

Rendered

Tracking issue

[scottmcm]

💯 Honestly I'd prefer to do it in Rust 2015 as well...

[ollie27]

I don't see any reason to wait for the 2018 edition. I wonder if we could even make it a hard error in the 2018 edition.

One issue is that the overflowing_literals lint covers floats as well. For floats, overflowing to infinity is perfectly normal so a warning does seem fine there. Maybe the lint should be split into overflowing_int_literal and overflowing_float_literals?

[bluetech]

I've used something like let x: i32 = 0xffff_ffff; on occasion; the type is signed but the bit pattern is emphasized. That said, I also think deny is the more appropriate default.

[bstrie]

@ollie27 , good point about floats.

On the one hand, since the behavior with floats is just "clamp to inf or -inf" rather than true wrapping overflow, it's true that allowing float literals to be "too large" wouldn't be inconsistent with our general stance against implicit wrapping overflow.

On the other hand, I don't know if that alone is a good rationale for allowing too-large float literals. Consider our usual guideline for any behavioral change: if we didn't have the current behavior, would we want to transition to the current behavior? In other words, if we already rejected too-large float literals, would we want to start accepting them? Personally, I believe the answer is no.[1]

Of course the prior argument has to take potential breakage into account, and it's true that it's technically more likely that one would encounter a too-large float literal in the wild compared to an overflowing integer literal. However, my instinct is that this would still lead to no breakage in practice, since the upper bound of f32 is pretty big and you'd need an obviously huge literal on the order of 340282346638528859811704183484516925441.0 (or use scientific notation, but c'mon, who even realizes that we support that... :P), and you'd have to be ignoring the warning already, and you'd need to simply be using it as an alias for inf which is pretty noticeably disruptive in nearly every floating-point calculation. And even in the case of breakage, the error message is infallibly precise as to the location of the error, and fixing the breakage is as simple as replacing the literal with std::f32::INFINITY (conceivably this could even be done by an automatic tool, though I doubt it's worth going to such lengths to implement that for this).

So on balance, I think I'm content with catching too-large floating point literals in the same net. That said I'll edit the RFC to note this behavior, and also add a paragraph to the section on alternatives.

[1] Off-topic, but personally I think float literals, in every programming language, are too loosey-goosey already; if I had my way, we'd be writing all float literals as explicit significand/exponent pairs and rejecting any float literals that didn't precisely map to exact representations. But I digress. :P

[bstrie]

@bluetech , what use case is that where people might want to be doing bit-twiddling on signed integers? Note that, for people who didn't want to use -1 instead, let x: i32 = 0xFFFF_FFFFu32 as i32; would still be permitted since as is deliberately intended to paper over differences between integer types (and I'll note this in the RFC).

[Ixrec]

if I had my way, we'd be ... rejecting any float literals that didn't precisely map to exact representations. But I digress. :P

Off-topic reply: I could get behind a clippy lint for this.

[mark-i-m]

This would also prevent subtle bugs like

fn foo(x: u8) { }

fn main() {
    for x in 0 .. 256 {
        foo(x);
    } 
}

[bstrie]

Noticing that @ollie27 mentioned that this could be a hard error rather than merely a deny-by-default lint; I'm not sure if we're allowed to do that according to the rules of the edition, but I'd be all for it. I've added this to the section on alternatives, feedback appreciated!

[mark-i-m]

@bstrie I think a hard error is a good idea.

[scottmcm]

I don't think this is serious enough that it needs to be a hard error. It's not UB, it doesn't block typeck or borrowck, it doesn't cause knock-on errors elsewhere, etc. It's almost certainly wrong, so I'm a big fan of deny-by-default, but Rust seems to be moving away from hard errors where feasible (like #2086).

[retep998]

C, surprisingly enough, has well defined unsigned integer overflow, which is used quite frequently in constant definitions, such as in the Windows SDK. The fact that I can't directly port those definitions over to Rust is kinda ridiculous. Something as simple as const FOO: u32 = -1; won't work and requires const FOO: u32 = -1i32 as u32;.

I really wish Rust hadn't taken this stance on overflowing integers.

[ssokolow]

@retep998 I have to disagree there.

To me, using underflow/overflow to define constants feels like grandfathering a footgun into safe Rust purely because it's a popular idiom in legacy code.

I'd only be in favour if one of the following were the case:

• Requiring that it happen inside an unsafe block.
• Requiring some kind of special notation to make it clear that it was intended as "MAX - 1" rather than "0 - 1".

[RalfJung]

intended as "MAX - 1" rather than "0 - 1".

It's in fact "MAX", not "MAX - 1". And that's what the constant should use, IMHO:

const FOO : u32 = std::u32::MAX;

[MajorBreakfast]

I'm all for putting this into Rust 2018 and leaving Rust 2015's behavior untouched: Rust's commitment to stability should be taken seriously. This is just a footgun, not a safety critical bug. Shipping this as part of Rust 2018 is the right way to do it.

[bstrie]

@ssokolow Using unsafe blocks for this would be counter to our existing philosophy of the meaning of the keyword. It's not a memory safety issue by itself; rather, it's a "I can't fathom why Rust, of all languages, implemented a warning rather than an error for overflowing integer literals".

@retep998 , if you're doing that then your code already contains #![allow(overflowing_literals)], surely? In that case, turning this lint to deny-by-default (as seems to be the gathering consensus) will have no effect on your code.

[bstrie]

@MajorBreakfast I think the suggestions that this be turned on today are mostly tongue-in-cheek; not only would it be rather controversial in the face of our backwards-compatibility guarantee, it would be mostly moot given that Rust 2018 is only a few months away anyway. :) It won't be added as an alternative to the RFC.

[ssokolow]

@RalfJung

Ugh. Last night was not a good night for me saying correct things.

I should know that. I've used -1 as a list index in Python how many times?

(That said, I suppose it demonstrates one way the footgun could fire if the programmer is tired or distracted.)

[MajorBreakfast]

I think the suggestions that this be turned on today are mostly tongue-in-cheek

@bstrie I figured that. But, stability is very important. Therefore, I had to write that comment, so that it can remain a just joke :)

[mark-i-m]

@retep998

const FOO: u32 = -1;

IMHO this is 100% hack, even in C. The correct way to do this IMHO should be either u32::MAX or !0u32.

[rpjohnst]

It may be a hack but it's an extremely widely used hack. The workaround of const FOO: u32 = -1 as u32 isn't too bad--it's noninvasive and fairly mechanical--but the fact is that bindgen-like situations will run into it all the time and it needs to be considered.

[retep998]

@RalfJung @mark-i-m Negative constants are not just limited to -1. Approximately half of all such negative unsigned constants I've bound so far have other values such as -699 or -1000. Your "suggestions" of u32::MAX or !0u32 are worthless in those cases.

[mark-i-m]

Approximately half of all such negative unsigned constants I've bound so far have other values such as -699 or -1000

That surprises me. What are such values used for?

[retep998]

@bstrie The thing forbidding negative unsigned constants is not this lint, but actually a hard error error[E0600]: cannot apply unary operator -to typeu32``. But yes, I do have a #![allow(overflowing_literals)] for exactly the opposite reason: large signed constants that overflow into negative. In fact I have 5,450 individual cases of constants which would trigger that lint, so anyone who wants to make that a hard error is evil.

For example, how would I write these constants if I couldn't have overflowing literals?
pub const HKEY_CLASSES_ROOT: HKEY = 0x80000000i32 as isize as HKEY
pub const VSS_E_ASRERROR_SYSTEM_PARTITION_HIDDEN: HRESULT = 0x80042416

[bstrie]

@rpjohnst if this is a concern for code generation via bindgen then bindgen can choose to insert #![allow(overflowing_literals)].

@retep998 , given that the RFC as written would allow the lint to be turned off, I think I'm unclear on what your objection is? You're already using the allow attribute, so you wouldn't notice if this were turned on tomorrow.

[nikomatsakis]

@rfcbot concern extern-C-code

I have mixed feelings on various axes.

First, I think that using this for "extern bindings" is a very legitimate use case -- as evidence by e.g. @retep998's comments etc. I am concerned that people who are attempting to do this may not realize they can -- and in fact are encouraged to! -- allow the lint. My experience is that people seem to dislike allowing lints in general and feel bad about it (which I do not entirely understand, I admit), and I think that will only be more intense for a deny-by-default lint.

This is part of why I've been reluctant to have deny-by-default lints at all. Ultimately it's probably silly to deny ourselves of the option, but it does feel like it just makes the range of diagnostic settings we offer that much more complex.

Perhaps what is missing for me is a good guideline of what makes something a deny-by-default lint: I guess it is "this is almost certainly a bug -- but there are a few legitimate reasons that are quite uncommon"? (This overlaps of course with #2476... traditionally rustc has been warn-by-default for lints whereas clippy has been deny-by-default for some categories...)

I wonder if some kind of tailored hint might resolve my first concern. e.g., adding to the lint something like, "If you are intentionally trying to achieve overflow behavior, then feel free to 'allow' the lint and leave a comment" or something like that? Either in the main lint error or perhaps the diagnostics "explain" text for the lint? (Probably we should have made allow take a comment syntactically, but oh well.)

As for the second concern, about wanting stricter guidelines, I don't think we'll decide them here, so that's ok, but I'd like to at some point write them down.

[nikomatsakis]

So, yes, to pull out my concern:

I think we should include some text in the lint (possibly in a --explain page) advising people how we recommend modeling the extern binding case. I think that @oli-obk's suggestion of MAX - V + 1 is ok, I personally also feel allowing the lint with a comment is ok too, though it may surprise some readers.

Presumably nobody will object to including such text, but I'd like it included in the RFC to help ensure it's not forgotten.

[Ixrec]

This is part of why I've been reluctant to have deny-by-default lints at all. Ultimately it's probably silly to deny ourselves of the option, but it does feel like it just makes the range of diagnostic settings we offer that much more complex.

Perhaps what is missing for me is a good guideline of what makes something a deny-by-default lint: I guess it is "this is almost certainly a bug -- but there are a few legitimate reasons that are quite uncommon"?

FWIW, before today I was honestly under the impression that deny-by-default lints existed only because we allow users to deny lints and lints have a default status, so the possibility just sort of exists on its own unless we actively prevent it from existing, and we've only started hearing about it recently because it turned out to be ideal for "this would be a hard error if we had a time machine" lints.

Several warn-by-default lints already arguably meet "this is almost certainly a bug", so maybe the non-time machine criterion is more like "the cases where you want this are so rare or so domain-specific you should explicitly annotate them with an #[allow()]". I do think const FOO: u32 = -1; meets that bar. In fact, I'm still don't understand why anyone would prefer that to MAX or !0 or whatever (outside of autogenerated-from-C code where you probably #[allow()] dozens of other lints already).

[RalfJung]

Much of this discussion has been about const FOO: u32 = -1; so I kind of assumed that would compile right now, but it actually does not -- and neither does let foo : u32 = -1. Both complain that you cannot use unary - on things of type u32. So, there's not even any working code that would need @oli-obk's suggestion, is there?

So, the only code that currently compiles that would make the lint fire is things like let x : u8 = 256; or const FOO : i32 = 0x80000000;. I do not see any good reason to be compatible with that part of C. The lint can just suggest to use 0x80000000 as i32 instead, right?

[scottmcm]

@nikomatsakis

Perhaps what is missing for me is a good guideline of what makes something a deny-by-default lint

My test: Is this something that's going to make what I'm trying right now not work?

An unused function isn't going to keep another function from working. Extra parens aren't going to keep something from working. An unused .clone() call is bad for perf, but isn't going to keep it from working. Whereas for i in 0..256, if it overflows, is going to make it not do what I wanted.

Also, #2484 would add T::wrapping_from which would be the explicit way to do things like u64::wrapping_from(-1) without needing to allow the lint.

[ollie27]

Are there any other lints going from warn to deny by default in edition 2018? It just seems like an unnecessary complication for the default lint level to differ based on edition here. const_err was recently made deny be default (rust-lang/rust#50653) without an edition bump even though it catches very similar errors like let x: u8 = 255 + 1;.

[RalfJung]

Are there any other lints going from warn to deny by default in edition 2018?

I was hoping we'd do this for rust-lang/rust#49441, but I am not sure if that will actually happen.

[scottmcm]

@nikomatsakis Would having wrapping_from (as in #2484) and suggesting the use of that in the lint (like i8::wrapping_from(200)) resolve your concern here?

[nikomatsakis]

@scottmcm

Would having wrapping_from .. and suggesting the use of that in the lint (like i8::wrapping_from(200)) resolve your concern here?

Probably, yes. I think that all lints should have some kind of "positive change" you can make to make the lint go away -- and ideally it should not be "allow the lint". So having a more explicit way to achieve the same thing seems pretty good to me. If that is rustfix-able (which it sounds like it would be), so much the better.

In that case, I am in favor. I don't know if we have to tie this to the edition, but I think we certainly could, at least to start. If we expect a lot of people to be affected (I'm not sure I do), perhaps that makes sense.

@rfcbot resolve extern-C-code

[nikomatsakis]

That said, we don't actually have #2484, do we?

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[tspiteri]

Imo the escape hatch is to replace -N with T::max_value() - N + 1 or to use -N_iT as uT as it has been suggested multiple times.

Or else even the more terse !N + 1, which has the advantage that you don't need to write the type anywhere as you would in T::max_value() or in as T.

It might look kinda hacky, but !N is T::max_value() - N as it's like subtracting N from the all ones value, which is the maximum value, and then the C-style -N is kinda hacky anyway.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#54502