Warn about unused macro arms.

[iago-lito]

rustc now warns about unused macros. With the following code:

macro_rules! used {
    () => {};
}

macro_rules! unused { // <- This should be cleaned up.
    () => {};
}

fn main() {
    used!();
} 

we get the warning:

warning: unused macro definition
 --> src/main.rs:5:1
  |
5 | / macro_rules! unused {
6 | |     () => {};
7 | | }
  | |_^
  |
  = note: `#[warn(unused_macros)]` on by default

This is really helpful while cleaning up a large meta-programming mess.

However, it does not warn about unused macro arms, so the following:

macro_rules! used_macro {
    (used_arm) => {};
    (unused_arm) => {}; // <-- This should be cleaned up.
}

fn main() {
    used_macro!(used_arm);
} 

yields no warning yet.

Following #34938, I open this issue for discussing this feature. Is this something desirable to have rustc warn about unused macro arms?

[lcnr]

I personally think that we should not warn for unused macro arms by default, as I tend to fairly often write the following:

macro_rules! used_macro {
    ($($e:expr),*) => {};
    ($($e:expr),*,) => {}; // <-- This is unused.
}

fn main() {
    used_macro!(7, 13);
} 

To not have to worry about this in case I ever change a macro invocation to

use_macro!(
    7,
    13,
);

[iago-lito]

@lcnr This is convenient indeed :) But I don't think it should be blocking, for several reasons:

• First, ($($e:expr),*$(,)?) already works fine in this situation:

macro_rules! used_macro {
    ($($e:expr),*$(,)?) => {}; // Only one used arm.
}

fn main() {
    used_macro!(7, 13); // Without the comma.
    used_macro!(
        7,
        13, // With the comma.
        );
}

• Or, if this alternate solution is still not acceptable for some reason:

  • Then this problem is still only a matter of style, so it could maybe be special-cased and/or handled by rustfmt?

  • In any case, like we have the option to opt-out from this warning on a local basis for unused variables (let _unused_var = ();) with a _ prefix, we might consider an option to opt-out from this warning for unused macro arms, although I'm afraid it would need new syntax, like

    _(unused_arm) => { ... } // or
    (unused_arm) _=> { ... } // (eew.) or
    (unused_arm) => _{ ... } // (ew.)
    

  • Alternately, we could consider the warning to be a way to discourage such syntax extensions with redundant syntactic forms (like a macro working both with and without the comma).

In any case, I think that this is a good point, but also that it does not hinder the need for a warning about unused macro arms.

[est31]

One concern I have is that it's not possible to cfg-gate single macro arms, which would be needed in situations I've outlined in this comment: #34938 (comment)

[petrochenkov]

@est31
You could put an allow on the whole macro.

#[allow(unused_macro_arms)]
macro foo {
    used_arm => {},
    conditionally_used_arm => {}
}

Not perfect, but still a pretty good approximation.
Macros with platform-dependent use of arms are not common.

[est31]

Good point. Concern resolved! Would love to see this implemented. Begone, unused stuff!

[iago-lito]

@petrochenkov Is this finer-grained alternate allow line straightforward to feature, or would it need new syntax?

macro foo {
    used_arm => {},
    #[allow(unused_macro_arm)]
    conditionally_used_arm => {}
}

[petrochenkov]

@iago-lito
It will need some work, right now the whole macro body is treated as a single token stream.
The individual arms will need to be kept as nodes in AST and assigned NodeIds, then you'll be able to attach lint attributes like allow to them.
Also, the attributes on macro arms are indeed a new syntax.

[est31]

You'd likely require ids to implement the lint in the first place, so at least that cost has to be paid already. At least that's how I did it in #41907 (with a set of ids of unused macros), idk how much it has changed since then.

[petrochenkov]

For uniquely identifying the arm having only the macro's NodeId + the arm's index should be enough, then this pair can be used to track which arms are used and which are not.

For applying allows to macro arms you need real NodeIds for them because that's what the lint level infra works with.

[est31]

The lint might also be useful from a performance point of view, as each unused arm causes additional compile time. According to this blog post, there is sometimes a lot of compile time spent in expansion, it also reports about PRs that have reordered match arms and removed unused ones to increase whole-crate compile times by double digit percentages.

[est31]

I'm wondering about the name for the lint. I couldn't find the term "arm" used anywhere in official documentation to refer to macro matchers, like the reference. Instead I found the term "rule", while arm was only used for match expressions, which follow a similar, but not the same, model as declarative macros.

Per that, the name of the lint should be unused_macro_rules, not unused_macro_arms. Which is a bit of a problem, because it can be read as (unused) (macro_rules), making people think it is meant for entire macros, not just their rules. One solution would be to abandon the plural i.e. unused_macro_rule. But that would be inconsistent with other lints. The lints that use singular forms for their name like unused_unsafe, unused_mut usually don't have corresponding plurals (singularia tantum).

I'm thinking about unused_macro_matchers or unused_macro_transcribers but I don't know, those only describe parts of the rules... What do you think?