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

Consider substituting type aliases in "public-in-private" checker #30503

Closed
petrochenkov opened this Issue Dec 21, 2015 · 15 comments

Comments

Projects
None yet
9 participants
@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 21, 2015

E.g. allowing things like

pub struct SomethingLongButPublicWithLotsOfParameters;
type ShortPrivAlias = SomethingLongButPublicWithLotsOfParameters;
pub fn public_function(arg: ShortPrivAlias) {}

This was originally done in #29973, but reverted later to follow the letter of RFC 136.
Now it starts hitting people. I think this should be reconsidered sooner than later to reduce the number of annoyed people.
Type alias substitution reduces the breakage by 25% according to the crater data.

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Dec 21, 2015

Just to give a concrete example, the current behavior even disallows type aliases in locations which are effectively private. An example from diesel is https://github.com/sgrif/diesel/blob/c8127d408f759faa056503014cd00e92479215e0/diesel/src/query_builder/select_statement/dsl_impls.rs#L54-L67, where the type alias is purely used for brevity in what is effectively an internal impl of a trait, but is disallowed because the trait itself is public.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Dec 21, 2015

what is effectively an internal impl of a trait, but is disallowed because the trait itself is public.

The logic is that both the trait and the type are public, so the impl is public as well. (But this is a digression)

@sgrif

This comment has been minimized.

Copy link
Contributor

sgrif commented Dec 21, 2015

Right, I fully understand the logic. My argument is that it's an ergonomics
issue in places where while that is technically true, it is not
semantically true (for lack of a better word).

On Sun, Dec 20, 2015, 6:04 PM Vadim Petrochenkov notifications@github.com
wrote:

what is effectively an internal impl of a trait, but is disallowed because
the trait itself is public.

The logic is that both the trait and the type are public, so the impl is
public as well.


Reply to this email directly or view it on GitHub
#30503 (comment).

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Dec 21, 2015

cc @seanmonstar
(You also wanted this)

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Dec 21, 2015

Yes! 👍

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Dec 21, 2015

I'm basically in favor of this; I have some vague concerns about specifying it literally as substitution of the right hand side given that we have parameters to consider in the general case, but it seems plausible that kind of substitution works out fine.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Dec 21, 2015

@pnkfelix
An implementation is here.
(It may be missing some corner cases, but I'm not aware of them.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jan 14, 2016

cc @retep998
I guess this can affect -sys crates (maybe winapi?) with lots of typedefs positively.

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Jan 14, 2016

I pretty much make everything pub anyway so this doesn't really affect winapi either way. The thing that did affect winapi was things like DECLARE_HANDLE! which created a private opaque type, with a public typedef to a raw pointer to that.

@nixpulvis

This comment has been minimized.

Copy link

nixpulvis commented Feb 8, 2016

I'd be in favor of something like

type Foo = usize;  // This is a type alias which must always be preserved.
alias Bar = usize;  // This is a type alias which always gets substituted when crossing modules.

This allows

alias Return = (usize, isize);

mod a {
    fn foo() -> super::Return;
}

mod b {
    fn foo() -> super::Return;
}

But is documented as returning a (usize, isize) everywhere.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jun 13, 2016

I think the primary concern with this was rustdoc -- which will show these private aliases -- but we could probably change rustdoc's behavior to expand private aliases.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jun 27, 2016

I'm nominating this issue -- I agree with @petrochenkov that it makes sense to make this change before making the warnings into hard errors -- I am a bit unclear on whether we think an RFC would be required or what.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jun 27, 2016

Also as it happens @wycats was just complaining to me today that private type aliases cause these issues. :)

@aturon aturon removed the I-nominated label Jul 7, 2016

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jul 9, 2016

Just opened rust-lang/rfcs#1671 which includes this feature.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 4, 2016

So @nrc and I discussed this -- but we're the only ones in the @rust-lang/lang meeting due to various reasons -- we both feel like we should accept this change (use the semantic type, not the type alias) and probably also adopt the simpler version of @eddyb's proposal that is described on internals.

bors added a commit that referenced this issue Aug 11, 2016

Auto merge of #34193 - petrochenkov:privalias, r=nikomatsakis
privacy: Substitute type aliases in private-in-public checker

Closes #30503
Closes #34293

Everyone in the issue discussion seemed to be in favor, @huonw also spoke about this [here](https://www.reddit.com/r/rust/comments/3xldr9/surfaces_and_signatures_component_privacy_versus/cy615wq), but the issue haven't got any movement.
I think it's reasonable to do this before turning `private_in_public` warnings into errors.

r? @nikomatsakis

@bors bors closed this in #34193 Aug 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.