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

rustc accepts "---128i8" as a valid constant #34395

Closed
eefriedman opened this issue Jun 21, 2016 · 12 comments · Fixed by #34497
Closed

rustc accepts "---128i8" as a valid constant #34395

eefriedman opened this issue Jun 21, 2016 · 12 comments · Fixed by #34497

Comments

@eefriedman
Copy link
Contributor

pub fn main() {
    const X: i8 = ---128;
    const Z: [u8; X as u8 as usize] = [0; X as u8 as usize];
    println!("{}", Z[0]);
}

This is clearly ridiculous, but rustc is perfectly happy with it.

Regression from 1.9.

@eefriedman eefriedman changed the title rustc accepts "---128" as a valid constant rustc accepts "---128i8" as a valid constant Jun 21, 2016
@tbu-
Copy link
Contributor

tbu- commented Jun 21, 2016

use std::mem;
pub fn main() {
    const X: i8 = ---128;
    const Z: [u8; X as u8 as usize] = [0; X as u8 as usize];
    println!("{}", mem::size_of_val(&Z));
}

prints 128.

Using the constant in any other way, e.g.

pub fn main() {
    const X: i8 = ---128;
    const Z: [u8; X as u8 as usize] = [0; X as u8 as usize];
    println!("{}", Z[0]);
    println!("{}", X);
}

errors out:

$ rustc a.rs
a.rs:2:20: 2:25 error: attempted to negate with overflow
a.rs:2     const X: i8 = ---128;
                          ^~~~~
error: aborting due to previous error

@oli-obk
Copy link
Contributor

oli-obk commented Jun 22, 2016

cc @eddyb is that fallout from your new const eval?

@eddyb
Copy link
Member

eddyb commented Jun 22, 2016

@oli-obk I haven't removed anything, not that I can think of, to cause this.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 22, 2016

Then it's very likely fallout from 735c018 (skipping double negations)

@eddyb
Copy link
Member

eddyb commented Jun 22, 2016

@oli-obk The remaining error in that testcase feels wrong. --128i8 should, IMO, be -(-128i8) - the literal is in range, but the negation overflows.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 22, 2016

Actually I think it should be valid code, but yield -128, at least that's what the original literal out of range lints do

@eddyb
Copy link
Member

eddyb commented Jun 22, 2016

@oli-obk But -(-128i8) overflows when negating, which is an error. Why should it be different than -(-64i8 - 64)?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 22, 2016

I have no particular opinion on this. But the lint has code handling double negations:

// propagate negation, if the negation itself isn't negated
(it's treating double negations as if they weren't there at all)

@dsprenkels
Copy link
Contributor

dsprenkels commented Jun 25, 2016

The double negation is not the issue here. The value of X evaluates to -128, as expected. But consider the following code:

fn main() {
    const X: i8 = -100;
    const Y: u8 = X as u8;
    println!("{}", Y);
}

This will also compile just fine and print 156. It goes wrong in the cast X as u8. Rust doesn't check if cast overflows, so the <u8> value becomes 156.

cast_const_int can be changed to disallow casts that fail. (I'd be happy to implement this myself.) Or we can accept that these casts are can overflow silently.

@tbu-
Copy link
Contributor

tbu- commented Jun 25, 2016

@dsprenkels I believe the issue is that --128 (aka 128) is an intermediate result of the computation that is out of bounds for the i8.

EDIT: The issue is about the fact that the compiler doesn't complain.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 25, 2016

jup, that was a misunderstanding of some code on my part + the issue that processing HIR works the wrong way around (you start with the outer most expression). Reverting 735c018 should do the job

@dsprenkels
Copy link
Contributor

dsprenkels commented Jun 25, 2016

I thought that it was intended that ---128 would succesfully evaluate to -128. (That rustc knows that it's overflowing, but is smart to know that double negation will fix it in the end.)

Pretend I said nothing.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 29, 2016
Revert "skip double negation in const eval"

This reverts commit 735c018.

fixes rust-lang#34395

The original commit was based on a mis-understanding of the overflowing literal lint.

This needs to be ported to beta.

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants