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

Mir undefined behaviour false positive when comparing Option<&T> #55454

Closed
jesskfullwood opened this issue Oct 28, 2018 · 10 comments
Closed

Mir undefined behaviour false positive when comparing Option<&T> #55454

jesskfullwood opened this issue Oct 28, 2018 · 10 comments
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@jesskfullwood
Copy link

jesskfullwood commented Oct 28, 2018

I discovered this bug while tinkering with the frunk crate.

#[derive(PartialEq, Debug)]
struct Cons<H, T: Hlist> {
    head: H,
    tail: T
}

#[derive(PartialEq, Debug)]
struct Nil;

trait Hlist {}
impl<H, T: Hlist> Hlist for Cons<H, T> {}
impl Hlist for Nil {}

fn main() {

    // This part works fine
    let x = Cons {head: Some(&0), tail: Nil};
    let y = Cons {head: Some(&0), tail: Nil};
    assert_eq!(x, y);

    // This apparently-equivalent code fails to compile (passes cargo-check but not cargo-build)
    assert_eq!(
        Cons {head: Some(&0), tail: Nil},
        Cons {head: Some(&0), tail: Nil}
    );
}

This code compiles and runs on stable.

➜  rustup default stable
info: using existing install for 'stable-x86_64-unknown-linux-gnu'
info: default toolchain set to 'stable-x86_64-unknown-linux-gnu'

  stable-x86_64-unknown-linux-gnu unchanged - rustc 1.30.0 (da5f414c2 2018-10-24)

➜ cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s    

But fails on nightly

➜  rustup default nightly
info: using existing install for 'nightly-x86_64-unknown-linux-gnu'
info: default toolchain set to 'nightly-x86_64-unknown-linux-gnu'

  nightly-x86_64-unknown-linux-gnu unchanged - rustc 1.31.0-nightly (cae6efc37 2018-10-27)

➜  cargo build
   Compiling typeck v0.1.0                                               
error[E0080]: it is undefined behavior to use this value                                                
  --> src/main.rs:15:1                                                                                  
   |                                                                                                    
15 | / fn main() {                                                                                      
16 | |                                                                                                  
17 | |     // This part works fine                                                                      
18 | |     let x = Cons {head: Some(&0), tail: Nil};                                                    
...  |                                                                                                  
26 | |     );                                                                                           
27 | | }                                                                                                
   | |_^ type validation failed: encountered a pointer, but expected something that cannot possibly be outside the (wrapping) range 1..=0
   |                                                                                                    
   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
                                                                                                        
error: aborting due to previous error                                                                   
                                                                                                        
For more information about this error, try `rustc --explain E0080`.                                     
error: Could not compile `typeck`.                                                                      

To learn more, run the command again with --verbose.
@jesskfullwood
Copy link
Author

jesskfullwood commented Oct 28, 2018

Reduced repro:

#[derive(PartialEq)]
struct This<T>(T);

fn main() {
    This(Some(&1)) == This(Some(&1));
}

@memoryruins
Copy link
Contributor

memoryruins commented Oct 29, 2018

nightly builds
rustc 1.31.0-nightly (4bd4e4130 2018-10-25) ✔️
rustc 1.31.0-nightly (3e6f30ec3 2018-10-26)
rustc 1.31.0-nightly (cae6efc37 2018-10-27)
rustc 1.31.0-nightly (96064eb61 2018-10-28)

@oli-obk oli-obk added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Oct 29, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2018

I cannot reproduce this on the playground

@RalfJung
Copy link
Member

Playground has an "ancient" nightly (2018-10-25).

@RalfJung
Copy link
Member

@memoryruins can you also test 2018-10-26?

So far, we got this commit range: 4bd4e41...cae6efc. @oli-obk's CTFE query PR landed there.

@memoryruins
Copy link
Contributor

(3e6f30ec3 2018-10-26) also produces the error

@RalfJung
Copy link
Member

@memoryruins thanks! Regression range is 4bd4e41...3e6f30e then.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2018

I have confirmed the issue to been exposed by the CTFE query PR.

We didn't use to validate promoteds. My PR only exposed the preexisting issue:

struct This<T>(T);

const C: This<Option<&i32>> = This(Some(&1));

Fails on nightly even before my PR.

@memoryruins
Copy link
Contributor

memoryruins commented Oct 29, 2018

Oh cool! cargo-bisect can pinpoint a regressing PR. Unable to try it out today, but definitely bookmarking :)

edit: wonder how it would handle this after seeing oli’s comment.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2018

wonder how it would handle this after seeing oli’s comment.

easy: bisect my example with the constant instead of the original one

I have a very good idea which change is at fault though: https://github.com/rust-lang/rust/pull/54762/files#diff-269a7510489262b796ff39a89b2dbdc2R283 was introduced replacing https://github.com/rust-lang/rust/pull/54762/files#diff-269a7510489262b796ff39a89b2dbdc2L142

fix-PR incoming

kennytm added a commit to kennytm/rust that referenced this issue Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

No branches or pull requests

4 participants