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.

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

store location of loop boundary #41857

Merged
merged 4 commits into from
Aug 25, 2021
Merged

store location of loop boundary #41857

merged 4 commits into from
Aug 25, 2021

Conversation

simeonschaub
Copy link
Member

Since lowering always inserts gotos and other instructions after loop
bodies, this allows debuggers to give a more useful location for these
instructions.

fixes JuliaDebug/JuliaInterpreter.jl#485

Since lowering always inserts gotos and other instructions after loop
bodies, this allows debuggers to give a more useful location for these
instructions.

fixes JuliaDebug/JuliaInterpreter.jl#485
@simeonschaub simeonschaub added the parser Language parsing and surface syntax label Aug 10, 2021
@simeonschaub simeonschaub reopened this Aug 10, 2021
@simeonschaub
Copy link
Member Author

The failure in Distributed shows this could potentially be an issue for macros, so we should do a PkgEval run.

@simeonschaub simeonschaub added the needs pkgeval Tests for all registered packages should be run with this change label Aug 11, 2021
@simeonschaub
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@simeonschaub
Copy link
Member Author

simeonschaub commented Aug 11, 2021

Ok, a lot of the failures do seem to be real. ChainRulesTestUtils and Cthulhu are probably ok, since they are testing Base internals, but Hyperopt and InteractBase seem to have similar issues as Distributed.@distributed.

Are we ok with just fixing those packages, or is this change a bit too risky? Alternatively, we could also make the LineNumberNode part of the for expression itself, but that might be disruptive in a similar way.

@simeonschaub simeonschaub added triage This should be discussed on a triage call and removed needs pkgeval Tests for all registered packages should be run with this change labels Aug 11, 2021
@JeffBezanson
Copy link
Member

Can you elaborate on why this broke the macro?

@simeonschaub
Copy link
Member Author

The macros extract the loop bodies and actually use the return value of that block expression, which is now nothing because the last entry is a LineNumberNode.

@JeffBezanson
Copy link
Member

vtjnash says we should probably fix those packages first, then it might be ok to do this.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Aug 12, 2021
simeonschaub added a commit to simeonschaub/Hyperopt.jl that referenced this pull request Aug 12, 2021
simeonschaub added a commit to simeonschaub/Widgets.jl that referenced this pull request Aug 12, 2021
piever pushed a commit to piever/Widgets.jl that referenced this pull request Aug 16, 2021
* fix `@manipulate` for upcoming base changes

ref JuliaLang/julia#41857

* fix
@simeonschaub
Copy link
Member Author

OK, all affected packages should now have been fixed. Could I get a review?

@vchuravy vchuravy requested a review from vtjnash August 25, 2021 15:39
@vtjnash vtjnash merged commit a11de31 into master Aug 25, 2021
@vtjnash vtjnash deleted the sds/loop_end_loc branch August 25, 2021 16:19
@vtjnash
Copy link
Member

vtjnash commented Aug 25, 2021

I don't think this should be backported

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
Since lowering always inserts gotos and other instructions after loop
bodies, this allows debuggers to give a more useful location for these
instructions.

fixes JuliaDebug/JuliaInterpreter.jl#485
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Since lowering always inserts gotos and other instructions after loop
bodies, this allows debuggers to give a more useful location for these
instructions.

fixes JuliaDebug/JuliaInterpreter.jl#485
Keno pushed a commit that referenced this pull request Jun 5, 2024
Since lowering always inserts gotos and other instructions after loop
bodies, this allows debuggers to give a more useful location for these
instructions.

fixes JuliaDebug/JuliaInterpreter.jl#485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code locations of iteration with break are incorrect.
4 participants