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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions src/librustc_privacy/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -873,19 +873,12 @@ impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
// Return the visibility of the type alias's least visible component type when substituted
fn substituted_alias_visibility(&self, item: &hir::Item, path: &hir::Path)
-> Option<ty::Visibility> {
// We substitute type aliases only when determining impl publicity
// FIXME: This will probably change and all type aliases will be substituted,
// requires an amendment to RFC 136.
if self.required_visibility != ty::Visibility::PrivateExternal {
return None;
}
// Type alias is considered public if the aliased type is
// public, even if the type alias itself is private. So, something
// like `type A = u8; pub fn f() -> A {...}` doesn't cause an error.
if let hir::ItemTy(ref ty, ref generics) = item.node {
let mut check = SearchInterfaceForPrivateItemsVisitor {
min_visibility: ty::Visibility::Public, ..*self
};
let mut check = SearchInterfaceForPrivateItemsVisitor::new(self.tcx,
self.old_error_set);
check.visit_ty(ty);
// If a private type alias with default type parameters is used in public
// interface we must ensure, that the defaults are public if they are actually used.
Expand Down
1 change: 1 addition & 0 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1744,6 +1744,7 @@ impl<'a> Resolver<'a> {
let def_id = self.definitions.local_def_id(type_parameter.id);
let def = Def::TyParam(space, index as u32, def_id, name);
function_type_rib.bindings.insert(ast::Ident::with_empty_ctxt(name), def);
self.record_def(type_parameter.id, PathResolution::new(def));
}
self.type_ribs.push(function_type_rib);
}
Expand Down
81 changes: 79 additions & 2 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ use syntax_pos::{self, DUMMY_SP, Pos};
use rustc_trans::back::link;
use rustc::middle::cstore;
use rustc::middle::privacy::AccessLevels;
use rustc::middle::resolve_lifetime::DefRegion::*;
use rustc::hir::def::Def;
use rustc::hir::def_id::{DefId, DefIndex, CRATE_DEF_INDEX};
use rustc::hir::fold::Folder;
use rustc::hir::print as pprust;
use rustc::ty::subst::{self, ParamSpace, VecPerParamSpace};
use rustc::ty;
Expand Down Expand Up @@ -1636,6 +1638,43 @@ impl PrimitiveType {
}
}


// Poor man's type parameter substitution at HIR level.
// Used to replace private type aliases in public signatures with their aliased types.
struct SubstAlias<'a, 'tcx: 'a> {
tcx: &'a ty::TyCtxt<'a, 'tcx, 'tcx>,
// Table type parameter definition -> substituted type
ty_substs: HashMap<Def, hir::Ty>,
// Table node id of lifetime parameter definition -> substituted lifetime
lt_substs: HashMap<ast::NodeId, hir::Lifetime>,
}

impl<'a, 'tcx: 'a, 'b: 'tcx> Folder for SubstAlias<'a, 'tcx> {
fn fold_ty(&mut self, ty: P<hir::Ty>) -> P<hir::Ty> {
if let hir::TyPath(..) = ty.node {
let def = self.tcx.expect_def(ty.id);
if let Some(new_ty) = self.ty_substs.get(&def).cloned() {
return P(new_ty);
}
}
hir::fold::noop_fold_ty(ty, self)
}
fn fold_lifetime(&mut self, lt: hir::Lifetime) -> hir::Lifetime {
let def = self.tcx.named_region_map.defs.get(&lt.id).cloned();
match def {
Some(DefEarlyBoundRegion(_, _, node_id)) |
Some(DefLateBoundRegion(_, node_id)) |
Some(DefFreeRegion(_, node_id)) => {
if let Some(lt) = self.lt_substs.get(&node_id).cloned() {
return lt;
}
}
_ => {}
}
hir::fold::noop_fold_lifetime(lt, self)
}
}

impl Clean<Type> for hir::Ty {
fn clean(&self, cx: &DocContext) -> Type {
use rustc::hir::*;
Expand Down Expand Up @@ -1665,8 +1704,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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

if let Some(tcx) = cx.tcx_opt() {
// Substitute private type aliases
let def = tcx.expect_def(self.id);
if let Def::TyAlias(def_id) = def {
if let Some(node_id) = tcx.map.as_local_node_id(def_id) {
if !cx.access_levels.borrow().is_exported(def_id) {
let item = tcx.map.expect_item(node_id);
if let hir::ItemTy(ref ty, ref generics) = item.node {
let provided_params = &path.segments.last().unwrap().parameters;
let mut ty_substs = HashMap::new();
let mut lt_substs = HashMap::new();
for (i, ty_param) in generics.ty_params.iter().enumerate() {
let ty_param_def = tcx.expect_def(ty_param.id);
if let Some(ty) = provided_params.types().get(i).cloned()
.cloned() {
ty_substs.insert(ty_param_def, ty.unwrap());
} else if let Some(default) = ty_param.default.clone() {
ty_substs.insert(ty_param_def, default.unwrap());
}
}
for (i, lt_param) in generics.lifetimes.iter().enumerate() {
if let Some(lt) = provided_params.lifetimes().get(i)
.cloned()
.cloned() {
lt_substs.insert(lt_param.lifetime.id, lt);
}
}
let mut subst_alias = SubstAlias {
tcx: &tcx,
ty_substs: ty_substs,
lt_substs: lt_substs
};
return subst_alias.fold_ty(ty.clone()).clean(cx);
}
}
}
}
}
resolve_type(cx, path.clean(cx), self.id)
}
TyPath(Some(ref qself), ref p) => {
let mut segments: Vec<_> = p.segments.clone().into();
Expand Down
7 changes: 4 additions & 3 deletions src/test/compile-fail/private-in-public-warn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,10 @@ mod aliases_pub {
}

pub fn f1(arg: PrivUseAlias) {} // OK
pub fn f2(arg: PrivAlias) {} // OK

pub trait Tr1: PrivUseAliasTr {} // OK
// This should be OK, if type aliases are substituted
pub trait Tr2: PrivUseAliasTr<PrivAlias> {} //~ WARN private type in public interface
//~^ WARNING hard error
pub trait Tr2: PrivUseAliasTr<PrivAlias> {} // OK

impl PrivAlias {
pub fn f(arg: Priv) {} //~ WARN private type in public interface
Expand Down Expand Up @@ -285,6 +284,8 @@ mod aliases_params {
struct Priv;
type PrivAliasGeneric<T = Priv> = T;
type Result<T> = ::std::result::Result<T, Priv>;

pub fn f1(arg: PrivAliasGeneric<u8>) {} // OK, not an error
}

#[rustc_error]
Expand Down
4 changes: 0 additions & 4 deletions src/test/compile-fail/private-in-public.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 should be OK, but associated type aliases are not substituted yet
pub fn f3(arg: <Priv as PrivTr>::AssocAlias) {} //~ ERROR private type in public interface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Expand Down Expand Up @@ -143,8 +141,6 @@ mod aliases_params {
type PrivAliasGeneric<T = Priv> = T;
type Result<T> = ::std::result::Result<T, Priv>;

// This should be OK, if type aliases are substituted
pub fn f1(arg: PrivAliasGeneric<u8>) {} //~ ERROR private type in public interface
pub fn f2(arg: PrivAliasGeneric) {} //~ ERROR private type in public interface
pub fn f3(arg: Result<u8>) {} //~ ERROR private type in public interface
}
Expand Down
41 changes: 41 additions & 0 deletions src/test/rustdoc/private-type-alias.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

type MyResultPriv<T> = Result<T, u16>;
pub type MyResultPub<T> = Result<T, u64>;

// @has private_type_alias/fn.get_result_priv.html '//pre' 'Result<u8, u16>'
pub fn get_result_priv() -> MyResultPriv<u8> {
panic!();
}

// @has private_type_alias/fn.get_result_pub.html '//pre' 'MyResultPub<u32>'
pub fn get_result_pub() -> MyResultPub<u32> {
panic!();
}

pub type PubRecursive = u16;
type PrivRecursive3 = u8;
type PrivRecursive2 = PubRecursive;
type PrivRecursive1 = PrivRecursive3;

// PrivRecursive1 is expanded twice and stops at u8
// PrivRecursive2 is expanded once and stops at public type alias PubRecursive
// @has private_type_alias/fn.get_result_recursive.html '//pre' '(u8, PubRecursive)'
pub fn get_result_recursive() -> (PrivRecursive1, PrivRecursive2) {
panic!();
}

type MyLifetimePriv<'a> = &'a isize;

// @has private_type_alias/fn.get_lifetime_priv.html '//pre' "&'static isize"
pub fn get_lifetime_priv() -> MyLifetimePriv<'static> {
panic!();
}