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

Improve parse_eval_all line number reporting #13444

Merged
merged 1 commit into from
Oct 19, 2015

Conversation

ihnorton
Copy link
Member

@ihnorton ihnorton commented Oct 4, 2015

Wraps a LoadError thrown by jl_parse_eval_all with the original top-level line/file from the parser.

Ref: jump-dev/JuMP.jl#7 where we have a macro expansion that will throw UndefVarError:

using JuMP
m = Model()
@setObjective(m, Max, z)

before:

ERROR: LoadError: UndefVarError: z not defined
 in include at ./boot.jl:260
 in include_from_node1 at ./loading.jl:304
while loading /cmn/julia/dummy.jl, in expression starting on line 584

after:

julia> include("dummy.jl")
ERROR: LoadError: LoadError: UndefVarError: z not defined
 in include at ./boot.jl:261
 in include_from_node1 at ./loading.jl:304
while loading /home/isaiah/.julia/v0.5/JuMP/src/macros.jl, in expression starting on line 584
while loading /cmn/julia/dummy.jl, in expression starting on line 3

@ihnorton
Copy link
Member Author

ihnorton commented Oct 4, 2015

(I realize that the repetitive while loading and LoadError: LoadError messages are not ideal. Maybe we should add a wrapper error type that displays like a backtrace entry?)

@ihnorton ihnorton force-pushed the ihn/better_evalall_lineno branch from 04e0c9c to 5e9d02f Compare October 18, 2015 00:46
@ihnorton ihnorton added the error handling Handling of exceptions by Julia or the user label Oct 18, 2015
@ihnorton
Copy link
Member Author

Rebased and passes tests now.

jakebolewski added a commit that referenced this pull request Oct 19, 2015
Improve parse_eval_all line number reporting
@jakebolewski jakebolewski merged commit c615d71 into JuliaLang:master Oct 19, 2015
@jakebolewski
Copy link
Member

This isn't quite correct, it doesn't handle recursive include's correctly so you end up with a bunch of -1 line numbers being reported.

One improvement is to move, line 578 of this diff needs to be move right after jl_parse_next(), that will get the last load error to report the correct line number.

Ex:

LoadError(at "sysimg.jl" line -1: LoadError(at "sysimg.jl" line 241: LoadError(at "linalg.jl" line -1: LoadError(at "linalg.jl" line 225: LoadError(at "linalg/cholesky.jl" line -1: LoadError(at "linalg/cholesky.jl" line 26: UndefVarError(var=:Typer)))))))

I also think the load error's need to be improved. Tracing through where the file is included from is pretty useful, but the output needs to be printed in a better way.

@jakebolewski
Copy link
Member

Sorry for the noise on master, but this is more obvious during bootstrap.

@ihnorton
Copy link
Member Author

it doesn't handle recursive include's correctly so you end up with a bunch of -1 line numbers being reported.

I didn't see your issue or this because it works fine in a debug build (note that top_lineno is a local variable).
I added the line a = Typer= at linalg/cholesky.jl:25, and get the following with make debug:

linalg.jl
error during bootstrap:
LoadError(at "sysimg.jl" line 5: LoadError(at "sysimg.jl" line 241: LoadError(at "linalg.jl" line 3: LoadError(at "linalg.jl" line 225: LoadError(at "linalg/cholesky.jl" line 25: UndefVarError(var=:Typer))))))

whereas in a release build:

linalg.jl
error during bootstrap:
LoadError(at "sysimg.jl" line -1: LoadError(at "sysimg.jl" line 241: LoadError(at "linalg.jl" line -1: LoadError(at "linalg.jl" line 225: LoadError(at "linalg/cholesky.jl" line -1: LoadError(at "linalg/cholesky.jl" line 25: UndefVarError(var=:Typer)))))))

Maybe top_lineno is not properly restored by the catch handler under some conditions?
(when I step through the release build in gdb, it is set correctly when I return from jl_parse_next, but then reverts to -1 when the expression is evaluated / error thrown)

@ihnorton
Copy link
Member Author

I also think the load error's need to be improved. Tracing through where the file is included from is pretty useful, but the output needs to be printed in a better way.

Isn't this mostly an issue during bootstrap? The output is somewhat better in the REPL (see example in the OP).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling Handling of exceptions by Julia or the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants