lint/ctypes: Don't warn on sized structs with PhantomData.

[emilio]

Fixes #34798

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nikomatsakis]

Can you add a brief comment here indicating what this test is testing for? (with a link to the issue, ideally)

[nagisa]

Or just call the test issue-xxyyzz, as it makes it easy to look up how the test came to be just from the filename.

[nikomatsakis]

Can we remove this check_kind parameter and instead pass back FfiPhantom (in which case a to-level caller will error out?). Or, alternatively, return FfiPhantomWrapper or something so that the top-level can distinguish the two cases? Threading the check_kind parameter doesn't seem necessary and is kind of annoying.

[emilio]

Sure, all this mess was mainly to distinguish the top-level phantom-data case from the zero-size struct. But @nagisa is right that not doing it simplifies a bunch this code, so we can also just use the same error message.

[nagisa]

Why is this in a run-pass test?

[emilio]

Because it's a simple wrapper over PhantomData, and itsn't used directly, but inside a sized struct.

So the type used directly in an FFI interface would raise the warning (the compile-fail test checks for that), but I've allowed in a sized struct for the same reason I've allowed PhantomData. Let me know if it's not sound for some reason.

[nagisa]

The PR seems overcomplicated to me. Wouldn’t something along the lines of returning FfiSafe for PhantomData and then checking that any of the fields is not a phantom data?

let is_phantom_safe = adt_def.struct_variant().fields().iter().any(|field| {
    !(field_type).sty.is_phantom_data() 
});

or some such?

---

The comments point to an observation that rustc cannot trust repr(C), but I’d argue that rustc ought to trust it – primarily because I do not want rustc to report double warnings for something like this:

#[repr(C)] struct A(PhantomData<i32>); // reports warning here
#[repr(C)] struct B(A); // and also here if checks are recursive

which is sure to happen if some sort of recursive checks are done (and if we do not want rustc to report double warnings, recursive checks are not necessary).

[emilio]

Ok, this approach should be quite simpler :)

[nikomatsakis]

@nagisa I believe the point of the comment is that if one has the following:

#[repr(C)]
struct Foo { bar: Bar }
struct Bar { x: u32 }

This is not sufficient to allow C code like the following to use fields reliably:

// woah, it hurts my head to write these declarations now:
struct Foo { Bar bar; };
struct Bar { u32 x; };

[nagisa]

@nikomatsakis

Of course it isn’t. Bar is not repr(C) and can be trivially declared to be non-FFI-safe just by looking at its repr attribute. What I’m not understanding is how recursive checking was at all necessary in the previous patch.

They removed the recursion now, so no point in discussing that anymore.

[nikomatsakis]

@nagisa ok, I must be confused, but I would consider the fact that we must examine the types of the fields to decide if Foo is repr(C) to be "recursive". I'll note that the comment is still present and that check_type_for_ffi is still recursive -- but maybe you are referring to some other recursion?

[nikomatsakis]

@emilio why is it important to highlight the fact that the zero-sized type consists only of phantom data? Isn't it true that any zero-sized type is suspicious, and that is the real problem here?

[emilio]

It's not necessarily important. It's just that we happen to have that info. I can omit it if you want.

[emilio]

(Note that other kind of zero-sized types get reported with different messages by the struct or union checks).

[nikomatsakis]

@bors r+

[bors]

📌 Commit e866d07 has been approved by nikomatsakis