From b32246279d6059564358c4759af08ded7834468b Mon Sep 17 00:00:00 2001 From: comex Date: Tue, 3 Jan 2017 20:40:29 +0100 Subject: [PATCH 1/4] 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: // 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. --- src/librustc/lint/context.rs | 57 ++++++++---------------------------- src/llvm | 2 +- 2 files changed, 13 insertions(+), 46 deletions(-) diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index cce79820ca8da..7f908f13dff43 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -718,15 +718,6 @@ impl<'a, 'tcx> LateContext<'a, 'tcx> { level_stack: vec![], } } - - 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> { @@ -795,10 +786,19 @@ 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_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 +918,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 +926,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 +933,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 +1100,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 @@ -1242,10 +1213,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/llvm b/src/llvm index ceb177eeefa7d..16b79d01fd6d9 160000 --- a/src/llvm +++ b/src/llvm @@ -1 +1 @@ -Subproject commit ceb177eeefa7d67ca29230d2e7e8584f97d4fdad +Subproject commit 16b79d01fd6d942cf3c9120b92df56b13ec92665 From b16a54dd308534940c61fa867ea67c7bba95c2fd Mon Sep 17 00:00:00 2001 From: comex Date: Fri, 6 Jan 2017 18:45:14 -0500 Subject: [PATCH 2/4] Remove accidental submodule change. --- src/llvm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/llvm b/src/llvm index 16b79d01fd6d9..ceb177eeefa7d 160000 --- a/src/llvm +++ b/src/llvm @@ -1 +1 @@ -Subproject commit 16b79d01fd6d942cf3c9120b92df56b13ec92665 +Subproject commit ceb177eeefa7d67ca29230d2e7e8584f97d4fdad From 55513696bd8864ff3296ee657e1b25f9a39ad871 Mon Sep 17 00:00:00 2001 From: comex Date: Fri, 6 Jan 2017 18:51:16 -0500 Subject: [PATCH 3/4] Fix test/ui/span/issue-24690.stderr The errors are now emitted in a different order (in order of source location rather than going back and forth) but otherwise everything's the same. --- src/test/ui/span/issue-24690.stderr | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/test/ui/span/issue-24690.stderr b/src/test/ui/span/issue-24690.stderr index 0d2a2ef751666..dbe5e31287e97 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 From 743535a643ff9c7f5791a71f6b62c27617cdbb3e Mon Sep 17 00:00:00 2001 From: comex Date: Fri, 6 Jan 2017 19:05:27 -0500 Subject: [PATCH 4/4] Add test case. --- .../compile-fail/lint-attr-non-item-node.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 src/test/compile-fail/lint-attr-non-item-node.rs 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 0000000000000..930f69e51e176 --- /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 or the MIT license +// , 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 + } +}