Rust 1.84 sometimes allows overlapping impls in incremental re-builds

[steffahn]

This repro uses a bit of lexical comments trickery, but only for convenience. (Quite useful while manually testing multiple rust versions), so the whole change you need to do between incremental compilations is turning

// /* // <- uncomment this line

into

/* // <- uncomment this line

The main relevant change that this entails is

impl Trait for W {}

turning into

impl Trait for S<W> {}

which makes the Other-impls overlapping. As an additional effect, the code turning the overlap into UB is also uncommented. (The const _ … stuff only “fixes” the * symbols left behind by the */* in the middle.)

trait Trait {}

struct S0<T>(T);

struct S<T>(T);
impl<T> Trait for S<T> where S0<T>: Trait {}

struct W;

trait Other {
    type Choose<L, R>;
}

struct A;
struct B;

// first impl
impl<T: Trait> Other for T {
    type Choose<L, R> = L;
}

// second impl
impl<T> Other for S<T> {
    type Choose<L, R> = R;
}

const _: u8 = 0

// /* // <- uncomment this line

*0;

impl Trait for W {}

pub fn transmute<L, R>(l: L) -> R {
    todo!();
}

const _: u8 = 0
*/*
0;

impl Trait for S<W> {}

fn use_first_impl<T: Trait, L, R>(l: L) -> <<T as TyEq>::To as Other>::Choose<L, R> {
    l
}

fn use_second_impl<T, L, R>(l: <S<T> as Other>::Choose<L, R>) -> R {
    l
}

trait TyEq {
    type To;
}
impl<T> TyEq for T {
    type To = T;
}

fn transmute_inner<W, T, L, R>(l: L) -> R
where
    T: Trait + TyEq<To = S<W>>,
{
    use_second_impl::<W, L, R>(use_first_impl::<T, L, R>(l))
}

pub fn transmute<L, R>(l: L) -> R {
    transmute_inner::<W, S<W>, L, R>(l)
}

const _: u8 =
// */
0;

fn main() {
    let v = vec![65_u8, 66, 67];
    let s: String = transmute(v);
    println!("{}", s);
}

Reproduce

cargo new repro
cd repro
…write above to src/main…
cargo run
…uncomment the line in question as described…
cargo run

   Compiling repro v0.1.0 (/home/frank/repro)
warning: struct `A` is never constructed
  --> src/main.rs:14:8
   |
14 | struct A;
   |        ^
   |
   = note: `#[warn(dead_code)]` on by default

warning: struct `B` is never constructed
  --> src/main.rs:15:8
   |
15 | struct B;
   |        ^

warning: `repro` (bin "repro") generated 2 warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running `target/debug/repro`
ABC

Is safe code that compiles to UB, but only in incremental re-builds (compilation error otherwise), considered a soundness issue to be labelled I-unsound?

This was already fixed with #133828 (which also explains what was the underlying issue). That PR seems like a fairly small & straightforward fix to me… should it perhaps be considered for backporting to stable? cc @compiler-errors

@rustbot label regression-from-stable-to-stable, T-compiler, A-incr-comp, A-coherence

[steffahn]

For context, I ran into this accidentally. (Without prior knowledge of #133828.)

Granted I was trying to explore the behavior of overlap checks; my first reaction this was “wait, why does this compile despite the overlap?” Only a while later (after making a bunch of changes, e.g. crafting the whole exploitation into transmute) it was even more surprising how it didn’t work on the playground. Only then I realized it might be an incremental compilation issue and confirmed via rebuilding after cargo clean.

I.e: when they don’t ICE, incr-comp issues can be very confusing to the user, especially if they result in illegal code being accepted rather than legal code erroring. My main concern wouldn’t be the “tricky safe code could be unsound” aspect of the issue as much as the “normal users might accidentally create overlapping trait impls and be really confused when it creates compilation errors potentially only much much later”, [assuming that such an overlap can realistically happen for normal users accidentally, that aren’t deliberately exploring the behavior of overlap checks].

[compiler-errors]

Yep, let's backport it at least to 1.85.

[steffahn]

The fix should already be on track for 1.85 naturally (and beta doesn’t reproduce, confirming the milestones labelling).

[compiler-errors]

oh, lemme un-beta-nominate it then

[compiler-errors]

Also would be cool if this test was written with cfg's rather than commenting. Then we could probably add it to the suite.

[steffahn]

Yes, that'd definitely be cleaner. TBH, I simply didn't know before that cfgs work for testing incremental compilation 😆

[zachs18]

Here's a cfg'd version (with separate cfg(before) and cfg(after) flags; could easily be rewritten to cfg(foo) and cfg(not(foo)) instead):

main.rs

trait Trait {}

struct S0<T>(T);

struct S<T>(T);
impl<T> Trait for S<T> where S0<T>: Trait {}

struct W;

trait Other {
    type Choose<L, R>;
}

struct A;
struct B;

// first impl
impl<T: Trait> Other for T {
    type Choose<L, R> = L;
}

// second impl
impl<T> Other for S<T> {
    type Choose<L, R> = R;
}



#[cfg(before)]
impl Trait for W {}

#[cfg(before)]
pub fn transmute<L, R>(l: L) -> R {
    todo!();
}


#[cfg(after)]
impl Trait for S<W> {}

#[cfg(after)]
fn use_first_impl<T: Trait, L, R>(l: L) -> <<T as TyEq>::To as Other>::Choose<L, R> {
    l
}

#[cfg(after)]
fn use_second_impl<T, L, R>(l: <S<T> as Other>::Choose<L, R>) -> R {
    l
}

#[cfg(after)]
trait TyEq {
    type To;
}
#[cfg(after)]
impl<T> TyEq for T {
    type To = T;
}

#[cfg(after)]
fn transmute_inner<W, T, L, R>(l: L) -> R
where
    T: Trait + TyEq<To = S<W>>,
{
    use_second_impl::<W, L, R>(use_first_impl::<T, L, R>(l))
}

#[cfg(after)]
pub fn transmute<L, R>(l: L) -> R {
    transmute_inner::<W, S<W>, L, R>(l)
}


fn main() {
    let v = vec![65_u8, 66, 67];
    let s: String = transmute(v);
    println!("{}", s);
}

$ RUSTFLAGS="--cfg before" cargo run
// cargo output
thread 'main' panicked at main.rs:34:5:
not yet implemented
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ RUSTFLAGS="--cfg after" cargo run
// cargo output
ABC

or, without cargo (edition doesn't appear to matter, as long as it's consistent)

$ rustc -Cincremental=incremental --cfg before main.rs && ./main
// rustc output
thread 'main' panicked at main.rs:34:5:
not yet implemented
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ rustc -Cincremental=incremental --cfg after main.rs && ./main
// rustc output
ABC

(all on 1.84.0 stable)

I couldn't get a version using cfg(feature = "...") working with cargo feature flags, so I assume cargo clears the incremental cache on feature changes? (but not RUSTFLAGS changes?)

[lqd]

Incremental tests require special cfgs to model fail/pass etc as well. I had already done this locally so I've opened PR #135522 for it. (I didn't end up including the actual safe transmute exploit into the test, just that the overlapping impl is rejected)