Skip to content

Commit

Permalink
librustc: Forbid private types in public APIs.
Browse files Browse the repository at this point in the history
This breaks code like:

    struct Foo {
        ...
    }

    pub fn make_foo() -> Foo {
        ...
    }

Change this code to:

    pub struct Foo {    // note `pub`
        ...
    }

    pub fn make_foo() -> Foo {
        ...
    }

The `visible_private_types` lint has been removed, since it is now an
error to attempt to expose a private type in a public API. In its place
a `#[feature(visible_private_types)]` gate has been added.

Closes #16463.

RFC #48.

[breaking-change]
  • Loading branch information
pcwalton committed Sep 23, 2014
1 parent 43fd619 commit e9ad12c
Show file tree
Hide file tree
Showing 49 changed files with 169 additions and 80 deletions.
2 changes: 1 addition & 1 deletion src/libcore/any.rs
Expand Up @@ -94,7 +94,7 @@ pub enum Void { }
pub trait Any: AnyPrivate {}

/// An inner trait to ensure that only this module can call `get_type_id()`.
trait AnyPrivate {
pub trait AnyPrivate {
/// Get the `TypeId` of `self`
fn get_type_id(&self) -> TypeId;
}
Expand Down
1 change: 0 additions & 1 deletion src/libcore/iter.rs
Expand Up @@ -2178,7 +2178,6 @@ pub type Iterate<'a, T> = Unfold<'a, T, IterateState<'a, T>>;

/// Creates a new iterator that produces an infinite sequence of
/// repeated applications of the given function `f`.
#[allow(visible_private_types)]
pub fn iterate<'a, T: Clone>(seed: T, f: |T|: 'a -> T) -> Iterate<'a, T> {
Unfold::new((f, Some(seed), true), |st| {
let &(ref mut f, ref mut val, ref mut first) = st;
Expand Down
2 changes: 1 addition & 1 deletion src/libdebug/repr.rs
Expand Up @@ -32,7 +32,7 @@ macro_rules! try( ($me:expr, $e:expr) => (

/// Representations

trait Repr {
pub trait Repr {
fn write_repr(&self, writer: &mut io::Writer) -> io::IoResult<()>;
}

Expand Down
4 changes: 2 additions & 2 deletions src/libgreen/lib.rs
Expand Up @@ -218,7 +218,7 @@

// NB this does *not* include globs, please keep it that way.
#![feature(macro_rules, phase, default_type_params)]
#![allow(visible_private_types, deprecated)]
#![allow(deprecated)]

#[cfg(test)] #[phase(plugin, link)] extern crate log;
#[cfg(test)] extern crate rustuv;
Expand Down Expand Up @@ -385,7 +385,7 @@ pub struct SchedPool {
/// keep track of how many tasks are currently running in the pool and then
/// sending on a channel once the entire pool has been drained of all tasks.
#[deriving(Clone)]
struct TaskState {
pub struct TaskState {
cnt: Arc<AtomicUint>,
done: Sender<()>,
}
Expand Down
3 changes: 1 addition & 2 deletions src/libnative/io/timer_unix.rs
Expand Up @@ -66,15 +66,14 @@ pub struct Timer {
inner: Option<Box<Inner>>,
}

struct Inner {
pub struct Inner {
cb: Option<Box<rtio::Callback + Send>>,
interval: u64,
repeat: bool,
target: u64,
id: uint,
}

#[allow(visible_private_types)]
pub enum Req {
// Add a new timer to the helper thread.
NewTimer(Box<Inner>),
Expand Down
1 change: 0 additions & 1 deletion src/libregex/compile.rs
Expand Up @@ -10,7 +10,6 @@

// Enable this to squash warnings due to exporting pieces of the representation
// for use with the regex! macro. See lib.rs for explanation.
#![allow(visible_private_types)]

use std::cmp;
use parse;
Expand Down
4 changes: 1 addition & 3 deletions src/libregex/re.rs
Expand Up @@ -102,7 +102,6 @@ pub fn is_match(regex: &str, text: &str) -> Result<bool, parse::Error> {
/// More details about the `regex!` macro can be found in the `regex` crate
/// documentation.
#[deriving(Clone)]
#[allow(visible_private_types)]
pub enum Regex {
// The representation of `Regex` is exported to support the `regex!`
// syntax extension. Do not rely on it.
Expand Down Expand Up @@ -516,7 +515,6 @@ impl Regex {
}

#[doc(hidden)]
#[allow(visible_private_types)]
#[experimental]
pub fn names_iter<'a>(&'a self) -> NamesIter<'a> {
match *self {
Expand All @@ -534,7 +532,7 @@ impl Regex {

}

enum NamesIter<'a> {
pub enum NamesIter<'a> {
NamesIterNative(::std::slice::Items<'a, Option<&'static str>>),
NamesIterDynamic(::std::slice::Items<'a, Option<String>>)
}
Expand Down
4 changes: 0 additions & 4 deletions src/librustc/lint/builtin.rs
Expand Up @@ -1601,9 +1601,6 @@ declare_lint!(pub DEAD_ASSIGNMENT, Warn,
declare_lint!(pub DEAD_CODE, Warn,
"detect piece of code that will never be used")

declare_lint!(pub VISIBLE_PRIVATE_TYPES, Warn,
"detect use of private types in exported type signatures")

declare_lint!(pub UNREACHABLE_CODE, Warn,
"detects unreachable code")

Expand Down Expand Up @@ -1636,7 +1633,6 @@ impl LintPass for HardwiredLints {
UNUSED_VARIABLE,
DEAD_ASSIGNMENT,
DEAD_CODE,
VISIBLE_PRIVATE_TYPES,
UNREACHABLE_CODE,
WARNINGS,
UNKNOWN_FEATURES,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/mem_categorization.rs
Expand Up @@ -224,7 +224,7 @@ pub fn deref_kind(tcx: &ty::ctxt, t: ty::t) -> deref_kind {
}
}

trait ast_node {
pub trait ast_node {
fn id(&self) -> ast::NodeId;
fn span(&self) -> Span;
}
Expand Down
66 changes: 52 additions & 14 deletions src/librustc/middle/privacy.rs
Expand Up @@ -16,7 +16,6 @@ use std::mem::replace;

use metadata::csearch;
use middle::def;
use lint;
use middle::resolve;
use middle::ty;
use middle::typeck::{MethodCall, MethodMap, MethodOrigin, MethodParam, MethodTypeParam};
Expand Down Expand Up @@ -1289,19 +1288,38 @@ impl<'a, 'tcx> VisiblePrivateTypesVisitor<'a, 'tcx> {
};
// A path can only be private if:
// it's in this crate...
is_local(did) &&
// ... it's not exported (obviously) ...
!self.exported_items.contains(&did.node) &&
// .. and it corresponds to a type in the AST (this returns None for
// type parameters)
self.tcx.map.find(did.node).is_some()
if !is_local(did) {
return false
}
// .. and it corresponds to a private type in the AST (this returns
// None for type parameters)
match self.tcx.map.find(did.node) {
Some(ast_map::NodeItem(ref item)) => item.vis != ast::Public,
Some(_) | None => false,
}
}

fn trait_is_public(&self, trait_id: ast::NodeId) -> bool {
// FIXME: this would preferably be using `exported_items`, but all
// traits are exported currently (see `EmbargoVisitor.exported_trait`)
self.public_items.contains(&trait_id)
}

fn check_ty_param_bound(&self,
span: Span,
ty_param_bound: &ast::TyParamBound) {
match *ty_param_bound {
ast::TraitTyParamBound(ref trait_ref) => {
if !self.tcx.sess.features.borrow().visible_private_types &&
self.path_is_private_type(trait_ref.ref_id) {
self.tcx.sess.span_err(span,
"private type in exported type \
parameter bound");
}
}
_ => {}
}
}
}

impl<'a, 'b, 'tcx, 'v> Visitor<'v> for CheckTypeForPrivatenessVisitor<'a, 'b, 'tcx> {
Expand Down Expand Up @@ -1338,7 +1356,15 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
// namespace (the contents have their own privacies).
ast::ItemForeignMod(_) => {}

ast::ItemTrait(..) if !self.trait_is_public(item.id) => return,
ast::ItemTrait(_, _, ref bounds, _) => {
if !self.trait_is_public(item.id) {
return
}

for bound in bounds.iter() {
self.check_ty_param_bound(item.span, bound)
}
}

// impls need some special handling to try to offer useful
// error messages without (too many) false positives
Expand Down Expand Up @@ -1471,6 +1497,19 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
visit::walk_item(self, item);
}

fn visit_generics(&mut self, generics: &ast::Generics) {
for ty_param in generics.ty_params.iter() {
for bound in ty_param.bounds.iter() {
self.check_ty_param_bound(ty_param.span, bound)
}
}
for predicate in generics.where_clause.predicates.iter() {
for bound in predicate.bounds.iter() {
self.check_ty_param_bound(predicate.span, bound)
}
}
}

fn visit_foreign_item(&mut self, item: &ast::ForeignItem) {
if self.exported_items.contains(&item.id) {
visit::walk_foreign_item(self, item)
Expand All @@ -1488,12 +1527,11 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
fn visit_ty(&mut self, t: &ast::Ty) {
match t.node {
ast::TyPath(ref p, _, path_id) => {
if self.path_is_private_type(path_id) {
self.tcx.sess.add_lint(
lint::builtin::VISIBLE_PRIVATE_TYPES,
path_id, p.span,
"private type in exported type \
signature".to_string());
if !self.tcx.sess.features.borrow().visible_private_types &&
self.path_is_private_type(path_id) {
self.tcx.sess.span_err(p.span,
"private type in exported type \
signature");
}
}
_ => {}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/typeck/infer/error_reporting.rs
Expand Up @@ -1645,7 +1645,7 @@ impl<'a, 'tcx> ErrorReportingHelpers for InferCtxt<'a, 'tcx> {
}
}

trait Resolvable {
pub trait Resolvable {
fn resolve(&self, infcx: &InferCtxt) -> Self;
fn contains_error(&self) -> bool;
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_llvm/lib.rs
Expand Up @@ -153,7 +153,7 @@ pub enum AttributeSet {
FunctionIndex = !0
}

trait AttrHelper {
pub trait AttrHelper {
fn apply_llfn(&self, idx: c_uint, llfn: ValueRef);
fn apply_callsite(&self, idx: c_uint, callsite: ValueRef);
}
Expand Down
1 change: 0 additions & 1 deletion src/librustrt/local.rs
Expand Up @@ -26,7 +26,6 @@ pub trait Local<Borrowed> {
unsafe fn try_unsafe_borrow() -> Option<*mut Self>;
}

#[allow(visible_private_types)]
impl Local<local_ptr::Borrowed<Task>> for Task {
#[inline]
fn put(value: Box<Task>) { unsafe { local_ptr::put(value) } }
Expand Down
1 change: 0 additions & 1 deletion src/librustrt/local_ptr.rs
Expand Up @@ -374,7 +374,6 @@ pub mod native {

#[inline]
#[cfg(not(test))]
#[allow(visible_private_types)]
pub fn maybe_tls_key() -> Option<tls::Key> {
unsafe {
// NB: This is a little racy because, while the key is
Expand Down
12 changes: 4 additions & 8 deletions src/librustrt/unwind.rs
Expand Up @@ -237,7 +237,6 @@ fn rust_exception_class() -> uw::_Unwind_Exception_Class {

#[cfg(not(target_arch = "arm"), not(windows, target_arch = "x86_64"), not(test))]
#[doc(hidden)]
#[allow(visible_private_types)]
pub mod eabi {
use libunwind as uw;
use libc::c_int;
Expand Down Expand Up @@ -291,7 +290,6 @@ pub mod eabi {

#[cfg(target_os = "ios", target_arch = "arm", not(test))]
#[doc(hidden)]
#[allow(visible_private_types)]
pub mod eabi {
use libunwind as uw;
use libc::c_int;
Expand Down Expand Up @@ -347,7 +345,6 @@ pub mod eabi {
// but otherwise works the same.
#[cfg(target_arch = "arm", not(target_os = "ios"), not(test))]
#[doc(hidden)]
#[allow(visible_private_types)]
pub mod eabi {
use libunwind as uw;
use libc::c_int;
Expand Down Expand Up @@ -397,21 +394,20 @@ pub mod eabi {

#[cfg(windows, target_arch = "x86_64", not(test))]
#[doc(hidden)]
#[allow(visible_private_types)]
#[allow(non_camel_case_types, non_snake_case)]
pub mod eabi {
use libunwind as uw;
use libc::{c_void, c_int};

#[repr(C)]
struct EXCEPTION_RECORD;
pub struct EXCEPTION_RECORD;
#[repr(C)]
struct CONTEXT;
pub struct CONTEXT;
#[repr(C)]
struct DISPATCHER_CONTEXT;
pub struct DISPATCHER_CONTEXT;

#[repr(C)]
enum EXCEPTION_DISPOSITION {
pub enum EXCEPTION_DISPOSITION {
ExceptionContinueExecution,
ExceptionContinueSearch,
ExceptionNestedException,
Expand Down
2 changes: 1 addition & 1 deletion src/librustuv/addrinfo.rs
Expand Up @@ -19,7 +19,7 @@ use net;
use super::{Loop, UvError, Request, wait_until_woken_after, wakeup};
use uvll;

struct Addrinfo {
pub struct Addrinfo {
handle: *const libc::addrinfo,
}

Expand Down
1 change: 0 additions & 1 deletion src/librustuv/lib.rs
Expand Up @@ -46,7 +46,6 @@ via `close` and `delete` methods.

#![feature(macro_rules, unsafe_destructor)]
#![deny(unused_result, unused_must_use)]
#![allow(visible_private_types)]

#![reexport_test_harness_main = "test_main"]

Expand Down
4 changes: 4 additions & 0 deletions src/libsyntax/feature_gate.rs
Expand Up @@ -69,6 +69,7 @@ static KNOWN_FEATURES: &'static [(&'static str, Status)] = &[
("advanced_slice_patterns", Active),
("tuple_indexing", Active),
("associated_types", Active),
("visible_private_types", Active),

// if you change this list without updating src/doc/rust.md, cmr will be sad

Expand Down Expand Up @@ -100,6 +101,7 @@ pub struct Features {
pub overloaded_calls: bool,
pub rustc_diagnostic_macros: bool,
pub import_shadowing: bool,
pub visible_private_types: bool,
}

impl Features {
Expand All @@ -109,6 +111,7 @@ impl Features {
overloaded_calls: false,
rustc_diagnostic_macros: false,
import_shadowing: false,
visible_private_types: false,
}
}
}
Expand Down Expand Up @@ -479,6 +482,7 @@ pub fn check_crate(span_handler: &SpanHandler, krate: &ast::Crate) -> (Features,
overloaded_calls: cx.has_feature("overloaded_calls"),
rustc_diagnostic_macros: cx.has_feature("rustc_diagnostic_macros"),
import_shadowing: cx.has_feature("import_shadowing"),
visible_private_types: cx.has_feature("visible_private_types"),
},
unknown_features)
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/auxiliary/iss.rs
Expand Up @@ -12,7 +12,7 @@

// part of issue-6919.rs

struct C<'a> {
pub struct C<'a> {
pub k: ||: 'a,
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/auxiliary/noexporttypelib.rs
Expand Up @@ -8,5 +8,5 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

type oint = Option<int>;
pub type oint = Option<int>;
pub fn foo() -> oint { Some(3) }
2 changes: 1 addition & 1 deletion src/test/auxiliary/priv-impl-prim-ty.rs
Expand Up @@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

trait A {
pub trait A {
fn frob(&self);
}

Expand Down
Expand Up @@ -16,7 +16,7 @@ pub enum MaybeOwned<'a> {
Borrowed(&'a int)
}

struct Inv<'a> { // invariant w/r/t 'a
pub struct Inv<'a> { // invariant w/r/t 'a
x: &'a mut &'a int
}

Expand Down

5 comments on commit e9ad12c

@bors
Copy link
Contributor

@bors bors commented on e9ad12c Sep 23, 2014

Choose a reason for hiding this comment

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

saw approval from alexcrichton
at pcwalton@e9ad12c

@bors
Copy link
Contributor

@bors bors commented on e9ad12c Sep 23, 2014

Choose a reason for hiding this comment

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

merging pcwalton/rust/private-items-in-public-apis = e9ad12c into auto

@bors
Copy link
Contributor

@bors bors commented on e9ad12c Sep 23, 2014

Choose a reason for hiding this comment

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

pcwalton/rust/private-items-in-public-apis = e9ad12c merged ok, testing candidate = 3941d3c

@bors
Copy link
Contributor

@bors bors commented on e9ad12c Sep 23, 2014

Choose a reason for hiding this comment

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

@bors
Copy link
Contributor

@bors bors commented on e9ad12c Sep 23, 2014

Choose a reason for hiding this comment

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

fast-forwarding master to auto = 3941d3c

Please sign in to comment.