Skip to content

Commit 1b6b20a

Browse files
borsbors
committed
Auto merge of #38932 - petrochenkov:privctor, r=jseyfried
Privatize constructors of tuple structs with private fields This PR implements the strictest version of such "privatization" - it just sets visibilities for struct constructors, this affects everything including imports. ``` visibility(struct_ctor) = min(visibility(struct), visibility(field_1), ..., visibility(field_N)) ``` Needs crater run before proceeding. Resolves rust-lang/rfcs#902 r? @nikomatsakis
2 parents 2a6f7e4 + d38a8ad commit 1b6b20a

File tree

17 files changed

+295
-170
lines changed

17 files changed

+295
-170
lines changed

‎src/librustc/lint/builtin.rs

+7
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,12 @@ declare_lint! {
223223
"detects names that resolve to ambiguous glob imports with RFC 1560"
224224
}
225225

226+
declare_lint! {
227+
pub LEGACY_CONSTRUCTOR_VISIBILITY,
228+
Deny,
229+
"detects use of struct constructors that would be invisible with new visibility rules"
230+
}
231+
226232
declare_lint! {
227233
pub DEPRECATED,
228234
Warn,
@@ -271,6 +277,7 @@ impl LintPass for HardwiredLints {
271277
EXTRA_REQUIREMENT_IN_IMPL,
272278
LEGACY_DIRECTORY_OWNERSHIP,
273279
LEGACY_IMPORTS,
280+
LEGACY_CONSTRUCTOR_VISIBILITY,
274281
DEPRECATED
275282
)
276283
}

‎src/librustc_lint/lib.rs

+4
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,10 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
240240
id: LintId::of(LEGACY_IMPORTS),
241241
reference: "issue #38260 <https://github.com/rust-lang/rust/issues/38260>",
242242
},
243+
FutureIncompatibleInfo {
244+
id: LintId::of(LEGACY_CONSTRUCTOR_VISIBILITY),
245+
reference: "issue #39207 <https://github.com/rust-lang/rust/issues/39207>",
246+
},
243247
]);
244248

245249
// Register renamed and removed lints

‎src/librustc_metadata/encoder.rs

+7-1
Original file line numberDiff line numberDiff line change
@@ -396,10 +396,16 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
396396

397397
let struct_id = tcx.hir.as_local_node_id(adt_def_id).unwrap();
398398
let struct_vis = &tcx.hir.expect_item(struct_id).vis;
399+
let mut ctor_vis = ty::Visibility::from_hir(struct_vis, struct_id, tcx);
400+
for field in &variant.fields {
401+
if ctor_vis.is_at_least(field.vis, tcx) {
402+
ctor_vis = field.vis;
403+
}
404+
}
399405

400406
Entry {
401407
kind: EntryKind::Struct(self.lazy(&data)),
402-
visibility: self.lazy(&ty::Visibility::from_hir(struct_vis, struct_id, tcx)),
408+
visibility: self.lazy(&ctor_vis),
403409
span: self.lazy(&tcx.def_span(def_id)),
404410
attributes: LazySeq::empty(),
405411
children: LazySeq::empty(),

‎src/librustc_privacy/diagnostics.rs

+4-39
Original file line numberDiff line numberDiff line change
@@ -115,45 +115,6 @@ pub enum Foo {
115115
```
116116
"##,
117117

118-
E0450: r##"
119-
A tuple constructor was invoked while some of its fields are private. Erroneous
120-
code example:
121-
122-
```compile_fail,E0450
123-
mod Bar {
124-
pub struct Foo(isize);
125-
}
126-
127-
let f = Bar::Foo(0); // error: cannot invoke tuple struct constructor with
128-
// private fields
129-
```
130-
131-
To solve this issue, please ensure that all of the fields of the tuple struct
132-
are public. Alternatively, provide a `new()` method to the tuple struct to
133-
construct it from a given inner value. Example:
134-
135-
```
136-
mod Bar {
137-
pub struct Foo(pub isize); // we set its field to public
138-
}
139-
140-
let f = Bar::Foo(0); // ok!
141-
142-
// or:
143-
mod bar {
144-
pub struct Foo(isize);
145-
146-
impl Foo {
147-
pub fn new(x: isize) -> Foo {
148-
Foo(x)
149-
}
150-
}
151-
}
152-
153-
let f = bar::Foo::new(1);
154-
```
155-
"##,
156-
157118
E0451: r##"
158119
A struct constructor with private fields was invoked. Erroneous code example:
159120
@@ -204,3 +165,7 @@ let f = Bar::Foo::new(); // ok!
204165
"##,
205166

206167
}
168+
169+
register_diagnostics! {
170+
// E0450, moved into resolve
171+
}

‎src/librustc_privacy/lib.rs

+1-28
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ extern crate syntax_pos;
2727

2828
use rustc::dep_graph::DepNode;
2929
use rustc::hir::{self, PatKind};
30-
use rustc::hir::def::{self, Def, CtorKind};
30+
use rustc::hir::def::{self, Def};
3131
use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId};
3232
use rustc::hir::intravisit::{self, Visitor, NestedVisitorMap};
3333
use rustc::hir::itemlikevisit::DeepVisitor;
@@ -478,33 +478,6 @@ impl<'a, 'tcx> Visitor<'tcx> for PrivacyVisitor<'a, 'tcx> {
478478
}
479479
}
480480
}
481-
hir::ExprPath(hir::QPath::Resolved(_, ref path)) => {
482-
if let Def::StructCtor(_, CtorKind::Fn) = path.def {
483-
let adt_def = self.tcx.expect_variant_def(path.def);
484-
let private_indexes = adt_def.fields.iter().enumerate().filter(|&(_, field)| {
485-
!field.vis.is_accessible_from(self.curitem, self.tcx)
486-
}).map(|(i, _)| i).collect::<Vec<_>>();
487-
488-
if !private_indexes.is_empty() {
489-
let mut error = struct_span_err!(self.tcx.sess, expr.span, E0450,
490-
"cannot invoke tuple struct constructor \
491-
with private fields");
492-
error.span_label(expr.span,
493-
&format!("cannot construct with a private field"));
494-
495-
if let Some(node_id) = self.tcx.hir.as_local_node_id(adt_def.did) {
496-
let node = self.tcx.hir.find(node_id);
497-
if let Some(hir::map::NodeStructCtor(vdata)) = node {
498-
for i in private_indexes {
499-
error.span_label(vdata.fields()[i].span,
500-
&format!("private field declared here"));
501-
}
502-
}
503-
}
504-
error.emit();
505-
}
506-
}
507-
}
508481
_ => {}
509482
}
510483

‎src/librustc_resolve/build_reduced_graph.rs

+24-11
Original file line numberDiff line numberDiff line change
@@ -327,21 +327,26 @@ impl<'a> Resolver<'a> {
327327
let def = Def::Struct(self.definitions.local_def_id(item.id));
328328
self.define(parent, ident, TypeNS, (def, vis, sp, expansion));
329329

330-
// If this is a tuple or unit struct, define a name
331-
// in the value namespace as well.
332-
if !struct_def.is_struct() {
333-
let ctor_def = Def::StructCtor(self.definitions.local_def_id(struct_def.id()),
334-
CtorKind::from_ast(struct_def));
335-
self.define(parent, ident, ValueNS, (ctor_def, vis, sp, expansion));
336-
}
337-
338330
// Record field names for error reporting.
331+
let mut ctor_vis = vis;
339332
let field_names = struct_def.fields().iter().filter_map(|field| {
340-
self.resolve_visibility(&field.vis);
333+
let field_vis = self.resolve_visibility(&field.vis);
334+
if ctor_vis.is_at_least(field_vis, &*self) {
335+
ctor_vis = field_vis;
336+
}
341337
field.ident.map(|ident| ident.name)
342338
}).collect();
343339
let item_def_id = self.definitions.local_def_id(item.id);
344340
self.insert_field_names(item_def_id, field_names);
341+
342+
// If this is a tuple or unit struct, define a name
343+
// in the value namespace as well.
344+
if !struct_def.is_struct() {
345+
let ctor_def = Def::StructCtor(self.definitions.local_def_id(struct_def.id()),
346+
CtorKind::from_ast(struct_def));
347+
self.define(parent, ident, ValueNS, (ctor_def, ctor_vis, sp, expansion));
348+
self.struct_constructors.insert(def.def_id(), (ctor_def, ctor_vis));
349+
}
345350
}
346351

347352
ItemKind::Union(ref vdata, _) => {
@@ -434,9 +439,17 @@ impl<'a> Resolver<'a> {
434439
Def::Variant(..) | Def::TyAlias(..) => {
435440
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
436441
}
437-
Def::Fn(..) | Def::Static(..) | Def::Const(..) |
438-
Def::VariantCtor(..) | Def::StructCtor(..) => {
442+
Def::Fn(..) | Def::Static(..) | Def::Const(..) | Def::VariantCtor(..) => {
443+
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
444+
}
445+
Def::StructCtor(..) => {
439446
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
447+
448+
if let Some(struct_def_id) =
449+
self.session.cstore.def_key(def_id).parent
450+
.map(|index| DefId { krate: def_id.krate, index: index }) {
451+
self.struct_constructors.insert(struct_def_id, (def, vis));
452+
}
440453
}
441454
Def::Trait(..) => {
442455
let module_kind = ModuleKind::Def(def, ident.name);

‎src/librustc_resolve/lib.rs

+32-2
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ use rustc::hir::def::*;
4545
use rustc::hir::def_id::{CrateNum, CRATE_DEF_INDEX, LOCAL_CRATE, DefId};
4646
use rustc::ty;
4747
use rustc::hir::{Freevar, FreevarMap, TraitCandidate, TraitMap, GlobMap};
48-
use rustc::util::nodemap::{NodeMap, NodeSet, FxHashMap, FxHashSet};
48+
use rustc::util::nodemap::{NodeMap, NodeSet, FxHashMap, FxHashSet, DefIdMap};
4949

5050
use syntax::ext::hygiene::{Mark, SyntaxContext};
5151
use syntax::ast::{self, Name, NodeId, Ident, SpannedIdent, FloatTy, IntTy, UintTy};
@@ -1131,6 +1131,10 @@ pub struct Resolver<'a> {
11311131
warned_proc_macros: FxHashSet<Name>,
11321132

11331133
potentially_unused_imports: Vec<&'a ImportDirective<'a>>,
1134+
1135+
// This table maps struct IDs into struct constructor IDs,
1136+
// it's not used during normal resolution, only for better error reporting.
1137+
struct_constructors: DefIdMap<(Def, ty::Visibility)>,
11341138
}
11351139

11361140
pub struct ResolverArenas<'a> {
@@ -1310,6 +1314,7 @@ impl<'a> Resolver<'a> {
13101314
proc_macro_enabled: features.proc_macro,
13111315
warned_proc_macros: FxHashSet(),
13121316
potentially_unused_imports: Vec::new(),
1317+
struct_constructors: DefIdMap(),
13131318
}
13141319
}
13151320

@@ -2205,6 +2210,15 @@ impl<'a> Resolver<'a> {
22052210
_ => {}
22062211
},
22072212
_ if ns == ValueNS && is_struct_like(def) => {
2213+
if let Def::Struct(def_id) = def {
2214+
if let Some((ctor_def, ctor_vis))
2215+
= this.struct_constructors.get(&def_id).cloned() {
2216+
if is_expected(ctor_def) && !this.is_accessible(ctor_vis) {
2217+
err.span_label(span, &format!("constructor is not visible \
2218+
here due to private fields"));
2219+
}
2220+
}
2221+
}
22082222
err.span_label(span, &format!("did you mean `{} {{ /* fields */ }}`?",
22092223
path_str));
22102224
return err;
@@ -2235,7 +2249,23 @@ impl<'a> Resolver<'a> {
22352249
if is_expected(resolution.base_def) || resolution.base_def == Def::Err {
22362250
resolution
22372251
} else {
2238-
report_errors(self, Some(resolution.base_def))
2252+
// Add a temporary hack to smooth the transition to new struct ctor
2253+
// visibility rules. See #38932 for more details.
2254+
let mut res = None;
2255+
if let Def::Struct(def_id) = resolution.base_def {
2256+
if let Some((ctor_def, ctor_vis))
2257+
= self.struct_constructors.get(&def_id).cloned() {
2258+
if is_expected(ctor_def) && self.is_accessible(ctor_vis) {
2259+
let lint = lint::builtin::LEGACY_CONSTRUCTOR_VISIBILITY;
2260+
self.session.add_lint(lint, id, span,
2261+
"private struct constructors are not usable through \
2262+
reexports in outer modules".to_string());
2263+
res = Some(PathResolution::new(ctor_def));
2264+
}
2265+
}
2266+
}
2267+
2268+
res.unwrap_or_else(|| report_errors(self, Some(resolution.base_def)))
22392269
}
22402270
}
22412271
Some(resolution) if source.defer_to_typeck() => {

‎src/librustdoc/html/markdown.rs

-2
Original file line numberDiff line numberDiff line change
@@ -660,8 +660,6 @@ mod tests {
660660
t("no_run", false, true, false, true, false, false, Vec::new());
661661
t("test_harness", false, false, false, true, true, false, Vec::new());
662662
t("compile_fail", false, true, false, true, false, true, Vec::new());
663-
t("E0450", false, false, false, true, false, false,
664-
vec!["E0450".to_owned()]);
665663
t("{.no_run .example}", false, true, false, true, false, false, Vec::new());
666664
t("{.sh .should_panic}", true, false, false, true, false, false, Vec::new());
667665
t("{.example .rust}", false, false, false, true, false, false, Vec::new());

‎src/libsyntax/symbol.rs

+9-10
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ macro_rules! declare_keywords {(
140140
$(
141141
#[allow(non_upper_case_globals)]
142142
pub const $konst: Keyword = Keyword {
143-
ident: ast::Ident::with_empty_ctxt(ast::Name($index))
143+
ident: ast::Ident::with_empty_ctxt(super::Symbol($index))
144144
};
145145
)*
146146
}
@@ -282,25 +282,24 @@ impl Encodable for InternedString {
282282
#[cfg(test)]
283283
mod tests {
284284
use super::*;
285-
use ast::Name;
286285

287286
#[test]
288287
fn interner_tests() {
289288
let mut i: Interner = Interner::new();
290289
// first one is zero:
291-
assert_eq!(i.intern("dog"), Name(0));
290+
assert_eq!(i.intern("dog"), Symbol(0));
292291
// re-use gets the same entry:
293-
assert_eq!(i.intern ("dog"), Name(0));
292+
assert_eq!(i.intern ("dog"), Symbol(0));
294293
// different string gets a different #:
295-
assert_eq!(i.intern("cat"), Name(1));
296-
assert_eq!(i.intern("cat"), Name(1));
294+
assert_eq!(i.intern("cat"), Symbol(1));
295+
assert_eq!(i.intern("cat"), Symbol(1));
297296
// dog is still at zero
298-
assert_eq!(i.intern("dog"), Name(0));
297+
assert_eq!(i.intern("dog"), Symbol(0));
299298
// gensym gets 3
300-
assert_eq!(i.gensym("zebra"), Name(2));
299+
assert_eq!(i.gensym("zebra"), Symbol(2));
301300
// gensym of same string gets new number :
302-
assert_eq!(i.gensym("zebra"), Name(3));
301+
assert_eq!(i.gensym("zebra"), Symbol(3));
303302
// gensym of *existing* string gets new number:
304-
assert_eq!(i.gensym("dog"), Name(4));
303+
assert_eq!(i.gensym("dog"), Symbol(4));
305304
}
306305
}

‎src/test/compile-fail-fulldeps/explore-issue-38412.rs

-7
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,14 @@ use pub_and_stability::{Record, Trait, Tuple};
2525
fn main() {
2626
// Okay
2727
let Record { .. } = Record::new();
28-
// Okay (for now; see RFC Issue #902)
29-
let Tuple(..) = Tuple::new();
3028

3129
// Okay
3230
let Record { a_stable_pub: _, a_unstable_declared_pub: _, .. } = Record::new();
33-
// Okay (for now; see RFC Issue #902)
34-
let Tuple(_, _, ..) = Tuple::new(); // analogous to above
3531

3632
let Record { a_stable_pub: _, a_unstable_declared_pub: _, a_unstable_undeclared_pub: _, .. } =
3733
Record::new();
3834
//~^^ ERROR use of unstable library feature 'unstable_undeclared'
3935

40-
let Tuple(_, _, _, ..) = Tuple::new(); // analogous to previous
41-
//~^ ERROR use of unstable library feature 'unstable_undeclared'
42-
4336
let r = Record::new();
4437
let t = Tuple::new();
4538

‎src/test/compile-fail/E0451.rs

-5
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,6 @@ fn pat_match(foo: Bar::Foo) {
2525
//~^ NOTE field `b` is private
2626
}
2727

28-
fn pat_match_tuple(foo: Bar::FooTuple) {
29-
let Bar::FooTuple(a,b) = foo; //~ ERROR E0451
30-
//~^ NOTE field `1` is private
31-
}
32-
3328
fn main() {
3429
let f = Bar::Foo{ a: 0, b: 0 }; //~ ERROR E0451
3530
//~^ NOTE field `b` is private

‎src/test/compile-fail/issue-38412.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010

1111
fn main() {
1212
let Box(a) = loop { };
13-
//~^ ERROR field `0` of struct `std::boxed::Box` is private
13+
//~^ ERROR expected tuple struct/variant, found struct `Box`
14+
//~| ERROR expected tuple struct/variant, found struct `Box`
1415

1516
// (The below is a trick to allow compiler to infer a type for
1617
// variable `a` without attempting to ascribe a type to the

‎src/test/compile-fail/privacy/legacy-ctor-visibility.rs

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
#![allow(unused)]
12+
13+
use m::S;
14+
15+
mod m {
16+
pub struct S(u8);
17+
18+
mod n {
19+
use S;
20+
fn f() {
21+
S(10);
22+
//~^ ERROR private struct constructors are not usable through reexports in outer modules
23+
//~| WARN this was previously accepted
24+
}
25+
}
26+
}
27+
28+
fn main() {}

0 commit comments

Comments
 (0)