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

Misleading diagnostic for macro error! when called with no parameters #58796

Closed
MarkSteinbrick95 opened this issue Feb 28, 2019 · 5 comments
Closed
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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@MarkSteinbrick95
Copy link

When calling the macro with no arguments error!():

error: Could not compile
warning: build failed, waiting for other jobs to finish...
error: expected identifier, found `,`
  --> mod.rs:37:15
   |
37 |               error!();
   |               ^^^^^^^^^
   |               |
   |               expected identifier
   |               help: remove this comma
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error

error: Could not compile

The diagnostic should instruct the user that a string literal is required as a parameter to the macro.

@Centril Centril added A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Feb 28, 2019
@jonas-schievink
Copy link
Contributor

The diagnostic should instruct the user that a string literal is required as a parameter to the macro.

error! is declared as taking $($arg:tt)*, so I don't think this is possible. If it would take a $fmt:lit instead, maybe.

@jonas-schievink jonas-schievink added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Feb 28, 2019
@estebank
Copy link
Contributor

estebank commented Mar 9, 2019

Ran using -Zexternal-macro-backtrace:

error: expected identifier, found `,`
  --> <::log::macros::error macros>:4:28
   |
1  | / ( target : $ target : expr , $ ( $ arg : tt ) * ) => (
2  | | log ! ( target : $ target , $ crate :: Level :: Error , $ ( $ arg ) * ) ; ) ;
3  | | ( $ ( $ arg : tt ) * ) => (
4  | | log ! ( $ crate :: Level :: Error , $ ( $ arg ) * ) ; )
   | |____________________________^__________________________- in this expansion of `error!`
   |                              |
   |                              expected identifier
   |                              help: remove this comma
   |
  ::: src/main.rs:29:5
   |
29 |       error!();
   |       --------- in this macro invocation

Getting the compiler to behave correctly in this case will be difficult, but making the macro itself handle this case and have an appropriate compile time error might be reasonable. What does @rust-lang/libs think?

@estebank estebank added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 9, 2019
@dtolnay
Copy link
Member

dtolnay commented Mar 10, 2019

I opened rust-lang/log#322 to fix the problem on log's side. The compiler was largely doing the right thing and I consider it a problem with the way log's macro was written that resulted in a misleading error.

There is something going wrong in the compiler here too though. Minimized:

macro_rules! log {
    ($lvl:expr, $($arg:tt)+) => {}
}

fn main() {
    log!(Level::Error ,);
}
error: expected identifier, found `,`
 --> src/main.rs:6:17
  |
6 |     log!(Level::Error ,);
  |                 ^
  |                 |
  |                 expected identifier
  |                 help: remove this comma

The error points to something that is an identifier and says it expected an identifier 😞 and points to something that is not a comma and says remove this comma. 😢

@estebank
Copy link
Contributor

I noticed that too. My guess is that it's just a token off by one error somewhere.

@estebank
Copy link
Contributor

estebank commented Mar 11, 2019

@dtolnay I can confirm that that incorrect diagnostic is caused by #52397 :(

It'd be the following without it

error: unexpected end of macro invocation
 --> file.rs:6:24
  |
1 | macro_rules! log {
  | ---------------- when calling this macro
...
6 |     log!(Level::Error ,);
  |                        ^ missing tokens in macro arguments

pietroalbini added a commit to pietroalbini/rust that referenced this issue Mar 13, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
Centril added a commit to Centril/rust that referenced this issue Mar 13, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Mar 14, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
Centril added a commit to Centril/rust that referenced this issue Mar 16, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
Centril added a commit to Centril/rust that referenced this issue Mar 16, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
sanxiyn added a commit to sanxiyn/rust that referenced this issue Mar 18, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
Centril added a commit to Centril/rust that referenced this issue Mar 19, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
Centril added a commit to Centril/rust that referenced this issue Mar 19, 2019
Be more discerning on when to attempt suggesting a comma in a macro invocation

Fix rust-lang#58796.
bors added a commit that referenced this issue Apr 25, 2019
Add guard for missing comma in macro call suggestion

Fix #60233. Follow up to #58796.

r? @oli-obk
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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants