Fix "new trace_macros doesn't work if there's an error during expansion" by bjorn3 · Pull Request #44088 · rust-lang/rust

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

 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

 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

 _ => {
 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

 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

- 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" Fixesrust-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

Fixrust-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 ofrust-lang#44088- bjorn3:better_trace_macros, r=jseyf…
cafcdbb
…ried Fix "new trace_macros doesn't work if there's an error during expansion" Fixesrust-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 ofrust-lang#44088- bjorn3:better_trace_macros, r=jseyf…
ef1f04f
…ried Fix "new trace_macros doesn't work if there's an error during expansion" Fixesrust-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 ofrust-lang#44088- bjorn3:better_trace_macros, r=jseyf…
bc638b8
…ried Fix "new trace_macros doesn't work if there's an error during expansion" Fixesrust-lang#43493

TimNN mentioned this pull request Sep 17, 2017
Rollup of 17 pull requests #44654
Merged

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

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

Uh oh!

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