Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should improper_ctypes pierce through impl Trait? #60855

Closed
hanna-kruppe opened this issue May 15, 2019 · 4 comments
Closed

Should improper_ctypes pierce through impl Trait? #60855

hanna-kruppe opened this issue May 15, 2019 · 4 comments

Comments

@hanna-kruppe
Copy link
Contributor

@hanna-kruppe hanna-kruppe commented May 15, 2019

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
Copy link
Member

@eddyb eddyb commented May 22, 2019

@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:

/// 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
Copy link
Member

@davidtwco davidtwco commented Jun 12, 2020

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
Copy link
Contributor Author

@hanna-kruppe hanna-kruppe commented Jun 12, 2020

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
Copy link
Member

@davidtwco davidtwco commented Jun 26, 2020

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

@bors bors closed this in d5205f2 Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants