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

new trace_macros doesn't work if there's an error during expansion #43493

Closed
durka opened this issue Jul 26, 2017 · 5 comments
Closed

new trace_macros doesn't work if there's an error during expansion #43493

durka opened this issue Jul 26, 2017 · 5 comments
Assignees
Labels
A-syntaxext Area: Syntax extensions C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@durka
Copy link
Contributor

durka commented Jul 26, 2017

The new trace_macros has much better output, but it has regressed in functionality! The old trace_macros could be used to debug macro expansion. Example using rustc 1.6.0:

$ cargo +1.6.0 script -u trace_macros -e 'trace_macros!(true); macro_rules! m { (x $($t:tt)*) => { m!() } } m!(foo);'
   Compiling expr v0.1.0 (file:///Users/alex/.cargo/.cargo/script-cache/expr-634e13f197b6cae5)
m! { foo }
.cargo/.cargo/script-cache/expr-634e13f197b6cae5/expr.rs:11:81: 11:84 error: no rules expected the token `foo`
.cargo/.cargo/script-cache/expr-634e13f197b6cae5/expr.rs:11     match {trace_macros!(true); macro_rules! m { (x $($t:tt)*) => { m!() } } m!(foo);} {
                                                                                                                                            ^~~
Could not compile `expr`.

OK, you can see m!(foo) was invoked, and then it didn't match so there's an error.

Now with current rustc (all channels):

$ cargo script -u trace_macros -e 'trace_macros!(true); macro_rules! m { (x $($t:tt)*) => { m!() } } m!(foo);'
   Compiling expr v0.1.0 (file:///Users/alex/.cargo/.cargo/script-cache/expr-634e13f197b6cae5)
error: no rules expected the token `foo`
  --> .cargo/.cargo/script-cache/expr-634e13f197b6cae5/expr.rs:11:81
   |
11 |     match {trace_macros!(true); macro_rules! m { (x $($t:tt)*) => { m!() } } m!(foo);} {
   |                                                                                 ^^^

error: Could not compile `expr`.

You no longer get to see the chain of expansions leading up to the error. This typically occurs much deeper in a string of recursive macros, making debugging much harder.

cc #41520 @estebank @nikomatsakis

@Mark-Simulacrum Mark-Simulacrum added A-syntaxext Area: Syntax extensions C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated P-high High priority labels Jul 26, 2017
@Mark-Simulacrum
Copy link
Member

Nominating for prioritization. Seems good to fix since this is very painful for macro authors.

@nikomatsakis nikomatsakis removed the P-high High priority label Jul 27, 2017
@nikomatsakis
Copy link
Contributor

triage: P-medium

Prioritizing as medium. @estebank -- any thoughts on how to include more information? @durka -- any suggestions for what the output should maybe look like?

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Jul 27, 2017
@jseyfried jseyfried self-assigned this Jul 27, 2017
@durka
Copy link
Contributor Author

durka commented Jul 27, 2017

I think the output provided by #41520/#42103 is fine (modulo the verbosity issue #42223). It just needs to show up even when expansion doesn't finish due to an error. Something like this:

$ cargo script -u trace_macros -e 'trace_macros!(true); macro_rules! m { (x $($t:tt)*) => { m!() } } m!(foo);'
   Compiling expr v0.1.0 (file:///Users/alex/.cargo/.cargo/script-cache/expr-634e13f197b6cae5)
error: no rules expected the token `foo`
  --> .cargo/.cargo/script-cache/expr-634e13f197b6cae5/expr.rs:11:81
   |
11 |     match {trace_macros!(true); macro_rules! m { (x $($t:tt)*) => { m!() } } m!(foo);} {
   |                                                                                 ^^^
   = note: expanding `m! { foo }`

This is probably also the issue in #42170.

@jseyfried
Copy link
Contributor

This underlying issue here is that macro errors cause aborts way too much. For a quick fix, we could make sure we always call trace_macros_diag in the fatal error handling methods.

@estebank
Copy link
Contributor

I think that @jseyfried's solution would be correct one.

bjorn3 added a commit to bjorn3/rust that referenced this issue Aug 25, 2017
TimNN added a commit to TimNN/rust that referenced this issue Sep 17, 2017
…ried

Fix "new trace_macros doesn't work if there's an error during expansion"

Fixes rust-lang#43493
TimNN added a commit to TimNN/rust that referenced this issue Sep 17, 2017
…ried

Fix "new trace_macros doesn't work if there's an error during expansion"

Fixes rust-lang#43493
TimNN added a commit to TimNN/rust that referenced this issue Sep 17, 2017
…ried

Fix "new trace_macros doesn't work if there's an error during expansion"

Fixes rust-lang#43493
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-syntaxext Area: Syntax extensions C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

6 participants