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

Bad macro usage error message does not include correct error location #31022

Open
SimonSapin opened this issue Jan 19, 2016 · 7 comments
Open
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

Consider this reduced test case:

macro_rules! match_ignore_ascii_case {
    (@inner $value:expr) => { () };
    ( $($rest:tt)* ) => { match_ignore_ascii_case!(@inner $($rest)*) };
}

fn main() {
    // This is fine
    match_ignore_ascii_case!(1);

    // This causes an error as it doesn’t match the expected syntax
    // but the error message does not the location of the actual error.
    match_ignore_ascii_case!(2 => 3);
}

The @inner indirection exists because the non-reduced macro is recursive:

This fails to compile (as it should) but the error message does not include the real location of the error, which is line 12. It can be hard to track down in a large crate with many users of the macro.

a.rs:3:52: 3:53 error: unexpected token: `@`
a.rs:3     ( $($rest:tt)* ) => { match_ignore_ascii_case!(@inner $($rest)*) };

The error message looks even worse when the macro is used (with incorrect syntax) from another crate

<cssparser macros>:12:1: 12:2 error: unexpected token: `@`
<cssparser macros>:12 @ inner $ value , ( $ ( $ rest ) * ) -> (
@dotdash dotdash added the A-diagnostics Area: Messages for errors, warnings, and lints label Jan 19, 2016
SimonSapin added a commit to servo/rust-cssparser that referenced this issue Jan 19, 2016
Change the usage syntax to require a comma on the last non-fallback arm,
which is a [breaking-change].

The old definition has started to emit a warning in recent nightlies
and is likely to be an error in future versions:
rust-lang/rfcs#1384

The new definition is recursive to resolve ambiguities.
Unfortunately this makes error reporting terrible:
rust-lang/rust#31022
bors-servo pushed a commit to servo/rust-cssparser that referenced this issue Jan 19, 2016
Make match_ignore_ascii_case! future-proof.

Change the usage syntax to require a comma on the last non-fallback arm, which is a [breaking-change].

The old definition has started to emit a warning in recent nightlies and is likely to be an error in future versions: rust-lang/rfcs#1384

The new definition is recursive to resolve ambiguities. Unfortunately this makes error reporting terrible: rust-lang/rust#31022

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/91)
<!-- Reviewable:end -->
@durka
Copy link
Contributor

durka commented Jan 19, 2016

When the expanded code contains a non-macro-related error (say, a type mismatch), it at least shows a macro expansion backtrace. It's very hard to read, but gives spans at least. I guess that would be a start.

@durka
Copy link
Contributor

durka commented Jan 19, 2016

The trouble is you get the same error if you screw up writing the macro, e.g. misspell @inner or order something backwards in one of the arms. It isn't the caller's fault for sure.

@SimonSapin
Copy link
Contributor Author

Yes, I don’t have an example at hand to copy/past but I see the kind of error message you mean. It may be the caller’s fault even if it isn’t necessarily. A macro expansion backtrace would be useful here.

@steveklabnik steveklabnik removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 24, 2017
@estebank
Copy link
Contributor

Current output:

error: recursion limit reached while expanding the macro `match_ignore_ascii_case`
  --> src/main.rs:3:27
   |
3  |     ( $($rest:tt)* ) => { match_ignore_ascii_case!(@inner $($rest)*) };
   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
12 |     match_ignore_ascii_case!(2 => 3);
   |     --------------------------------- in this macro invocation
   |
   = help: consider adding a `#![recursion_limit="128"]` attribute to your crate

@estebank
Copy link
Contributor

Triage: no change.

@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
@estebank
Copy link
Contributor

estebank commented Jun 8, 2022

Triage: no change.

@estebank estebank added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) D-papercut Diagnostics: An error or lint that needs small tweaks. labels Jun 8, 2022
@estebank
Copy link
Contributor

Triage: no change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants