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

Implement public/private dependency feature #57586

Merged
merged 22 commits into from
Feb 1, 2019
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
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2878,6 +2878,7 @@ dependencies = [
name = "rustc_privacy"
version = "0.0.0"
dependencies = [
"log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
"rustc 0.0.0",
"rustc_data_structures 0.0.0",
"rustc_typeck 0.0.0",
Expand Down
7 changes: 7 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,12 @@ declare_lint! {
"detect private items in public interfaces not caught by the old implementation"
}

declare_lint! {
pub EXPORTED_PRIVATE_DEPENDENCIES,
Warn,
"public interface leaks type from a private dependency"
}

declare_lint! {
pub PUB_USE_OF_PRIVATE_EXTERN_CRATE,
Deny,
Expand Down Expand Up @@ -405,6 +411,7 @@ impl LintPass for HardwiredLints {
TRIVIAL_CASTS,
TRIVIAL_NUMERIC_CASTS,
PRIVATE_IN_PUBLIC,
EXPORTED_PRIVATE_DEPENDENCIES,
PUB_USE_OF_PRIVATE_EXTERN_CRATE,
INVALID_TYPE_PARAM_DEFAULT,
CONST_ERR,
Expand Down
26 changes: 25 additions & 1 deletion src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,10 @@ top_level_options!(
remap_path_prefix: Vec<(PathBuf, PathBuf)> [UNTRACKED],

edition: Edition [TRACKED],

// The list of crates to consider private when
// checking leaked private dependency types in public interfaces
extern_private: Vec<String> [TRACKED],
}
);

Expand Down Expand Up @@ -606,6 +610,7 @@ impl Default for Options {
cli_forced_thinlto_off: false,
remap_path_prefix: Vec::new(),
edition: DEFAULT_EDITION,
extern_private: Vec::new()
}
}
}
Expand Down Expand Up @@ -1724,6 +1729,12 @@ pub fn rustc_optgroups() -> Vec<RustcOptGroup> {
"Specify where an external rust library is located",
"NAME=PATH",
),
opt::multi_s(
"",
"extern-private",
"Specify where an extern rust library is located, marking it as a private dependency",
"NAME=PATH",
),
opt::opt_s("", "sysroot", "Override the system root", "PATH"),
opt::multi("Z", "", "Set internal debugging options", "FLAG"),
opt::opt_s(
Expand Down Expand Up @@ -1905,6 +1916,7 @@ pub fn build_session_options_and_crate_config(
let crate_types = parse_crate_types_from_list(unparsed_crate_types)
.unwrap_or_else(|e| early_error(error_format, &e[..]));


let (lint_opts, describe_lints, lint_cap) = get_cmd_lint_options(matches, error_format);

let mut debugging_opts = build_debugging_options(matches, error_format);
Expand Down Expand Up @@ -2218,8 +2230,18 @@ pub fn build_session_options_and_crate_config(
);
}

if matches.opt_present("extern-private") && !debugging_opts.unstable_options {
early_error(
ErrorOutputType::default(),
"'--extern-private' is unstable and only \
available for nightly builds of rustc."
)
}

let extern_private = matches.opt_strs("extern-private");

let mut externs: BTreeMap<_, BTreeSet<_>> = BTreeMap::new();
for arg in &matches.opt_strs("extern") {
for arg in matches.opt_strs("extern").into_iter().chain(matches.opt_strs("extern-private")) {
let mut parts = arg.splitn(2, '=');
let name = parts.next().unwrap_or_else(||
early_error(error_format, "--extern value must not be empty"));
Expand Down Expand Up @@ -2287,6 +2309,7 @@ pub fn build_session_options_and_crate_config(
cli_forced_thinlto_off: disable_thinlto,
remap_path_prefix,
edition,
extern_private
},
cfg,
)
Expand Down Expand Up @@ -2460,6 +2483,7 @@ mod dep_tracking {
impl_dep_tracking_hash_via_hash!(Option<usize>);
impl_dep_tracking_hash_via_hash!(Option<String>);
impl_dep_tracking_hash_via_hash!(Option<(String, u64)>);
impl_dep_tracking_hash_via_hash!(Option<Vec<String>>);
impl_dep_tracking_hash_via_hash!(Option<MergeFunctions>);
impl_dep_tracking_hash_via_hash!(Option<PanicStrategy>);
impl_dep_tracking_hash_via_hash!(Option<RelroLevel>);
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_privacy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ rustc = { path = "../librustc" }
rustc_typeck = { path = "../librustc_typeck" }
syntax = { path = "../libsyntax" }
syntax_pos = { path = "../libsyntax_pos" }
rustc_data_structures = { path = "../librustc_data_structures" }
rustc_data_structures = { path = "../librustc_data_structures" }
log = "0.4"
38 changes: 38 additions & 0 deletions src/librustc_privacy/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#[macro_use] extern crate rustc;
#[macro_use] extern crate syntax;
#[macro_use] extern crate log;
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved
extern crate rustc_typeck;
extern crate syntax_pos;
extern crate rustc_data_structures;
Expand Down Expand Up @@ -1451,13 +1452,15 @@ impl<'a, 'tcx> Visitor<'tcx> for ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx> {

struct SearchInterfaceForPrivateItemsVisitor<'a, 'tcx: 'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
item_id: ast::NodeId,
item_def_id: DefId,
span: Span,
/// The visitor checks that each component type is at least this visible.
required_visibility: ty::Visibility,
has_pub_restricted: bool,
has_old_errors: bool,
in_assoc_ty: bool,
private_crates: FxHashSet<CrateNum>
}

impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
Expand Down Expand Up @@ -1492,6 +1495,16 @@ impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
}

fn check_def_id(&mut self, def_id: DefId, kind: &str, descr: &dyn fmt::Display) -> bool {
if self.leaks_private_dep(def_id) {
self.tcx.lint_node(lint::builtin::EXPORTED_PRIVATE_DEPENDENCIES,
self.item_id,
self.span,
&format!("{} `{}` from private dependency '{}' in public \
interface", kind, descr,
self.tcx.crate_name(def_id.krate)));

}

let node_id = match self.tcx.hir().as_local_node_id(def_id) {
Some(node_id) => node_id,
None => return false,
Expand All @@ -1514,9 +1527,23 @@ impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
self.tcx.lint_node(lint::builtin::PRIVATE_IN_PUBLIC, node_id, self.span,
&format!("{} (error {})", msg, err_code));
}

}

false
}

/// An item is 'leaked' from a private dependency if all
/// of the following are true:
/// 1. It's contained within a public type
/// 2. It comes from a private crate
fn leaks_private_dep(&self, item_id: DefId) -> bool {
let ret = self.required_visibility == ty::Visibility::Public &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This will report things used in pub, but unreachable items, e.g.

mod private_details {
    pub fn details() -> PrivateDep { ... } // Will be linted
}

, but that's probably okay for a start.

self.private_crates.contains(&item_id.krate);

debug!("leaks_private_dep(item_id={:?})={}", item_id, ret);
return ret;
}
}

impl<'a, 'tcx> DefIdVisitor<'a, 'tcx> for SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
Expand All @@ -1530,6 +1557,7 @@ struct PrivateItemsInPublicInterfacesVisitor<'a, 'tcx: 'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
has_pub_restricted: bool,
old_error_set: &'a NodeSet,
private_crates: FxHashSet<CrateNum>
}

impl<'a, 'tcx> PrivateItemsInPublicInterfacesVisitor<'a, 'tcx> {
Expand Down Expand Up @@ -1560,12 +1588,14 @@ impl<'a, 'tcx> PrivateItemsInPublicInterfacesVisitor<'a, 'tcx> {

SearchInterfaceForPrivateItemsVisitor {
tcx: self.tcx,
item_id,
item_def_id: self.tcx.hir().local_def_id(item_id),
span: self.tcx.hir().span(item_id),
required_visibility,
has_pub_restricted: self.has_pub_restricted,
has_old_errors,
in_assoc_ty: false,
private_crates: self.private_crates.clone()
}
}

Expand Down Expand Up @@ -1690,6 +1720,7 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) -> Lrc<AccessLevels> {
fn check_mod_privacy<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>, module_def_id: DefId) {
let empty_tables = ty::TypeckTables::empty(None);


// Check privacy of names not checked in previous compilation stages.
let mut visitor = NamePrivacyVisitor {
tcx,
Expand Down Expand Up @@ -1725,6 +1756,12 @@ fn privacy_access_levels<'tcx>(
queries::check_mod_privacy::ensure(tcx, tcx.hir().local_def_id(module));
}

let private_crates: FxHashSet<CrateNum> = tcx.sess.opts.extern_private.iter()
.flat_map(|c| {
tcx.crates().iter().find(|&&krate| &tcx.crate_name(krate) == c).cloned()
}).collect();


// Build up a set of all exported items in the AST. This is a set of all
// items which are reachable from external crates based on visibility.
let mut visitor = EmbargoVisitor {
Expand Down Expand Up @@ -1767,6 +1804,7 @@ fn privacy_access_levels<'tcx>(
tcx,
has_pub_restricted,
old_error_set: &visitor.old_error_set,
private_crates
};
krate.visit_all_item_likes(&mut DeepVisitor::new(&mut visitor));
}
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/feature-gates/auxiliary/pub_dep.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub struct PubType;
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// This test is different from other feature gate tests.
// Instead of checking that an error occurs without the feature gate,
// it checks that *no* errors/warnings occurs without the feature gate.
// This is due to the fact that 'public_private_dependencies' just enables
// a lint, so disabling it shouldn't cause any code to stop compiling.

// run-pass
// aux-build:pub_dep.rs

// Without ![feature(public_private_dependencies)],
// this should do nothing/
#![deny(exported_private_dependencies)]

extern crate pub_dep;

pub struct Foo {
pub field: pub_dep::PubType
}

fn main() {}
2 changes: 2 additions & 0 deletions src/test/ui/privacy/pub-priv-dep/auxiliary/priv_dep.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub struct OtherType;
pub trait OtherTrait {}
1 change: 1 addition & 0 deletions src/test/ui/privacy/pub-priv-dep/auxiliary/pub_dep.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub struct PubType;
46 changes: 46 additions & 0 deletions src/test/ui/privacy/pub-priv-dep/pub-priv1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// aux-build:priv_dep.rs
// aux-build:pub_dep.rs
// compile-flags: --extern-private priv_dep
#![deny(exported_private_dependencies)]

// This crate is a private dependency
extern crate priv_dep;
// This crate is a public dependenct
extern crate pub_dep;

use priv_dep::{OtherType, OtherTrait};
use pub_dep::PubType;

// Type from private dependency used in private
// type - this is fine
struct PrivateType {
field: OtherType
}

pub struct PublicType {
pub field: OtherType,
//~^ ERROR type `priv_dep::OtherType` from private dependency 'priv_dep' in public interface
priv_field: OtherType, // Private field - this is fine
pub other_field: PubType // Type from public dependency - this is fine
}

impl PublicType {
pub fn pub_fn(param: OtherType) {}
//~^ ERROR type `priv_dep::OtherType` from private dependency 'priv_dep' in public interface

fn priv_fn(param: OtherType) {}
}

pub trait MyPubTrait {
type Foo: OtherTrait;
}
//~^^^ ERROR trait `priv_dep::OtherTrait` from private dependency 'priv_dep' in public interface

pub struct AllowedPrivType {
#[allow(exported_private_dependencies)]
pub allowed: OtherType
}



fn main() {}
28 changes: 28 additions & 0 deletions src/test/ui/privacy/pub-priv-dep/pub-priv1.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
error: type `priv_dep::OtherType` from private dependency 'priv_dep' in public interface
--> $DIR/pub-priv1.rs:21:5
|
LL | pub field: OtherType,
| ^^^^^^^^^^^^^^^^^^^^
|
note: lint level defined here
--> $DIR/pub-priv1.rs:4:9
|
LL | #![deny(exported_private_dependencies)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: type `priv_dep::OtherType` from private dependency 'priv_dep' in public interface
--> $DIR/pub-priv1.rs:28:5
|
LL | pub fn pub_fn(param: OtherType) {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: trait `priv_dep::OtherTrait` from private dependency 'priv_dep' in public interface
--> $DIR/pub-priv1.rs:34:1
|
LL | / pub trait MyPubTrait {
LL | | type Foo: OtherTrait;
LL | | }
| |_^

error: aborting due to 3 previous errors

12 changes: 12 additions & 0 deletions src/test/ui/privacy/pub-priv-dep/std-pub.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// The 'std' crates should always be implicitly public,
// without having to pass any compiler arguments

// run-pass

#![deny(exported_private_dependencies)]

pub struct PublicType {
pub field: Option<u8>
}

fn main() {}