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.

Sign up for GitHub

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

Change privacy checks, particularly for tuple structs #58173

Closed
wants to merge 1 commit into from
Closed

Change privacy checks, particularly for tuple structs #58173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 11 additions & 4 deletions src/librustc/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2242,13 +2242,20 @@ impl<'a, 'gcx, 'tcx> AdtDef {
.0
}

pub fn variant_of_def(&self, def: Def) -> &VariantDef {
pub fn opt_variant_of_def(&self, def: Def) -> Option<&VariantDef> {
match def {
Def::Variant(vid) | Def::VariantCtor(vid, ..) => self.variant_with_id(vid),
Def::Variant(vid) | Def::VariantCtor(vid, ..) => Some(self.variant_with_id(vid)),
Def::Struct(..) | Def::StructCtor(..) | Def::Union(..) |
Def::TyAlias(..) | Def::AssociatedTy(..) | Def::SelfTy(..) |
Def::SelfCtor(..) => self.non_enum_variant(),
_ => bug!("unexpected def {:?} in variant_of_def", def)
Def::SelfCtor(..) => Some(self.non_enum_variant()),
_ => None,
}
}

pub fn variant_of_def(&self, def: Def) -> &VariantDef {
match self.opt_variant_of_def(def) {
Some(vd) => vd,
None => bug!("unexpected def {:?} in variant_of_def", def),
}
}

Expand Down
204 changes: 184 additions & 20 deletions src/librustc_privacy/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,10 @@ impl<'a, 'tcx, V> TypeVisitor<'tcx> for DefIdVisitorSkeleton<'_, 'a, 'tcx, V>
}
}

fn def_id_visibility<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId)
-> (ty::Visibility, Span, &'static str) {
fn def_id_visibility<'a, 'tcx>(
tcx: TyCtxt<'a, 'tcx, 'tcx>,
def_id: DefId,
) -> (ty::Visibility, Span, &'static str) {
match tcx.hir().as_local_node_id(def_id) {
Some(node_id) => {
let vis = match tcx.hir().get(node_id) {
Expand Down Expand Up @@ -799,22 +801,71 @@ struct NamePrivacyVisitor<'a, 'tcx: 'a> {
tables: &'a ty::TypeckTables<'tcx>,
current_item: ast::NodeId,
empty_tables: &'a ty::TypeckTables<'tcx>,
reported_tuple_structs: FxHashSet<Span>,
Copy link
Contributor

Choose a reason for hiding this comment

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

explain this field here instead of just at the use sites

}

impl<'a, 'tcx> NamePrivacyVisitor<'a, 'tcx> {
// Checks that a field in a struct constructor (expression or pattern) is accessible.
fn check_field(&mut self,
use_ctxt: Span, // syntax context of the field name at the use site
span: Span, // span of the field pattern, e.g., `x: 0`
def: &'tcx ty::AdtDef, // definition of the struct or enum
field: &'tcx ty::FieldDef) { // definition of the field
fn check_field(
&mut self,
use_ctxt: Span, // syntax context of the field name at the use site
span: Span, // span of the field pattern, e.g., `x: 0`
def: &'tcx ty::AdtDef, // definition of the struct or enum
field: &'tcx ty::FieldDef, // definition of the field
) -> Option<(String /* field name */, Span)> {
let ident = Ident::new(keywords::Invalid.name(), use_ctxt);
let def_id = self.tcx.adjust_ident(ident, def.did, self.current_item).1;
if !def.is_enum() && !field.vis.is_accessible_from(def_id, self.tcx) {
struct_span_err!(self.tcx.sess, span, E0451, "field `{}` of {} `{}` is private",
field.ident, def.variant_descr(), self.tcx.item_path_str(def.did))
.span_label(span, format!("field `{}` is private", field.ident))
.emit();
return Some((field.ident.to_string(), span));
}
None
}

/// If appropriate, construct a privacy error pointing at all the fields of a literal struct
/// that are private both when constructing an instance or destructuring a pattern.
fn emit_field_checks(
&mut self,
// d: Def,
def: &'tcx ty::AdtDef, // definition of the struct or enum
span: Span, // struct span at use site
fields: Vec<(String, Span)>, // inaccessible ADT fields
action: &str, // "built" or "destructured" depending of where this happened
) {
let item_path = self.tcx.item_path_str(def.did);

if !fields.is_empty() {
self.reported_tuple_structs.insert(span);
let mut err = struct_span_err!(
self.tcx.sess,
fields.iter().map(|(_, sp)| *sp).collect::<Vec<Span>>(),
E0451,
"field{} of {} `{}` {} private",
if fields.len() == 1 {
format!(" `{}`", fields[0].0)
} else {
"s".to_owned()
},
def.variant_descr(),
item_path,
if fields.len() == 1 {
"is"
} else {
"are"
},
);
err.span_label(span, format!(
"`{}` cannot be {} due to private field{}",
item_path,
action,
if fields.len() == 1 { "" } else { "s" },
));
for (_field_name, field) in fields {
err.span_label(field, "private field");
}

// Point at definition
err.span_label(self.tcx.def_span(def.did), format!("`{}` defined here", item_path));
err.emit();
}
}
}
Expand Down Expand Up @@ -867,6 +918,7 @@ impl<'a, 'tcx> Visitor<'tcx> for NamePrivacyVisitor<'a, 'tcx> {
let def = self.tables.qpath_def(qpath, expr.hir_id);
let adt = self.tables.expr_ty(expr).ty_adt_def().unwrap();
let variant = adt.variant_of_def(def);
let mut field_errors = vec![];
if let Some(ref base) = *base {
// If the expression uses FRU we need to make sure all the unmentioned fields
// are checked for privacy (RFC 736). Rather than computing the set of
Expand All @@ -879,13 +931,48 @@ impl<'a, 'tcx> Visitor<'tcx> for NamePrivacyVisitor<'a, 'tcx> {
Some(field) => (field.ident.span, field.span),
None => (base.span, base.span),
};
self.check_field(use_ctxt, span, adt, variant_field);
if let Some(err) = self.check_field(use_ctxt, span, adt, variant_field) {
field_errors.push(err);
}
}
} else {
for field in fields {
let use_ctxt = field.ident.span;
let index = self.tcx.field_index(field.id, self.tables);
self.check_field(use_ctxt, field.span, adt, &variant.fields[index]);
if let Some(err) = self.check_field(
use_ctxt,
field.span,
adt,
&variant.fields[index],
) {
field_errors.push(err);
}
}
}
self.emit_field_checks(adt, expr.span, field_errors, "built");
}
hir::ExprKind::Call(ref path, ref fields) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment that this is about tuple struct constructors

if let hir::ExprKind::Path(qpath) = &path.node {
let def = self.tables.qpath_def(qpath, path.hir_id);
if let Some(_) = def.opt_def_id() {
if let Some(adt) = self.tables.expr_ty(expr).ty_adt_def() {
if let Some(variant) = adt.opt_variant_of_def(def) {
let mut field_errors = vec![];
for (idx, field) in variant.fields.iter().enumerate() {
let use_ctxt = fields.get(idx).map(|f| f.span)
.unwrap_or(path.span);
if let Some(err) = self.check_field(
use_ctxt,
use_ctxt,
adt,
&field,
) {
field_errors.push(err);
}
}
self.emit_field_checks(adt, path.span, field_errors, "built");
}
}
}
}
}
Expand All @@ -901,11 +988,39 @@ impl<'a, 'tcx> Visitor<'tcx> for NamePrivacyVisitor<'a, 'tcx> {
let def = self.tables.qpath_def(qpath, pat.hir_id);
let adt = self.tables.pat_ty(pat).ty_adt_def().unwrap();
let variant = adt.variant_of_def(def);
let mut field_errors = vec![];
for field in fields {
let use_ctxt = field.node.ident.span;
let index = self.tcx.field_index(field.node.id, self.tables);
self.check_field(use_ctxt, field.span, adt, &variant.fields[index]);
if let Some(err) = self.check_field(
use_ctxt,
field.span,
adt,
&variant.fields[index],
) {
field_errors.push(err);
}
}
self.emit_field_checks(adt, pat.span, field_errors, "destructured");
}
PatKind::TupleStruct(ref qpath, ref patterns, ..) => {
let def = self.tables.qpath_def(qpath, pat.hir_id);
let adt = self.tables.pat_ty(pat).ty_adt_def().unwrap();
let variant = adt.variant_of_def(def);
let mut field_errors = vec![];
for (vf_index, variant_field) in variant.fields.iter().enumerate() {
if let Some(pat) = patterns.get(vf_index) {
if let Some(err) = self.check_field(
pat.span,
pat.span,
adt,
variant_field,
) {
field_errors.push(err);
}
}
}
self.emit_field_checks(adt, pat.span, field_errors, "destructured");
}
_ => {}
}
Expand All @@ -927,11 +1042,13 @@ struct TypePrivacyVisitor<'a, 'tcx: 'a> {
in_body: bool,
span: Span,
empty_tables: &'a ty::TypeckTables<'tcx>,
reported_tuple_structs: FxHashSet<Span>,
Copy link
Contributor

Choose a reason for hiding this comment

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

explain this field here instead of at the use sites

}

impl<'a, 'tcx> TypePrivacyVisitor<'a, 'tcx> {
fn item_is_accessible(&self, did: DefId) -> bool {
def_id_visibility(self.tcx, did).0.is_accessible_from(self.current_item, self.tcx)
let (a, ..) = def_id_visibility(self.tcx, did);
Copy link
Contributor

Choose a reason for hiding this comment

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

use a better variable name than a

a.is_accessible_from(self.current_item, self.tcx)
}

// Take node-id of an expression or pattern and check its type for privacy.
Expand All @@ -951,11 +1068,33 @@ impl<'a, 'tcx> TypePrivacyVisitor<'a, 'tcx> {
}

fn check_def_id(&mut self, def_id: DefId, kind: &str, descr: &dyn fmt::Display) -> bool {
let is_error = !self.item_is_accessible(def_id);
if is_error {
self.tcx.sess.span_err(self.span, &format!("{} `{}` is private", kind, descr));
let is_ok = self.item_is_accessible(def_id);
if !is_ok {
match self.tcx.hir().as_local_node_id(def_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use if let instead of matches here

Some(node_id) => {
match self.tcx.hir().get(node_id) {
Node::StructCtor(hir::VariantData::Tuple(..)) => {
// Ignore tuple structs, as they are handled in `visit_path`
return false;
}
_ => {}
}
}
_ => {}
}
let msg = if let Some(def) = self.tcx.describe_def(def_id) {
format!("{} `{}` is private", def.kind_name(), self.tcx.item_path_str(def_id))
} else {
format!("{} `{}` is private", kind, descr)
};
if !self.reported_tuple_structs.iter().any(|sp| sp.overlaps(self.span)) {
self.tcx.sess
.struct_span_err(self.span, &msg)
.span_label(self.span, "private")
.emit();
}
}
is_error
!is_ok
}
}

Expand Down Expand Up @@ -1079,14 +1218,37 @@ impl<'a, 'tcx> Visitor<'tcx> for TypePrivacyVisitor<'a, 'tcx> {
hir::QPath::TypeRelative(_, ref segment) => segment.ident.to_string(),
};
let msg = format!("{} `{}` is private", def.kind_name(), name);
self.tcx.sess.span_err(span, &msg);
let label = format!("{} not accessible from here", def.kind_name());
self.tcx.sess.struct_span_err(span, &msg)
.span_label(span, label)
.emit();
return;
}
}

intravisit::walk_qpath(self, qpath, id, span);
}

// Prohibit access to tuple structs that are either unreachable *or* have private fields.
fn visit_path(&mut self, path: &'tcx hir::Path, _id: hir::HirId) {
// We handle tuple struct visibility here to only complain about bare paths referencing an
// unreachable tuple struct or one that has private fields.
if let Def::StructCtor(def_id, hir::def::CtorKind::Fn) = path.def {
if !self.item_is_accessible(def_id) &&
// only report if this is a bare path, not part of a tuple struct literal
!self.reported_tuple_structs.iter().any(|sp| sp.overlaps(path.span))
{
let kind_name = path.def.kind_name();
let sp = path.span;
let msg = format!("{} `{}` is private", kind_name, path);
let label = format!("{} not accesssible from here", kind_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let label = format!("{} not accesssible from here", kind_name);
let label = format!("private {} is not accesssible from here", kind_name);

self.tcx.sess.struct_span_err(sp, &msg)
.span_label(sp, label)
.emit();
}
}
}

// Check types of patterns.
fn visit_pat(&mut self, pattern: &'tcx hir::Pat) {
if self.check_expr_pat_type(pattern.hir_id, pattern.span) {
Expand Down Expand Up @@ -1770,6 +1932,7 @@ fn check_mod_privacy<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>, module_def_id: DefId) {
tables: &empty_tables,
current_item: DUMMY_NODE_ID,
empty_tables: &empty_tables,
reported_tuple_structs: FxHashSet::default(),
};
let (module, span, node_id) = tcx.hir().get_module(module_def_id);
intravisit::walk_mod(&mut visitor, module, node_id);
Expand All @@ -1783,6 +1946,7 @@ fn check_mod_privacy<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>, module_def_id: DefId) {
in_body: false,
span,
empty_tables: &empty_tables,
reported_tuple_structs: visitor.reported_tuple_structs,
};
intravisit::walk_mod(&mut visitor, module, node_id);
}
Expand Down
20 changes: 18 additions & 2 deletions src/librustc_resolve/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5046,8 +5046,24 @@ impl<'a> Resolver<'a> {
let mut reported_spans = FxHashSet::default();
for &PrivacyError(dedup_span, ident, binding) in &self.privacy_errors {
if reported_spans.insert(dedup_span) {
span_err!(self.session, ident.span, E0603, "{} `{}` is private",
binding.descr(), ident.name);
if let NameBindingKind::Def(
Def::StructCtor(_def_id, CtorKind::Fn), false,
) = binding.kind {
// For tuple structs we want to be clearer about the reason for the ctor being
// private, as we'd want to identify whether the visibility failure is due to a
// non-accessible field. Because of this, ignore them at the resolve time and
// defer to privacy checking step.
} else {
let mut err = struct_span_err!(
self.session,
ident.span,
E0603,
"{} `{}` is private",
binding.descr(),
ident.name,
);
err.emit();
}
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/librustc_resolve/resolve_imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,11 @@ impl<'a> Resolver<'a> {
}
}

if !self.is_accessible(binding.vis) &&
let is_accessible = self.is_accessible(binding.vis);
if !is_accessible &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems unnecessary

// Remove this together with `PUB_USE_OF_PRIVATE_EXTERN_CRATE`
!(self.last_import_segment && binding.is_extern_crate()) {
!(self.last_import_segment && binding.is_extern_crate())
{
self.privacy_errors.push(PrivacyError(path_span, ident, binding));
}

Expand Down
Loading