improper_ctypes incorrectly warns on projections featuring opaque types

[lcnr]

cc @davidtwco

#![feature(type_alias_impl_trait)]

pub trait Foo {
    type Assoc;
}

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

type Bar = impl Foo;

fn assign() -> Bar {}

extern "C" {
    pub fn lint_me() -> <Bar as Foo>::Assoc;
}

Gives the following incorrect warning:

warning: `extern` block uses type `Bar`, which is not FFI-safe
  --> src/lib.rs:16:25
   |
16 |     pub fn lint_me() -> <Bar as Foo>::Assoc;
   |                         ^^^^^^^^^^^^^^^^^^^ not FFI-safe
   |
   = note: `#[warn(improper_ctypes)]` on by default
   = note: opaque types have no C equivalent

warning: 1 warning emitted

[davidtwco]

I'm not sure if this is incorrect - I think this is what I'd expect. In the declaration of lint_me, we don't know what concrete type Bar is - if it were (), then we could normalize to u32 - but we don't know? Maybe I'm missing something.

[lcnr]

Hmm, that might be true. I definitely think that the following should not lint here (with Assoc bound to u32)

#![feature(type_alias_impl_trait)]

pub trait Foo {
    type Assoc;
}

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

type Bar = impl Foo<Assoc = u32>;

fn assign() -> Bar {}

extern "C" {
    pub fn lint_me() -> <Bar as Foo>::Assoc;
}

[davidtwco]

I agree that seems like it should be fine. So, this happens because of this call:

rust/src/librustc_lint/types.rs

Lines 962 to 967 in a37c32e

	// We have to check for opaque types before `normalize_erasing_regions`,
	// which will replace opaque types with their underlying concrete type.
	if self.check_for_opaque_ty(sp, ty) {
	// We've already emitted an error due to an opaque type.
	return;
	}

It makes sense that we want to lint on opaque types on extern declarations, it's unclear whether we would want to allow projections which contain opaque types - this would solve the case you've shown here, but could be used to "sneak" opaque types by the lint (in this playground, the projection normalizes to an opaque type; it's hard to detect because the normalization goes further and resolves it to u32 so we can't tell that it was an opaque type) - I have a build with this locally.

Any thoughts on whether we should change the behavior here?

cc @hanna-kruppe

[lcnr]

Hm, I think it might be worth it to normalize using Reveal::UserFacing here, which should not normalize the first case and your example while fixing the second one if I am not mistaken.

[hanna-kruppe]

Long ago I filed #60855 where we also concluded Reveal::UserFacing is the right behavior for normalization in this lint. The current behavior comes from #64359 cc @varkor

[davidtwco]

I'll put up a PR with my experiments from last night later in that case.