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

Make privacy checking aware that a use directive can point to two defintions (namespaces) with different privacy. #12245

Merged
merged 1 commit into from
Feb 19, 2014
Merged

Make privacy checking aware that a use directive can point to two defintions (namespaces) with different privacy. #12245

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
156 changes: 118 additions & 38 deletions src/librustc/middle/privacy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ pub type ExportedItems = HashSet<ast::NodeId>;
/// reexporting a public struct doesn't inline the doc).
pub type PublicItems = HashSet<ast::NodeId>;

/// Result of a checking operation - None => no errors were found. Some => an
/// error and contains the span and message for reporting that error and
/// optionally the same for a note about the error.
type CheckResult = Option<(Span, ~str, Option<(Span, ~str)>)>;

////////////////////////////////////////////////////////////////////////////////
/// The parent visitor, used to determine what's the parent of what (node-wise)
////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -510,40 +515,50 @@ impl<'a> PrivacyVisitor<'a> {
}
}

/// Guarantee that a particular definition is public, possibly emitting an
/// error message if it's not.
fn report_error(&self, result: CheckResult) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this report method and I think it could be used in other places where the same form of reporting is necessary. What do you think about making this a default method in ast::visit::Visitor (or moving it somewhere else)? If you prefer to respect YAGNI, I can do it myself later, I've already a case where I'd benefit from this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, maybe this could go in the session itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about maybe pulling out CheckResult and report_error into somewhere more generic - I was thinking of some utils file somewhere. The session is probably a good place (I prefer Visitor since it is not really related to visiting). I decided not to for now since I didn't know if it was generally useful. If you think it would be, feel free to pull it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be useful and I could use it in the patch I'm currently working on. Also, the pattern Error + Note is used in other places too.

I can pull it out later. This is not a blocker for this patch at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another case where this could be helpful #12347

match result {
None => true,
Some((span, msg, note)) => {
self.tcx.sess.span_err(span, msg);
match note {
Some((span, msg)) => self.tcx.sess.span_note(span, msg),
None => {},
}
false
},
}
}

/// Guarantee that a particular definition is public. Returns a CheckResult
/// which contains any errors found. These can be reported using `report_error`.
/// If the result is `None`, no errors were found.
fn ensure_public(&self, span: Span, to_check: ast::DefId,
source_did: Option<ast::DefId>, msg: &str) -> bool {
source_did: Option<ast::DefId>, msg: &str) -> CheckResult {
match self.def_privacy(to_check) {
ExternallyDenied => {
self.tcx.sess.span_err(span, format!("{} is private", msg))
}
ExternallyDenied => Some((span, format!("{} is private", msg), None)),
DisallowedBy(id) => {
if id == source_did.unwrap_or(to_check).node {
self.tcx.sess.span_err(span, format!("{} is private", msg));
return false;
let (err_span, err_msg) = if id == source_did.unwrap_or(to_check).node {
return Some((span, format!("{} is private", msg), None));
} else {
self.tcx.sess.span_err(span, format!("{} is inaccessible",
msg));
}
(span, format!("{} is inaccessible", msg))
};
match self.tcx.map.find(id) {
Some(ast_map::NodeItem(item)) => {
let desc = match item.node {
ast::ItemMod(..) => "module",
ast::ItemTrait(..) => "trait",
_ => return false,
_ => return Some((err_span, err_msg, None)),
};
let msg = format!("{} `{}` is private",
desc,
token::get_ident(item.ident));
self.tcx.sess.span_note(span, msg);
}
Some(..) | None => {}
Some((err_span, err_msg, Some((span, msg))))
},
_ => Some((err_span, err_msg, None)),
}
}
Allowable => return true
},
Allowable => None,
}
return false;
}

// Checks that a field is in scope.
Expand Down Expand Up @@ -613,34 +628,99 @@ impl<'a> PrivacyVisitor<'a> {
let method_id = ty::method(self.tcx, method_id).provided_source
.unwrap_or(method_id);

self.ensure_public(span,
method_id,
None,
format!("method `{}`", token::get_ident(name)));
let string = token::get_ident(name);
self.report_error(self.ensure_public(span,
method_id,
None,
format!("method `{}`", string)));
}

// Checks that a path is in scope.
fn check_path(&mut self, span: Span, path_id: ast::NodeId, path: &ast::Path) {
debug!("privacy - path {}", self.nodestr(path_id));
let def_map = self.tcx.def_map.borrow();
let def = def_map.get().get_copy(&path_id);
let orig_def = def_map.get().get_copy(&path_id);
let ck = |tyname: &str| {
let origdid = def_id_of_def(def);
let ck_public = |def: ast::DefId| {
let name = token::get_ident(path.segments
.last()
.unwrap()
.identifier);
let origdid = def_id_of_def(orig_def);
self.ensure_public(span,
def,
Some(origdid),
format!("{} `{}`",
tyname,
name))
};

match *self.last_private_map.get(&path_id) {
resolve::AllPublic => {},
resolve::DependsOn(def) => {
let name = token::get_ident(path.segments
.last()
.unwrap()
.identifier);
self.ensure_public(span,
def,
Some(origdid),
format!("{} `{}`",
tyname, name));
}
resolve::LastMod(resolve::AllPublic) => {},
resolve::LastMod(resolve::DependsOn(def)) => {
self.report_error(ck_public(def));
},
resolve::LastImport{value_priv: value_priv,
value_used: check_value,
type_priv: type_priv,
type_used: check_type} => {
// This dance with found_error is because we don't want to report
// a privacy error twice for the same directive.
let found_error = match (type_priv, check_type) {
(Some(resolve::DependsOn(def)), resolve::Used) => {
!self.report_error(ck_public(def))
},
_ => false,
};
if !found_error {
match (value_priv, check_value) {
(Some(resolve::DependsOn(def)), resolve::Used) => {
self.report_error(ck_public(def));
},
_ => {},
}
}
// If an import is not used in either namespace, we still want to check
// that it could be legal. Therefore we check in both namespaces and only
// report an error if both would be illegal. We only report one error,
// even if it is illegal to import from both namespaces.
match (value_priv, check_value, type_priv, check_type) {
(Some(p), resolve::Unused, None, _) |
(None, _, Some(p), resolve::Unused) => {
let p = match p {
resolve::AllPublic => None,
resolve::DependsOn(def) => ck_public(def),
};
if p.is_some() {
self.report_error(p);
}
},
(Some(v), resolve::Unused, Some(t), resolve::Unused) => {
let v = match v {
resolve::AllPublic => None,
resolve::DependsOn(def) => ck_public(def),
};
let t = match t {
resolve::AllPublic => None,
resolve::DependsOn(def) => ck_public(def),
};
match (v, t) {
(Some(_), Some(t)) => {
self.report_error(Some(t));
},
_ => {},
}
},
_ => {},
}
},
}
};
// FIXME(#12334) Imports can refer to definitions in both the type and
// value namespaces. The privacy information is aware of this, but the
// def map is not. Therefore the names we work out below will not always
// be accurate and we can get slightly wonky error messages (but type
// checking is always correct).
let def_map = self.tcx.def_map.borrow();
match def_map.get().get_copy(&path_id) {
ast::DefStaticMethod(..) => ck("static method"),
Expand Down Expand Up @@ -668,7 +748,7 @@ impl<'a> PrivacyVisitor<'a> {
// is whether the trait itself is accessible or not.
method_param(method_param { trait_id: trait_id, .. }) |
method_object(method_object { trait_id: trait_id, .. }) => {
self.ensure_public(span, trait_id, None, "source trait");
self.report_error(self.ensure_public(span, trait_id, None, "source trait"));
}
}
}
Expand Down
Loading