Should improper_ctypes pierce through impl Trait?

[hanna-kruppe]

While reviewing #60300 I noticed for the first time that the improper_ctypes lint normalizes with the "reveal_all" mode. I don't have a nuanced understanding of what that means exactly but based on the general description in the rustc docs and experimentation (see below) I think that's probably the wrong choice for this lint, because it exposes details to the user that are normally hidden during type checking.

For example, in this program using existential types, the lint pierces through otherwise-opaque existentials to look at the underlying (hidden) type, thus it both

• accepts code that is only FFI-safe because of the particular type underlying the existential, which users of that type shouldn't know or rely on
• leaks the underlying type to the user in error messages when (correctly) rejecting uses of an existential type as FFI-unsafe

I can't quickly find an equivalent using only stable impl Trait, because we don't have typeof(function), but if there's a way to write "Option of the return type of this function" in stable Rust, then that would presumably have the same issue.

cc @eddyb for fact-checking my understanding that this is because of reveal_all

[eddyb]

@rkruppe User-facing diagnostics should avoid the Reveal::All mode, yeah. In fact, the other variant of Reveal is literally Reveal::UserFacing - also the doc comments are pretty explicit about this:

rust/src/librustc/traits/project.rs

Lines 28 to 68 in 37ff5d3

	/// Depending on the stage of compilation, we want projection to be
	/// more or less conservative.
	#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, HashStable)]
	pub enum Reveal {
	/// At type-checking time, we refuse to project any associated
	/// type that is marked `default`. Non-`default` ("final") types
	/// are always projected. This is necessary in general for
	/// soundness of specialization. However, we *could* allow
	/// projections in fully-monomorphic cases. We choose not to,
	/// because we prefer for `default type` to force the type
	/// definition to be treated abstractly by any consumers of the
	/// impl. Concretely, that means that the following example will
	/// fail to compile:
	///
	/// ```
	/// trait Assoc {
	/// type Output;
	/// }
	///
	/// impl<T> Assoc for T {
	/// default type Output = bool;
	/// }
	///
	/// fn main() {
	/// let <() as Assoc>::Output = true;
	/// }
	UserFacing,
	/// At codegen time, all monomorphic projections will succeed.
	/// Also, `impl Trait` is normalized to the concrete type,
	/// which has to be already collected by type-checking.
	///
	/// NOTE: as `impl Trait`'s concrete type should *never*
	/// be observable directly by the user, `Reveal::All`
	/// should not be used by checks which may expose
	/// type equality or type contents to the user.
	/// There are some exceptions, e.g., around OIBITS and
	/// transmute-checking, which expose some details, but
	/// not the whole concrete type of the `impl Trait`.
	All,
	}

cc @nikomatsakis @nagisa @oli-obk @Manishearth

[davidtwco]

I've been using LateContext's param_env over ParamEnv::reveal_all() in recent PRs that modify the improper_ctypes lint (see #72700, #73287, and #73257).

In each recent PR, I've ensured that opaque types are prohibited (in the spirit of "FFI-safety of a type not relying on particular type behind an opaque type being FFI-safe") - with the exception of when a opaque type is used in a projection (#73287).

Unless there are specific examples of behavior that still need to be changed as a result of this issue - should this issue be closed?

[hanna-kruppe]

My litmus test for closing this would be "all the normalizing done in improper_ctypes uses Reveal::UserFacing", which is hard to audit for while there's multiple PRs in flight that address parts of the problem. After those PRs are all merged, we can see if there's any Reveal::All remaining and if so, their location should tell us how to exercise any remaining questionable behavior.

[davidtwco]

I've opened #73758 to fix the remaining two places that Reveal:All is used now that the other PRs have landed.