Auto merge of #37660 - nikomatsakis:incremental-36349, r=eddyb

Separate impl items from the parent impl

This change separates impl item bodies out of the impl itself. This gives incremental more resolution. In so doing, it refactors how the visitors work, and cleans up a bit of the collect/check logic (mostly by moving things out of collect that didn't really belong there, because they were just checking conditions).

However, this is not as effective as I expected, for a kind of frustrating reason. In particular, when invoking `foo.bar()` you still wind up with dependencies on private items. The problem is that the method resolution code scans that list for methods with the name `bar` -- and this winds up touching *all* the methods, even private ones.

I can imagine two obvious ways to fix this:

- separating fn bodies from fn sigs (#35078, currently being pursued by @flodiebold)
- a more aggressive model of incremental that @michaelwoerister has been advocating, in which we hash the intermediate results (e.g., the outputs of collect) so that we can see that the intermediate result hasn't changed, even if a particular impl item has changed.

So all in all I'm not quite sure whether to land this or not. =) It still seems like it has to be a win in some cases, but not with the test cases we have just now. I can try to gin up some test cases, but I'm not sure if they will be totally realistic. On the other hand, some of the early refactorings to the visitor trait seem worthwhile to me regardless.

cc #36349 -- well, this is basically a fix for that issue, I guess

r? @michaelwoerister

NB: Based atop of @eddyb's PR #37402; don't land until that lands.

src/librustc/dep_graph/mod.rs

@@ -25,5 +25,5 @@ pub use self::dep_node::WorkProductId;
 pub use self::graph::DepGraph;
 pub use self::graph::WorkProduct;
 pub use self::query::DepGraphQuery;
-pub use self::visit::visit_all_items_in_krate;
+pub use self::visit::visit_all_item_likes_in_krate;
 pub use self::raii::DepTask;

src/librustc/dep_graph/visit.rs

@@ -10,31 +10,30 @@
 
 use hir;
 use hir::def_id::DefId;
-use hir::intravisit::Visitor;
+use hir::itemlikevisit::ItemLikeVisitor;
 use ty::TyCtxt;
 
 use super::dep_node::DepNode;
 
-
 /// Visit all the items in the krate in some order. When visiting a
 /// particular item, first create a dep-node by calling `dep_node_fn`
 /// and push that onto the dep-graph stack of tasks, and also create a
 /// read edge from the corresponding AST node. This is used in
 /// compiler passes to automatically record the item that they are
 /// working on.
-pub fn visit_all_items_in_krate<'a, 'tcx, V, F>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                                                mut dep_node_fn: F,
-                                                visitor: &mut V)
-    where F: FnMut(DefId) -> DepNode<DefId>, V: Visitor<'tcx>
+pub fn visit_all_item_likes_in_krate<'a, 'tcx, V, F>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
+                                                     mut dep_node_fn: F,
+                                                     visitor: &mut V)
+    where F: FnMut(DefId) -> DepNode<DefId>, V: ItemLikeVisitor<'tcx>
 {
     struct TrackingVisitor<'visit, 'tcx: 'visit, F: 'visit, V: 'visit> {
         tcx: TyCtxt<'visit, 'tcx, 'tcx>,
         dep_node_fn: &'visit mut F,
         visitor: &'visit mut V
     }
 
-    impl<'visit, 'tcx, F, V> Visitor<'tcx> for TrackingVisitor<'visit, 'tcx, F, V>
-        where F: FnMut(DefId) -> DepNode<DefId>, V: Visitor<'tcx>
+    impl<'visit, 'tcx, F, V> ItemLikeVisitor<'tcx> for TrackingVisitor<'visit, 'tcx, F, V>
+        where F: FnMut(DefId) -> DepNode<DefId>, V: ItemLikeVisitor<'tcx>
     {
         fn visit_item(&mut self, i: &'tcx hir::Item) {
             let item_def_id = self.tcx.map.local_def_id(i.id);
@@ -46,6 +45,17 @@ pub fn visit_all_items_in_krate<'a, 'tcx, V, F>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
             self.visitor.visit_item(i);
             debug!("Ended task {:?}", task_id);
         }
+
+        fn visit_impl_item(&mut self, i: &'tcx hir::ImplItem) {
+            let impl_item_def_id = self.tcx.map.local_def_id(i.id);
+            let task_id = (self.dep_node_fn)(impl_item_def_id);
+            let _task = self.tcx.dep_graph.in_task(task_id.clone());
+            debug!("Started task {:?}", task_id);
+            assert!(!self.tcx.map.is_inlined_def_id(impl_item_def_id));
+            self.tcx.dep_graph.read(DepNode::Hir(impl_item_def_id));
+            self.visitor.visit_impl_item(i);
+            debug!("Ended task {:?}", task_id);
+        }
     }
 
     let krate = tcx.dep_graph.with_ignore(|| tcx.map.krate());
@@ -54,5 +64,5 @@ pub fn visit_all_items_in_krate<'a, 'tcx, V, F>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
         dep_node_fn: &mut dep_node_fn,
         visitor: visitor
     };
-    krate.visit_all_items(&mut tracking_visitor)
+    krate.visit_all_item_likes(&mut tracking_visitor)
 }

src/librustc/hir/def.rs

@@ -106,7 +106,7 @@ pub type DefMap = NodeMap<PathResolution>;
 // within.
 pub type ExportMap = NodeMap<Vec<Export>>;
 
-#[derive(Copy, Clone, RustcEncodable, RustcDecodable)]
+#[derive(Copy, Clone, Debug, RustcEncodable, RustcDecodable)]
 pub struct Export {
     pub name: ast::Name, // The name of the target.
     pub def: Def, // The definition of the target.

src/librustc/hir/intravisit.rs

@@ -8,7 +8,15 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-//! HIR walker. Each overridden visit method has full control over what
+//! HIR walker for walking the contents of nodes.
+//!
+//! **For an overview of the visitor strategy, see the docs on the
+//! `super::itemlikevisit::ItemLikeVisitor` trait.**
+//!
+//! If you have decided to use this visitor, here are some general
+//! notes on how to do it:
+//!
+//! Each overridden visit method has full control over what
 //! happens with its node, it can do its own traversal of the node's children,
 //! call `intravisit::walk_*` to apply the default traversal algorithm, or prevent
 //! deeper traversal by doing nothing.
@@ -30,6 +38,8 @@ use syntax::ast::{NodeId, CRATE_NODE_ID, Name, Attribute};
 use syntax::codemap::Spanned;
 use syntax_pos::Span;
 use hir::*;
+use hir::map::Map;
+use super::itemlikevisit::DeepVisitor;
 
 use std::cmp;
 use std::u32;
@@ -76,22 +86,70 @@ pub trait Visitor<'v> : Sized {
     ///////////////////////////////////////////////////////////////////////////
     // Nested items.
 
-    /// Invoked when a nested item is encountered. By default, does
-    /// nothing. If you want a deep walk, you need to override to
-    /// fetch the item contents. But most of the time, it is easier
-    /// (and better) to invoke `Crate::visit_all_items`, which visits
-    /// all items in the crate in some order (but doesn't respect
-    /// nesting).
+    /// The default versions of the `visit_nested_XXX` routines invoke
+    /// this method to get a map to use; if they get back `None`, they
+    /// just skip nested things. Otherwise, they will lookup the
+    /// nested item-like things in the map and visit it. So the best
+    /// way to implement a nested visitor is to override this method
+    /// to return a `Map`; one advantage of this is that if we add
+    /// more types of nested things in the future, they will
+    /// automatically work.
+    ///
+    /// **If for some reason you want the nested behavior, but don't
+    /// have a `Map` are your disposal:** then you should override the
+    /// `visit_nested_XXX` methods, and override this method to
+    /// `panic!()`. This way, if a new `visit_nested_XXX` variant is
+    /// added in the future, we will see the panic in your code and
+    /// fix it appropriately.
+    fn nested_visit_map(&mut self) -> Option<&Map<'v>> {
+        None
+    }
+
+    /// Invoked when a nested item is encountered. By default does
+    /// nothing unless you override `nested_visit_map` to return
+    /// `Some(_)`, in which case it will walk the item. **You probably
+    /// don't want to override this method** -- instead, override
+    /// `nested_visit_map` or use the "shallow" or "deep" visit
+    /// patterns described on `itemlikevisit::ItemLikeVisitor`. The only
+    /// reason to override this method is if you want a nested pattern
+    /// but cannot supply a `Map`; see `nested_visit_map` for advice.
     #[allow(unused_variables)]
     fn visit_nested_item(&mut self, id: ItemId) {
+        let opt_item = self.nested_visit_map()
+                           .map(|map| map.expect_item(id.id));
+        if let Some(item) = opt_item {
+            self.visit_item(item);
+        }
     }
 
-    /// Visit the top-level item and (optionally) nested items. See
+    /// Like `visit_nested_item()`, but for impl items. See
+    /// `visit_nested_item()` for advice on when to override this
+    /// method.
+    #[allow(unused_variables)]
+    fn visit_nested_impl_item(&mut self, id: ImplItemId) {
+        let opt_item = self.nested_visit_map()
+                           .map(|map| map.impl_item(id));
+        if let Some(item) = opt_item {
+            self.visit_impl_item(item);
+        }
+    }
+
+    /// Visit the top-level item and (optionally) nested items / impl items. See
     /// `visit_nested_item` for details.
     fn visit_item(&mut self, i: &'v Item) {
         walk_item(self, i)
     }
 
+    /// When invoking `visit_all_item_likes()`, you need to supply an
+    /// item-like visitor.  This method converts a "intra-visit"
+    /// visitor into an item-like visitor that walks the entire tree.
+    /// If you use this, you probably don't want to process the
+    /// contents of nested item-like things, since the outer loop will
+    /// visit them as well.
+    fn as_deep_visitor<'s>(&'s mut self) -> DeepVisitor<'s, Self> {
+        DeepVisitor::new(self)
+    }
+
     ///////////////////////////////////////////////////////////////////////////
 
     fn visit_id(&mut self, _node_id: NodeId) {
@@ -147,6 +205,9 @@ pub trait Visitor<'v> : Sized {
     fn visit_impl_item(&mut self, ii: &'v ImplItem) {
         walk_impl_item(self, ii)
     }
+    fn visit_impl_item_ref(&mut self, ii: &'v ImplItemRef) {
+        walk_impl_item_ref(self, ii)
+    }
     fn visit_trait_ref(&mut self, t: &'v TraitRef) {
         walk_trait_ref(self, t)
     }
@@ -206,6 +267,12 @@ pub trait Visitor<'v> : Sized {
     fn visit_vis(&mut self, vis: &'v Visibility) {
         walk_vis(self, vis)
     }
+    fn visit_associated_item_kind(&mut self, kind: &'v AssociatedItemKind) {
+        walk_associated_item_kind(self, kind);
+    }
+    fn visit_defaultness(&mut self, defaultness: &'v Defaultness) {
+        walk_defaultness(self, defaultness);
+    }
 }
 
 pub fn walk_opt_name<'v, V: Visitor<'v>>(visitor: &mut V, span: Span, opt_name: Option<Name>) {
@@ -341,12 +408,14 @@ pub fn walk_item<'v, V: Visitor<'v>>(visitor: &mut V, item: &'v Item) {
             visitor.visit_id(item.id);
             visitor.visit_trait_ref(trait_ref)
         }
-        ItemImpl(.., ref type_parameters, ref opt_trait_reference, ref typ, ref impl_items) => {
+        ItemImpl(.., ref type_parameters, ref opt_trait_reference, ref typ, ref impl_item_refs) => {
             visitor.visit_id(item.id);
             visitor.visit_generics(type_parameters);
             walk_list!(visitor, visit_trait_ref, opt_trait_reference);
             visitor.visit_ty(typ);
-            walk_list!(visitor, visit_impl_item, impl_items);
+            for impl_item_ref in impl_item_refs {
+                visitor.visit_impl_item_ref(impl_item_ref);
+            }
         }
         ItemStruct(ref struct_definition, ref generics) |
         ItemUnion(ref struct_definition, ref generics) => {
@@ -677,10 +746,14 @@ pub fn walk_trait_item<'v, V: Visitor<'v>>(visitor: &mut V, trait_item: &'v Trai
 }
 
 pub fn walk_impl_item<'v, V: Visitor<'v>>(visitor: &mut V, impl_item: &'v ImplItem) {
-    visitor.visit_vis(&impl_item.vis);
-    visitor.visit_name(impl_item.span, impl_item.name);
-    walk_list!(visitor, visit_attribute, &impl_item.attrs);
-    match impl_item.node {
+    // NB: Deliberately force a compilation error if/when new fields are added.
+    let ImplItem { id: _, name, ref vis, ref defaultness, ref attrs, ref node, span } = *impl_item;
+
+    visitor.visit_name(span, name);
+    visitor.visit_vis(vis);
+    visitor.visit_defaultness(defaultness);
+    walk_list!(visitor, visit_attribute, attrs);
+    match *node {
         ImplItemKind::Const(ref ty, ref expr) => {
             visitor.visit_id(impl_item.id);
             visitor.visit_ty(ty);
@@ -703,6 +776,17 @@ pub fn walk_impl_item<'v, V: Visitor<'v>>(visitor: &mut V, impl_item: &'v ImplIt
     }
 }
 
+pub fn walk_impl_item_ref<'v, V: Visitor<'v>>(visitor: &mut V, impl_item_ref: &'v ImplItemRef) {
+    // NB: Deliberately force a compilation error if/when new fields are added.
+    let ImplItemRef { id, name, ref kind, span, ref vis, ref defaultness } = *impl_item_ref;
+    visitor.visit_nested_impl_item(id);
+    visitor.visit_name(span, name);
+    visitor.visit_associated_item_kind(kind);
+    visitor.visit_vis(vis);
+    visitor.visit_defaultness(defaultness);
+}
+
+
 pub fn walk_struct_def<'v, V: Visitor<'v>>(visitor: &mut V, struct_definition: &'v VariantData) {
     visitor.visit_id(struct_definition.id());
     walk_list!(visitor, visit_struct_field, struct_definition.fields());
@@ -872,6 +956,18 @@ pub fn walk_vis<'v, V: Visitor<'v>>(visitor: &mut V, vis: &'v Visibility) {
     }
 }
 
+pub fn walk_associated_item_kind<'v, V: Visitor<'v>>(_: &mut V, _: &'v AssociatedItemKind) {
+    // No visitable content here: this fn exists so you can call it if
+    // the right thing to do, should content be added in the future,
+    // would be to walk it.
+}
+
+pub fn walk_defaultness<'v, V: Visitor<'v>>(_: &mut V, _: &'v Defaultness) {
+    // No visitable content here: this fn exists so you can call it if
+    // the right thing to do, should content be added in the future,
+    // would be to walk it.
+}
+
 #[derive(Copy, Clone, RustcEncodable, RustcDecodable, Debug, PartialEq, Eq)]
 pub struct IdRange {
     pub min: NodeId,

src/librustc/hir/itemlikevisit.rs

@@ -0,0 +1,84 @@
+// Copyright 2012-2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use super::{Item, ImplItem};
+use super::intravisit::Visitor;
+
+/// The "item-like visitor" visitor defines only the top-level methods
+/// that can be invoked by `Crate::visit_all_item_likes()`. Whether
+/// this trait is the right one to implement will depend on the
+/// overall pattern you need. Here are the three available patterns,
+/// in roughly the order of desirability:
+///
+/// 1. **Shallow visit**: Get a simple callback for every item (or item-like thing) in the HIR.
+///    - Example: find all items with a `#[foo]` attribute on them.
+///    - How: Implement `ItemLikeVisitor` and call `tcx.visit_all_item_likes_in_krate()`.
+///    - Pro: Efficient; just walks the lists of item-like things, not the nodes themselves.
+///    - Pro: Integrates well into dependency tracking.
+///    - Con: Don't get information about nesting
+///    - Con: Don't have methods for specific bits of HIR, like "on
+///      every expr, do this".
+/// 2. **Deep visit**: Want to scan for specific kinds of HIR nodes within
+///    an item, but don't care about how item-like things are nested
+///    within one another.
+///    - Example: Examine each expression to look for its type and do some check or other.
+///    - How: Implement `intravisit::Visitor` and use
+///      `tcx.visit_all_item_likes_in_krate(visitor.as_deep_visitor())`. Within
+///      your `intravisit::Visitor` impl, implement methods like
+///      `visit_expr()`; don't forget to invoke
+///      `intravisit::walk_visit_expr()` to keep walking the subparts.
+///    - Pro: Visitor methods for any kind of HIR node, not just item-like things.
+///    - Pro: Integrates well into dependency tracking.
+///    - Con: Don't get information about nesting between items
+/// 3. **Nested visit**: Want to visit the whole HIR and you care about the nesting between
+///    item-like things.
+///    - Example: Lifetime resolution, which wants to bring lifetimes declared on the
+///      impl into scope while visiting the impl-items, and then back out again.
+///    - How: Implement `intravisit::Visitor` and override the `visit_nested_foo()` foo methods
+///      as needed. Walk your crate with `intravisit::walk_crate()` invoked on `tcx.map.krate()`.
+///    - Pro: Visitor methods for any kind of HIR node, not just item-like things.
+///    - Pro: Preserves nesting information
+///    - Con: Does not integrate well into dependency tracking.
+///
+/// Note: the methods of `ItemLikeVisitor` intentionally have no
+/// defaults, so that as we expand the list of item-like things, we
+/// revisit the various visitors to see if they need to change. This
+/// is harder to do with `intravisit::Visitor`, so when you add a new
+/// `visit_nested_foo()` method, it is recommended that you search for
+/// existing `fn visit_nested` methods to see where changes are
+/// needed.
+pub trait ItemLikeVisitor<'hir> {
+    fn visit_item(&mut self, item: &'hir Item);
+    fn visit_impl_item(&mut self, impl_item: &'hir ImplItem);
+}
+
+pub struct DeepVisitor<'v, V: 'v> {
+    visitor: &'v mut V,
+}
+
+impl<'v, 'hir, V> DeepVisitor<'v, V>
+    where V: Visitor<'hir> + 'v
+{
+    pub fn new(base: &'v mut V) -> Self {
+        DeepVisitor { visitor: base }
+    }
+}
+
+impl<'v, 'hir, V> ItemLikeVisitor<'hir> for DeepVisitor<'v, V>
+    where V: Visitor<'hir>
+{
+    fn visit_item(&mut self, item: &'hir Item) {
+        self.visitor.visit_item(item);
+    }
+
+    fn visit_impl_item(&mut self, impl_item: &'hir ImplItem) {
+        self.visitor.visit_impl_item(impl_item);
+    }
+}