ty: projections in transparent_newtype_field

[davidtwco]

Fixes #73249.

This PR modifies transparent_newtype_field so that it handles
projections with generic parameters, where normalize_erasing_regions
would ICE.

[rust-highfive]

r? @eddyb

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

[lcnr]

I don't think this is the best approach here.
Afaict the following still causes an ICE:

pub trait Foo {
    type Assoc: 'static;
}

impl Foo for () {
    type Assoc = u32;
}

extern "C" {
    pub fn lint_me(x: Bar<()>);
}

#[repr(transparent)]
pub struct Bar<T: Foo> {
    value: &'static <T as Foo>::Assoc,
}

Afaik extern functions must be fully monomorphic (ignoring lifetimes), in which case you should probably have relevant substs here.

[davidtwco]

I don't think this is the best approach here.

I've force pushed over the commit to make the case you highlighted work - well spotted.

I added the function being modified here in #72890. It deliberately does not apply substitutions so that it can identify the field in a transparent struct which is a non-ZST. If the non-zero-sized field were a generic parameter, and if it were substituted for () and we didn't do this then the previous logic would find that all fields were zero-sized and lint incorrectly for that reason. Using this function, we can identify which field is normally non-zero-sized, and handle that case correctly.

Given that, I think that having this function is the appropriate approach and it being unable to handle this case is just an oversight. If you have a better approach, then I'm open to making changes though.

[lcnr]

Your current approach still seems somewhat unfortunate to me, I think this warns on:

use std::marker::PhantomData;

trait Foo {
    type Assoc;
}

impl Foo for () {
    type Assoc = PhantomData<()>;
}

#[repr(transparent)]
struct Wow<T> where T: Foo<Assoc = PhantomData<T>> {
    x: <T as Foo>::Assoc,
    v: u32,
}

extern "C" {
    fn test(v: Wow<()>);
}

It deliberately does not apply substitutions so that it can identify the field in a transparent struct which is a non-ZST. If the non-zero-sized field were a generic parameter, and if it were substituted for () and we didn't do this then the previous logic would find that all fields were zero-sized and lint incorrectly for that reason.

I can't quite follow you here, do you explicitly not want to lint for

#[repr(transparent)]
struct Foo<T> {
    inner: T,
}

extern "C" {
    fn foo(v: Foo<()>);
}

If I understand this correctly, structs with #[repr(transparent)] must have exactly 0 or 1 non zero sized fields. Would it work to add FfiResult::FfiZst and delay emitting errors on ZST fields of transparent structs similar to what is already done with FfiPhantom.

[davidtwco]

Your current approach still seems somewhat unfortunate to me, I think this warns on:

It does - I've pushed a commit that fixes this and simplifies transparent_newtype_field further.

I can't quite follow you here, do you explicitly not want to lint for

This case does lint and it should.

The previous behavior for transparent types was to skip any that were zero-sized. Consider the second example from your comment, with struct Foo, and the old logic:

• In argument position, this type's fields would all be skipped and the type would be considered "composed only of PhantomData" - that's not entirely correct, but the lint would trigger, so we're good.
• In return position, this type is equivalent to () - which shouldn't lint, because that's just a function which doesn't return anything - that was the subject of Types equal to/wrapping () result in "not FFI-safe" warning #66202. However, a lint would be produced, as the type is still considered "composed only of PhantomData".

With the new behavior for transparent types, where the non-ZST field is identified and only that field is checked, we would correctly determine that the () is FFI-unsafe; but that result can then be threaded up to the logic that says "ignore that FFI-unsafe result if this is the return type and it was a ()".

In the previous behaviour, the FfiPhantom that we had contained the whole type (not just the field's type) and so we couldn't handle that return-type case easily.

There are certainly other ways I could have solved this - I don't think that identifying the non-ZST field is the only way, but it's quite explicit about what we're checking and I prefer that; I am open to better approaches, but I don't think using a FfiZst would necessarily handle that case (it could be made to, in the same way as the FfiPhantom approach taken previously to transparent types could have but I don't think that would be better necessarily).

---

I've pushed some other commits too:

• I noticed that the old logic had actually been duplicated for enum variants and unions, so I've unified these under some functions.
• When looking into improper_ctypes incorrectly warns on projections featuring opaque types #73251, I found some cases (some of which required this patch to not ICE) where opaque types in struct fields (via type_alias_impl_trait) weren't being linted before normalization (there are explicit pre-normalization checks for opaque types in the signature, but not inside of types - which seemed incorrect based on Should improper_ctypes pierce through impl Trait? #60855).
  • I'd like to unify this logic with check_for_opaque_type but it requires the changes I'm working on for improper_ctypes incorrectly warns on projections featuring opaque types #73251 (update: lint: normalize projections using opaque types #73287).

[lcnr]

LGTM, the new transparent_newtype_field now also looks good 👍

[davidtwco]

Pushed a minor commit re-wording to re-trigger CI as the last failure was spurious.

[jonas-schievink]

r? @varkor

[varkor]

@bors r=lcnr,varkor

[bors]

📌 Commit 61b24fd2561beccc4787878272c96b52d4d8a0a8 has been approved by lcnr,varkor

[spastorino]

@bors p=1

[bors]

⌛ Testing commit 61b24fd2561beccc4787878272c96b52d4d8a0a8 with merge 70af37bb5fb964d44612cfb095dc2c498e8f2a6e...

[jonas-schievink]

@bors r=lcnr,varkor

[bors]

📌 Commit a730d88 has been approved by lcnr,varkor

[bors]

⌛ Testing commit a730d88 with merge 3406beccc47c0d5beafc066cb3b14dbba08f1298...

[Manishearth]

@bors retry yield

[bors]

⌛ Testing commit a730d88 with merge 2d8bd9b...

[Manishearth]

@bors yield

[Manishearth]

@bors retry yield

[bors]

☀️ Test successful - checks-azure
Approved by: lcnr,varkor
Pushing 2d8bd9b to master...

[Manishearth]

wh...what the hell? @pietroalbini this had yielded to the rollup, but it still merged

is yield implemented properly?

[Manishearth]

I think this is how those "test timed out" locks happen, because bors will continue testing the rollup and then realize it can't merge it

[pietroalbini]

wh...what the hell? @pietroalbini this had yielded to the rollup, but it still merged

is yield implemented properly?

@Manishearth "yield" does not exist in the bors code, is just a comment saying why the PR was retried. The yield usually works because when the retry happens bors starts testing the PR with higher priority, and cancelbot kills the previous build since a new commit was pushed to auto.