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.

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

Add lint for unused macros #41907

Merged
merged 8 commits into from
May 17, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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/libcore/num/wrapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use super::Wrapping;

use ops::*;

#[allow(unused_macros)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we can't just remove the unused macro here?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a FIXME below to get the remaining impls uncommented, including invocations of the macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

macro_rules! sh_impl_signed {
($t:ident, $f:ident) => (
#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
1 change: 1 addition & 0 deletions src/libcore/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,7 @@ macro_rules! neg_impl_numeric {
($($t:ty)*) => { neg_impl_core!{ x => -x, $($t)*} }
}

#[allow(unused_macros)]
macro_rules! neg_impl_unsigned {
($($t:ty)*) => {
neg_impl_core!{ x => {
Expand Down
7 changes: 7 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ declare_lint! {
"detects unreachable patterns"
}

declare_lint! {
pub UNUSED_MACROS,
Warn,
"detects macros that were not used"
}

declare_lint! {
pub WARNINGS,
Warn,
Expand Down Expand Up @@ -259,6 +265,7 @@ impl LintPass for HardwiredLints {
DEAD_CODE,
UNREACHABLE_CODE,
UNREACHABLE_PATTERNS,
UNUSED_MACROS,
WARNINGS,
UNUSED_FEATURES,
STABLE_FEATURES,
Expand Down
8 changes: 8 additions & 0 deletions src/librustc/lint/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use hir;
use hir::def_id::LOCAL_CRATE;
use hir::intravisit as hir_visit;
use syntax::visit as ast_visit;
use syntax::tokenstream::ThinTokenStream;

/// Information about the registered lints.
///
Expand Down Expand Up @@ -1125,6 +1126,13 @@ impl<'a> ast_visit::Visitor<'a> for EarlyContext<'a> {
fn visit_attribute(&mut self, attr: &'a ast::Attribute) {
run_lints!(self, check_attribute, early_passes, attr);
}

fn visit_mac_def(&mut self, _mac: &'a ThinTokenStream, id: ast::NodeId) {
let lints = self.sess.lints.borrow_mut().take(id);
for early_lint in lints {
self.early_lint(&early_lint);
}
}
}

enum CheckLintNameResult {
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_driver/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,8 @@ pub fn phase_2_configure_and_expand<F>(sess: &Session,

let krate = ecx.monotonic_expander().expand_crate(krate);

ecx.check_unused_macros();

let mut missing_fragment_specifiers: Vec<_> =
ecx.parse_sess.missing_fragment_specifiers.borrow().iter().cloned().collect();
missing_fragment_specifiers.sort();
Expand Down
11 changes: 0 additions & 11 deletions src/librustc_incremental/persist/preds/compress/test_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,3 @@ macro_rules! graph {
}
}
}

macro_rules! set {
($( $value:expr ),*) => {
{
use $crate::rustc_data_structures::fx::FxHashSet;
let mut set = FxHashSet();
$(set.insert($value);)*
set
}
}
}
3 changes: 2 additions & 1 deletion src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
UNUSED_MUST_USE,
UNUSED_UNSAFE,
PATH_STATEMENTS,
UNUSED_ATTRIBUTES);
UNUSED_ATTRIBUTES,
UNUSED_MACROS);

// Guidelines for creating a future incompatibility lint:
//
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_plugin/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ impl<'a> Registry<'a> {
}
self.syntax_exts.push((name, match extension {
NormalTT(ext, _, allow_internal_unstable) => {
NormalTT(ext, Some(self.krate_span), allow_internal_unstable)
let nid = ast::CRATE_NODE_ID;
NormalTT(ext, Some((nid, self.krate_span)), allow_internal_unstable)
}
IdentTT(ext, _, allow_internal_unstable) => {
IdentTT(ext, Some(self.krate_span), allow_internal_unstable)
Expand Down
7 changes: 7 additions & 0 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,12 @@ pub struct Resolver<'a> {
pub whitelisted_legacy_custom_derives: Vec<Name>,
pub found_unresolved_macro: bool,

// List of macros that we need to warn about as being unused.
// The bool is true if the macro is unused, and false if its used.
// Setting a bool to false should be much faster than removing a single
// element from a FxHashSet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify that this is only for crate-local macro_rules!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, but hopefully the upcoming macros 2.0 macros could be included as well.

unused_macros: FxHashMap<DefId, 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 still think this should be an FxHashSet<DefId> -- I believe the perf difference is negligible here (removing a value from a hash set is already many orders of magnitude faster than the rest the processing we do per used crate-local macro_rules!).


// Maps the `Mark` of an expansion to its containing module or block.
invocations: FxHashMap<Mark, &'a InvocationData<'a>>,

Expand Down Expand Up @@ -1400,6 +1406,7 @@ impl<'a> Resolver<'a> {
potentially_unused_imports: Vec::new(),
struct_constructors: DefIdMap(),
found_unresolved_macro: false,
unused_macros: FxHashMap(),
}
}

Expand Down
26 changes: 24 additions & 2 deletions src/librustc_resolve/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use resolve_imports::ImportResolver;
use rustc::hir::def_id::{DefId, BUILTIN_MACROS_CRATE, CRATE_DEF_INDEX, DefIndex};
use rustc::hir::def::{Def, Export};
use rustc::hir::map::{self, DefCollector};
use rustc::ty;
use rustc::{ty, lint};
use syntax::ast::{self, Name, Ident};
use syntax::attr::{self, HasAttrs};
use syntax::errors::DiagnosticBuilder;
Expand Down Expand Up @@ -291,12 +291,32 @@ impl<'a> base::Resolver for Resolver<'a> {
},
};
self.macro_defs.insert(invoc.expansion_data.mark, def.def_id());
self.unused_macros.get_mut(&def.def_id()).map(|m| *m = false);
Ok(Some(self.get_macro(def)))
}

fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, kind: MacroKind, force: bool)
-> Result<Rc<SyntaxExtension>, Determinacy> {
self.resolve_macro_to_def(scope, path, kind, force).map(|def| self.get_macro(def))
self.resolve_macro_to_def(scope, path, kind, force).map(|def| {
self.unused_macros.get_mut(&def.def_id()).map(|m| *m = false);
self.get_macro(def)
})
}

fn check_unused_macros(&self) {
for (did, _) in self.unused_macros.iter().filter(|&(_, b)| *b) {
let id_span = match *self.macro_map[did] {
SyntaxExtension::NormalTT(_, isp, _) => isp,
_ => None
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is usually formatted

            let id_span = match *self.macro_map[did] {
                SyntaxExtension::NormalTT(_, isp, _) => isp,
                _ => None
            };

if let Some((id, span)) = id_span {
let lint = lint::builtin::UNUSED_MACROS;
let msg = "unused macro definition".to_string();
self.session.add_lint(lint, id, span, msg);
} else {
bug!("attempted to create unused macro error, but span not available");
}
}
}
}

Expand Down Expand Up @@ -687,6 +707,8 @@ impl<'a> Resolver<'a> {
if attr::contains_name(&item.attrs, "macro_export") {
let def = Def::Macro(def_id, MacroKind::Bang);
self.macro_exports.push(Export { name: ident.name, def: def, span: item.span });
} else {
self.unused_macros.insert(def_id, true);
}
}

Expand Down
8 changes: 7 additions & 1 deletion src/libsyntax/ext/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ pub enum SyntaxExtension {
///
/// The `bool` dictates whether the contents of the macro can
/// directly use `#[unstable]` things (true == yes).
NormalTT(Box<TTMacroExpander>, Option<Span>, bool),
NormalTT(Box<TTMacroExpander>, Option<(ast::NodeId, Span)>, bool),

/// A function-like syntax extension that has an extra ident before
/// the block.
Expand Down Expand Up @@ -589,6 +589,7 @@ pub trait Resolver {
-> Result<Option<Rc<SyntaxExtension>>, Determinacy>;
fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, kind: MacroKind, force: bool)
-> Result<Rc<SyntaxExtension>, Determinacy>;
fn check_unused_macros(&self);
}

#[derive(Copy, Clone, Debug)]
Expand Down Expand Up @@ -618,6 +619,7 @@ impl Resolver for DummyResolver {
_force: bool) -> Result<Rc<SyntaxExtension>, Determinacy> {
Err(Determinacy::Determined)
}
fn check_unused_macros(&self) {}
}

#[derive(Clone)]
Expand Down Expand Up @@ -800,6 +802,10 @@ impl<'a> ExtCtxt<'a> {
pub fn name_of(&self, st: &str) -> ast::Name {
Symbol::intern(st)
}

pub fn check_unused_macros(&self) {
self.resolver.check_unused_macros();
}
}

/// Extract a string literal from the macro expanded version of `expr`,
Expand Down
2 changes: 1 addition & 1 deletion src/libsyntax/ext/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
call_site: span,
callee: NameAndSpan {
format: MacroBang(Symbol::intern(&format!("{}", path))),
span: exp_span,
span: exp_span.map(|(_, s)| s),
allow_internal_unstable: allow_internal_unstable,
},
});
Expand Down
6 changes: 5 additions & 1 deletion src/libsyntax/ext/tt/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,11 @@ pub fn compile(sess: &ParseSess, features: &RefCell<Features>, def: &ast::Item)
valid: valid,
});

NormalTT(exp, Some(def.span), attr::contains_name(&def.attrs, "allow_internal_unstable"))
NormalTT(
exp,
Some((def.id, def.span)),
attr::contains_name(&def.attrs, "allow_internal_unstable")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: nonstandard formatting -- should be three lines with "visual indenting" or indented one block with four/five lines.

}

fn check_lhs_nt_follows(sess: &ParseSess,
Expand Down
6 changes: 5 additions & 1 deletion src/libsyntax/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use abi::Abi;
use ast::*;
use syntax_pos::Span;
use codemap::Spanned;
use tokenstream::ThinTokenStream;

#[derive(Copy, Clone, PartialEq, Eq)]
pub enum FnKind<'a> {
Expand Down Expand Up @@ -110,6 +111,9 @@ pub trait Visitor<'ast>: Sized {
// definition in your trait impl:
// visit::walk_mac(self, _mac)
}
fn visit_mac_def(&mut self, _mac: &'ast ThinTokenStream, _id: NodeId) {
// Nothing to do
}
fn visit_path(&mut self, path: &'ast Path, _id: NodeId) {
walk_path(self, path)
}
Expand Down Expand Up @@ -288,7 +292,7 @@ pub fn walk_item<'a, V: Visitor<'a>>(visitor: &mut V, item: &'a Item) {
walk_list!(visitor, visit_trait_item, methods);
}
ItemKind::Mac(ref mac) => visitor.visit_mac(mac),
ItemKind::MacroDef(..) => {},
ItemKind::MacroDef(ref ts) => visitor.visit_mac_def(ts, item.id),
}
walk_list!(visitor, visit_attribute, &item.attrs);
}
Expand Down
1 change: 1 addition & 0 deletions src/test/compile-fail-fulldeps/proc-macro/resolve-error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// aux-build:bang_proc_macro.rs

#![feature(proc_macro)]
#![allow(unused_macros)]

#[macro_use]
extern crate derive_foo;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

// gate-test-allow_internal_unstable

#![allow(unused_macros)]

macro_rules! bar {
() => {
// more layers don't help:
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/feature-gate-allow-internal-unstable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

#[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps
macro_rules! foo {
() => {}
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/invalid-macro-matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

macro_rules! invalid {
_ => (); //~ ERROR invalid macro matcher
}
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/issue-21356.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

macro_rules! test { ($wrong:t_ty ..) => () }
//~^ ERROR: invalid fragment specifier `t_ty`

Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/issue-39388.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

macro_rules! assign {
(($($a:tt)*) = ($($b:tt))*) => { //~ ERROR expected `*` or `+`
$($a)* = $($b)*
Expand Down
1 change: 1 addition & 0 deletions src/test/compile-fail/issue-39404.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// except according to those terms.

#![deny(missing_fragment_specifier)] //~ NOTE lint level defined here
#![allow(unused_macros)]

macro_rules! m { ($i) => {} }
//~^ ERROR missing fragment specifier
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/issue-5067.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

macro_rules! foo {
( $()* ) => {};
//~^ ERROR repetition matches empty token tree
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/macro-expansion-tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

mod macros_cant_escape_fns {
fn f() {
macro_rules! m { () => { 3 + 4 } }
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/macro-follow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
//
// Check the macro follow sets (see corresponding rpass test).

#![allow(unused_macros)]

// FOLLOW(pat) = {FatArrow, Comma, Eq, Or, Ident(if), Ident(in)}
macro_rules! follow_pat {
($p:pat ()) => {}; //~ERROR `$p:pat` is followed by `(`
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/macro-followed-by-seq-bad.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
// Regression test for issue #25436: check that things which can be
// followed by any token also permit X* to come afterwards.

#![allow(unused_macros)]

macro_rules! foo {
( $a:expr $($b:tt)* ) => { }; //~ ERROR not allowed for `expr` fragments
( $a:ty $($b:tt)* ) => { }; //~ ERROR not allowed for `ty` fragments
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/macro-input-future-proofing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

macro_rules! errors_everywhere {
($ty:ty <) => (); //~ ERROR `$ty:ty` is followed by `<`, which is not allowed for `ty`
($ty:ty < foo ,) => (); //~ ERROR `$ty:ty` is followed by `<`, which is not allowed for `ty`
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/macro-shadowing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

// aux-build:two_macros.rs

#![allow(unused_macros)]

macro_rules! foo { () => {} }
macro_rules! macro_one { () => {} }
#[macro_use(macro_two)] extern crate two_macros;
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/unused-macro-with-bad-frag-spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

// Issue #21370

macro_rules! test {
Expand Down
2 changes: 2 additions & 0 deletions src/test/compile-fail/unused-macro-with-follow-violation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![allow(unused_macros)]

macro_rules! test {
($e:expr +) => () //~ ERROR not allowed for `expr` fragments
}
Expand Down
Loading