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

bjorn3 · 2017-08-25T18:48:03Z

Fixes #43493

…r during expansion)

rust-highfive · 2017-08-25T18:48:15Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

bjorn3 · 2017-08-25T18:48:28Z

r? @jseyfried

durka · 2017-08-25T18:50:42Z

Let's work up a test?

bjorn3 · 2017-08-25T18:52:05Z

Will do it tommorrow when I have more time to build rust

jseyfried · 2017-08-25T21:52:09Z

src/libsyntax/ext/expand.rs

            ProcMacroDerive(..) | BuiltinDerive(..) => {
                self.cx.span_err(attr.span, &format!("`{}` is a derive mode", attr.path));
+                self.cx.trace_macros_diag();
                kind.dummy(attr.span)


I don't think we need self.cx.trace_macros_diag(); when we're not panicking immediately after.

shepmaster · 2017-09-01T19:59:59Z

@bjorn3 A gentle reminder - it's been 7 days since your last message!

bjorn3 · 2017-09-02T10:17:07Z

@shepmaster Working on it.

bjorn3 · 2017-09-04T16:41:05Z

@jseyfried: Should be working.

Travis job #58449.5 failed: The command "eval git fetch origin +refs/pull/44088/merge: " failed 3 times.

jseyfried

LGTM other than what appears to be excessive self.cx.trace_macros_diag();.

jseyfried · 2017-09-05T17:16:35Z

src/libsyntax/ext/base.rs

            db.emit();
        }
+        // Fixme: does this result in errors?
+        self.expansions.clear();


Why is this necessary?

As this prevent reporting an expansion trace twice or more

jseyfried · 2017-09-05T17:17:35Z

src/libsyntax/ext/expand.rs

            _ => {
                let msg = &format!("macro `{}` may not be used in attributes", attr.path);
                self.cx.span_err(attr.span, msg);
+                self.cx.trace_macros_diag();


Why do we need this here? This error shouldn't abort expansion.

The order of the trace notes is unstable otherwise.

jseyfried · 2017-09-05T17:18:43Z

src/libsyntax/ext/expand.rs

                if let Err(msg) = validate_and_set_expn_info(def_span.map(|(_, s)| s),
                                                             false, false) {
                    self.cx.span_err(path.span, &msg);
+                    self.cx.trace_macros_diag();


Also this, and the most of the others in this file.
We use dummy expansions (kind.dummy(span)) so that we don't have to abort.

jseyfried · 2017-09-05T17:19:20Z

src/libsyntax/ext/tt/macro_rules.rs

-    cx.span_fatal(best_fail_spot.substitute_dummy(sp), &best_fail_msg);
+    cx.span_err(best_fail_spot.substitute_dummy(sp), &best_fail_msg);
+    cx.trace_macros_diag();
+    DummyResult::any(sp)


With DummyResult we shouldn't need cx.trace_macros_diag(); here.

bjorn3 · 2017-09-16T09:40:48Z

ping @jseyfried

jseyfried · 2017-09-17T06:16:00Z

@bjorn3 Ok, makes sense.
@bors r+

bors · 2017-09-17T06:16:01Z

📌 Commit 0a2c95b has been approved by jseyfried

…ried Fix "new trace_macros doesn't work if there's an error during expansion" Fixes rust-lang#43493

Rollup of 17 pull requests - Successful merges: #44073, #44088, #44381, #44397, #44509, #44533, #44549, #44553, #44562, #44567, #44595, #44604, #44617, #44622, #44630, #44639, #44647 - Failed merges:

bors · 2017-09-17T16:29:33Z

⌛ Testing commit 0a2c95b with merge 1cdd689...

kennytm · 2017-09-18T15:15:20Z

@bors p=-1 retry

Why are you still appearing in the queue

Fix rust-lang#43493 (new trace_macros doesn't work if there's an erro…

1ed962b

…r during expansion)

rust-highfive assigned nikomatsakis Aug 25, 2017

rust-highfive assigned jseyfried and unassigned nikomatsakis Aug 25, 2017

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 25, 2017

jseyfried reviewed Aug 25, 2017

View reviewed changes

Fix error

878013c

Dont abort on first macro error

8bb7dba

bjorn3 changed the title ~~Fix "new trace_macros doesn't work if there's an error during expansion"~~ [WIP] Fix "new trace_macros doesn't work if there's an error during expansion" Sep 2, 2017

bjorn3 added 3 commits September 2, 2017 18:13

Better trace-macro and less span_err_fatal

8b71e0b

Fix tidy error

30603ee

Fix test

0a2c95b

jseyfried changed the title ~~[WIP] Fix "new trace_macros doesn't work if there's an error during expansion"~~ Fix "new trace_macros doesn't work if there's an error during expansion" Sep 5, 2017

jseyfried reviewed Sep 5, 2017

View reviewed changes

aidanhs added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 13, 2017

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017

Rollup merge of rust-lang#44088 - bjorn3:better_trace_macros, r=jseyf…

cafcdbb

…ried Fix "new trace_macros doesn't work if there's an error during expansion" Fixes rust-lang#43493

TimNN mentioned this pull request Sep 17, 2017

Rollup of 19 pull requests #44649

Closed

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017

Rollup merge of rust-lang#44088 - bjorn3:better_trace_macros, r=jseyf…

ef1f04f

…ried Fix "new trace_macros doesn't work if there's an error during expansion" Fixes rust-lang#43493

TimNN mentioned this pull request Sep 17, 2017

Rollup of 18 pull requests #44652

Closed

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017

Rollup merge of rust-lang#44088 - bjorn3:better_trace_macros, r=jseyf…

bc638b8

…ried Fix "new trace_macros doesn't work if there's an error during expansion" Fixes rust-lang#43493

TimNN mentioned this pull request Sep 17, 2017

Rollup of 17 pull requests #44654

Merged

bors merged commit 0a2c95b into rust-lang:master Sep 17, 2017

bjorn3 deleted the better_trace_macros branch September 18, 2017 06:09

Uh oh!

Conversation

bjorn3 commented Aug 25, 2017

Uh oh!

rust-highfive commented Aug 25, 2017

Uh oh!

bjorn3 commented Aug 25, 2017

Uh oh!

durka commented Aug 25, 2017

Uh oh!

bjorn3 commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shepmaster commented Sep 1, 2017

Uh oh!

bjorn3 commented Sep 2, 2017

Uh oh!

bjorn3 commented Sep 4, 2017

Uh oh!

jseyfried left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 commented Sep 16, 2017

Uh oh!

jseyfried commented Sep 17, 2017

Uh oh!

bors commented Sep 17, 2017

Uh oh!

bors commented Sep 17, 2017

Uh oh!

kennytm commented Sep 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants