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

Add err global to REPL to store most recent errors #40642

Merged
merged 28 commits into from
Nov 19, 2021

Conversation

BioTurboNick
Copy link
Contributor

Split off from #40537, allows the last exception to be reprinted in the REPL.

base/client.jl Outdated Show resolved Hide resolved
base/client.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Contributor

Is it possible that err, and in fact ans, should have a value on startup? This would prevent this:

 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> ans(x) = "function";

julia> 1+1
ERROR: invalid redefinition of constant ans

julia> 2;
ERROR: invalid redefinition of constant ans

in favour of this:

 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> 1+1
2

julia> ans(x) = "function";
ERROR: cannot define function ans; it already has a value

@StefanKarpinski
Copy link
Member

I do like err as it feels analogous to ans, even if errs may be more accurate. Perhaps we should be a bit more delicate about setting both ans and err if it already has a value that cannot be overwritten.

base/client.jl Outdated Show resolved Hide resolved
@BioTurboNick BioTurboNick changed the title Add errs global to REPL to store most recent errors Add err global to REPL to store most recent errors May 3, 2021
@antoine-levitt
Copy link
Contributor

Using the awesome AbbreviatedStackTraces, I find myself very often typing "errs" instead of "err". This creates a new error, and sends my previous stacktrace to oblivion. Not sure what if anything should be done to prevent against that, but I'm just putting it out there in case someone has an idea.

@BioTurboNick
Copy link
Contributor Author

The same thing happens with ans but that might be less of an issue than losing error info.

Could it be a vector of exceptions that is pushed into each time? Maybe just keep the last 5 to avoid unending growth in a long session?

Though wouldn't be as nice to dump 5 full traces at once accidentally either.

@BioTurboNick
Copy link
Contributor Author

BioTurboNick commented Jun 7, 2021

Maybe stack traces originating right at top level don't get stored? Or just those errors that Julia doesn't display traces for normally. (maybe that's the same thing)

@antoine-levitt
Copy link
Contributor

Could it be a vector of exceptions that is pushed into each time? Maybe just keep the last 5 to avoid unending growth in a long session?
Though wouldn't be as nice to dump 5 full traces at once accidentally either.

Could be good: err for the latest, errs for an array of the latest. You can avoid dumping them all by having custom printing for the errs object and displaying "array of last 3 errors", which you could then do errs[i] on

@BioTurboNick
Copy link
Contributor Author

@antoine-levitt - I'm testing out, both here and in AbbreviatedStackTraces.jl, the idea that err is only set when the stack goes deeper than top-level. That should solve the typo trap you mentioned and I think is limited enough to avoid losing anything you wouldn't want to lose.

@BioTurboNick
Copy link
Contributor Author

Now that 1.7 is just about out, wanted to raise this PR again.

Summarizing:

  • err global on the REPL
  • Trivial errors (where there is no stack trace, occurs at top level) aren't stored, which helps prevent issues like flubbing an attempt to consume it.

I believe the one outstanding previously-mentioned issue might be ensuring err is set when the REPL starts, so that it is reserved. (And if so, same should be done for ans presumably.) Should I include that?

Is there anything else?

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I'm very excited about this, and every couple weeks wonder why it isn't merged yet. Thanks for getting back to it.

base/client.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Oct 13, 2021

Could it be a vector of exceptions that is pushed into each time

I don't want it to be added to this PR, but I think having another variable that is all of the previous results, errors, and expressions would be excellent.

@BioTurboNick
Copy link
Contributor Author

I think that does it. I also noticed that the display_error(er, bt) methods don't seem to be used anywhere anymore. Assuming tests prove that out, and since the method isn't documented, removing them is probably okay? But waiting to see if the tests confirm that.

@BioTurboNick
Copy link
Contributor Author

BioTurboNick commented Oct 29, 2021

Hmm... Lots of random test failures but one consistent one. Somehow a precompile test is failing. Unfortunately the test failure message is a bit unclear as to what was expected vs. actual.

Error in testset precompile:
Test Failed at /buildworker/worker/package_linuxaarch64/build/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:811
  Expression: contains_warn(read(fname, String), $(Expr(:escape, :(r"LoadError: break me\nStacktrace:\n \[1\] [\e01m\[]*error"))))

EDIT: Oh, it's just that it's not scrubbed of REPL frames now, which adds a double-digit frame and indents the whole output by an additional space.

@BioTurboNick
Copy link
Contributor Author

That should do it. Restored scrubbing of REPL frames everywhere.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, but need to put back the old functions (they don't need the scrub_repl_backtrace work), since there are many packages that have been relying on them.

base/client.jl Show resolved Hide resolved
@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Nov 5, 2021
@vtjnash vtjnash self-requested a review November 5, 2021 20:54
base/client.jl Outdated Show resolved Hide resolved
stdlib/REPL/test/repl.jl Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@vtjnash vtjnash merged commit 2c2e724 into JuliaLang:master Nov 19, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@DilumAluthge DilumAluthge removed the forget me not PRs that one wants to make sure aren't forgotten label Sep 6, 2022
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.

10 participants