[RFC] Reduce false positives for "private type in public interface"


#1

Deriving has this problem where it can’t generate the desired impls, such as in the following example:

struct Private<T>(T);

#[derive(Clone)]
pub struct Foo<T>(Rc<Private<T>>);

// should generate:
impl<T> Clone for Foo<T>
where Rc<Private<T>>: Clone {...}

// but instead generates the more restrictive:
impl<T> Clone for Foo<T>
where T: Clone {...}

Because the bound mentioning Private would cause a “private type in public interface” error, deriving has no choice but to place bounds on type parameters directly, making Foo<NonClone> not Clone even though Rc is always Clone.
Similar situations arise with references and raw pointers.

Serde is generating the correct bounds but at the cost of warnings that will turn into errors in the future (unless the rules get relaxed in the future).

Armed with the knowledge that these bounds mentioning private types can be usually rewritten to no bounds (like above) or bounds on type parameters, I opened RFC PR 1671.
That RFC contains a complex where clause elaboration rule which lets deriving work with the exact bounds without letting users observe bounds that depend on impls they can’t see.

However, @petrochenkov pointed out that the current check looks at bounds to handle cases like this:

pub trait Trait<T> {
    type Assoc; // can be T in some impls
}
pub fn foo<T: Trait<Private>>() -> T::Assoc {...}

The current syntactic check needs to look at both the function signature (fn() -> T::Assoc) and bound (T: Trait<Private>) and only the latter suggests that the signature might be exposing a private type.
Note that this is not the case for deriving, which is collateral damage here.

If we expand associated types to the fully qualified form, we get:

pub fn foo<T: Trait<Private>>() -> <T as Trait<Private>>::Assoc {...}

The resulting return type is enough to ban this case.

Therefore, the complex RFC can be replaced with simply ignoring bounds and checking types only after expanding associated types to the fully qualified form (<X as Trait<Y>>::Assoc).
The guarantee you then get is that no values of private types could be exported from the module (or taken as arguments by exported functions), deriving “just” works, but users can see the effects of private impls (although you can cause that right now with re-exporting a subset of a private module).

I am leaning towards the simpler solution but I wanted to get a few more opinions before I throw away the more complex RFC.
Do you think the simpler solution that doesn’t look at the bounds at all and lets deriving do its thing in peace is the right choice? Or does it allow too much code (i.e. code which is bounded on hidden impls) to compile?

EDIT: Added an explanation of the associated type issue that caused bounds to be checked.


#2

I am not sure what I think about allowing foreign types in bounds, but I agree that:

  • the problem of deriving making “overly complex” bounds is a real one;
    • though, interestingly, it is arguably backwards incompatible to fix, but probably not a big deal (i.e., because maybe you intended for the stricter bounds to be required?)
  • having the “private type” rules be based on the expanded UFCS-style path seems like the obvious thing to do (and…I kind of thought it was what we were doing?) vs relying on the bounds in scope

In other words, I think you should change the RFC to the simpler approach, despite the fact that I didn’t read the complex version yet. :slight_smile:


#3

So, it’s taken me a long time to understand the issues here, but I think I mostly get it now. I’ve decided I really don’t like the RFC 1671 suggestion - the rules around where clauses seem really complex and hard to explain, both in documentation and in error messages.

IIUC, this alternative basically comes down to “ignore bounds and where clauses in the pub-in-priv checks”, with the caveat, that we can continue to guarantee soundness of the privacy system by fully qualifying assoc types.

I assume that this solution does not address #30503 and that whether we check pub-in-priv before or after expanding type aliases (effectively) is orthogonal to this?

So, it seems we are left with a trade-off, on the one hand this safely admits more code, on the other hand, some cases where we reject code (due to a non-applicable impl or function with a where clause) would cause annoying error messages which mention private traits which we can’t do anything about. Is that the only downside? If so, then I am in favour of this proposal, in any case, I prefer it to RFC 1671.


#4

It has to address type aliases as well, because the syntactical form is gone, all you have is a Ty which doesn’t support type aliases.


#5

Ah, I missed that. I guess that is implicit due to implementation ordering - i.e., fully elaborated assoc types do not exist in the compiler until after type aliases have been substituted away?

That might be a downside - I’m not sure how I feel about #30503 - it certainly makes life easier, but I’m not sure it is the right thing to do. Anyway, it certainly doesn’t seem like a showstopper for this idea.