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

Unsoundness due to variance of trait objects WRT associated types #71550

Closed
trha opened this issue Apr 25, 2020 · 14 comments · Fixed by #71896
Closed

Unsoundness due to variance of trait objects WRT associated types #71550

trha opened this issue Apr 25, 2020 · 14 comments · Fixed by #71896
Assignees
Labels
A-trait-system Area: Trait system A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@trha
Copy link

trha commented Apr 25, 2020

IIUC currently a trait object is always covariant in its associated types, regardless of their positions.

trait Foo {
  type A;
  // `A` appears in contravariant position
  fn foo(&self, _: Self::A) {}
}

impl Foo for () {
  type A = &'static ();
}

fn bar<'a>(_: &'a dyn Foo<A = &'a ()>) {}

fn main() {
  let x = ();
  // Let 'x be the lifetime of `&x`
  // 'static <: 'x
  // `dyn Foo` covariant to `Foo::A`
  // `&dyn Foo<A = &'static ()>` <: `&dyn Foo<A = &'x ()>`
  bar(&x);
}

This makes it possible to pass a non-static reference to a function expecting a static reference:

use std::marker::PhantomData;

trait Foo {
    type A;
    type B;
    fn foo(&self, x: Self::A) -> Self::B;
}

struct Bar<T>(PhantomData<T>);

impl<T: 'static> Foo for Bar<T> {
    type A = &'static T;
    type B = &'static T;
    fn foo(&self, x: Self::A) -> Self::B {
        x
    }
}

fn bad<'a, T>(x: &dyn Foo<A = &'a T, B = &'static T>, k: &'a T) -> &'static T {
    x.foo(k)
}

/// Extends the lifetime of an arbitrary reference.
fn extend<'a, T>(x: &'a T) -> &'static T {
    bad(&Bar(PhantomData), x)
}

fn dangle_ref() -> &'static String {
    let x = "hello world".to_string();
    extend(&x)
}

fn main() {
    // This segfaults.
    println!("{}", dangle_ref());
}
@jonas-schievink jonas-schievink added A-trait-system Area: Trait system C-bug Category: This is a bug. I-nominated I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 25, 2020
@memoryruins
Copy link
Contributor

memoryruins commented Apr 25, 2020

With rustc 1.0.0 ..= 1.16.0, the following error was emitted:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter 'a in function call due to conflicting requirements
  --> <source>:25:5
   |
25 |     bad(&Bar(PhantomData), x)
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime 'a as defined on the body at 24:45...
  --> <source>:24:46
   |
24 |   pub fn extend<'a, T>(x: &'a T) -> &'static T {
   |  ______________________________________________^ starting here...
25 | |     bad(&Bar(PhantomData), x)
26 | | }
   | |_^ ...ending here
note: ...so that reference does not outlive borrowed content
  --> <source>:25:28
   |
25 |     bad(&Bar(PhantomData), x)
   |                            ^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that types are compatible (expected &'static T, found &T)
  --> <source>:25:9
   |
25 |     bad(&Bar(PhantomData), x)
   |         ^^^^^^^^^^^^^^^^^

error: aborting due to previous error

Compiler returned: 101

@Mark-Simulacrum
Copy link
Member

@nikomatsakis -- this looks plausibly similar to #44454, but I don't know enough to be certain.

@RalfJung
Copy link
Member

Oh wow how are these not invariant? Cc @eddyb

@spastorino
Copy link
Member

Assigning P-high as discussed as part of the Prioritization Working Group process and removing I-prioritize.

@spastorino spastorino added P-high High priority I-nominated and removed I-nominated I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 27, 2020
@spastorino
Copy link
Member

Just in case, in my last action I've removed I-nominated by accident and added it back :).

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 28, 2020

Wait, these are supposed to be invariant. This is definitely a bug. It would be great to figure out just which nightly regressed this, maybe somebody can run cargo bisect?

@rustbot cleanup

@nikomatsakis
Copy link
Contributor

Er, I guess I meant this?

@rustbot ping cleanup

@rustbot rustbot added the ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections label Apr 28, 2020
@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2020

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @jakevossen5 @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

@LeSeulArtichaut LeSeulArtichaut added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Apr 28, 2020
@hellow554
Copy link
Contributor

hellow554 commented Apr 29, 2020

searched toolchains nightly-2016-06-01 through nightly-2020-01-01
regression in nightly-2017-03-12

e4eb964...824c9eb

Previous error message:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter 'a in function call due to conflicting requirements
  --> src/main.rs:25:5
   |
25 |     bad(&Bar(PhantomData), x)
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime 'a as defined on the body at 24:41...
  --> src/main.rs:24:42
   |
24 |   fn extend<'a, T>(x: &'a T) -> &'static T {
   |  __________________________________________^ starting here...
25 | |     bad(&Bar(PhantomData), x)
26 | | }
   | |_^ ...ending here
note: ...so that reference does not outlive borrowed content
  --> src/main.rs:25:28
   |
25 |     bad(&Bar(PhantomData), x)
   |                            ^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that types are compatible (expected &'static T, found &T)
  --> src/main.rs:25:9
   |
25 |     bad(&Bar(PhantomData), x)
   |         ^^^^^^^^^^^^^^^^^

error: aborting due to previous error

I used a slightly modified variant (just removed the dyn IIRC):

Code used
use std::marker::PhantomData;

trait Foo {
    type A;
    type B;
    fn foo(&self, x: Self::A) -> Self::B;
}

struct Bar<T>(PhantomData<T>);

impl<T: 'static> Foo for Bar<T> {
    type A = &'static T;
    type B = &'static T;
    fn foo(&self, x: Self::A) -> Self::B {
        x
    }
}

fn bad<'a, T>(x: &Foo<A = &'a T, B = &'static T>, k: &'a T) -> &'static T {
    x.foo(k)
}

/// Extends the lifetime of an arbitrary reference.
fn extend<'a, T>(x: &'a T) -> &'static T {
    bad(&Bar(PhantomData), x)
}

fn dangle_ref() -> &'static String {
    let x = "hello world".to_string();
    extend(&x)
}

fn main() {
    // This segfaults.
    println!("{}", dangle_ref());
}

@varkor
Copy link
Member

varkor commented Apr 29, 2020

Most likely candidate seems to be #40319, cc @eddyb.

@nikomatsakis
Copy link
Contributor

Thanks! Super useful.

@spastorino spastorino added P-critical Critical priority and removed P-high High priority labels Apr 29, 2020
@spastorino
Copy link
Member

After discussing a bit with @nikomatsakis this sounds more like P-critical than P-high.

@jonas-schievink jonas-schievink added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Apr 29, 2020
@spastorino spastorino self-assigned this Apr 30, 2020
@bjorn3
Copy link
Member

bjorn3 commented May 6, 2020

@rustbot modify labels:-E-needs-bisection

@rustbot rustbot removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label May 6, 2020
@nikomatsakis
Copy link
Contributor

Crater run from the fix suggested 16 non-spurious regressions (thanks to @bjorn3 for the analysis).

I was thinking about that and I realize that it's plausible that we could do variance inference on associated types and use that to inform the variance for dyn types. It is sensible, for example, that a dyn Iterator<Item = X> is covariant with respect to X (that is the trait used in one of the affected examples).

I'm a bit nervous about this though. For one thing, the core operations exposed by a dyn type are clearly just its methods, but people can also project out from dyn types (though that projection is generally considered invariant over the input types). But for another, it seems to give rise to some asymmetry and complexity around traits and variance that we have thus far avoided (and which I might prefer to continue to avoid).

@Elinvynia Elinvynia added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 9, 2020
@LeSeulArtichaut LeSeulArtichaut removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 9, 2020
@bors bors closed this as completed in 3ddf480 Jun 11, 2020
Mark-Simulacrum added a commit to Mark-Simulacrum/rustc-perf that referenced this issue Jun 28, 2020
This was exposed by a recent fix on nightly to rust-lang/rust#71550.
Mark-Simulacrum added a commit to rust-lang/rustc-perf that referenced this issue Jun 28, 2020
This was exposed by a recent fix on nightly to rust-lang/rust#71550.
@workingjubilee workingjubilee added the A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) label Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.