`thirtyfour` non-`Send` witness regression

[lcnr]

cargo download thirtyfour@0.33.0-alpha.2
cd thirtyfour-0.33.0-alpha.2
RUSTFLAGS=-Znext-solver cargo +nightly b

results in 16 similar errors

error[E0277]: `{coroutine witness@src/extensions/query/element_query.rs:460:43: 505:6}` cannot be sent between threads safely
   --> src/components/wrapper/resolver.rs:366:9
    |
361 |             async move {
    |             ---------- within this `{async block@src/components/wrapper/resolver.rs:361:13: 361:23}`
...
366 |         Self::new_custom(base_element, resolver)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `{coroutine witness@src/extensions/query/element_query.rs:460:43: 505:6}` cannot be sent between threads safely
    |
...

this error is surprising. By testing with cargo rustc -- -Ztreat-err-as-bug the ICE does happen in query check_coroutine_obligations as it should. We apparently actually do a witness which previously implemented Send but no longer does so.

Would be good to minimize this issue to figure out what exactly is going wrong.

[Threated]

Spend some time minimizing:

trait AsyncFn: Send + 'static {
    type Fut: Future<Output = ()> + Send;

    fn call(&self) -> Self::Fut;
}

fn filter<I, P>(filters: I) -> impl Future<Output = ()>
where
    I: IntoIterator<Item = Box<P>>,
    P: AsyncFn + ?Sized,
{
    async move {
        let func = filters.into_iter().next().unwrap();
        func.call().await;
    }
}

async fn calls_it() {
    let filters: Vec<Box<dyn AsyncFn<Fut = BoxFuture<'static, ()>>>> = Vec::new();
    filter(filters).await;
}

fn observe_fut_not_send() {
    assert_send(calls_it());
}

fn assert_send<T: Send>(t: T) -> T {
    t
}

pub type BoxFuture<'a, T> = std::pin::Pin<Box<dyn Future<Output = T> + Send + 'a>>;

Things I found:

1. Either removing the 'static super trait bound on AsyncFn or adding a Send bound to the RPIT of filter and I: Send make the error go away.
2. With the following changes this code fails to compile in the old solver and ICEs in the new solver:

fn filter2<P>(filter: &P) -> impl Future<Output = ()>
where
    P: AsyncFn + ?Sized,
{
    async move {
        filter.call().await;
    }
}


async fn calls_it() {
    let filters: Vec<Box<dyn AsyncFn<Fut = BoxFuture<'static, ()>>>> = Vec::new();
    filter2(filters.first().unwrap().as_ref()).await;
}

3. While minimizing this I also found another ICE but that may be related to 2. maybe I will look into it some other time.

[Threated]

Simpler version of 1:

trait AsyncFn: Send + 'static {
    type Fut: Future<Output = ()> + Send;

    fn call(&self) -> Self::Fut;
}

type BoxFuture<'a, T> = std::pin::Pin<Box<dyn Future<Output = T> + Send + 'a>>;
type DynAsyncFnBoxed = dyn AsyncFn<Fut = BoxFuture<'static, ()>>;

fn wrap_call<P: AsyncFn + ?Sized>(func: Box<P>) -> impl Future<Output = ()> {
    func.call()
}

fn get_boxed_fn() -> Box<DynAsyncFnBoxed> {
    todo!()
}

async fn cursed_fut() {
    wrap_call(get_boxed_fn()).await;
}

fn observe_fut_not_send() {
    fn assert_send<T: Send>(t: T) -> T {
        t
    }
    assert_send(cursed_fut());
}

[Threated]

This is the ice I was talking about in 3:

trait AsyncFn: Send + Sync + 'static {
    type Fut: Future<Output = ()> + Send + 'static;

    fn call(&self) -> Self::Fut;
}

impl<Fut, Fun> AsyncFn for Fun
where
    Fun: Fn() -> Fut + Send + Sync + 'static,
    Fut: Future<Output = ()> + Send + 'static,
{
    type Fut = Fut;

    fn call(&self) -> Fut {
        self()
    }
}

type BoxFuture<'a, T> = std::pin::Pin<Box<dyn Future<Output = T> + Send + 'a>>;
type DynAsyncFnBoxed = dyn AsyncFn<Fut = BoxFuture<'static, ()>>;

fn wrap_call<P>(func: Box<P>) -> impl Future<Output = ()> + Send
where
    P: AsyncFn + ?Sized,
{
    func.call()
}

fn get_boxed_fn() -> Box<DynAsyncFnBoxed> {
    todo!()
}

async fn cursed_fut() {
    wrap_call(get_boxed_fn()).await;
}

pub fn triger_ice() {
    fn takes_resolver<F: AsyncFn>(r: F) {
        Box::new(move || Box::pin(r.call()) as BoxFuture<'static, ()>) as Box<DynAsyncFnBoxed>;
    }
    takes_resolver(|| cursed_fut());
}

The trait bounds are slightly different than in the example above to make it work. The main difference is the added Send bound of the RPIT of wrap_call. The older solver compiles this with or without the bound while the new one rejects to compile without it and ICEs with it.

[Threated]

Also here is a minimal version of 2 which fails to compile in the old solver and ICEs in the new one:

trait AsyncFn: Send + 'static {
    type Fut: Future<Output = ()> + Send;

    fn call(&self) -> Self::Fut;
}

async fn wrap_call<P: AsyncFn + ?Sized>(filter: &P) {
    filter.call().await;
}

fn get_boxed_fn() -> Box<DynAsyncFnBoxed> {
    todo!()
}

async fn cursed_fut() {
    wrap_call(get_boxed_fn().as_ref()).await;
}

fn observe_fut_not_send() {
    assert_send(cursed_fut());
}

fn assert_send<T: Send>(t: T) -> T {
    t
}

pub type BoxFuture<'a, T> = std::pin::Pin<Box<dyn Future<Output = T> + Send + 'a>>;
type DynAsyncFnBoxed = dyn AsyncFn<Fut = BoxFuture<'static, ()>>;

The ICE error message seems different from the one in 3 but the root cause is probably the same cursed future.

I hope this helps 🙈

[lcnr]

Thanks a lot ❤️ I still need to take the time to look into this issue and while I would be happy for someone else to figure this out, it seems challenging. Assume this happens because the new solver introduces an outlives constraint which isn't there in the old solver

[adwinwhite]

Based on this minimization:

trait AsyncFn: Send + 'static {
    type Fut: Future<Output = ()> + Send;

    fn call(&self) -> Self::Fut;
}

type BoxFuture<'a, T> = std::pin::Pin<Box<dyn Future<Output = T> + Send + 'a>>;
type DynAsyncFnBoxed = dyn AsyncFn<Fut = BoxFuture<'static, ()>>;

fn wrap_call<P: AsyncFn + ?Sized>(func: Box<P>) -> impl Future<Output = ()> {
    func.call()
}

fn get_boxed_fn() -> Box<DynAsyncFnBoxed> {
    todo!()
}

async fn cursed_fut() {
    wrap_call(get_boxed_fn()).await;
}

fn observe_fut_not_send() {
    fn assert_send<T: Send>(t: T) -> T {
        t
    }
    assert_send(cursed_fut());
}

The witness of cursed_fut is not Send due to leak check failure.
All static lifetimes in DynAsyncFnBoxed, or dyn AsyncFn<Fut = BoxFuture<'static, ()>> + 'static are transformed into placeholders which can't outlive 'static. The region constraints come from the trait definition AsyncFn: 'static.

The whole transformation:
'static -> {erased} -> '^1/ '^1_0 -> '!1_1 / '!1_0, respectively by
mir_coroutine_witnesses -> coroutine_hidden_types -> enter_forall in probe_and_evaluate_goal_for_constituent_tys.

Similar issues: 135470, 140577, 119095.

[lcnr]

Hmm, why does this error with the new solver but not the old one? There has to be something which is checked by the new solver but not by the old one right now 🤔

[adwinwhite]

I guess that it has something to do with where we do leak check.

In the old solver, we do leak check mostly at two places:

• in evaluate_root_obligation which uses a branch new infcx and ignore regions when evaluating predicates. So we can skip this one.
• in evaluate_candidate which is possible to trigger leak check failure if infcx.consider_regions = true. But we often don't consider regions in typeck. Another problem is that we don't evaluate candidate if only one candidate is collected.
  At one point we do have constraints added into infcx by processing outlive obligations, but it's not really consumed by leak check as most obligations are satisfied by cache at this time.

In the new solver, we do leak check after trying to evaluate all added goals.
Since canonical response includes region constraints, constraints are always kept no matter the goals are satisfied via cache or not.
Thus we have all regions constraints accumulated, and checked later.

[lcnr]

• At one point we do have constraints added into infcx by processing outlive obligations, but it's not really consumed by leak check as most obligations are satisfied by cache at this time.

What's surprising to me is that we should never do just evaluation if we would have added any region constraints. We should always prove these region constraints via fulfill at which point we should still get unsatisfied placeholder outlives, even if it doesn't cause a leak check failure.

So somehow we unsoundly use the (normalization?) cache without proving its nested obligations. So I guess the next question is: where do we use evaluate for something referencing regions without then also proving the same goals in fulfill?

[adwinwhite]

I'm not sure I fully understand your comment.

The following observation is still based on the minimization above.
In the round of process_obligations where we end up having placeholder outlives constraints, when we process obligation TraitPredicate(<(dyn AsyncFn<Fut = Pin<Box<(dyn Future<Output = ()> + Send + '!1_0)>>> + '!1_1) as AsyncFn>, it hits candidate cache and the confirmation produces tons of nested obligations including outlive predicates which are then added into fulfillment queue and processed as well. That's how we get the constraints in RegionConstraintCollector in infcx.

Back when there's no candidate cache for this AsyncFn trait predicate, it doesn't trigger leak check(evaluate_candidate) since it has only one object candidate.

No leak check is triggered in this round of process_obligations except a global predicate which is handled by evaluate_root_obligation. Although these obligations basically all hit candidate cache, nested goals are all properly proved.

[lcnr]

I'm not sure I fully understand your comment.

The following observation is still based on the minimization above. In the round of process_obligations where we end up having placeholder outlives constraints, when we process obligation TraitPredicate(<(dyn AsyncFn<Fut = Pin<Box<(dyn Future<Output = ()> + Send + '!1_0)>>> + '!1_1) as AsyncFn>, it hits candidate cache and the confirmation produces tons of nested obligations including outlive predicates which are then added into fulfillment queue and processed as well. That's how we get the constraints in RegionConstraintCollector in infcx.

Back when there's no candidate cache for this AsyncFn trait predicate, it doesn't trigger leak check(evaluate_candidate) since it has only one object candidate.

No leak check is triggered in this round of process_obligations except a global predicate which is handled by evaluate_root_obligation. Although these obligations basically all hit candidate cache, nested goals are all properly proved.

This is in the old solver? So we add these region constraints and they do not result in region checking errors when they are not detected by the leak check?

I am working on the assumption that any code which gets a trait solver error due to the leak check would have been guaranteed to get a region checking error without it.

---

No leak check is triggered in this round of process_obligations except a global predicate which is handled by evaluate_root_obligation. Although these obligations basically all hit candidate cache, nested goals are all properly proved.

Is this (or the similar fast path for Projection goals) responsible that this compiles in the old solver? https://github.com/rust-lang/rust/blob/f2bae990e89bf696e86706b524ac32ab72945438/compiler/rustc_trait_selection/src/traits/fulfill.rs#L840-L851

With my current understanding of this issue, I expect that the fact this compiles is a bug in the old trait solver and this should actually error in both.