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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Sep 17, 2017
Merged

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

merged 6 commits into from
Sep 17, 2017

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Aug 25, 2017

Fixes #43493

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 25, 2017

r? @jseyfried

@durka
Copy link
Contributor

durka commented Aug 25, 2017

Let's work up a test?

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 25, 2017

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

@shepmaster 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
jseyfried reviewed Aug 25, 2017
View reviewed changes
src/libsyntax/ext/expand.rs
@@ -439,11 +440,13 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
}
ProcMacroDerive(..) | BuiltinDerive(..) => {
self.cx.span_err(attr.span, &format!("`{}` is a derive mode", attr.path));
self.cx.trace_macros_diag();
kind.dummy(attr.span)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shepmaster
Copy link
Member

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

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 2, 2017

@shepmaster Working on it.

@bjorn3 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
Copy link
Member Author

bjorn3 commented Sep 4, 2017

@jseyfried: Should be working.

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

@jseyfried 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
jseyfried reviewed Sep 5, 2017
View reviewed changes
Copy link
Contributor

@jseyfried jseyfried left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/libsyntax/ext/base.rs
for (sp, notes) in self.expansions.iter() {
let mut db = self.parse_sess.span_diagnostic.span_note_diag(*sp, "trace_macro");
for note in notes {
db.note(note);
}
db.emit();
}
// Fixme: does this result in errors?
self.expansions.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this prevent reporting an expansion trace twice or more

src/libsyntax/ext/expand.rs
kind.dummy(attr.span)
}
_ => {
let msg = &format!("macro `{}` may not be used in attributes", attr.path);
self.cx.span_err(attr.span, msg);
self.cx.trace_macros_diag();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of the trace notes is unstable otherwise.

src/libsyntax/ext/expand.rs
@@ -482,6 +485,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@aidanhs 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
@bjorn3
Copy link
Member Author

bjorn3 commented Sep 16, 2017

ping @jseyfried

@jseyfried
Copy link
Contributor

@bjorn3 Ok, makes sense.
@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2017

📌 Commit 0a2c95b has been approved by jseyfried

TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017
@TimNN
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 TimNN mentioned this pull request Sep 17, 2017
Closed
TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017
@TimNN
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 TimNN mentioned this pull request Sep 17, 2017
Closed
TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017
@TimNN
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 TimNN mentioned this pull request Sep 17, 2017
Merged
bors added a commit that referenced this pull request Sep 17, 2017
@bors
Auto merge of #44654 - TimNN:rollup, r=TimNN
1cdd689 
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 bors merged commit 0a2c95b into rust-lang:master Sep 17, 2017
@bors
Copy link
Contributor

bors commented Sep 17, 2017

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

@bjorn3 bjorn3 deleted the better_trace_macros branch September 18, 2017 06:09
@kennytm
Copy link
Member

kennytm commented Sep 18, 2017

@bors p=-1 retry

Why are you still appearing in the queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jseyfried jseyfried jseyfried left review comments

Assignees

@jseyfried jseyfried

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@bjorn3 @rust-highfive @durka @shepmaster @jseyfried @bors @kennytm @nikomatsakis @aidanhs