librustc: Require that types mentioned anywhere in public signatures be

[pcwalton]

public, including in trait bounds and typedefs.

There are three main patterns that this breaks. First is code that uses
private trait bounds in public APIs:

pub mod a {
    trait Trait { ... }
    pub fn f<T:Trait>() { ... }
}

Change that code to export the trait. For example:

pub mod a {
    pub trait Trait { ... }     // note `pub`
    pub fn f<T:Trait>() { ... }
}

Second, this change breaks code that was relying on
#[allow(visible_private_types)]. That lint is now a hard error, and
cannot be overridden. For example:

mod helper {
    pub struct Foo { ... }
}
pub mod public {
    use helper::Foo;
    pub fn f() -> Foo;
}

Because there is no way for code that uses public::f to name Foo,
this is no longer allowed. Change affected code to look like this:

pub mod helper {    // note `pub`
    pub struct Foo { ... }
}
pub mod public {
    use helper::Foo;
    pub fn f() -> Foo;
}

Third, this change breaks code that was relying on being able to
reference private typedefs in public APIs. For example:

pub mod b {
    type Foo = int;
    pub fn f(_: Foo) { ... }
}

Change this code to:

pub mod b {
    pub type Foo = int; // note `pub`
    pub fn f(_: Foo) { ... }
}

This implements RFC #48.

Closes #16463.

[breaking-change]

r? @alexcrichton

[lilyball]

The RFC says this is supposed to be a feature gate.

[pcwalton]

Feature gates for this kind of thing are to ease deprecation per the discussion in the imports RFC. Because I've removed all offending code this simply fast tracks deprecation. So I don't believe a gate is warranted here.

[lilyball]

@pcwalton The RFC was very clear that this would introduce a feature gate. I don't care what you think the motivation was, not adding the feature gate means you're not following the RFC. Personally, I think the feature gate is important because it allows anyone who discovers that they cannot reasonably update their code to do what they want after this change a way to still make their code compile, while they raise an issue here regarding the breakage.

[pcwalton]

You are free to submit a PR to add the feature gate.

[lilyball]

@pcwalton That's not at all appropriate. You are violating the RFC process if you aren't implementing the RFC as merged. It's not my responsibility to clean up after you.

[pcwalton]

Fine, I'll put in the feature gate. It's ridiculous cruft and nobody will ever use it, but you can have broken features if you want them.

[lilyball]

@pcwalton I don't understand your approach here. If you think the feature gate isn't necessary, then you should have had it removed during the RFC process. Furthermore, the adoption path that the feature gate provides isn't just for rustc, it's also for any third-party code that is impacted by this. And it's also more important with this change than with most, because this change is very deliberately removing functionality from Rust with no replacement (notably, because private supertraits are disallowed despite the expectation of nearly everyone who responded to the RFC, AFAIK there's no way to expose a trait that cannot be implemented by third-parties).

It's also important to note that this is a very controversial change to begin with. Not just because there was not a clear community consensus, but also because you have chosen to merge in the RFC despite the majority of the feedback being negative. You should be trying to avoid giving the community any more cause to be unhappy with this change.

[pcwalton]

Updated with feature gate.

[sfackler]

@kballard it is true that this should be feature gated (as it now is), but it is not true that there is no workaround for this change:

pub struct Privatizer<T> { t: marker::InvariantType<T> }

pub trait InternalImplsOnly {
    fn force_internal() -> Privatizer<T>;
}

pub trait Foo: InternalImplsOnly {}

Privatizer::t is a private field so Privatizer can't be instantiated by code outside of its crate, which prevents cross-crate implementations of InternalImplsOnly.

[lilyball]

Any code that has access to an existing value that confirms to Foo can use it to construct that private value. Granted, that's awkward and not necessarily doable, but it is a hole.

[sfackler]

Oops, should be Privatizer<Self>.

[alexcrichton]

From the technical side of things, could you opt-in all non-syntax/rustc libs into the feature gate for now? Almost everything that was made public is intentionally not-public, and I'd like to take some time to analyze why they need to be public (unless you'd like to do it of course!). Once this lands I'll do a follow up patch to remove all the feature gates soon after.

Otherwise this looks good to me, thanks @pcwalton!

[nikomatsakis]

Pretty sure this is due to a bug. I don't see why this ought to be publicly accessible.

[nikomatsakis]

I did a read through of the first half or so of the changes. The majority I looked at did not appear to be things that must be made public by my understanding of the rules. I think though that we do need to amend the rules in two ways:

1. Associated items in an impl count as public if the impl is implemented for a public type (when multidispatch comes around: if impl for only public types).
2. pub use requires that its end point be public and reachable, but not that each individual step along the way be public.

The intution for point 1 is that, if these rules are doing their job, you the outsider cannot get your hands on an instance of a private type, nor can you name it, therefore you could not trigger the impl nor invoke its methods etc if any of the types are private.

[pcwalton]

@nikomatsakis Since these changes would require an RFC, can we land this for now with the feature gate that @alexcrichton suggested, as well as the types you audited and determined to be public-due-to-a-bug made private again?

[pcwalton]

re-r? @alexcrichton

[nikomatsakis]

@pcwalton I don't object to landing the code with feature gate, but I'd prefer not to make things public until we settle on the rules we want. Some of the items I see in the current commit, for example, don't seem like they should be public (e.g., Scope in the resolve_lifetimes module).

[pcwalton]

@nikomatsakis Would you prefer opting librustc and friends into the feature gate too?

[nikomatsakis]

I'm still confused as to why some of those changes were needed, even under the rules as accepted. I think I'd prefer not to land the patch, actually, until that is better understood (and ideally until we have a chance to discuss which rules we wanted). Alternatively we could change feature gate to opt IN to the new rules for time being until they are settled (and then only use it in tests). I just don't want people to start getting errors for rules that are under active debate. 

Niko

-------- Original message --------
From: Patrick Walton [email protected]
Date:08/15/2014 22:40 (GMT-05:00)
To: rust-lang/rust [email protected]
Cc: Niko Matsakis [email protected]
Subject: Re: [rust] librustc: Require that types mentioned anywhere in public
signatures be (#16467)

@nikomatsakis Would you prefer opting librustc and friends into the feature gate too?

—
Reply to this email directly or view it on GitHub.

[pcwalton]

@nikomatsakis Works for me. Do you mind if I close the issue/nominate it for removal from 1.0 until such time as we come up with a workable set of rules? I don't like having 1.0 blocker issues that we can't implement

[pcwalton]

Closing per @nikomatsakis' request.

[nikomatsakis]

No objection.

Niko

-------- Original message --------
From: Patrick Walton [email protected]
Date:08/16/2014 00:16 (GMT-05:00)
To: rust-lang/rust [email protected]
Cc: Niko Matsakis [email protected]
Subject: Re: [rust] librustc: Require that types mentioned anywhere in public
signatures be (#16467)

@nikomatsakis Works for me. Do you mind if I close the issue/nominate it for removal from 1.0 until such time as we come up with a workable set of rules? I don't like having 1.0 blocker issues that we can't implement

—
Reply to this email directly or view it on GitHub.