Fix overconstrained Send impls in btree internals

[dtolnay]

Fixes dtolnay/async-trait#215.

Minimal repro:

use std::collections::btree_map::Iter;

fn require_send<T: Send>(_: T) {}

fn main() {
    require_send(async {
        let _iter = None::<Iter<(), &()>>;
        async {}.await;
    });
}

error: higher-ranked lifetime error
 --> src/main.rs:6:5
  |
6 | /     require_send(async {
7 | |         let _iter = None::<Iter<(), &()>>;
8 | |         async {}.await;
9 | |     });
  | |______^
  |
  = note: could not prove `impl Future<Output = ()>: Send`

Not-quite-so-minimal repro:

use std::collections::BTreeMap;
use std::future::Future;

fn spawn<T: Future + Send>(_: T) {}

async fn f() {
    let map = BTreeMap::<u32, Box<dyn Send + Sync>>::new();
    for _ in &map {
        async {}.await;
    }
}

fn main() {
    spawn(f());
}

error: higher-ranked lifetime error
  --> src/main.rs:14:5
   |
14 |     spawn(f());
   |     ^^^^^^^^^^
   |
   = note: could not prove `impl Future<Output = ()>: Send`

I am not familiar with the btree internals, but it seems clear to me that the async fn f above should return a Send future. Using HashMap instead of BTreeMap in that code makes it already return a Send future.

The "higher-ranked lifetime error" message may be a regression in Rust 1.63. Using older compilers the error message was more detailed:

error: implementation of `Send` is not general enough
  --> src/main.rs:14:5
   |
14 |     spawn(f());
   |     ^^^^^ implementation of `Send` is not general enough
   |
   = note: `Send` would have to be implemented for the type `alloc::collections::btree::node::NodeRef<alloc::collections::btree::node::marker::Immut<'0>, u32, Box<(dyn Send + Sync + '1)>, alloc::collections::btree::node::marker::LeafOrInternal>`, for any two lifetimes `'0` and `'1`...
   = note: ...but `Send` is actually implemented for the type `alloc::collections::btree::node::NodeRef<alloc::collections::btree::node::marker::Immut<'2>, u32, Box<dyn Send + Sync>, alloc::collections::btree::node::marker::LeafOrInternal>`, for some specific lifetime `'2`

error: implementation of `Send` is not general enough
  --> src/main.rs:14:5
   |
14 |     spawn(f());
   |     ^^^^^ implementation of `Send` is not general enough
   |
   = note: `Send` would have to be implemented for the type `alloc::collections::btree::node::NodeRef<alloc::collections::btree::node::marker::Immut<'0>, u32, Box<(dyn Send + Sync + '1)>, alloc::collections::btree::node::marker::Leaf>`, for any two lifetimes `'0` and `'1`...
   = note: ...but `Send` is actually implemented for the type `alloc::collections::btree::node::NodeRef<alloc::collections::btree::node::marker::Immut<'2>, u32, Box<dyn Send + Sync>, alloc::collections::btree::node::marker::Leaf>`, for some specific lifetime `'2`

[rust-highfive]

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[Aaron1011]

The general issue is #64552 - this fixes a particular instance by eliminating the need for rustc to attempt to prove a lifetime bound on the erased witness types.

As long as the new impl is sound, I think it's fine to merge this. However, users will continue to see these unhelpful errors in other cases until the underlying issue is resolved.

[thomcc]

Huh, good catch. It doesn't make sense that we'd bound those on the same lifetime we use in the marker, so this looks fine to me.

I don't think this needs libs-api sign-off, as it's hard to imagine how this could be intentionally forbidden, but you'd have a better idea (as someone actually part of that team) so I'll leave that choice to you.

r=me if you move the tests out of uitest (or feel strongly that they shouldn't be moved).

[thomcc]

I think most people don't bother to run the uitests locally for library patches, and the error message is the same as any other Sync error, so IMO its a bit better to do this as a compile_fail doctest on something private, and put check-pass in the normal tests.

That said this is unlikely to unexpectedly break in the future, so if you feel strongly that they should be a uitest, fine by me as-is.

[dtolnay]

Moved test to library/alloc/tests/autotraits.rs and extended it to cover alloc::collections more thoroughly, which revealed a linked_list::DrainFilter bug #43244 (comment).

I left out negative tests for now but I am thinking of following up with some based on https://github.com/dtolnay/proc-macro2/blob/1.0.46/tests/marker.rs or the static_assertions crate.

[thomcc]

@bors r+

[bors]

📌 Commit 4fdd0d9 has been approved by thomcc

It is now in the queue for this repository.