Skip to content

Let the dep-tracking test framework check that all #[rustc_dirty] attrs have been actually checked #39500

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

Merged
merged 2 commits into from
Feb 6, 2017
Merged

Let the dep-tracking test framework check that all #[rustc_dirty] attrs have been actually checked #39500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14191ee
Let the ICH testing framework check that all #[rustc_dirty] attrs hav…
michaelwoerister Feb 3, 2017
ab3c826
Add comment about why the regular unused-attributes infrastructure
michaelwoerister Feb 6, 2017
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 src/librustc/hir/intravisit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,7 @@ pub fn walk_decl<'v, V: Visitor<'v>>(visitor: &mut V, declaration: &'v Decl) {

pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr) {
visitor.visit_id(expression.id);
walk_list!(visitor, visit_attribute, expression.attrs.iter());
Copy link
Contributor

Choose a reason for hiding this comment

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

d'oh

match expression.node {
ExprBox(ref subexpression) => {
visitor.visit_expr(subexpression)
Expand Down
153 changes: 122 additions & 31 deletions src/librustc_incremental/persist/dirty_clean.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ use rustc::dep_graph::{DepGraphQuery, DepNode};
use rustc::hir;
use rustc::hir::def_id::DefId;
use rustc::hir::itemlikevisit::ItemLikeVisitor;
use rustc::hir::intravisit;
use syntax::ast::{self, Attribute, NestedMetaItem};
use rustc_data_structures::fx::{FxHashSet, FxHashMap};
use syntax_pos::Span;
Expand Down Expand Up @@ -73,17 +74,32 @@ pub fn check_dirty_clean_annotations<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
let query = tcx.dep_graph.query();
debug!("query-nodes: {:?}", query.nodes());
let krate = tcx.hir.krate();
krate.visit_all_item_likes(&mut DirtyCleanVisitor {
let mut dirty_clean_visitor = DirtyCleanVisitor {
tcx: tcx,
query: &query,
dirty_inputs: dirty_inputs,
});
checked_attrs: FxHashSet(),
};
krate.visit_all_item_likes(&mut dirty_clean_visitor);

let mut all_attrs = FindAllAttrs {
tcx: tcx,
attr_names: vec![ATTR_DIRTY, ATTR_CLEAN],
found_attrs: vec![],
};
intravisit::walk_crate(&mut all_attrs, krate);

// Note that we cannot use the existing "unused attribute"-infrastructure
// here, since that is running before trans. This is also the reason why
// all trans-specific attributes are `Whitelisted` in syntax::feature_gate.
all_attrs.report_unchecked_attrs(&dirty_clean_visitor.checked_attrs);
}

pub struct DirtyCleanVisitor<'a, 'tcx:'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
query: &'a DepGraphQuery<DefId>,
dirty_inputs: FxHashSet<DepNode<DefId>>,
checked_attrs: FxHashSet<ast::AttrId>,
}

impl<'a, 'tcx> DirtyCleanVisitor<'a, 'tcx> {
Expand All @@ -109,7 +125,7 @@ impl<'a, 'tcx> DirtyCleanVisitor<'a, 'tcx> {
dep_node.map_def(|&def_id| Some(self.tcx.item_path_str(def_id))).unwrap()
}

fn assert_dirty(&self, item: &hir::Item, dep_node: DepNode<DefId>) {
fn assert_dirty(&self, item_span: Span, dep_node: DepNode<DefId>) {
debug!("assert_dirty({:?})", dep_node);

match dep_node {
Expand All @@ -121,7 +137,7 @@ impl<'a, 'tcx> DirtyCleanVisitor<'a, 'tcx> {
if !self.dirty_inputs.contains(&dep_node) {
let dep_node_str = self.dep_node_str(&dep_node);
self.tcx.sess.span_err(
item.span,
item_span,
&format!("`{:?}` not found in dirty set, but should be dirty",
dep_node_str));
}
Expand All @@ -132,14 +148,14 @@ impl<'a, 'tcx> DirtyCleanVisitor<'a, 'tcx> {
if self.query.contains_node(&dep_node) {
let dep_node_str = self.dep_node_str(&dep_node);
self.tcx.sess.span_err(
item.span,
item_span,
&format!("`{:?}` found in dep graph, but should be dirty", dep_node_str));
}
}
}
}

fn assert_clean(&self, item: &hir::Item, dep_node: DepNode<DefId>) {
fn assert_clean(&self, item_span: Span, dep_node: DepNode<DefId>) {
debug!("assert_clean({:?})", dep_node);

match dep_node {
Expand All @@ -150,7 +166,7 @@ impl<'a, 'tcx> DirtyCleanVisitor<'a, 'tcx> {
if self.dirty_inputs.contains(&dep_node) {
let dep_node_str = self.dep_node_str(&dep_node);
self.tcx.sess.span_err(
item.span,
item_span,
&format!("`{:?}` found in dirty-node set, but should be clean",
dep_node_str));
}
Expand All @@ -160,35 +176,43 @@ impl<'a, 'tcx> DirtyCleanVisitor<'a, 'tcx> {
if !self.query.contains_node(&dep_node) {
let dep_node_str = self.dep_node_str(&dep_node);
self.tcx.sess.span_err(
item.span,
item_span,
&format!("`{:?}` not found in dep graph, but should be clean",
dep_node_str));
}
}
}
}
}

impl<'a, 'tcx> ItemLikeVisitor<'tcx> for DirtyCleanVisitor<'a, 'tcx> {
fn visit_item(&mut self, item: &'tcx hir::Item) {
let def_id = self.tcx.hir.local_def_id(item.id);
fn check_item(&mut self, item_id: ast::NodeId, item_span: Span) {
let def_id = self.tcx.hir.local_def_id(item_id);
for attr in self.tcx.get_attrs(def_id).iter() {
if attr.check_name(ATTR_DIRTY) {
if check_config(self.tcx, attr) {
self.assert_dirty(item, self.dep_node(attr, def_id));
self.checked_attrs.insert(attr.id);
self.assert_dirty(item_span, self.dep_node(attr, def_id));
}
} else if attr.check_name(ATTR_CLEAN) {
if check_config(self.tcx, attr) {
self.assert_clean(item, self.dep_node(attr, def_id));
self.checked_attrs.insert(attr.id);
self.assert_clean(item_span, self.dep_node(attr, def_id));
}
}
}
}
}

impl<'a, 'tcx> ItemLikeVisitor<'tcx> for DirtyCleanVisitor<'a, 'tcx> {
fn visit_item(&mut self, item: &'tcx hir::Item) {
self.check_item(item.id, item.span);
}

fn visit_trait_item(&mut self, _trait_item: &hir::TraitItem) {
fn visit_trait_item(&mut self, item: &hir::TraitItem) {
self.check_item(item.id, item.span);
}

fn visit_impl_item(&mut self, _impl_item: &hir::ImplItem) {
fn visit_impl_item(&mut self, item: &hir::ImplItem) {
self.check_item(item.id, item.span);
}
}

Expand All @@ -201,46 +225,69 @@ pub fn check_dirty_clean_metadata<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

tcx.dep_graph.with_ignore(||{
let krate = tcx.hir.krate();
krate.visit_all_item_likes(&mut DirtyCleanMetadataVisitor {
let mut dirty_clean_visitor = DirtyCleanMetadataVisitor {
tcx: tcx,
prev_metadata_hashes: prev_metadata_hashes,
current_metadata_hashes: current_metadata_hashes,
});
checked_attrs: FxHashSet(),
};
krate.visit_all_item_likes(&mut dirty_clean_visitor);

let mut all_attrs = FindAllAttrs {
tcx: tcx,
attr_names: vec![ATTR_DIRTY_METADATA, ATTR_CLEAN_METADATA],
found_attrs: vec![],
};
intravisit::walk_crate(&mut all_attrs, krate);

// Note that we cannot use the existing "unused attribute"-infrastructure
// here, since that is running before trans. This is also the reason why
// all trans-specific attributes are `Whitelisted` in syntax::feature_gate.
all_attrs.report_unchecked_attrs(&dirty_clean_visitor.checked_attrs);
});
}

pub struct DirtyCleanMetadataVisitor<'a, 'tcx:'a, 'm> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
prev_metadata_hashes: &'m FxHashMap<DefId, Fingerprint>,
current_metadata_hashes: &'m FxHashMap<DefId, Fingerprint>,
checked_attrs: FxHashSet<ast::AttrId>,
}

impl<'a, 'tcx, 'm> ItemLikeVisitor<'tcx> for DirtyCleanMetadataVisitor<'a, 'tcx, 'm> {
fn visit_item(&mut self, item: &'tcx hir::Item) {
let def_id = self.tcx.hir.local_def_id(item.id);
self.check_item(item.id, item.span);
}

fn visit_trait_item(&mut self, item: &hir::TraitItem) {
self.check_item(item.id, item.span);
}

fn visit_impl_item(&mut self, item: &hir::ImplItem) {
self.check_item(item.id, item.span);
}
}

impl<'a, 'tcx, 'm> DirtyCleanMetadataVisitor<'a, 'tcx, 'm> {

fn check_item(&mut self, item_id: ast::NodeId, item_span: Span) {
let def_id = self.tcx.hir.local_def_id(item_id);

for attr in self.tcx.get_attrs(def_id).iter() {
if attr.check_name(ATTR_DIRTY_METADATA) {
if check_config(self.tcx, attr) {
self.assert_state(false, def_id, item.span);
self.checked_attrs.insert(attr.id);
self.assert_state(false, def_id, item_span);
}
} else if attr.check_name(ATTR_CLEAN_METADATA) {
if check_config(self.tcx, attr) {
self.assert_state(true, def_id, item.span);
self.checked_attrs.insert(attr.id);
self.assert_state(true, def_id, item_span);
}
}
}
}

fn visit_trait_item(&mut self, _trait_item: &hir::TraitItem) {
}

fn visit_impl_item(&mut self, _impl_item: &hir::ImplItem) {
}
}

impl<'a, 'tcx, 'm> DirtyCleanMetadataVisitor<'a, 'tcx, 'm> {

fn assert_state(&self, should_be_clean: bool, def_id: DefId, span: Span) {
let item_path = self.tcx.item_path_str(def_id);
debug!("assert_state({})", item_path);
Expand Down Expand Up @@ -274,7 +321,7 @@ impl<'a, 'tcx, 'm> DirtyCleanMetadataVisitor<'a, 'tcx, 'm> {
/// Given a `#[rustc_dirty]` or `#[rustc_clean]` attribute, scan
/// for a `cfg="foo"` attribute and check whether we have a cfg
/// flag called `foo`.
fn check_config(tcx: TyCtxt, attr: &ast::Attribute) -> bool {
fn check_config(tcx: TyCtxt, attr: &Attribute) -> bool {
debug!("check_config(attr={:?})", attr);
let config = &tcx.sess.parse_sess.config;
debug!("check_config: config={:?}", config);
Expand Down Expand Up @@ -304,3 +351,47 @@ fn expect_associated_value(tcx: TyCtxt, item: &NestedMetaItem) -> ast::Name {
tcx.sess.span_fatal(item.span, &msg);
}
}


// A visitor that collects all #[rustc_dirty]/#[rustc_clean] attributes from
// the HIR. It is used to verfiy that we really ran checks for all annotated
// nodes.
pub struct FindAllAttrs<'a, 'tcx:'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
attr_names: Vec<&'static str>,
found_attrs: Vec<&'tcx Attribute>,
}

impl<'a, 'tcx> FindAllAttrs<'a, 'tcx> {

fn is_active_attr(&mut self, attr: &Attribute) -> bool {
for attr_name in &self.attr_names {
if attr.check_name(attr_name) && check_config(self.tcx, attr) {
return true;
}
}

false
}

fn report_unchecked_attrs(&self, checked_attrs: &FxHashSet<ast::AttrId>) {
for attr in &self.found_attrs {
if !checked_attrs.contains(&attr.id) {
self.tcx.sess.span_err(attr.span, &format!("found unchecked \
#[rustc_dirty]/#[rustc_clean] attribute"));
}
}
}
}

impl<'a, 'tcx> intravisit::Visitor<'tcx> for FindAllAttrs<'a, 'tcx> {
fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
intravisit::NestedVisitorMap::All(&self.tcx.hir)
}

fn visit_attribute(&mut self, attr: &'tcx Attribute) {
if self.is_active_attr(attr) {
self.found_attrs.push(attr);
}
}
}
Loading