inference: propagate variable changes to all exception frames #42081
Conversation
vtjnash
added
bugfix
| cur_hand #::Union{Nothing, Pair{LineNum, prev_handler}} | ||
| handler_at::Vector{Any} | ||
| n_handlers::Int | ||
| handler_at::Vector{LineNum} |
There was a problem hiding this comment.
I think "handler_at" naming is actually a bit confusing ? To me "entered_at" sounds more reasonable ("handler" usually mean catch clause ... ?).
There was a problem hiding this comment.
I think it is the same to say it is handled by the try statement. The catch / leave is where we stop handling errors (which is why it is simpler to point to the try statement also, since we have more information there)
| if stupdate1!(states[l]::VarTable, changes::StateUpdate) !== false | ||
| if l < frame.pc´´ | ||
| frame.pc´´ = l | ||
| end | ||
| push!(W, l) | ||
| end |
There was a problem hiding this comment.
Well, why do we want to propagate changes via :enter ? Can't we propagate changes directly to :leave statements here ?
There was a problem hiding this comment.
The l value here is short for :leave (strictly speaking, it doesn't have to be a :leave, but it normally will be)
There was a problem hiding this comment.
frame.src.code[l] is supposed to be :enter expression always, no ?
My understanding is that the algorithm accumulates all changes within try/catch clauses to :enter expression's states, and then :enter will propagate the changes to :leave.
So I wonder why we don't want to do:
if isa(changes, StateUpdate)
while cur_hand != 0
let l = frame.handler_at[cur_hand + 1]
# propagate new type info to exception handler
enter = frame.src.code[l]
@assert isexpr(enter, :enter)
leavestate = states[enter.args[1]::Int]::VarTable
stupdate1!(leavestate, changes::StateUpdate) !== false
end
cur_hand = frame.handler_at[cur_hand]
end
endThere was a problem hiding this comment.
Because l is almost always a :leave expr there
There was a problem hiding this comment.
oh, or was supposed to be, until I rearranged the code, and then it wasn't anymore
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
* inference: propagate variable changes to all exception frames Fix #42022 * Update test/compiler/inference.jl * Update test/compiler/inference.jl Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> * fixup! inference: propagate variable changes to all exception frames Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> (cherry picked from commit e83b317)
|
This has quite a few merge conflicts with 1.6 so would need a rebase somewhere that can be cherry picked |
|
Directly cherry-picking this PR on the backport branch is an option ? I think I can take it. |
Yes. |
|
@aviatesk when merging, please check that the commit message is also correct. Fixup should never appear, for example. |
|
So the commit message of this merge commit should be like: ? |
|
Yes, though I am not sure you want to claim co-authorship, since you are already claiming the reviewer, and it would imply you are claiming ownership of the content also. |
cherry-picked from #42081 Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
cherry-picked from #42081 Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
DilumAluthge
removed
the
merge me
* inference: propagate variable changes to all exception frames Fix #42022 * Update test/compiler/inference.jl * Update test/compiler/inference.jl Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> * fixup! inference: propagate variable changes to all exception frames Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> (cherry picked from commit e83b317)
…ang#42081) * inference: propagate variable changes to all exception frames Fix JuliaLang#42022 * Update test/compiler/inference.jl * Update test/compiler/inference.jl Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> * fixup! inference: propagate variable changes to all exception frames Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
…ang#42081) * inference: propagate variable changes to all exception frames Fix JuliaLang#42022 * Update test/compiler/inference.jl * Update test/compiler/inference.jl Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> * fixup! inference: propagate variable changes to all exception frames Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Switch to precomputing the handler_at for each statement, instead of copying around a linked list for more efficiency, and chain backwards on that linked list to update all places the exception might get thrown to.
Fix #42022