-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Match errors using the callsite of macro expansions #52175
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
Looks like you tagged the wrong issue there |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
As in the tagged issue, the question here is whether the current implementation ignores the callsite of macro expansions on purpose or not. From the tests it looks that yes, some tests expect the error to be reported inside the macro definition. If this works as intended I can close the PR and that's all. However, if the test calls a macro from a library and an error arises from the macro expansion, then there is no way to write the That said, this pull request would still require some fixes because on some tests it doesn't match the expected error. Maybe span expansions are not used just for macros? |
Ping from triage, @alexcrichton: This PR requires your review. |
Oh dear sorry for the delay! I don't personally know much about spans/diagnostics, but this does look like we may not want to change it due to the number of failing tests? For UI tests as have the full output anyway to match against, so this is largely an issue for compile-fail tests which we're migrating over time to UI tests I think. @fpoli do you still want to land this PR as-is after fixing up some tests? Or were you thinking of taking an alternate strategy? |
I think that the number of failing tests is not a problem: usually the fix requires to move the |
For tests in which new |
Also see #52531 which is somewhat relevant. |
For macros we should ideally go up the macro call stack until we are in the current file, but then stop. |
That's a great suggestion, quite easy to implement I think. I'll try |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Done. Existing Overall, I think that the tests are strictly better. |
This is great, thanks! |
📌 Commit c473b92adcb3587aa992118557b16667069e37e2 has been approved by |
⌛ Testing commit c473b92adcb3587aa992118557b16667069e37e2 with merge 65613dca98c352edaa15758d6010ea24c4f7e25c... |
💔 Test failed - status-appveyor |
These UI tests failed (seeing unexpected errors)
|
With this PR the test suite no longer ignores some errors, which is good,
but existing tests need to be updated with more `//~ ERROR` annotations.
How can I reproduce these tests locally? `./x.py test [--bless]` seems to be not enough.
|
Travis passed, so this seems to be a Windows-specific error. |
☔ The latest upstream changes (presumably #52394) made this pull request unmergeable. Please resolve the merge conflicts. |
I attempted a fix for the failing tests on windows, but I'm not super confident because I don't have a windows machine on which to run the tests. I'm waiting for my own execution of the tests on AppVeyor. |
No way, my AppVeyor timed out after 1h... |
@bors: r=petrochenkov Ok let's see how it fares! |
📌 Commit 8ec9d72 has been approved by |
Match errors using the callsite of macro expansions Fix for issue #51848
☀️ Test successful - status-appveyor, status-travis |
Fix for issue #51848