Skip to content

Commit

Permalink
Rollup merge of rust-lang#96150 - est31:unused_macro_rules, r=petroch…
Browse files Browse the repository at this point in the history
…enkov

Implement a lint to warn about unused macro rules

This implements a new lint to warn about unused macro rules (arms/matchers), similar to the `unused_macros` lint added by rust-lang#41907 that warns about entire macros.

```rust
macro_rules! unused_empty {
    (hello) => { println!("Hello, world!") };
    () => { println!("empty") }; //~ ERROR: 1st rule of macro `unused_empty` is never used
}

fn main() {
    unused_empty!(hello);
}
```

Builds upon rust-lang#96149 and rust-lang#96156.

Fixes rust-lang#73576
  • Loading branch information
GuillaumeGomez committed May 6, 2022
2 parents 08e74e4 + 3989f02 commit 914f947
Show file tree
Hide file tree
Showing 33 changed files with 399 additions and 77 deletions.
2 changes: 2 additions & 0 deletions compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,7 @@ fn generic_simd_intrinsic<'ll, 'tcx>(
span: Span,
) -> Result<&'ll Value, ()> {
// macros for error handling:
#[cfg_attr(not(bootstrap), allow(unused_macro_rules))]
macro_rules! emit_error {
($msg: tt) => {
emit_error!($msg, )
Expand Down Expand Up @@ -1144,6 +1145,7 @@ fn generic_simd_intrinsic<'ll, 'tcx>(
span: Span,
args: &[OperandRef<'tcx, &'ll Value>],
) -> Result<&'ll Value, ()> {
#[cfg_attr(not(bootstrap), allow(unused_macro_rules))]
macro_rules! emit_error {
($msg: tt) => {
emit_error!($msg, )
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_expand/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,8 @@ pub trait ResolverExpand {
force: bool,
) -> Result<Lrc<SyntaxExtension>, Indeterminate>;

fn record_macro_rule_usage(&mut self, mac_id: NodeId, rule_index: usize);

fn check_unused_macros(&mut self);

// Resolver interfaces for specific built-in macros.
Expand Down
40 changes: 28 additions & 12 deletions compiler/rustc_expand/src/mbe/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,13 @@ impl<'a> ParserAnyMacro<'a> {
}

struct MacroRulesMacroExpander {
node_id: NodeId,
name: Ident,
span: Span,
transparency: Transparency,
lhses: Vec<Vec<MatcherLoc>>,
rhses: Vec<mbe::TokenTree>,
valid: bool,
is_local: bool,
}

impl TTMacroExpander for MacroRulesMacroExpander {
Expand All @@ -179,12 +179,12 @@ impl TTMacroExpander for MacroRulesMacroExpander {
cx,
sp,
self.span,
self.node_id,
self.name,
self.transparency,
input,
&self.lhses,
&self.rhses,
self.is_local,
)
}
}
Expand All @@ -207,14 +207,17 @@ fn generic_extension<'cx, 'tt>(
cx: &'cx mut ExtCtxt<'_>,
sp: Span,
def_span: Span,
node_id: NodeId,
name: Ident,
transparency: Transparency,
arg: TokenStream,
lhses: &'tt [Vec<MatcherLoc>],
rhses: &'tt [mbe::TokenTree],
is_local: bool,
) -> Box<dyn MacResult + 'cx> {
let sess = &cx.sess.parse_sess;
// Macros defined in the current crate have a real node id,
// whereas macros from an external crate have a dummy id.
let is_local = node_id != DUMMY_NODE_ID;

if cx.trace_macros() {
let msg = format!("expanding `{}! {{ {} }}`", name, pprust::tts_to_string(&arg));
Expand Down Expand Up @@ -296,6 +299,10 @@ fn generic_extension<'cx, 'tt>(
let mut p = Parser::new(sess, tts, false, None);
p.last_type_ascription = cx.current_expansion.prior_type_ascription;

if is_local {
cx.resolver.record_macro_rule_usage(node_id, i);
}

// Let the context choose how to interpret the result.
// Weird, but useful for X-macros.
return Box::new(ParserAnyMacro {
Expand Down Expand Up @@ -372,7 +379,7 @@ pub fn compile_declarative_macro(
features: &Features,
def: &ast::Item,
edition: Edition,
) -> SyntaxExtension {
) -> (SyntaxExtension, Vec<Span>) {
debug!("compile_declarative_macro: {:?}", def);
let mk_syn_ext = |expander| {
SyntaxExtension::new(
Expand All @@ -385,6 +392,7 @@ pub fn compile_declarative_macro(
&def.attrs,
)
};
let dummy_syn_ext = || (mk_syn_ext(Box::new(macro_rules_dummy_expander)), Vec::new());

let diag = &sess.parse_sess.span_diagnostic;
let lhs_nm = Ident::new(sym::lhs, def.span);
Expand Down Expand Up @@ -445,17 +453,17 @@ pub fn compile_declarative_macro(
let s = parse_failure_msg(&token);
let sp = token.span.substitute_dummy(def.span);
sess.parse_sess.span_diagnostic.struct_span_err(sp, &s).span_label(sp, msg).emit();
return mk_syn_ext(Box::new(macro_rules_dummy_expander));
return dummy_syn_ext();
}
Error(sp, msg) => {
sess.parse_sess
.span_diagnostic
.struct_span_err(sp.substitute_dummy(def.span), &msg)
.emit();
return mk_syn_ext(Box::new(macro_rules_dummy_expander));
return dummy_syn_ext();
}
ErrorReported => {
return mk_syn_ext(Box::new(macro_rules_dummy_expander));
return dummy_syn_ext();
}
};

Expand Down Expand Up @@ -530,6 +538,15 @@ pub fn compile_declarative_macro(
None => {}
}

// Compute the spans of the macro rules
// We only take the span of the lhs here,
// so that the spans of created warnings are smaller.
let rule_spans = if def.id != DUMMY_NODE_ID {
lhses.iter().map(|lhs| lhs.span()).collect::<Vec<_>>()
} else {
Vec::new()
};

// Convert the lhses into `MatcherLoc` form, which is better for doing the
// actual matching. Unless the matcher is invalid.
let lhses = if valid {
Expand All @@ -549,17 +566,16 @@ pub fn compile_declarative_macro(
vec![]
};

mk_syn_ext(Box::new(MacroRulesMacroExpander {
let expander = Box::new(MacroRulesMacroExpander {
name: def.ident,
span: def.span,
node_id: def.id,
transparency,
lhses,
rhses,
valid,
// Macros defined in the current crate have a real node id,
// whereas macros from an external crate have a dummy id.
is_local: def.id != DUMMY_NODE_ID,
}))
});
(mk_syn_ext(expander), rule_spans)
}

fn check_lhs_nt_follows(sess: &ParseSess, def: &ast::Item, lhs: &mbe::TokenTree) -> bool {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
PATH_STATEMENTS,
UNUSED_ATTRIBUTES,
UNUSED_MACROS,
UNUSED_MACRO_RULES,
UNUSED_ALLOCATION,
UNUSED_DOC_COMMENTS,
UNUSED_EXTERN_CRATES,
Expand Down
44 changes: 44 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,10 @@ declare_lint! {
declare_lint! {
/// The `unused_macros` lint detects macros that were not used.
///
/// Note that this lint is distinct from the `unused_macro_rules` lint,
/// which checks for single rules that never match of an otherwise used
/// macro, and thus never expand.
///
/// ### Example
///
/// ```rust
Expand All @@ -775,6 +779,45 @@ declare_lint! {
"detects macros that were not used"
}

declare_lint! {
/// The `unused_macro_rules` lint detects macro rules that were not used.
///
/// Note that the lint is distinct from the `unused_macros` lint, which
/// fires if the entire macro is never called, while this lint fires for
/// single unused rules of the macro that is otherwise used.
/// `unused_macro_rules` fires only if `unused_macros` wouldn't fire.
///
/// ### Example
///
/// ```rust
/// macro_rules! unused_empty {
/// (hello) => { println!("Hello, world!") }; // This rule is unused
/// () => { println!("empty") }; // This rule is used
/// }
///
/// fn main() {
/// unused_empty!(hello);
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Unused macro rules may signal a mistake or unfinished code. Furthermore,
/// they slow down compilation. Right now, silencing the warning is not
/// supported on a single rule level, so you have to add an allow to the
/// entire macro definition.
///
/// If you intended to export the macro to make it
/// available outside of the crate, use the [`macro_export` attribute].
///
/// [`macro_export` attribute]: https://doc.rust-lang.org/reference/macros-by-example.html#path-based-scope
pub UNUSED_MACRO_RULES,
Warn,
"detects macro rules that were not used"
}

declare_lint! {
/// The `warnings` lint allows you to change the level of other
/// lints which produce warnings.
Expand Down Expand Up @@ -3104,6 +3147,7 @@ declare_lint_pass! {
OVERLAPPING_RANGE_ENDPOINTS,
BINDINGS_WITH_VARIANT_NAME,
UNUSED_MACROS,
UNUSED_MACRO_RULES,
WARNINGS,
UNUSED_FEATURES,
STABLE_FEATURES,
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ macro_rules! make_mir_visitor {
// for best performance, we want to use an iterator rather
// than a for-loop, to avoid calling `body::Body::invalidate` for
// each basic block.
#[cfg_attr(not(bootstrap), allow(unused_macro_rules))]
macro_rules! basic_blocks {
(mut) => (body.basic_blocks_mut().iter_enumerated_mut());
() => (body.basic_blocks().iter_enumerated());
Expand All @@ -279,6 +280,7 @@ macro_rules! make_mir_visitor {
self.visit_local_decl(local, & $($mutability)? body.local_decls[local]);
}

#[cfg_attr(not(bootstrap), allow(unused_macro_rules))]
macro_rules! type_annotations {
(mut) => (body.user_type_annotations.iter_enumerated_mut());
() => (body.user_type_annotations.iter_enumerated());
Expand Down Expand Up @@ -932,6 +934,7 @@ macro_rules! make_mir_visitor {
body: &$($mutability)? Body<'tcx>,
location: Location
) {
#[cfg_attr(not(bootstrap), allow(unused_macro_rules))]
macro_rules! basic_blocks {
(mut) => (body.basic_blocks_mut());
() => (body.basic_blocks());
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_resolve/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ version = "0.0.0"
edition = "2021"

[lib]
test = false
doctest = false

[dependencies]
Expand Down
26 changes: 18 additions & 8 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ impl<'a> Resolver<'a> {
}

let ext = Lrc::new(match self.cstore().load_macro_untracked(def_id, &self.session) {
LoadedMacro::MacroDef(item, edition) => self.compile_macro(&item, edition),
LoadedMacro::MacroDef(item, edition) => self.compile_macro(&item, edition).0,
LoadedMacro::ProcMacro(ext) => ext,
});

Expand Down Expand Up @@ -1218,25 +1218,35 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
// Mark the given macro as unused unless its name starts with `_`.
// Macro uses will remove items from this set, and the remaining
// items will be reported as `unused_macros`.
fn insert_unused_macro(&mut self, ident: Ident, def_id: LocalDefId, node_id: NodeId) {
fn insert_unused_macro(
&mut self,
ident: Ident,
def_id: LocalDefId,
node_id: NodeId,
rule_spans: &[Span],
) {
if !ident.as_str().starts_with('_') {
self.r.unused_macros.insert(def_id, (node_id, ident));
for (rule_i, rule_span) in rule_spans.iter().enumerate() {
self.r.unused_macro_rules.insert((def_id, rule_i), (ident, *rule_span));
}
}
}

fn define_macro(&mut self, item: &ast::Item) -> MacroRulesScopeRef<'a> {
let parent_scope = self.parent_scope;
let expansion = parent_scope.expansion;
let def_id = self.r.local_def_id(item.id);
let (ext, ident, span, macro_rules) = match &item.kind {
let (ext, ident, span, macro_rules, rule_spans) = match &item.kind {
ItemKind::MacroDef(def) => {
let ext = Lrc::new(self.r.compile_macro(item, self.r.session.edition()));
(ext, item.ident, item.span, def.macro_rules)
let (ext, rule_spans) = self.r.compile_macro(item, self.r.session.edition());
let ext = Lrc::new(ext);
(ext, item.ident, item.span, def.macro_rules, rule_spans)
}
ItemKind::Fn(..) => match self.proc_macro_stub(item) {
Some((macro_kind, ident, span)) => {
self.r.proc_macro_stubs.insert(def_id);
(self.r.dummy_ext(macro_kind), ident, span, false)
(self.r.dummy_ext(macro_kind), ident, span, false, Vec::new())
}
None => return parent_scope.macro_rules,
},
Expand Down Expand Up @@ -1264,7 +1274,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
self.r.define(module, ident, MacroNS, (res, vis, span, expansion, IsMacroExport));
} else {
self.r.check_reserved_macro_name(ident, res);
self.insert_unused_macro(ident, def_id, item.id);
self.insert_unused_macro(ident, def_id, item.id, &rule_spans);
}
self.r.visibilities.insert(def_id, vis);
let scope = self.r.arenas.alloc_macro_rules_scope(MacroRulesScope::Binding(
Expand All @@ -1287,7 +1297,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
_ => self.resolve_visibility(&item.vis),
};
if vis != ty::Visibility::Public {
self.insert_unused_macro(ident, def_id, item.id);
self.insert_unused_macro(ident, def_id, item.id, &rule_spans);
}
self.r.define(module, ident, MacroNS, (res, vis, span, expansion));
self.r.visibilities.insert(def_id, vis);
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_resolve/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ use crate::{LexicalScopeBinding, NameBinding, NameBindingKind, PrivacyError, Vis
use crate::{ParentScope, PathResult, ResolutionError, Resolver, Scope, ScopeSet};
use crate::{Segment, UseError};

#[cfg(test)]
mod tests;

type Res = def::Res<ast::NodeId>;

/// A vector of spans and replacements, a message and applicability.
Expand Down Expand Up @@ -2675,3 +2678,14 @@ fn is_span_suitable_for_use_injection(s: Span) -> bool {
// import or other generated ones
!s.from_expansion()
}

/// Convert the given number into the corresponding ordinal
crate fn ordinalize(v: usize) -> String {
let suffix = match ((11..=13).contains(&(v % 100)), v % 10) {
(false, 1) => "st",
(false, 2) => "nd",
(false, 3) => "rd",
_ => "th",
};
format!("{v}{suffix}")
}
40 changes: 40 additions & 0 deletions compiler/rustc_resolve/src/diagnostics/tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
use super::ordinalize;

#[test]
fn test_ordinalize() {
assert_eq!(ordinalize(1), "1st");
assert_eq!(ordinalize(2), "2nd");
assert_eq!(ordinalize(3), "3rd");
assert_eq!(ordinalize(4), "4th");
assert_eq!(ordinalize(5), "5th");
// ...
assert_eq!(ordinalize(10), "10th");
assert_eq!(ordinalize(11), "11th");
assert_eq!(ordinalize(12), "12th");
assert_eq!(ordinalize(13), "13th");
assert_eq!(ordinalize(14), "14th");
// ...
assert_eq!(ordinalize(20), "20th");
assert_eq!(ordinalize(21), "21st");
assert_eq!(ordinalize(22), "22nd");
assert_eq!(ordinalize(23), "23rd");
assert_eq!(ordinalize(24), "24th");
// ...
assert_eq!(ordinalize(30), "30th");
assert_eq!(ordinalize(31), "31st");
assert_eq!(ordinalize(32), "32nd");
assert_eq!(ordinalize(33), "33rd");
assert_eq!(ordinalize(34), "34th");
// ...
assert_eq!(ordinalize(7010), "7010th");
assert_eq!(ordinalize(7011), "7011th");
assert_eq!(ordinalize(7012), "7012th");
assert_eq!(ordinalize(7013), "7013th");
assert_eq!(ordinalize(7014), "7014th");
// ...
assert_eq!(ordinalize(7020), "7020th");
assert_eq!(ordinalize(7021), "7021st");
assert_eq!(ordinalize(7022), "7022nd");
assert_eq!(ordinalize(7023), "7023rd");
assert_eq!(ordinalize(7024), "7024th");
}
Loading

0 comments on commit 914f947

Please sign in to comment.