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

Don't error if we don't have line information #634

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Don't error if we don't have line information #634

merged 2 commits into from
Jul 18, 2024

Conversation

Keno
Copy link
Member

@Keno Keno commented Jun 20, 2024

whereis is already allowed to return nothing. It's possible for codeloc to be != 0, but for Core.Compiler to still be unable to produce a location. In that case, also return nothing.

`whereis` is already allowed to return `nothing`. It's possible for codeloc to be != 0,
but for Core.Compiler to still be unable to produce a location. In that case, also
return `nothing`.
@Keno Keno requested review from timholy and aviatesk June 20, 2024 05:49
Keno added a commit to Keno/Revise.jl that referenced this pull request Jun 20, 2024
Companion PR to JuliaDebug/LoweredCodeUtils.jl#107. With both of
these and JuliaDebug/JuliaInterpreter.jl#634, basic Revise functionality
is working for me again on Julia master.
@@ -277,12 +277,14 @@ function linetable(arg)
return ci.linetable::Union{Vector{Core.LineInfoNode},Vector{Any}} # issue #264
end # @static if
end
function linetable(arg, i::Integer; macro_caller::Bool=false)::Union{Expr,LineTypes}
function linetable(arg, i::Integer; macro_caller::Bool=false)::Union{Expr,Nothing,LineTypes}
Copy link
Member

Choose a reason for hiding this comment

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

linetable(arg, ::Integer) is used in the other places too, like linenumber(::FrameCode, pc), so better to handle the nothing case there too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the other option would be to synthesize a special LineNumber node that indicates the absence of information. That way clients that don't care don't have to worry about it

Copy link
Member

Choose a reason for hiding this comment

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

I like the nothing option since anything that isn't prepared will hard-error.

@timholy
Copy link
Member

timholy commented Jul 18, 2024

I've finished off the nothing case (I think), please review.

Also, it sure would be nice to have a test! I know they can be hard to capture, but there's a real sense in which JuliaInterpreter/Revise/etc work only because of their test suites. If we don't keep capturing that we become vulnerable to regressions.

We also shouldn't be too optimistic that Revise will "work" once this is merged and a new LoweredCodeUtils is released; JuliaInterpreter tests still fail, and thus it's very likely that Revise will fail on certain files as a consequence. The difference in stepping on Julia 1.12 (#636) could absolutely break Revise.

@Keno Keno merged commit e2bbe47 into master Jul 18, 2024
9 of 10 checks passed
@timholy
Copy link
Member

timholy commented Jul 18, 2024

Thanks for the fix!

@timholy timholy deleted the kf/noloc branch July 18, 2024 14:02
timholy pushed a commit to timholy/Revise.jl that referenced this pull request Jul 21, 2024
Companion PR to JuliaDebug/LoweredCodeUtils.jl#107. With both of
these and JuliaDebug/JuliaInterpreter.jl#634, basic Revise functionality
is working for me again on Julia master.
timholy added a commit to timholy/Revise.jl that referenced this pull request Jul 24, 2024
Companion PR to JuliaDebug/LoweredCodeUtils.jl#107. With both of
these and JuliaDebug/JuliaInterpreter.jl#634, basic Revise functionality
is working for me again on Julia master. However, tests are still failing, so there will be more
work needed for full functionality.

Co-authored-by: Tim Holy <tim.holy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants