Skip to content

Commit

Permalink
Rollup merge of #129021 - compiler-errors:ptr-cast-outlives, r=lcnr
Browse files Browse the repository at this point in the history
Check WF of source type's signature on fn pointer cast

This PR patches the implied bounds holes slightly for #129005, #25860.

Like most implied bounds related unsoundness fixes, this isn't complete w.r.t. higher-ranked function signatures, but I believe it implements a pretty good heuristic for now.

### What does this do?

This PR makes a partial patch for a soundness hole in a `FnDef` -> `FnPtr` "reifying" pointer cast where we were never checking that the signature we are casting *from* is actually well-formed. Because of this, and because `FnDef` doesn't require its signature to be well-formed (just its predicates must hold), we are essentially allowed to "cast away" implied bounds that are assumed within the body of the `FnDef`:

```
fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }

fn bad<'short, T>(x: &'short T) -> &'static T {
    let f: fn(_, &'short T) -> &'static T = foo;
    f(&&(), x)
}
```

In this example, subtyping ends up casting the `_` type (which should be `&'static &'short ()`) to some other type that no longer serves as a "witness" to the lifetime relationship `'short: 'static` which would otherwise be required for this call to be WF. This happens regardless of if `foo`'s lifetimes are early- or late-bound.

This PR implements two checks:
1. We check that the signature of the `FnDef` is well-formed *before* casting it. This ensures that there is at least one point in the MIR where we ensure that the `FnDef`'s implied bounds are actually satisfied by the caller.
2. Implements a special case where if we're casting from a higher-ranked `FnDef` to a non-higher-ranked, we instantiate the binder of the `FnDef` with *infer vars* and ensure that it is a supertype of the target of the cast.

The (2.) is necessary to validate that these pointer casts are valid for higher-ranked `FnDef`. Otherwise, the example above would still pass even if `help`'s `'a` lifetime were late-bound.

### Further work

The WF checks for function calls are scattered all over the MIR. We check the WF of args in call terminators, we check the WF of `FnDef` when we create a `const` operand referencing it, and we check the WF of the return type in #115538, to name a few.

One way to make this a bit cleaner is to simply extend #115538 to always check that the signature is WF for `FnDef` types. I may do this as a follow-up, but I wanted to keep this simple since this leads to some pretty bad NLL diagnostics regressions, and AFAICT this solution is *complete enough*.

### Crater triage

Done here: #129021 (comment)

r? lcnr
  • Loading branch information
matthiaskrgr authored Sep 6, 2024
2 parents d678b81 + 67804c5 commit e903b29
Show file tree
Hide file tree
Showing 8 changed files with 145 additions and 12 deletions.
71 changes: 64 additions & 7 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1979,19 +1979,76 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {

match cast_kind {
CastKind::PointerCoercion(PointerCoercion::ReifyFnPointer) => {
let fn_sig = op.ty(body, tcx).fn_sig(tcx);
let src_sig = op.ty(body, tcx).fn_sig(tcx);

// HACK: This shouldn't be necessary... We can remove this when we actually
// get binders with where clauses, then elaborate implied bounds into that
// binder, and implement a higher-ranked subtyping algorithm that actually
// respects these implied bounds.
//
// This protects against the case where we are casting from a higher-ranked
// fn item to a non-higher-ranked fn pointer, where the cast throws away
// implied bounds that would've needed to be checked at the call site. This
// only works when we're casting to a non-higher-ranked fn ptr, since
// placeholders in the target signature could have untracked implied
// bounds, resulting in incorrect errors.
//
// We check that this signature is WF before subtyping the signature with
// the target fn sig.
if src_sig.has_bound_regions()
&& let ty::FnPtr(target_fn_tys, target_hdr) = *ty.kind()
&& let target_sig = target_fn_tys.with(target_hdr)
&& let Some(target_sig) = target_sig.no_bound_vars()
{
let src_sig = self.infcx.instantiate_binder_with_fresh_vars(
span,
BoundRegionConversionTime::HigherRankedType,
src_sig,
);
let src_ty = Ty::new_fn_ptr(self.tcx(), ty::Binder::dummy(src_sig));
self.prove_predicate(
ty::ClauseKind::WellFormed(src_ty.into()),
location.to_locations(),
ConstraintCategory::Cast { unsize_to: None },
);

let src_ty = self.normalize(src_ty, location);
if let Err(terr) = self.sub_types(
src_ty,
*ty,
location.to_locations(),
ConstraintCategory::Cast { unsize_to: None },
) {
span_mirbug!(
self,
rvalue,
"equating {:?} with {:?} yields {:?}",
target_sig,
src_sig,
terr
);
};
}

let src_ty = Ty::new_fn_ptr(tcx, src_sig);
// HACK: We want to assert that the signature of the source fn is
// well-formed, because we don't enforce that via the WF of FnDef
// types normally. This should be removed when we improve the tracking
// of implied bounds of fn signatures.
self.prove_predicate(
ty::ClauseKind::WellFormed(src_ty.into()),
location.to_locations(),
ConstraintCategory::Cast { unsize_to: None },
);

// The type that we see in the fcx is like
// `foo::<'a, 'b>`, where `foo` is the path to a
// function definition. When we extract the
// signature, it comes from the `fn_sig` query,
// and hence may contain unnormalized results.
let fn_sig = self.normalize(fn_sig, location);

let ty_fn_ptr_from = Ty::new_fn_ptr(tcx, fn_sig);

let src_ty = self.normalize(src_ty, location);
if let Err(terr) = self.sub_types(
ty_fn_ptr_from,
src_ty,
*ty,
location.to_locations(),
ConstraintCategory::Cast { unsize_to: None },
Expand All @@ -2000,7 +2057,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
self,
rvalue,
"equating {:?} with {:?} yields {:?}",
ty_fn_ptr_from,
src_ty,
ty,
terr
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//@ check-pass
//@ known-bug: #25860

static UNIT: &'static &'static () = &&();

fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T, _: &()) -> &'a T { v }

fn bad<'a, T>(x: &'a T) -> &'static T {
let f: fn(_, &'a T, &()) -> &'static T = foo;
f(UNIT, x, &())
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Regression test for #129021.

static UNIT: &'static &'static () = &&();

fn foo<'a: 'a, 'b: 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }

fn bad<'a, T>(x: &'a T) -> &'static T {
let f: fn(_, &'a T) -> &'static T = foo;
//~^ ERROR lifetime may not live long enough
f(UNIT, x)
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: lifetime may not live long enough
--> $DIR/implied-bounds-on-nested-references-plus-variance-early-bound.rs:8:12
|
LL | fn bad<'a, T>(x: &'a T) -> &'static T {
| -- lifetime `'a` defined here
LL | let f: fn(_, &'a T) -> &'static T = foo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`

error: aborting due to 1 previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Regression test for #129021.

trait ToArg<T> {
type Arg;
}
impl<T, U> ToArg<T> for U {
type Arg = T;
}

fn extend_inner<'a, 'b>(x: &'a str) -> <&'b &'a () as ToArg<&'b str>>::Arg { x }
fn extend<'a, 'b>(x: &'a str) -> &'b str {
(extend_inner as fn(_) -> _)(x)
//~^ ERROR lifetime may not live long enough
}

fn main() {
let y = extend(&String::from("Hello World"));
println!("{}", y);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: lifetime may not live long enough
--> $DIR/implied-bounds-on-nested-references-plus-variance-unnormalized.rs:12:5
|
LL | fn extend<'a, 'b>(x: &'a str) -> &'b str {
| -- -- lifetime `'b` defined here
| |
| lifetime `'a` defined here
LL | (extend_inner as fn(_) -> _)(x)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function was supposed to return data with lifetime `'b` but it is returning data with lifetime `'a`
|
= help: consider adding the following bound: `'a: 'b`

error: aborting due to 1 previous error

Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
//@ check-pass
//@ known-bug: #25860

// Should fail. The combination of variance and implied bounds for nested
// references allows us to infer a longer lifetime than we can prove.
// Regression test for #129021.

static UNIT: &'static &'static () = &&();

fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }

fn bad<'a, T>(x: &'a T) -> &'static T {
let f: fn(_, &'a T) -> &'static T = foo;
//~^ ERROR lifetime may not live long enough
f(UNIT, x)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: lifetime may not live long enough
--> $DIR/implied-bounds-on-nested-references-plus-variance.rs:8:12
|
LL | fn bad<'a, T>(x: &'a T) -> &'static T {
| -- lifetime `'a` defined here
LL | let f: fn(_, &'a T) -> &'static T = foo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`

error: aborting due to 1 previous error

0 comments on commit e903b29

Please sign in to comment.