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

privacy: Substitute type aliases in private-in-public checker #34193

Merged
merged 4 commits into from Aug 11, 2016

Conversation

Projects
None yet
4 participants
@petrochenkov
Copy link
Contributor

commented Jun 9, 2016

Closes #30503
Closes #34293

Everyone in the issue discussion seemed to be in favor, @huonw also spoke about this here, 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

@@ -246,6 +245,8 @@ mod aliases_priv {
use self::Priv1 as PrivUseAlias;
use self::PrivTr1 as PrivUseAliasTr;
type PrivAlias = Priv2;
//~^ WARN private type in public interface
//~| WARNING hard error

This comment has been minimized.

Copy link
@jseyfried

jseyfried Jun 12, 2016

Contributor

I'm having trouble seeing why this is an error.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Jun 12, 2016

Author Contributor

Priv2 is used in a public interface through an alias.
The error location here is suboptimal. On one hand Priv2 is the true source of the error and pointing to PrivAlias is misleading, on the other hand pointing to Priv2 is confusing too, because the public interface it's used in is not pointed to.
Ideally, there should be two diagnostic messages - one for the public interface location and the other for actual private type location. I'll implement this if positive decision about the whole PR is made.

This comment has been minimized.

Copy link
@jseyfried

jseyfried Jun 12, 2016

Contributor

Ahh, makes sense.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jun 27, 2016

Contributor

I'm confused a bit. This seems like it's causing a new error?

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Jun 28, 2016

Author Contributor

This seems like it's causing a new error?

No, the error from line 211 (alias location) migrated to this line (aliased type location).

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Jul 8, 2016

Author Contributor

Ideally, there should be two diagnostic messages - one for the public interface location and the other for actual private type location.

I implemented the extra note for the private type location for type aliases, but dropped it, the situation should be rare, likely happens in advanced code and doesn't worth the extra code.
Now the error is reported at the public interface location only.

@@ -105,8 +105,6 @@ mod aliases_pub {
}
impl PrivTr for Priv {}

// This should be OK, if type aliases are substituted
pub fn f2(arg: PrivAlias) {} //~ ERROR private type in public interface

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jun 27, 2016

Contributor

can we add a test to show that no error results?

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Jun 28, 2016

Author Contributor

It's tested, the check is just moved from this file to private-in-public-warn.rs

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

Ideally, we would fix rustdoc to not use private type aliases as part of this PR.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2016

Ideally, we would fix rustdoc to not use private type aliases as part of this PR.

I'll try to fix this and the error locations tomorrow. If I won't be able to, it'll have to wait for another week or so. (I also don't think these are blockers for merging the PR)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2016

fix rustdoc to not use private type aliases

Curiously, rustdoc doesn't currently substitute private use aliases.

use std::result::Result as MyResult;
pub fn get_result() -> MyResult<u8, u16> { // Still displayed as MyResult by rustdoc
    panic!();
}
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

@petrochenkov I guess it's debatable what's the right thing to do there. I mean, the link still goes to the right place, I guess, and it's not clear that showing the full path will help readability overall. (And the full path in that crate may not be the same as the full path from another crate.) In any case, it feels different, because MyResult is a name the user can use, even if their path to it is different.

@petrochenkov petrochenkov force-pushed the petrochenkov:privalias branch from af0df95 to 5679896 Jul 8, 2016

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2016

Updated with rustdoc changes and tweaked error locations.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

☔️ The latest upstream changes (presumably #34570) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov force-pushed the petrochenkov:privalias branch from 5679896 to 91063fc Jul 15, 2016

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

Rebased.

@@ -1644,8 +1683,46 @@ impl Clean<Type> for hir::Ty {
FixedVector(box ty.clean(cx), n)
},
TyTup(ref tys) => Tuple(tys.clean(cx)),
TyPath(None, ref p) => {
resolve_type(cx, p.clean(cx), self.id)
TyPath(None, ref path) => {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 10, 2016

Contributor

will this logic work if you have a private type alias that refers to another private type alias?

e.g., type A = u32; type B = A; fn foo(x: B) { }?

(Can you also add some tests for that scenario? I could imagine that even if your patch gets it right, it's something future patches might easily break.)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

r=me once new tests are added for recursive scenario

@petrochenkov petrochenkov force-pushed the petrochenkov:privalias branch from 91063fc to 5d4ae4b Aug 10, 2016

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

Updated with a test for recursive substitution.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

📌 Commit 5d4ae4b has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

⌛️ Testing commit 5d4ae4b with merge 11f8805...

bors added a commit that referenced this pull request 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 merged commit 5d4ae4b into rust-lang:master Aug 11, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@petrochenkov petrochenkov deleted the petrochenkov:privalias branch Sep 21, 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.