Remove 'static requirement on try_as_dyn

[oli-obk]

View all comments

tracking issue: #144361

cc @ivarflakstad @izagawd

[rustbot]

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[rustbot]

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[danielhenrymantilla]

Some drive-by comments; but I'm not rustc/HIR-savy, so take these with a grain of salt 🙇

View changes since this review

[BoxyUwU]

r? BoxyUwU

[oli-obk]

The hacky solution is obviously not a general fix. But I think it's progress. As a next step I will add the input type as a generic parameter on TryAsDynCompat, at which point we should be able to enforce (in borrowck) that the input type outlives any lifetimes on the dyn Trait or its generic parameters. So if a generic parameter T has a 'static bound, it could be used as an input type for a try_as_dyn irrespective of the bounds on the dyn Trait. In the other direction, we will likely end up rejecting many traits that have generic parameters as the bounds are not something that can be written in Rust.

[oli-obk]

change the TryAsDynCompatible trait to be a bound like dyn Trait: TryAsDynCompatible<'a>, and have a separate SourceType: 'a bound.

hmm... and desugar dyn Trait: TryAsDynCompatible<'a> to adding a constraint on the lifetime 'foo of the trait object? Bidirectional outlives bounds? It feels a bit icky to connect it that way, but I guess so does (): TryAsDynCompatible

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[oli-obk]

@rustbot ready

[BoxyUwU]

If we don't need to check elaborated clauses does that mean we actually don't need to check any predicates since we'll wind up proving them in Reflection mode too

View changes since the review

[oli-obk]

well, we do need to prevent outlives bounds, but I guess we could allow any trait bound that doesn't repeat generic params or uses 'static

[BoxyUwU]

this feels kind of arbitrary to me, Self parameters are not special in any way in the type system afaik, what's the reason we want this

View changes since the review

[oli-obk]

The reason is basically that things like T: Debug get checked for fully-genericness when the trait solver actually gets to the impl for it, so recursively everything will be fine. There is probably more we can allow, but this one was easy to reason about

[BoxyUwU]

why other than implied bounds? unlike normal specialization we don't have a base impl whose lifetime constraints have been checked to hold which we can rely on.

impl<T> Trait for &'_ T seems problematic for try_as_dyn given noone can ever actually check T: '_

View changes since the review

[BoxyUwU]

i guess you can rely on wfness of the traitref the user wrote for the try_as_dyn 🤔 so yeah maybe that is fine?

[BoxyUwU]

why do we not handle reservation impls in reflection mode. is there a test for try_as_dyn'ing a trait ref which has a reservation impl

View changes since the review

[oli-obk]

We do handle them, this is just the code path for when we fail to resolve the impl anyway. In case the impl is fully generic for reflection we fall through to the normal code path below

[BoxyUwU]

@rustbot author

[BoxyUwU]

why no assoc type bounds? this trait just exists to ensure that the type being try_as_dyn'd outlives the lifetime bound of the resulting trait object right?

View changes since the review

[oli-obk]

We'd need to find some other way to handle

#![feature(type_info, ptr_metadata, arbitrary_self_types_pointers)]

use std::any::TypeId;
use std::ptr::{self, DynMetadata};

type Payload = Box<i32>;

trait Trait {
    type Assoc;
    fn method(self: *const Self, value: Self::Assoc) -> &'static Payload;
}
struct Thing;
impl Trait for Thing {
    type Assoc = &'static Payload;
    fn method(self: *const Self, value: Self::Assoc) -> &'static Payload {
        value
    }
}

fn extend<'a>(payload: &'a Payload) -> &'static Payload {
    let metadata: DynMetadata<dyn Trait<Assoc = &'a Payload>> = const {
        TypeId::of::<Thing>()
            .trait_info_of::<dyn Trait<Assoc = &'a Payload>>()
            //~^ ERROR `dyn Trait<Assoc = &'a Box<i32>>: TryAsDynCompatible<'_>` is not satisfied
            .unwrap()
            .get_vtable()
    };
    let ptr: *const dyn Trait<Assoc = &'a Payload> =
        ptr::from_raw_parts(std::ptr::null::<()>(), metadata);
    ptr.method(payload)
}

fn main() {
    let payload: Box<Payload> = Box::new(Box::new(1i32));
    let wrong: &'static Payload = extend(&*payload);
    drop(payload);
    println!("{wrong}");
}

So I punted on all assoc types

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rust-log-analyzer]

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[TIMING:end] tool::Tidy { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu } -- 0.000
fmt check
Diff in /checkout/compiler/rustc_middle/src/ty/mod.rs:284:
     /// For trait impls, checks whether
     /// * the type and trait only use generic lifetime arguments (and no concrete ones like `'static`), and
     /// * uses any generic param (lifetime or type) only once.
-    /// 
+    ///
     /// This is a pessimistic analysis, so it will reject alias types
     /// and other types that may be actually ok. We can allow more in the future.
     ///