Give a better error when using an undeclared macro variable

[jyn514]

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a17456ca7139db13e945cbde87ae6fbd

macro_rules! m {
    () => {
        $x;
    }
}

fn foo() {
    m!()
}

The current output is:

error: expected expression, found `$`
 --> src/lib.rs:3:9
  |
3 |         $x;
  |         ^^ expected expression
...
8 |     m!()
  |     ---- in this macro invocation
  |
  = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info)

Ideally the output should look like:

error[E0425]: cannot find macro parameter `$x` in this scope
 --> src/lib.rs:8:5
  |
2 |    () => {
  |    ^^ in this macro matcher
3 |     $x;
  |     ^^ not found in this scope
...
8 |     m!()
  |     ---- in this macro invocation
  |
  = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info)

@rustbot label +A-macros

[PatchMixolydic]

This scenario is caught by meta_variable_misuse, which almost produces the desired diagnostic (though it doesn't point at the macro matchers or the invocation site) (playground):

warning: unknown macro variable `x`
 --> src/lib.rs:5:9
  |
5 |         $x;
  |         ^^
  |
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![warn(meta_variable_misuse)]
  |         ^^^^^^^^^^^^^^^^^^^^

It would be beneficial if this lint was documented somewhere a little less obscure than the rustc book, but I'm not sure what a good place would be. Maybe the Reference or the (unofficial) Little Book of Rust Macros?

cc #61053

[jyn514]

It seems strange that's a lint and not just a built-in diagnostic? I'd at least expect it to warn by default ...

[mark-i-m]

@jyn514 It's not an error or a warn-by-default because MBE macros are unbelievably permissive, which makes it impossible to detect things like this without false positives. For example, it is hard to determine such a snippet is occurring inside a nested macro definition, in which case the outer macro may be defining the meta-variable.

We spent a lot of time trying to figure out what we can check and how: #61053

IMHO, we should remove the MBE system altogether in a future edition and create something more constrained/well-defined that is designed to be (1) easy to document (current macros have terrible documentation), (2) statically checkable, and (3) easy to use. Right now, MBEs satisfy none of these conditions and are not fixable as far as I can tell.

[jyn514]

We spent a lot of time trying to figure out what we can check and how: #61053

I thought the issues you ran into were because you were trying to check it statically at definition time, right? I think giving an error about undefined metavariables should be easier at expansion time.

[mark-i-m]

Possibly, but is that useful? As a macro user, what would you do if you found that the macro author made a mistake? I suppose you would just report the bug and hope they fix it. I suppose that's marginally better than getting the error above...

[jyn514]

@mark-i-m not all macros are defined cross-crate; IME most of them are local to the current crate because people don't want to expose macros in the API. Having the better error is very useful for local macros, or just when testing your public macro.

[mark-i-m]

That's fair enough.

[mark-i-m]

It looks like this is the code you would want to change:

rust/compiler/rustc_expand/src/mbe/transcribe.rs

Lines 249 to 254 in 3bebee7

	// If we aren't able to match the meta-var, we push it back into the result but
	// with modified syntax context. (I believe this supports nested macros).
	marker.visit_span(&mut sp);
	marker.visit_ident(&mut original_ident);
	result.push(TokenTree::token(token::Dollar, sp).into());
	result.push(TokenTree::Token(Token::from_ast_ident(original_ident)).into());

I'm not sure how you could tell if you were in a nested invocation or not, though. One option is to just make it an error and do a crater run to see how wide the impact is.

Maybe the AST Validation might be another place to check for this? I don't fully understand what happens in that pass, but it happens after all macros are fully expanded.

[vincenzopalazzo]

@rustbot claim