Skip to content
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

Wrong warning?: literal out of range for i8 #48073

Closed
kkayal opened this issue Feb 8, 2018 · 20 comments
Closed

Wrong warning?: literal out of range for i8 #48073

kkayal opened this issue Feb 8, 2018 · 20 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@kkayal
Copy link

kkayal commented Feb 8, 2018

I have this code:

fn main() {
    println!("-1 = {:b}", -1 as i8);
    println!("0b11111111 = {}", 0b11111111 as i8);
}

Both lines return the expected result, namely

-1 = 11111111
0b11111111 = -1

However, the compiler raises a warning as follows:

warning: src\main.rs:3: literal out of range for i8
note: src\main.rs:3: #[warn(overflowing_literals)] on by default

To my understanding, 0b11111111 is a valid signed integer and means -1 in decimal. Therefore the warning seems to be wrong to me.

@hanna-kruppe
Copy link
Contributor

"all eight bits set to 1" is a valid bit pattern for i8, but the literal 0b11111111 describes the mathematical integer 255 (just as the literals 0xFF and 255 do), which does not fit into i8 and is lossily converted just as if you wrote 255u8 as i8. So the warning is correct in my opinion.

@kkayal
Copy link
Author

kkayal commented Feb 8, 2018

Thanks! Now I understand that the default type would be an unsigned integer and then there is a second step where a type casting happens and this is where the warning is raised. So this is clear to me and I would close the issue from my point of view.

But then I looked for an alternative syntax, which would allow me to tell the compiler that my bit pattern is meant to be a signed integer and found my answer under "Literals and operators" in rust by example. Here it says:

// Integer subtraction
println!("1 - 2 = {}", 1i32 - 2);
// TODO ^ Try changing 1i32 to 1u32 to see why the type is important

OK, so I have changed 1i32 to 1u32 and have seen that the compiler has taken the type into account correctly and raised correct warnings.

That means to me, for my example above I should have used 0b11111111i8 instead of 0b11111111 as i8 and the compiler would immediatly interpret my bit pattern as i8 and my warning would dissapear?

Unfortunatly, that didn't happen. I still get the warning. Now this has become a candidate for a different bug report, which could be just the following sentence:

It seems to me that the type is taken into account if the literal is a decimal, but not in binary form.

I haven't tried the hex form and other variable types, but this check would only makes sense after the 0b11111111i8 case is clarified.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Feb 8, 2018

Sorry, I'm not saying the 0b11111111 is interpreted as an unsigned integer in this context. It is treated as i8, but it's denoting a large positive value that does not fit into i8 and thus overflows. This overflow results in the bit pattern you intended, but the compiler complains that this is misleading. Consider 255i8 by analogy: clearly this is an error or at least misleading -- 255 does not fit into i8. Put differently, integer literals in general specify an integer value rather than a bit pattern, even when the literal is written in base 2 (or base 16). There is no literal syntax that is intended to be used for specifying an integer by its bit pattern. An "overflowing" literal does work, but it's contrary to the interpretation the language chose, and thus it's no surprise you get a warning.

I'd suggest just writing -1 if that's what you mean, or use an unsigned type if you want a "dumb container of bits".

@CAD97
Copy link
Contributor

CAD97 commented Feb 9, 2018

Or if you just want to specify "i8 with this representation":

fn main() {
    println!("-1 = {:b}", -1 as i8);
    println!("0b11111111 = {}", 0b11111111 as u8 as i8);
}

This has no warnings.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 9, 2018

Maybe the diagnostic could be improved in the presence of binary or hexadecimal representations. Since we lose the information about the original representation during AST generation we'll have to inspect the textual snippet though.

@kkayal
Copy link
Author

kkayal commented Feb 9, 2018

@rkruppe:

Sorry, I'm not saying the 0b11111111 is interpreted as an unsigned integer in this context. It is treated as i8, but it's denoting a large positive value that does not fit into i8 and thus overflows

In this case, I am afraid I have to disagree. If the syntax 0b11111111i8 means that the variable is already specified as an i8 to beginn with as "rust by example" explains and also proves that with the decimal example, then it should also be an i8. So the value should be directly -1 and not 255. Otherwise, the math is wrong.

Unfortunatly, using an u8 as a dumb container of bits and then typecasting to i8 seems to work, but that doesn't help about the warning and that warning indicates to me that something behind the scenes is not working the way I thought and that might lead to a bug some day.

@CAD97: Thanks for the workaround! Although I am happy that I can now achieve what I wanted, I am puzzled about this behaviour of the compiler. @rkruppe has explained why 0b11111111 as i8 gives a warning in his first reply. Although it is not really to my liking, I can follow that logic and very well live with it and 0b11111111i8 should fill the gap.

But, following the very same logic, 0b11111111 as u8 as i8 should give me the same warning, isn't it? In this case, 0b11111111 is first created as an unsigned variable, which is 255 in decimal. Then there is a type cast to an i8, but obviously 255 is out of the range of an i8 and here I should get the warning?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 9, 2018

Then there is a type cast to an i8, but obviously 255 is out of the range of an i8 and here I should get the warning?

This is covered by clippy (https://rust-lang-nursery.github.io/rust-clippy/master/index.html#cast_sign_loss) as an allow-by-default lint because there are real use cases for interpreting a u8 that's bigger than i8::max_value() as a negative number when casting to i8.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Feb 9, 2018

0b11111111i8 only differs from 0b11111111 in that it says right there the literal, rather than leaving it to type inference and falling back to i32 when it's not clear what type it should be. My point is, regardless of what base it's in and how (if at all) you communicate the type of the literal (<literal>i8, let x: i8 = <literal>;, <literal> as i8, etc.), the assumtion that Rust makes is that the programmer thought of a platonic number, then wrote it down in the source code and in the process picked a type.

A more clear cut example of this is foo(200) given fn foo(n: i8). It is pretty clear that this is a mistake, not an elaborate attempt to pass -56. Rust uniformly applies this reasoning (to all bases, and all integer types): If you write 0b11111111, Rust assumes you meant 255 in base 2 (just as it, reasonably, assumes you mean 200 when you write 200, and not -56), and 255 does not fit into i8, so it warns. You meant something different. This is just a disagreement, neither perspective is more or less "correct".

Because Rust does guarantee two's complement, this is just a surface level disagreement (the worst that can happen to the code in the OP is that the lint becomes deny-by-default and even this is unlikely) but it's probably still easier to "go with the flow" and adopt Rust's interpretation.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 9, 2018

cc @flip1995 this seems like a good entry level diagnostic change to make to the compiler (improving the reported lint in the case of hex/binary representations)

@estebank estebank added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. A-diagnostics Area: Messages for errors, warnings, and lints E-needs-mentor labels Feb 9, 2018
@flip1995
Copy link
Member

flip1995 commented Feb 9, 2018

Yeah, I'd like to try this!

But I'm not sure yet, what should be done here. I'm with @rkruppe , and think the warning is correct if you have an overflowing literal, regardless the representation.

I think of something like warning: literal will have a negative value with type _ as a better warning message. Should this only apply for hex/bin-literals with the type specified as the suffix like <hex/bin-literal>i8` or for every literal where the type is explicitly specified?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 9, 2018

While the warning is correct (and I, too, wouldn't change it), I think we should additionally produce a suggestion stating that if the user meant to create a literal with the exact bit representation, they should be doing $something_else

Should this only apply for hex/bin-literals with the type specified as the suffix like <hex/bin-literal>i8` or for every literal where the type is explicitly specified?

I think all hex/bin literals, no matter if explicit types are specified or not.

@estebank
Copy link
Contributor

I think we should add condensed version of @rkruppe's comment, along the lines of:

note: the literal `0b11111111` (decimal `255`) does not fit into an `i8` and will become `-1i8`

@estebank
Copy link
Contributor

CC #24361

@flip1995
Copy link
Member

I think this is a nice help message. Should we additionally suggest a type where the literal fits into?

e.g. iX -> uX if the literal fits in the corresponding unsigned type and iX -> iY (uX -> uY), Y > X, else.

@mrhollow
Copy link

mrhollow commented Mar 3, 2018

What if the programmer intends to make a negative number?

@estebank
Copy link
Contributor

estebank commented Mar 3, 2018

@mrhollow then they'll do so in an informed manner :)

@mrhollow
Copy link

mrhollow commented Mar 3, 2018

@estebank I respectfully disagree.

I think if someone knows enough to set binary, they probably have a grasp on how it works, and throwing a warning that I have to override or work around each time I set the leftmost bit, seems a little over the top.

Just my observation.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 3, 2018

@mrhollow the alternative would be a breaking change. I personally don't see a strong enough motivation to do that though.

@hanna-kruppe
Copy link
Contributor

@mrhollow As has been hashed out extensively earlier in this issue, this is essentially a question of how integer literals are interpreted -- as natural numbers, of as a way to spell out a bit pattern. The policy Rust has chosen here requires some extra work in the case where a programmer both wants to construct a negative number and wants to do so by writing "bits" rather than some other means, but on the other hand notifies people that don't want that of their error (this includes actual overflow like 1000u8, but also cases where you wanted an unsigned type but circumstances conspired to infer a signed type). You can't have both, so we have to choose. I think the current policy is a good trade off because an overflowing literal can be a serious and hard-to-find error, while a false positive in a warning is easy to work around (e.g., ignore the warning, write the number with negation or bitwise negation, cast an unsigned number, etc.) and often the work around makes the intent clearer anyway.

@mrhollow
Copy link

mrhollow commented Mar 3, 2018

@rkruppe Fair enough. :)

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 5, 2018
Suggest type for overflowing bin/hex-literals

Fixes rust-lang#48073

For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)).

Additionally it will suggest a type (`X < Y`):
- `iX`: if literal fits in `uX` => `uX`, else => `iY`
- `-iX` => `iY`
- `uX` => `uY`

Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case.

r? @oli-obk
kennytm added a commit to kennytm/rust that referenced this issue Mar 6, 2018
Suggest type for overflowing bin/hex-literals

Fixes rust-lang#48073

For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)).

Additionally it will suggest a type (`X < Y`):
- `iX`: if literal fits in `uX` => `uX`, else => `iY`
- `-iX` => `iY`
- `uX` => `uY`

Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case.

r? @oli-obk
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 6, 2018
Suggest type for overflowing bin/hex-literals

Fixes rust-lang#48073

For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)).

Additionally it will suggest a type (`X < Y`):
- `iX`: if literal fits in `uX` => `uX`, else => `iY`
- `-iX` => `iY`
- `uX` => `uY`

Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case.

r? @oli-obk
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Mar 6, 2018
Suggest type for overflowing bin/hex-literals

Fixes rust-lang#48073

For hexadecimal and binary literals, which overflow, it gives an additional note to the warning message, like in this [comment](rust-lang#48073 (comment)).

Additionally it will suggest a type (`X < Y`):
- `iX`: if literal fits in `uX` => `uX`, else => `iY`
- `-iX` => `iY`
- `uX` => `uY`

Exceptions: `isize`, `usize`. I don't think you can make a good suggestion here. The programmer has to figure it out on it's own in this case.

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

7 participants