Auto merge of #38806 - comex:lint-attr-fix, r=nrc

Fix lint attributes on non-item nodes.

Currently, late lint checking uses two HIR visitors: LateContext and
IdVisitor.  IdVisitor only overrides visit_id, and for each node searches
for builtin lints previously added to the session; LateContext overrides
a number of methods, and runs late lints.  When LateContext encounters an
item, it first has IdVisitor walk everything in it except nested items
(OnlyBodies), then recurses into it itself - i.e. there are two separate
walks.

Aside from apparently being unnecessary, this separation prevents lint
attributes (allow/deny/warn) on non-item HIR nodes from working
properly.  Test case:

```rust
// generates warning without this change
fn main() { #[allow(unreachable_code)] loop { break; break; } }
```

LateContext contains logic to merge attributes seen into the current lint
settings while walking (with_lint_attrs), but IdVisitor does not.  So
such attributes will affect late lints (because they are called from
LateContext), and if the node contains any items within it, they will
affect builtin lints within those items (because that IdVisitor is run
while LateContext is within the attributed node), but otherwise the
attributes will be ignored for builtin lints.

This change simply removes IdVisitor and moves its visit_id into
LateContext itself.  Hopefully this doesn't break anything...

Also added walk calls to visit_lifetime and visit_lifetime_def
respectively, so visit_lifetime_def will recurse into the lifetime and
visit_lifetime will recurse into the name.  In principle this could
confuse lint plugins.  This is "necessary" because walk_lifetime calls
visit_id on the lifetime; of course, an alternative would be directly
calling visit_id (which would require manually iterating over the
lifetimes in visit_lifetime_def), but that seems less clean.

3 files changed, 43 insertions, 59 deletions

diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs
index f7f9a17ee51..7d85f3607b5 100644
--- a/src/librustc/lint/context.rs
+++ b/src/librustc/lint/context.rs
@@ -705,17 +705,6 @@ impl<'a> EarlyContext<'a> {
     }
 }
 
-impl<'a, 'tcx> LateContext<'a, 'tcx> {
-    fn visit_ids<'b, F: 'b>(&'b mut self, f: F)
-        where F: FnOnce(&mut IdVisitor<'b, 'a, 'tcx>)
-    {
-        let mut v = IdVisitor::<'b, 'a, 'tcx> {
-            cx: self
-        };
-        f(&mut v);
-    }
-}
-
 impl<'a, 'tcx> LintContext<'tcx> for LateContext<'a, 'tcx> {
     /// Get the overall compiler `Session` object.
     fn sess(&self) -> &Session {
@@ -782,6 +771,16 @@ impl<'a, 'tcx> hir_visit::Visitor<'tcx> for LateContext<'a, 'tcx> {
         hir_visit::NestedVisitorMap::All(&self.tcx.map)
     }
 
+    // Output any lints that were previously added to the session.
+    fn visit_id(&mut self, id: ast::NodeId) {
+        if let Some(lints) = self.sess().lints.borrow_mut().remove(&id) {
+            debug!("LateContext::visit_id: id={:?} lints={:?}", id, lints);
+            for early_lint in lints {
+                self.early_lint(early_lint);
+            }
+        }
+    }
+
     fn visit_nested_body(&mut self, body: hir::BodyId) {
         let old_tables = self.tables;
         self.tables = self.tcx.body_tables(body);
@@ -793,7 +792,6 @@ impl<'a, 'tcx> hir_visit::Visitor<'tcx> for LateContext<'a, 'tcx> {
     fn visit_item(&mut self, it: &'tcx hir::Item) {
         self.with_lint_attrs(&it.attrs, |cx| {
             run_lints!(cx, check_item, late_passes, it);
-            cx.visit_ids(|v| v.visit_item(it));
             hir_visit::walk_item(cx, it);
             run_lints!(cx, check_item_post, late_passes, it);
         })
@@ -918,7 +916,6 @@ impl<'a, 'tcx> hir_visit::Visitor<'tcx> for LateContext<'a, 'tcx> {
     fn visit_trait_item(&mut self, trait_item: &'tcx hir::TraitItem) {
         self.with_lint_attrs(&trait_item.attrs, |cx| {
             run_lints!(cx, check_trait_item, late_passes, trait_item);
-            cx.visit_ids(|v| hir_visit::walk_trait_item(v, trait_item));
             hir_visit::walk_trait_item(cx, trait_item);
             run_lints!(cx, check_trait_item_post, late_passes, trait_item);
         });
@@ -927,7 +924,6 @@ impl<'a, 'tcx> hir_visit::Visitor<'tcx> for LateContext<'a, 'tcx> {
     fn visit_impl_item(&mut self, impl_item: &'tcx hir::ImplItem) {
         self.with_lint_attrs(&impl_item.attrs, |cx| {
             run_lints!(cx, check_impl_item, late_passes, impl_item);
-            cx.visit_ids(|v| hir_visit::walk_impl_item(v, impl_item));
             hir_visit::walk_impl_item(cx, impl_item);
             run_lints!(cx, check_impl_item_post, late_passes, impl_item);
         });
@@ -935,10 +931,12 @@ impl<'a, 'tcx> hir_visit::Visitor<'tcx> for LateContext<'a, 'tcx> {
 
     fn visit_lifetime(&mut self, lt: &'tcx hir::Lifetime) {
         run_lints!(self, check_lifetime, late_passes, lt);
+        hir_visit::walk_lifetime(self, lt);
     }
 
     fn visit_lifetime_def(&mut self, lt: &'tcx hir::LifetimeDef) {
         run_lints!(self, check_lifetime_def, late_passes, lt);
+        hir_visit::walk_lifetime_def(self, lt);
     }
 
     fn visit_path(&mut self, p: &'tcx hir::Path, id: ast::NodeId) {
@@ -1100,35 +1098,6 @@ impl<'a> ast_visit::Visitor<'a> for EarlyContext<'a> {
     }
 }
 
-struct IdVisitor<'a, 'b: 'a, 'tcx: 'a+'b> {
-    cx: &'a mut LateContext<'b, 'tcx>
-}
-
-// Output any lints that were previously added to the session.
-impl<'a, 'b, 'tcx> hir_visit::Visitor<'tcx> for IdVisitor<'a, 'b, 'tcx> {
-    fn nested_visit_map<'this>(&'this mut self) -> hir_visit::NestedVisitorMap<'this, 'tcx> {
-        hir_visit::NestedVisitorMap::OnlyBodies(&self.cx.tcx.map)
-    }
-
-    fn visit_id(&mut self, id: ast::NodeId) {
-        if let Some(lints) = self.cx.sess().lints.borrow_mut().remove(&id) {
-            debug!("LateContext::visit_id: id={:?} lints={:?}", id, lints);
-            for early_lint in lints {
-                self.cx.early_lint(early_lint);
-            }
-        }
-    }
-
-    fn visit_trait_item(&mut self, _ti: &'tcx hir::TraitItem) {
-        // Do not recurse into trait or impl items automatically. These are
-        // processed separately by calling hir_visit::walk_trait_item()
-    }
-
-    fn visit_impl_item(&mut self, _ii: &'tcx hir::ImplItem) {
-        // See visit_trait_item()
-    }
-}
-
 enum CheckLintNameResult {
     Ok,
     // Lint doesn't exist
@@ -1252,10 +1221,6 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
 
     // Visit the whole crate.
     cx.with_lint_attrs(&krate.attrs, |cx| {
-        cx.visit_ids(|v| {
-            hir_visit::walk_crate(v, krate);
-        });
-
         // since the root module isn't visited as an item (because it isn't an
         // item), warn for it here.
         run_lints!(cx, check_crate, late_passes, krate);
diff --git a/src/test/compile-fail/lint-attr-non-item-node.rs b/src/test/compile-fail/lint-attr-non-item-node.rs
new file mode 100644
index 00000000000..930f69e51e1
--- /dev/null
+++ b/src/test/compile-fail/lint-attr-non-item-node.rs
@@ -0,0 +1,19 @@
+// Copyright 2016 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.
+
+// Checks that lint attributes work on non-item AST nodes
+
+fn main() {
+    #[deny(unreachable_code)]
+    loop {
+        break;
+        "unreachable"; //~ ERROR unreachable statement
+    }
+}
diff --git a/src/test/ui/span/issue-24690.stderr b/src/test/ui/span/issue-24690.stderr
index 0d2a2ef7516..dbe5e31287e 100644
--- a/src/test/ui/span/issue-24690.stderr
+++ b/src/test/ui/span/issue-24690.stderr
@@ -1,15 +1,3 @@
-error: unused variable: `theOtherTwo`
-  --> $DIR/issue-24690.rs:20:9
-   |
-20 |     let theOtherTwo = 2;
-   |         ^^^^^^^^^^^
-   |
-note: lint level defined here
-  --> $DIR/issue-24690.rs:16:9
-   |
-16 | #![deny(warnings)]
-   |         ^^^^^^^^
-
 error: variable `theTwo` should have a snake case name such as `the_two`
   --> $DIR/issue-24690.rs:19:9
    |
@@ -28,5 +16,17 @@ error: variable `theOtherTwo` should have a snake case name such as `the_other_t
 20 |     let theOtherTwo = 2;
    |         ^^^^^^^^^^^
 
+error: unused variable: `theOtherTwo`
+  --> $DIR/issue-24690.rs:20:9
+   |
+20 |     let theOtherTwo = 2;
+   |         ^^^^^^^^^^^
+   |
+note: lint level defined here
+  --> $DIR/issue-24690.rs:16:9
+   |
+16 | #![deny(warnings)]
+   |         ^^^^^^^^
+
 error: aborting due to 3 previous errors