Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upExpend syntax::with_globals coverage and allow nested calls #53526
Conversation
rust-highfive
assigned
petrochenkov
Aug 20, 2018
This comment has been minimized.
This comment has been minimized.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
label
Aug 20, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
r? @Zoxc |
rust-highfive
assigned
Zoxc
and unassigned
petrochenkov
Aug 20, 2018
earthengine
changed the title
Move syntax::with_globals call from run_compiler to run
Move code from run to run_compiler for syntax::with_globals coverage
Aug 20, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
earthengine
changed the title
Move code from run to run_compiler for syntax::with_globals coverage
Refactoring run_compiler for different syntax::with_globals coverage
Aug 21, 2018
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
I am sorry but I am not in a computer that have the tools to compile right now; So I have to use Github to edit the code and let the bots to check the code. Maybe abusing the resource, but the other choice is to wait until night, and I just can't wait... |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
So I have to stop here. The test "super-fast-paren-parsing.rs" was failed and I have no idea. As the comment was talked about "big stack" maybe an issue about a slightly bigger call stack. Need to check in a real computer. |
earthengine
referenced this pull request
Aug 21, 2018
Closed
ICE in 2018 edition: 'cannot access a scoped thread local variable without calling `set` first' #53469
This comment has been minimized.
This comment has been minimized.
|
How are you getting on with this, @earthengine? Let me know if you could use some help, as I'm quite keen to get this issue solved too. Once you're done, maybe r? @oli-obk or @eddyb. |
earthengine
changed the title
Refactoring run_compiler for different syntax::with_globals coverage
Expend syntax::with_globals coverage and allow nested calls
Aug 21, 2018
This comment has been minimized.
This comment has been minimized.
|
It is now passed the tests. So it should be fine now! The problem seems to be that, when the compiler needs to spawn a new thread, as I used the unwrapped call for r? @oli-obk |
rust-highfive
assigned
oli-obk
and unassigned
Zoxc
Aug 21, 2018
This comment has been minimized.
This comment has been minimized.
|
Can you add the ICEing test from the issue as a regression test and squash the commits? Other than that it looks fine to me, but it seems very band-aidy. Not sure who we can ping on this though. |
This comment has been minimized.
This comment has been minimized.
|
This fix seems questionable to me, but I haven't looked at the issues yet. |
This comment has been minimized.
This comment has been minimized.
|
As this is the very first pull request, I don't know what a "regression test" mean and how to do it properly. I was thinking adding tests though; but any tests given will be fixed at some stage though, as the code is in a error detection/prevention branch, tests for the ICEs will be fixed and so the test will be no longer appropriate. I can squash the commits though. Will be done tonight. |
This comment has been minimized.
This comment has been minimized.
|
@earthengine I think @oli-obk just means to add your test case from the issue (a minimal reproducible version) to |
This comment has been minimized.
This comment has been minimized.
|
The point is that ICE was NOT fixed, yet (so the minimal reproducible cannot be put in |
This comment has been minimized.
This comment has been minimized.
|
@earthengine Okay. In that case wait until you (or whoever) fixes the ICE before adding the run-pass regression test. (Do you have any idea how to fix the ICE, for that matter?) |
This comment has been minimized.
This comment has been minimized.
|
The ICE is not in my scope right now. The scope of this PR is to give more precise detail about some panicking ICEs, and so it is not targeting any specific ICE. For this reason, I don't think we should wait for any ICEs to be fixed, as it is helping addressing issues, not fixes them. If we DO need a test, it will be like add a compiler option (I don't even know when I can dig deep enough to address the actual ICEs, though) |
This comment has been minimized.
This comment has been minimized.
|
I think the clean fix here is to change We might be able to test ICE output to check that we get the "correct" ICE with the ui tests. |
This comment has been minimized.
This comment has been minimized.
This is what I have tried but didn't work. The reason is that
Actually I did this in my own computer! It is just that I am not sure it is good to have that test in the code base, as they will not last long. |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
This is what I thought. But then Even the latest form of this PR didn't resolve this though; when the new thread being created, it will work on an irrelevant instance of the globals, and so the main thread will still only get the empty values. But it only happen when there is a need to spawn a new thread and so pretty limited. So it seems to be that the proper fix, is find a way to transfer a initialized value to the |
Zoxc
referenced this pull request
Aug 23, 2018
Merged
Move with_globals setup from run_compiler to run #53624
earthengine
force-pushed the
earthengine:Fix_53448
branch
from
7e683da
to
b6f254b
Aug 23, 2018
This comment has been minimized.
This comment has been minimized.
|
My commits is now squashed without modification. |
This comment has been minimized.
This comment has been minimized.
|
I'm going to close this in favour of #53624 since it doesn't modify the globals themselves. Thanks for pushing ahead on this issue! |
earthengine commentedAug 20, 2018
•
edited
#53448 (and proberly #53469) is partialy fixed in the sense the error messages, if any, will be shown as
rather than "cannot access a scoped thread local variable without calling
setfirst".Problem analysis
run_compilerfunction is wrapped by asyntax::with_globalscall to ensure it have access tosyntax::GLOBALS. However, as we have code after runningrun_compilerequires to emit errors, when it needs to show the position of the error, it willpanicassyntax::GLOBALSis unavailable.This pull request fixed this by introducing a new call to
syntax::with_globalsinrun. So code inrunwill use biggersyntax::with_globalscoverage than other calls torun_compiler.It also makes calling
syntax::with_globalswhen it is already called in the outside code uses the already initialized globals, rather than creating new instances.Although this does not actually fix #53448 nor #53469, it will make similar errors never hide the useful error message any more, and so will help diagnosis simular situations easier.
My very first pull request on the compiler ever. Please advise me if I did anything wrong.