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

Use covariance on type relations of field projection types if possible #107969

Merged
merged 2 commits into from
Feb 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
return PlaceTy::from_ty(self.tcx().ty_error());
}
}
place_ty = self.sanitize_projection(place_ty, elem, place, location);
place_ty = self.sanitize_projection(place_ty, elem, place, location, context);
}

if let PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) = context {
Expand Down Expand Up @@ -630,12 +630,14 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
}
}

#[instrument(skip(self), level = "debug")]
fn sanitize_projection(
&mut self,
base: PlaceTy<'tcx>,
pi: PlaceElem<'tcx>,
place: &Place<'tcx>,
location: Location,
context: PlaceContext,
) -> PlaceTy<'tcx> {
debug!("sanitize_projection: {:?} {:?} {:?}", base, pi, place);
let tcx = self.tcx();
Expand Down Expand Up @@ -713,8 +715,11 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
match self.field_ty(place, base, field, location) {
Ok(ty) => {
let ty = self.cx.normalize(ty, location);
if let Err(terr) = self.cx.eq_types(
debug!(?fty, ?ty);

if let Err(terr) = self.cx.relate_types(
ty,
self.get_ambient_variance(context),
fty,
location.to_locations(),
ConstraintCategory::Boring,
Expand Down Expand Up @@ -743,9 +748,10 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
let ty = self.sanitize_type(place, ty);
let ty = self.cx.normalize(ty, location);
self.cx
.eq_types(
base.ty,
.relate_types(
ty,
self.get_ambient_variance(context),
base.ty,
location.to_locations(),
ConstraintCategory::TypeAnnotation,
)
Expand All @@ -760,6 +766,13 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
self.tcx().ty_error()
}

fn get_ambient_variance(&self, context: PlaceContext) -> ty::Variance {
match context {
PlaceContext::MutatingUse(_) => ty::Invariant,
PlaceContext::NonMutatingUse(_) | PlaceContext::NonUse(_) => ty::Covariant,
Copy link
Contributor

@lcnr lcnr Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's NonUseContext::AscribeUserTy which I feel should maybe be invariant 🤔

I can also imagine us adding NonMutatingUse in the future which should be invariant. Can you also exhaustively match on the NonMutatingUseContext and probably make all NonUse invariant. Don't know how to write an example where Covariance is needed here.

Considering that covariance can be unsound if used incorrectly, we should be as defensive as possible here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late answer.

We would need NonUseContext::AscribeUserTy for field projections on enums in match statements where we always insert user type projections I believe, e.g.

struct Inv<'a>(&'a mut &'a ());
enum Foo<T> {
    Bar,
    Var(T),
}
type Supertype = Foo<for<'a> fn(Inv<'a>, Inv<'a>)>;

fn foo_nested(x: Foo<Foo<for<'a, 'b> fn(Inv<'a>, Inv<'b>)>>) {
    match x {
        Foo::Bar => {}
        Foo::Var(Supertype::Bar) => {}
        Foo::Var(Supertype::Var(x)) => {}
    }
}

would not compile if we would use ty::Invariant on NonUse(AscribeUserTy).

I agree that we should be very conservative here, but I also can't see how using covariance here might be unsound. Can you show that it is? I've kept this as covariant for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my worry is that we use AscribeUserTy in a position which isn't covariant. E.g. only for arguments of a function we're calling. Thinking more about it I actually can't imagine that happening 🤔

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AscribeUserType statement contains a variance. Should it be carried by NonUseContext::AscribeUserTy for use here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think so 👍 while I don't think is exploitable as we only use contravariance in AscribeUserType for places without any projections. Using the following in super_ascribe_user_ty does not cause any failures

                if variance != $(& $mutability)? ty::Variance::Covariant && !place.projection.is_empty() {
                    bug!()
                }

we should still add the variance to the use context though as this isn't something I want to implicitly rely on.

}
}

fn field_ty(
&mut self,
parent: &dyn fmt::Debug,
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/mir/field-projection-invariant.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// build-pass
struct Inv<'a>(&'a mut &'a ());
enum Foo<T> {
Bar,
Var(T),
}
type Supertype = Foo<for<'a> fn(Inv<'a>, Inv<'a>)>;

fn foo(x: Foo<for<'a, 'b> fn(Inv<'a>, Inv<'b>)>) {
match x {
Supertype::Bar => {}
Supertype::Var(x) => {}
}
}

fn foo_nested(x: Foo<Foo<for<'a, 'b> fn(Inv<'a>, Inv<'b>)>>) {
match x {
Foo::Bar => {}
Foo::Var(Supertype::Bar) => {}
Foo::Var(Supertype::Var(x)) => {}
}
}

fn main() {}
19 changes: 19 additions & 0 deletions tests/ui/mir/field-projection-mutating-context.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use std::sync::Mutex;

static GLOBAL: Mutex<&'static str> = Mutex::new("global str");

struct Foo<T>(T); // `T` is covariant.

fn foo() {
let mut x: Foo<for<'a> fn(&'a str)> = Foo(|_| ());
let Foo(ref mut y): Foo<fn(&'static str)> = x;
//~^ ERROR mismatched types
*y = |s| *GLOBAL.lock().unwrap() = s;
let string = String::from("i am shortlived");
(x.0)(&string);
}

fn main() {
foo();
println!("{}", GLOBAL.lock().unwrap());
}
12 changes: 12 additions & 0 deletions tests/ui/mir/field-projection-mutating-context.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0308]: mismatched types
--> $DIR/field-projection-mutating-context.rs:9:13
|
LL | let Foo(ref mut y): Foo<fn(&'static str)> = x;
| ^^^^^^^^^ one type is more general than the other
|
= note: expected fn pointer `for<'a> fn(&'a str)`
found fn pointer `fn(&str)`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
17 changes: 17 additions & 0 deletions tests/ui/mir/field-projection-mutating-context2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
use std::sync::Mutex;

static GLOBAL: Mutex<&'static str> = Mutex::new("global str");

struct Foo<T>(T); // `T` is covariant.

fn foo<'a>(mut x: Foo<fn(&'a str)>, string: &'a str) {
let Foo(ref mut y): Foo<fn(&'static str)> = x;
//~^ ERROR lifetime may not live long enough
*y = |s| *GLOBAL.lock().unwrap() = s;
(x.0)(&string);
}

fn main() {
foo(Foo(|_| ()), &String::from("i am shortlived"));
println!("{}", GLOBAL.lock().unwrap());
}
10 changes: 10 additions & 0 deletions tests/ui/mir/field-projection-mutating-context2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: lifetime may not live long enough
--> $DIR/field-projection-mutating-context2.rs:8:25
|
LL | fn foo<'a>(mut x: Foo<fn(&'a str)>, string: &'a str) {
| -- lifetime `'a` defined here
LL | let Foo(ref mut y): Foo<fn(&'static str)> = x;
| ^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`

error: aborting due to previous error

15 changes: 15 additions & 0 deletions tests/ui/mir/field-ty-ascription-enums.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// build-pass

enum Foo<T> {
Var(T),
} // `T` is covariant.

fn foo<'b>(x: Foo<for<'a> fn(&'a ())>) {
let Foo::Var(x): Foo<fn(&'b ())> = x;
}

fn foo_nested<'b>(x: Foo<Foo<for<'a> fn(&'a ())>>) {
let Foo::Var(Foo::Var(x)): Foo<Foo<fn(&'b ())>> = x;
}

fn main() {}
37 changes: 37 additions & 0 deletions tests/ui/mir/field-ty-ascription.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// build-pass

struct Foo<T>(T); // `T` is covariant.

struct Bar<T> {
x: T,
} // `T` is covariant.

fn bar<'b>(x: Bar<for<'a> fn(&'a ())>) {
let Bar { x }: Bar<fn(&'b ())> = x;
}

fn bar_nested<'b>(x: Bar<Bar<for<'a> fn(&'a ())>>) {
let Bar { x: Bar { x } }: Bar<Bar<fn(&'b ())>> = x;
}

fn bar_foo_nested<'b>(x: Bar<Foo<for<'a> fn(&'a ())>>) {
let Bar { x: Foo ( x ) }: Bar<Foo<fn(&'b ())>> = x;
}

fn foo<'b>(x: Foo<for<'a> fn(&'a ())>) {
let Foo(y): Foo<fn(&'b ())> = x;
}

fn foo_nested<'b>(x: Foo<Foo<for<'a> fn(&'a ())>>) {
let Foo(Foo(y)): Foo<Foo<fn(&'b ())>> = x;
}

fn tuple<'b>(x: (u32, for<'a> fn(&'a ()))) {
let (_, y): (u32, fn(&'b ())) = x;
}

fn tuple_nested<'b>(x: (u32, (u32, for<'a> fn(&'a ())))) {
let (_, (_, y)): (u32, (u32, fn(&'b ()))) = x;
}

fn main() {}