Skip to content

librustc: Require that types mentioned anywhere in public signatures be #16467

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

Closed
wants to merge 1 commit into from
Closed
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
6 changes: 5 additions & 1 deletion src/libarena/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@
html_favicon_url = "http://www.rust-lang.org/favicon.ico",
html_root_url = "http://doc.rust-lang.org/master/")]

#![feature(unsafe_destructor)]
#![feature(unsafe_destructor, visible_private_types)]
#![allow(missing_doc)]

// NOTE(stage0, pcwalton): Remove after snapshot.
#![allow(unknown_features)]

use std::cell::{Cell, RefCell};
use std::cmp;
use std::intrinsics::{TyDesc, get_tydesc};
Expand Down Expand Up @@ -359,6 +362,7 @@ pub struct TypedArena<T> {
/// A pointer to the first arena segment.
first: RefCell<TypedArenaChunkRef<T>>,
}

type TypedArenaChunkRef<T> = Option<Box<TypedArenaChunk<T>>>;

struct TypedArenaChunk<T> {
Expand Down
1 change: 0 additions & 1 deletion src/libcore/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2182,7 +2182,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>(f: |T|: 'a -> T, seed: T) -> Iterate<'a, T> {
Unfold::new((f, Some(seed), true), |st| {
let &(ref mut f, ref mut val, ref mut first) = st;
Expand Down
5 changes: 4 additions & 1 deletion src/libcore/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,12 @@

#![no_std]
#![feature(globs, intrinsics, lang_items, macro_rules, managed_boxes, phase)]
#![feature(simd, unsafe_destructor)]
#![feature(simd, unsafe_destructor, visible_private_types)]
#![deny(missing_doc)]

// NOTE(stage0, pcwalton): Remove after snapshot.
#![allow(unknown_features)]

mod macros;

#[path = "num/float_macros.rs"] mod float_macros;
Expand Down
5 changes: 4 additions & 1 deletion src/libdebug/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@
html_favicon_url = "http://www.rust-lang.org/favicon.ico",
html_root_url = "http://doc.rust-lang.org/master/")]
#![experimental]
#![feature(managed_boxes, macro_rules)]
#![feature(managed_boxes, macro_rules, visible_private_types)]
#![allow(experimental)]

// NOTE(stage0, pcwalton): Remove after snapshot.
#![allow(unknown_features)]

pub mod fmt;
pub mod reflect;
pub mod repr;
7 changes: 5 additions & 2 deletions src/libgreen/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,11 @@
html_playground_url = "http://play.rust-lang.org/")]

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

// NOTE(stage0, pcwalton): Remove after snapshot.
#![allow(unknown_features)]

#[cfg(test)] #[phase(plugin, link)] extern crate log;
#[cfg(test)] extern crate rustuv;
Expand Down
1 change: 0 additions & 1 deletion src/libnative/io/timer_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ struct Inner {
id: uint,
}

#[allow(visible_private_types)]
pub enum Req {
Copy link
Contributor

Choose a reason for hiding this comment

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

Inner mus be public because Req is public, but it's not clear to me why Req is public.

// Add a new timer to the helper thread.
NewTimer(Box<Inner>),
Expand Down
4 changes: 4 additions & 0 deletions src/libnative/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@
// consider whether they're needed before adding that feature here (the
// answer is that you don't need them)
#![feature(macro_rules, unsafe_destructor, default_type_params)]
#![feature(visible_private_types)]

// NOTE(stage0, pcwalton): Remove after snapshot.
#![allow(unknown_features)]

extern crate alloc;
extern crate libc;
Expand Down
4 changes: 1 addition & 3 deletions src/libregex/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// 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)]
#![allow(missing_doc)]

use std::cmp;
use parse;
Expand Down
5 changes: 4 additions & 1 deletion src/libregex/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,12 @@
html_root_url = "http://doc.rust-lang.org/master/",
html_playground_url = "http://play.rust-lang.org/")]

#![feature(macro_rules, phase)]
#![feature(macro_rules, phase, visible_private_types)]
#![deny(missing_doc)]

// NOTE(stage0, pcwalton): Remove after snapshot.
#![allow(unknown_features)]

#[cfg(test)]
extern crate stdtest = "test";
#[cfg(test)]
Expand Down
2 changes: 0 additions & 2 deletions src/libregex/re.rs
Original file line number Diff line number Diff line change
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 Down
8 changes: 6 additions & 2 deletions src/librustc/front/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ static KNOWN_FEATURES: &'static [(&'static str, Status)] = &[

("rustc_diagnostic_macros", Active),
("unboxed_closures", Active),
("visible_private_types", Active),

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

Expand Down Expand Up @@ -98,7 +99,8 @@ pub struct Features {
pub default_type_params: Cell<bool>,
pub issue_5723_bootstrap: Cell<bool>,
pub overloaded_calls: Cell<bool>,
pub rustc_diagnostic_macros: Cell<bool>
pub rustc_diagnostic_macros: Cell<bool>,
pub visible_private_types: Cell<bool>,
}

impl Features {
Expand All @@ -107,7 +109,8 @@ impl Features {
default_type_params: Cell::new(false),
issue_5723_bootstrap: Cell::new(false),
overloaded_calls: Cell::new(false),
rustc_diagnostic_macros: Cell::new(false)
rustc_diagnostic_macros: Cell::new(false),
visible_private_types: Cell::new(false),
}
}
}
Expand Down Expand Up @@ -439,4 +442,5 @@ pub fn check_crate(sess: &Session, krate: &ast::Crate) {
sess.features.issue_5723_bootstrap.set(cx.has_feature("issue_5723_bootstrap"));
sess.features.overloaded_calls.set(cx.has_feature("overloaded_calls"));
sess.features.rustc_diagnostic_macros.set(cx.has_feature("rustc_diagnostic_macros"));
sess.features.visible_private_types.set(cx.has_feature("visible_private_types"));
}
4 changes: 0 additions & 4 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1539,9 +1539,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 @@ -1570,7 +1567,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
Original file line number Diff line number Diff line change
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to Repr in debug. Harmless to be public.

fn id(&self) -> ast::NodeId;
fn span(&self) -> Span;
}
Expand Down
69 changes: 26 additions & 43 deletions src/librustc/middle/privacy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,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};
Expand Down Expand Up @@ -163,23 +162,6 @@ struct EmbargoVisitor<'a> {
prev_public: bool,
}

impl<'a> EmbargoVisitor<'a> {
// There are checks inside of privacy which depend on knowing whether a
// trait should be exported or not. The two current consumers of this are:
//
// 1. Should default methods of a trait be exported?
// 2. Should the methods of an implementation of a trait be exported?
//
// The answer to both of these questions partly rely on whether the trait
// itself is exported or not. If the trait is somehow exported, then the
// answers to both questions must be yes. Right now this question involves
// more analysis than is currently done in rustc, so we conservatively
// answer "yes" so that all traits need to be exported.
fn exported_trait(&self, _id: ast::NodeId) -> bool {
true
}
}

impl<'a> Visitor<()> for EmbargoVisitor<'a> {
fn visit_item(&mut self, item: &ast::Item, _: ()) {
let orig_all_pub = self.prev_public;
Expand All @@ -194,12 +176,6 @@ impl<'a> Visitor<()> for EmbargoVisitor<'a> {
// cannot have visibility qualifiers on them anyway
ast::ItemImpl(..) | ast::ItemForeignMod(..) => {}

// Traits are a little special in that even if they themselves are
// not public they may still be exported.
ast::ItemTrait(..) => {
self.prev_exported = self.exported_trait(item.id);
}

// Private by default, hence we only retain the "public chain" if
// `pub` is explicitly listed.
_ => {
Expand Down Expand Up @@ -1218,7 +1194,6 @@ impl<'a> SanePrivacyVisitor<'a> {
struct VisiblePrivateTypesVisitor<'a> {
tcx: &'a ty::ctxt,
exported_items: &'a ExportedItems,
public_items: &'a PublicItems,
}

struct CheckTypeForPrivatenessVisitor<'a, 'b> {
Expand Down Expand Up @@ -1250,9 +1225,14 @@ impl<'a> VisiblePrivateTypesVisitor<'a> {
}

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)
self.exported_items.contains(&trait_id)
}

fn check_path(&self, path: &ast::Path, path_id: ast::NodeId) {
if self.path_is_private_type(path_id) {
self.tcx.sess.span_err(path.span,
"private type in exported type signature");
}
}
}

Expand Down Expand Up @@ -1407,10 +1387,6 @@ impl<'a> Visitor<()> for VisiblePrivateTypesVisitor<'a> {
return
}

// `type ... = ...;` can contain private types, because
// we're introducing a new name.
ast::ItemTy(..) => return,

// not at all public, so we don't care
_ if !self.exported_items.contains(&item.id) => return,

Expand Down Expand Up @@ -1439,17 +1415,24 @@ impl<'a> Visitor<()> for VisiblePrivateTypesVisitor<'a> {
}
}

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());
fn visit_generics(&mut self, g: &ast::Generics, _: ()) {
for type_parameter in g.ty_params.iter() {
for bound in type_parameter.bounds.iter() {
match *bound {
ast::TraitTyParamBound(ref trait_ref) => {
self.check_path(&trait_ref.path, trait_ref.ref_id)
}
ast::StaticRegionTyParamBound |
ast::UnboxedFnTyParamBound(_) |
ast::OtherRegionTyParamBound(_) => {}
}
}
}
}

fn visit_ty(&mut self, t: &ast::Ty, _: ()) {
match t.node {
ast::TyPath(ref p, _, path_id) => self.check_path(p, path_id),
_ => {}
}
visit::walk_ty(self, t, ())
Expand Down Expand Up @@ -1535,13 +1518,13 @@ pub fn check_crate(tcx: &ty::ctxt,

let EmbargoVisitor { exported_items, public_items, .. } = visitor;

{
if !tcx.sess.features.visible_private_types.get() {
let mut visitor = VisiblePrivateTypesVisitor {
tcx: tcx,
exported_items: &exported_items,
public_items: &public_items
};
visit::walk_crate(&mut visitor, krate, ());
}

return (exported_items, public_items);
}
2 changes: 1 addition & 1 deletion src/librustc/middle/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ use std::uint;
// Definition mapping
pub type DefMap = RefCell<NodeMap<Def>>;

struct binding_info {
pub struct binding_info {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a good thing that this was made public. That is, it IS reachable, so it's clearer to see it public, even if the fields are private and hence it is not used from outside the module.

span: Span,
binding_mode: BindingMode,
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/middle/resolve_lifetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ struct LifetimeContext<'a> {
named_region_map: NamedRegionMap,
}

enum ScopeChain<'a> {
pub enum ScopeChain<'a> {
/// EarlyScope(i, ['a, 'b, ...], s) extends s with early-bound
/// lifetimes, assigning indexes 'a => i, 'b => i+1, ... etc.
EarlyScope(subst::ParamSpace, &'a Vec<ast::LifetimeDef>, Scope<'a>),
Expand All @@ -69,7 +69,7 @@ enum ScopeChain<'a> {
RootScope
}

type Scope<'a> = &'a ScopeChain<'a>;
pub type Scope<'a> = &'a ScopeChain<'a>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting case. I'm not sure what rule from RFC triggered the requirement that Scope be public, but it is a case I was wondering about: Here, we implement the Visitor trait, which means that conceivably there are functions that reference Scope that are visible from outside the module, except that the type we implemented Visitor for is itself private. So I think that this change should be unnecessary (I have to write out the rule I propose for impls, but the basic idea is that things that appear in the impl must be public iff the impl is associated with a public type). Is there another path to Scope I am not seeing that would warrant it being public?

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 because of EarlyScope/LateScope/BlockScope in ScopeChain.


pub fn krate(sess: &Session, krate: &ast::Crate) -> NamedRegionMap {
let mut ctxt = LifetimeContext {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/typeck/infer/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1429,7 +1429,7 @@ impl<'a> ErrorReportingHelpers for InferCtxt<'a> {
}
}

trait Resolvable {
pub trait Resolvable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Private trait that is only locally implemented. OK.

fn resolve(&self, infcx: &InferCtxt) -> Self;
fn contains_error(&self) -> bool;
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/typeck/infer/lattice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use util::ppaux::Repr;

use std::collections::HashMap;

trait LatticeValue : Clone + Repr + PartialEq {
pub trait LatticeValue : Clone + Repr + PartialEq {
Copy link
Contributor

Choose a reason for hiding this comment

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

Private trait that is only locally implemented. OK.

fn sub(cf: CombineFields, a: &Self, b: &Self) -> ures;
fn lub(cf: CombineFields, a: &Self, b: &Self) -> cres<Self>;
fn glb(cf: CombineFields, a: &Self, b: &Self) -> cres<Self>;
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/middle/typeck/variance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,11 @@ pub fn infer_variance(tcx: &ty::ctxt,
* a variable.
*/

type VarianceTermPtr<'a> = &'a VarianceTerm<'a>;
pub type VarianceTermPtr<'a> = &'a VarianceTerm<'a>;

struct InferredIndex(uint);
pub struct InferredIndex(uint);

enum VarianceTerm<'a> {
pub enum VarianceTerm<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure that these changes are not necessary (per the impl rule I described above).

ConstantTerm(ty::Variance),
TransformTerm(VarianceTermPtr<'a>, VarianceTermPtr<'a>),
InferredTerm(InferredIndex),
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_llvm/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ pub enum AttributeSet {
FunctionIndex = !0
}

trait AttrHelper {
pub trait AttrHelper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Private trait that is only locally implemented. OK.

fn apply_llfn(&self, idx: c_uint, llfn: ValueRef);
fn apply_callsite(&self, idx: c_uint, callsite: ValueRef);
}
Expand Down
Loading