New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deny the `overflowing_literals` lint for the 2018 edition #2438

Merged
merged 6 commits into from Sep 23, 2018

Conversation

Projects
None yet
@bstrie
Contributor

bstrie commented May 11, 2018

@scottmcm

This comment has been minimized.

Member

scottmcm commented May 11, 2018

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

@ollie27

This comment has been minimized.

ollie27 commented May 11, 2018

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

This comment has been minimized.

bluetech commented May 11, 2018

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

This comment has been minimized.

Contributor

bstrie commented May 11, 2018

@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

This comment has been minimized.

Contributor

bstrie commented May 11, 2018

@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

This comment has been minimized.

Contributor

Ixrec commented May 11, 2018

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 comment has been minimized.

Contributor

mark-i-m commented May 14, 2018

This would also prevent subtle bugs like

fn foo(x: u8) { }

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

This comment has been minimized.

Contributor

bstrie commented May 14, 2018

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

This comment has been minimized.

Contributor

mark-i-m commented May 14, 2018

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

@scottmcm

This comment has been minimized.

Member

scottmcm commented May 14, 2018

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

This comment has been minimized.

Member

retep998 commented May 16, 2018

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

This comment has been minimized.

ssokolow commented May 16, 2018

@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

This comment has been minimized.

Member

RalfJung commented May 16, 2018

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

This comment has been minimized.

Contributor

MajorBreakfast commented May 16, 2018

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

This comment has been minimized.

Contributor

bstrie commented May 16, 2018

@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

This comment has been minimized.

Contributor

bstrie commented May 16, 2018

@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

This comment has been minimized.

ssokolow commented May 16, 2018

@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

This comment has been minimized.

Contributor

MajorBreakfast commented May 16, 2018

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 :)

@Centril Centril added the T-lang label May 16, 2018

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented May 16, 2018

@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

This comment has been minimized.

rpjohnst commented May 16, 2018

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

This comment has been minimized.

Member

retep998 commented May 16, 2018

@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

This comment has been minimized.

Contributor

mark-i-m commented May 16, 2018

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

This comment has been minimized.

Member

retep998 commented May 16, 2018

@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

This comment has been minimized.

Contributor

bstrie commented May 16, 2018

@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

This comment has been minimized.

Contributor

nikomatsakis commented Jul 7, 2018

@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

This comment has been minimized.

Contributor

nikomatsakis commented Jul 7, 2018

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 comment has been minimized.

Contributor

Ixrec commented Jul 7, 2018

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

This comment has been minimized.

Member

RalfJung commented Jul 7, 2018

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

This comment has been minimized.

Member

scottmcm commented Jul 8, 2018

@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

This comment has been minimized.

ollie27 commented Jul 14, 2018

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

This comment has been minimized.

Member

RalfJung commented Jul 15, 2018

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

This comment has been minimized.

Member

scottmcm commented Aug 20, 2018

@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

This comment has been minimized.

Contributor

nikomatsakis commented Sep 11, 2018

@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

This comment has been minimized.

Contributor

nikomatsakis commented Sep 11, 2018

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

@rfcbot

This comment has been minimized.

rfcbot commented Sep 11, 2018

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

@tspiteri

This comment has been minimized.

tspiteri commented Sep 12, 2018

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

This comment has been minimized.

rfcbot commented Sep 21, 2018

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

@Centril Centril merged commit 933b90a into rust-lang:master Sep 23, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 23, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#54502

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment