Make iter::Empty<T> Send and Sync for any T

[KamilaBorowska]

Just because it can be. Likely nobody will be using that property of iter::empty, but at the same time, there is no reason to not allow it.

[rust-highfive]

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

[dtolnay]

Thanks!

[dtolnay]

@rust-lang/libs FYI

@bors r+

[bors]

📌 Commit f973f37165249e18671306f4d380973647355f4c has been approved by dtolnay

[KamilaBorowska]

Hm, I guess it cannot be done currently, as for const fn, a PhantomData<fn() -> T> is considered to be a function pointer, and I don't feel like putting unsafe impl<T> Send for Empty<T> {}.

[dtolnay]

@bors r-

[dtolnay]

error: function pointers in const fn are unstable
   --> src/libcore/iter/sources.rs:277:11
    |
277 |     Empty(marker::PhantomData)
    |           ^^^^^^^^^^^^^^^^^^^

Strange. I guess an unsafe impl would be the way to go.

[KamilaBorowska]

I wonder if special casing PhantomData in const fn check would make sense, but doing so would be way out of scope of this pull request.

[dtolnay]

function pointers in const fn are unstable

It sounds like this has been implemented already but not stabilized yet. Adding #![feature(const_fn)] makes the same error disappear in the playground. But libcore/lib.rs already has #![feature(const_fn)]:

rust/src/libcore/lib.rs

Line 73 in ceb2512

#![feature(const_fn)]

Any suggestions @oli-obk?

[Centril]

@dtolnay For #[stable] items in the standard library we impose an additional restriction that it must conform to min_const_fn and if it doesn't we emit an error. Having #![feature(const_fn)] does not affect that. You can see this by enabling #![feature(staged_api)].

The recourse you have here is to use something other than fn(T) to get the covariance and auto trait behavior you want. I think &'static T should work here but I would double check.

[dtolnay]

&'static T would impose T: 'static, and is only Send if T is Sync. playground

@xfix -- I would go ahead and add unsafe impls. playground

[Centril]

@dtolnay oh right; yeah, unsafe impls it is :)

[KamilaBorowska]

Or, I have another idea how to do it without unsafe.

[KamilaBorowska]

Arguably abusing a bug, but IMO PhantomData<fn() -> T> should be allowed, even in minimal const fn.

[Centril]

@xfix That's not a bug; an intentional choice was made in min_const_fn to not recurse into data types.

[KamilaBorowska]

@Centril I don't know, compiler rejecting PhantomData<fn() -> T> but being fine with PhantomData<PhantomFnWorkaround<T>> sounds inconsistent to me. Like, I understand why the rule is what it is - it's necessary for instance for Vec::<fn()>::new() to be const fn, as on the lowest levels, Vec<T> is using PhantomData<T>. And this is fine, except I don't understand why reject PhantomData<fn() -> T> in const fn subset.

[Centril]

@xfix It's an intentional inconsistency ;) (see #53604 (review), #53604 (comment)) and doesn't really have to do with Vec::<fn()>::new(). Anyways, I'm fine with the solution here.

[dtolnay]

This is a little crazy for my taste. I would prefer to see plain old unsafe impls over this.

Or, add a more readable and better tested mechanism internal to libcore for establishing variance and auto traits. By standardizing on something like this over PhantomData<T> / PhantomData<fn() -> T> / PhantomData<PhantomFnWorkaround<T>>, the variance and auto trait impls become clearly visible during code reviews and we are more likely to catch when they are wrong for some type.

pub struct Empty<T>(phantom_data![T, Covariant + AlwaysSendSync]);

We would want to support any combination of {Covariant, Contravariant, Invariant} × {AlwaysSendSync, NeverSendSync, InheritSendSync}.

[Centril]

Ping from triage @xfix, can you update to the unsafe impls as requested?

[Dylan-DPC-zz]

ping from triage @xfix any updates on this?

[Dylan-DPC-zz]

ping from triage @xfix Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

[KamilaBorowska]

Please re-open this merge request.

That's pretty funny. I said this a year ago.

Likely nobody will be using that property of iter::empty, but at the same time, there is no reason to not allow it.

And then now I'm writing code that went

= help: the trait `std::marker::Sync` is not implemented for `dyn postgres_types::ToSql`
...
= note: required because it appears within the type `std::iter::Empty<&dyn postgres_types::ToSql>`
...

The code looks like this.

    pub async fn fetch_fortunes(&self) -> Vec<Fortune> {
        self.client
            .query_raw(&self.fortune, iter::empty())
            .await
            .unwrap()
            .map(|row| row.unwrap())
            .map(|row| Fortune {
                id: row.get(0),
                message: Cow::Owned(row.get(1)),
            })
            .collect::<Vec<_>>()
            .await
    }

Empty<T> not being Sync is the only issue.

[Dylan-DPC-zz]

@xfix given the difference between when the PR was closed and now, it would be easier to create a new PR :)

[Mark-Simulacrum]

Notably, we cannot reopen this merge request due to GitHub limitations. We would be happy to review a new version!