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

Implement a lint to warn about unused macro rules #96150

Merged
merged 6 commits into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
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.
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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 @@ -3138,6 +3181,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 @@ -2663,3 +2666,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