From eb3c611e1d3829e05aec8f9887efa164ea2c6da5 Mon Sep 17 00:00:00 2001 From: est31 Date: Thu, 9 Jun 2022 04:46:51 +0200 Subject: [PATCH 1/3] Never regard macro rules with compile_error! invocations as unused The very point of compile_error! is to never be reached, and one of the use cases of the macro, currently also listed as examples in the documentation of compile_error, is to create nicer errors for wrong macro invocations. Thus, we shuuld never warn about unused macro arms that contain invocations of compile_error. --- compiler/rustc_expand/src/lib.rs | 1 + compiler/rustc_expand/src/mbe/macro_rules.rs | 36 +++++++++++++++++-- .../rustc_resolve/src/build_reduced_graph.rs | 6 ++-- compiler/rustc_resolve/src/macros.rs | 2 +- .../unused-macro-rules-compile-error.rs | 27 ++++++++++++++ .../unused-macro-rules-compile-error.stderr | 26 ++++++++++++++ 6 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 src/test/ui/lint/unused/unused-macro-rules-compile-error.rs create mode 100644 src/test/ui/lint/unused/unused-macro-rules-compile-error.stderr diff --git a/compiler/rustc_expand/src/lib.rs b/compiler/rustc_expand/src/lib.rs index 7043ad5464530..86ff110eec183 100644 --- a/compiler/rustc_expand/src/lib.rs +++ b/compiler/rustc_expand/src/lib.rs @@ -1,4 +1,5 @@ #![allow(rustc::potential_query_instability)] +#![feature(array_windows)] #![feature(associated_type_bounds)] #![feature(associated_type_defaults)] #![feature(if_let_guard)] diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index b86304ba6b1b1..2a767646eafe6 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -380,7 +380,7 @@ pub fn compile_declarative_macro( features: &Features, def: &ast::Item, edition: Edition, -) -> (SyntaxExtension, Vec) { +) -> (SyntaxExtension, Vec<(usize, Span)>) { debug!("compile_declarative_macro: {:?}", def); let mk_syn_ext = |expander| { SyntaxExtension::new( @@ -542,8 +542,17 @@ pub fn compile_declarative_macro( // 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. + // Also, we are only interested in non-foreign macros. let rule_spans = if def.id != DUMMY_NODE_ID { - lhses.iter().map(|lhs| lhs.span()).collect::>() + lhses + .iter() + .zip(rhses.iter()) + .enumerate() + // If the rhs contains an invocation like compile_error!, + // don't consider the rule for the unused rule lint. + .filter(|(_idx, (_lhs, rhs))| !has_compile_error_macro(rhs)) + .map(|(idx, (lhs, _rhs))| (idx, lhs.span())) + .collect::>() } else { Vec::new() }; @@ -651,6 +660,29 @@ fn check_matcher(sess: &ParseSess, def: &ast::Item, matcher: &[mbe::TokenTree]) err == sess.span_diagnostic.err_count() } +fn has_compile_error_macro(rhs: &mbe::TokenTree) -> bool { + match rhs { + mbe::TokenTree::Delimited(_sp, d) => { + let has_compile_error = d.tts.array_windows::<3>().any(|[ident, bang, args]| { + if let mbe::TokenTree::Token(ident) = ident && + let TokenKind::Ident(ident, _) = ident.kind && + ident == sym::compile_error && + let mbe::TokenTree::Token(bang) = bang && + let TokenKind::Not = bang.kind && + let mbe::TokenTree::Delimited(_, del) = args && + del.delim != Delimiter::Invisible + { + true + } else { + false + } + }); + if has_compile_error { true } else { d.tts.iter().any(has_compile_error_macro) } + } + _ => false, + } +} + // `The FirstSets` for a matcher is a mapping from subsequences in the // matcher to the FIRST set for that subsequence. // diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 20d9123e411ab..f8fa7a0941d00 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -1220,12 +1220,12 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { ident: Ident, def_id: LocalDefId, node_id: NodeId, - rule_spans: &[Span], + rule_spans: &[(usize, 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)); + for (rule_i, rule_span) in rule_spans.iter() { + self.r.unused_macro_rules.insert((def_id, *rule_i), (ident, *rule_span)); } } } diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index 2e2d3674560e8..3fb34cdcd9bd9 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -870,7 +870,7 @@ impl<'a> Resolver<'a> { &mut self, item: &ast::Item, edition: Edition, - ) -> (SyntaxExtension, Vec) { + ) -> (SyntaxExtension, Vec<(usize, Span)>) { let (mut result, mut rule_spans) = compile_declarative_macro( &self.session, self.session.features_untracked(), diff --git a/src/test/ui/lint/unused/unused-macro-rules-compile-error.rs b/src/test/ui/lint/unused/unused-macro-rules-compile-error.rs new file mode 100644 index 0000000000000..4d51db89bc0b3 --- /dev/null +++ b/src/test/ui/lint/unused/unused-macro-rules-compile-error.rs @@ -0,0 +1,27 @@ +#![deny(unused_macro_rules)] +// To make sure we are not hitting this +#![deny(unused_macros)] + +macro_rules! num { + (one) => { 1 }; + // Most simple (and common) case + (two) => { compile_error!("foo"); }; + // Some nested use + (two_) => { foo(compile_error!("foo")); }; + (three) => { 3 }; + (four) => { 4 }; //~ ERROR: rule of macro +} +const _NUM: u8 = num!(one) + num!(three); + +// compile_error not used as a macro invocation +macro_rules! num2 { + (one) => { 1 }; + // Only identifier present + (two) => { fn compile_error() {} }; //~ ERROR: rule of macro + // Only identifier and bang present + (two_) => { compile_error! }; //~ ERROR: rule of macro + (three) => { 3 }; +} +const _NUM2: u8 = num2!(one) + num2!(three); + +fn main() {} diff --git a/src/test/ui/lint/unused/unused-macro-rules-compile-error.stderr b/src/test/ui/lint/unused/unused-macro-rules-compile-error.stderr new file mode 100644 index 0000000000000..76af8c967db1e --- /dev/null +++ b/src/test/ui/lint/unused/unused-macro-rules-compile-error.stderr @@ -0,0 +1,26 @@ +error: 5th rule of macro `num` is never used + --> $DIR/unused-macro-rules-compile-error.rs:12:5 + | +LL | (four) => { 4 }; + | ^^^^^^ + | +note: the lint level is defined here + --> $DIR/unused-macro-rules-compile-error.rs:1:9 + | +LL | #![deny(unused_macro_rules)] + | ^^^^^^^^^^^^^^^^^^ + +error: 3rd rule of macro `num2` is never used + --> $DIR/unused-macro-rules-compile-error.rs:22:5 + | +LL | (two_) => { compile_error! }; + | ^^^^^^ + +error: 2nd rule of macro `num2` is never used + --> $DIR/unused-macro-rules-compile-error.rs:20:5 + | +LL | (two) => { fn compile_error() {} }; + | ^^^^^ + +error: aborting due to 3 previous errors + From 777e136f4cd811bec0c5dabd47e1ae4285f25ca2 Mon Sep 17 00:00:00 2001 From: est31 Date: Thu, 9 Jun 2022 23:34:06 +0200 Subject: [PATCH 2/3] Suppress the unused_macro_rules lint if malformed rules are encountered Prior to this commit, if a macro had any malformed rules, all rules would be reported as unused, regardless of whether they were used or not. So we just turn off unused rule checking completely for macros with malformed rules. --- compiler/rustc_expand/src/mbe/macro_rules.rs | 10 ++++++---- .../lint/unused/unused-macro-rules-malformed-rule.rs | 11 +++++++++++ .../unused/unused-macro-rules-malformed-rule.stderr | 8 ++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/lint/unused/unused-macro-rules-malformed-rule.rs create mode 100644 src/test/ui/lint/unused/unused-macro-rules-malformed-rule.stderr diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index 2a767646eafe6..04af1eaf67b6d 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -539,11 +539,11 @@ 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. + // Compute the spans of the macro rules for unused rule linting. + // To avoid warning noise, only consider the rules of this + // macro for the lint, if all rules are valid. // Also, we are only interested in non-foreign macros. - let rule_spans = if def.id != DUMMY_NODE_ID { + let rule_spans = if valid && def.id != DUMMY_NODE_ID { lhses .iter() .zip(rhses.iter()) @@ -551,6 +551,8 @@ pub fn compile_declarative_macro( // If the rhs contains an invocation like compile_error!, // don't consider the rule for the unused rule lint. .filter(|(_idx, (_lhs, rhs))| !has_compile_error_macro(rhs)) + // We only take the span of the lhs here, + // so that the spans of created warnings are smaller. .map(|(idx, (lhs, _rhs))| (idx, lhs.span())) .collect::>() } else { diff --git a/src/test/ui/lint/unused/unused-macro-rules-malformed-rule.rs b/src/test/ui/lint/unused/unused-macro-rules-malformed-rule.rs new file mode 100644 index 0000000000000..a826026ec405c --- /dev/null +++ b/src/test/ui/lint/unused/unused-macro-rules-malformed-rule.rs @@ -0,0 +1,11 @@ +#![deny(unused_macro_rules)] + +macro_rules! foo { + (v) => {}; + (w) => {}; + () => 0; //~ ERROR: macro rhs must be delimited +} + +fn main() { + foo!(v); +} diff --git a/src/test/ui/lint/unused/unused-macro-rules-malformed-rule.stderr b/src/test/ui/lint/unused/unused-macro-rules-malformed-rule.stderr new file mode 100644 index 0000000000000..797c867103f03 --- /dev/null +++ b/src/test/ui/lint/unused/unused-macro-rules-malformed-rule.stderr @@ -0,0 +1,8 @@ +error: macro rhs must be delimited + --> $DIR/unused-macro-rules-malformed-rule.rs:6:11 + | +LL | () => 0; + | ^ + +error: aborting due to previous error + From 787e24cdfd5bef5136540287dd918e48b8a645db Mon Sep 17 00:00:00 2001 From: est31 Date: Thu, 9 Jun 2022 23:46:40 +0200 Subject: [PATCH 3/3] Test that the unused_macros lint works correctly if rules are malformed The unused_macro_rules lint had a bug where it would regard all rules of a macro as unused if one rule were malformed. This bug doesn't exist with the unused_macros lint. To ensure it doesn't appear in the future, we add a test for it. --- .../unused/unused-macros-malformed-rule.rs | 15 +++++++++++ .../unused-macros-malformed-rule.stderr | 26 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 src/test/ui/lint/unused/unused-macros-malformed-rule.rs create mode 100644 src/test/ui/lint/unused/unused-macros-malformed-rule.stderr diff --git a/src/test/ui/lint/unused/unused-macros-malformed-rule.rs b/src/test/ui/lint/unused/unused-macros-malformed-rule.rs new file mode 100644 index 0000000000000..d4c35fad9b0f0 --- /dev/null +++ b/src/test/ui/lint/unused/unused-macros-malformed-rule.rs @@ -0,0 +1,15 @@ +#![deny(unused_macros)] + +macro_rules! foo { //~ ERROR: unused macro definition + (v) => {}; + () => 0; //~ ERROR: macro rhs must be delimited +} + +macro_rules! bar { + (v) => {}; + () => 0; //~ ERROR: macro rhs must be delimited +} + +fn main() { + bar!(v); +} diff --git a/src/test/ui/lint/unused/unused-macros-malformed-rule.stderr b/src/test/ui/lint/unused/unused-macros-malformed-rule.stderr new file mode 100644 index 0000000000000..9a880dccfbf6d --- /dev/null +++ b/src/test/ui/lint/unused/unused-macros-malformed-rule.stderr @@ -0,0 +1,26 @@ +error: macro rhs must be delimited + --> $DIR/unused-macros-malformed-rule.rs:5:11 + | +LL | () => 0; + | ^ + +error: macro rhs must be delimited + --> $DIR/unused-macros-malformed-rule.rs:10:11 + | +LL | () => 0; + | ^ + +error: unused macro definition: `foo` + --> $DIR/unused-macros-malformed-rule.rs:3:14 + | +LL | macro_rules! foo { + | ^^^ + | +note: the lint level is defined here + --> $DIR/unused-macros-malformed-rule.rs:1:9 + | +LL | #![deny(unused_macros)] + | ^^^^^^^^^^^^^ + +error: aborting due to 3 previous errors +