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

Do not run the default panic hook inside procedural macros. #47933

Merged
merged 1 commit into from
Feb 24, 2018
Merged

Do not run the default panic hook inside procedural macros. #47933

merged 1 commit into from
Feb 24, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Feb 1, 2018 •
edited by Mark-Simulacrum
Loading

@Zoxc Zoxc mentioned this pull request Feb 1, 2018
Closed
nikomatsakis
nikomatsakis requested changes Feb 1, 2018
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

clearly we need to fix that test you mentioned -- or maybe we can do a UI test? Seems like we ought to be able to, no?

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 1, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 2, 2018

I don't know why the test does not work. It is already an UI test which tests stderr.

@nikomatsakis
Copy link
Contributor

@Zoxc I'd like to understand that :/

@bors
Copy link
Contributor

bors commented Feb 6, 2018

☔ The latest upstream changes (presumably #47203) made this pull request unmergeable. Please resolve the merge conflicts.

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 10, 2018

I probably should move the panic handler to librustc. I want to sneak in some printing of the query stack on panic too.

@nikomatsakis
Copy link
Contributor

@Zoxc I did some investigating, not sure if I mentioned it here; I was not able to reproduce the problem except for on nightly builds. Any clue why that might be?

@nikomatsakis
Copy link
Contributor

I guess we can land in the meantime, but it's vexing.

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 13, 2018

I don't have any ideas about why the test failed.

@Mark-Simulacrum Mark-Simulacrum changed the title Do not run the default panic hook inside procedural macros. Fixes #47812 Do not run the default panic hook inside procedural macros. Feb 13, 2018
@Mark-Simulacrum
Copy link
Member

This is the reason for the test failure: https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/json.rs#L88. Specifically, non-JSON lines in the output are simply ignored by compiletest, which means that any UI test will fail to test this behavior.

I think the most straightforward fix here would be to change this to a run-make test.

@Zoxc Is there a chance you could make that change?

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 13, 2018

@Mark-Simulacrum So does compiletest ask for JSON output and then convert that into what the stderr output would be? Could we make it also emit the non-JSON lines to this output?

@Mark-Simulacrum
Copy link
Member

Actually, now that I do some deeper investigation, it looks like if we put // compile-flags:--error-format human everything will work. We detect this in compiletest and if we see any format of error-format in the compile args we'll bail out of the JSON parsing and just check the raw outputs.

So looks like the fix is even simpler than I thought!

@nikomatsakis
Copy link
Contributor

@Mark-Simulacrum thanks for that!

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 13, 2018

📌 Commit 11b74ed has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 13, 2018
@bors
Copy link
Contributor

bors commented Feb 17, 2018

🔒 Merge conflict

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 17, 2018
@nikomatsakis
Copy link
Contributor

@Zoxc needs rebase :(

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 20, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 20, 2018

📌 Commit 9d3719b has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 20, 2018
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 23, 2018
@Manishearth
Rollup merge of rust-lang#47933 - Zoxc:plugin-panics, r=nikomatsakis
abf4d70 
Do not run the default panic hook inside procedural macros.

Fixes rust-lang#47812

r? @nikomatsakis
@Manishearth Manishearth mentioned this pull request Feb 23, 2018
Merged
bors added a commit that referenced this pull request Feb 23, 2018
@bors
Auto merge of #48476 - Manishearth:rollup, r=Manishearth
2fbb63a 
Rollup of 12 pull requests

- Successful merges: #47933, #48072, #48083, #48123, #48157, #48219, #48221, #48245, #48429, #48436, #48438, #48472
- Failed merges:
bors added a commit that referenced this pull request Feb 24, 2018
@bors
Auto merge of #48476 - Manishearth:rollup, r=Manishearth
6070d3e 
Rollup of 12 pull requests

- Successful merges: #47933, #48072, #48083, #48123, #48157, #48219, #48221, #48245, #48429, #48436, #48438, #48472
- Failed merges:
@bors bors merged commit 9d3719b into rust-lang:master Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Zoxc @nikomatsakis @bors @Mark-Simulacrum @kennytm