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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

User-themable stacktraces #51816

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tecosaur
Copy link
Contributor

@tecosaur tecosaur commented Oct 22, 2023

Back in #49586, I claimed that it gave us the ability to start systematically solving annoyances like #40228 by making the styling used user-customisable.

This is one of the first PRs following through on that, by using a set of new faces (to be added in JuliaLang/StyledStrings.jl#4) and hence actually resolves #40228.

Along the way, we also make the file paths clickable links 馃檪.

To illustrate the user-customisability, I've put a garish customisation in my faces.toml

[julia.stacktrace.location]
foreground = "bright_blue"

[julia.stacktrace.filename]
foreground = "bright_green"

[julia.stacktrace.fileline]
foreground = "bright_yellow"

[julia.stacktrace.frameindex]
foreground = "bright_magenta"

[julia.stacktrace.basemodule]
foreground = "bright_red"

and here's a resulting screenshot

image

@tecosaur tecosaur added the domain:display and printing Aesthetics and correctness of printed representations of objects. label Oct 23, 2023
@tecosaur
Copy link
Contributor Author

tecosaur commented Dec 28, 2023

It would be great if somebody could review this PR. Friendly bump 馃檪

@tecosaur
Copy link
Contributor Author

As before, if someone could spare the time to take a look at this it would be appreciated.

base/path.jl Outdated Show resolved Hide resolved
Copy link
Sponsor 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.

LGTM

Related future question: is printstyled still usable, with face = :julia_stacktrace_location instead of color=, rather than needing to explicitly name the AnnotatedString type name?

base/errorshow.jl Outdated Show resolved Hide resolved
@tecosaur
Copy link
Contributor Author

Thanks for giving this a look Jameson!

@tecosaur
Copy link
Contributor Author

Related future question: is printstyled still usable, with face = :julia_stacktrace_location instead of color=, rather than needing to explicitly name the AnnotatedString type name?

Currently, we haven't touched printstyled as part of the work with the AnnotatedString type and StyledStrings (other than JuliaLang/StyledStrings.jl#12), so there is no face keyword.

I'd be inclined to not bother with printstyled("hey", face = :julia_stacktrace_location) since you can just use print(styled"{julia_stacktrace_location:hey}").

vtjnash added a commit that referenced this pull request Feb 1, 2024
This has some value by itself, but mainly serves as a prerequisite for
#51811, #51816, and #51829, along with two other PRs that have yet to be
made.
@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 4, 2024

You may or may not want to squash merge this given the first commit is just introducing uripath, but it's only used in the context of the second content.

@tecosaur
Copy link
Contributor Author

tecosaur commented Feb 4, 2024

Since this PR is approved now, I'll go ahead and merge JuliaLang/StyledStrings.jl#32.

base/path.jl Outdated Show resolved Hide resolved
base/path.jl Outdated Show resolved Hide resolved
base/path.jl Outdated Show resolved Hide resolved
base/path.jl Show resolved Hide resolved
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 8, 2024
@tecosaur
Copy link
Contributor Author

Hmmm, the tests seem to have a lot of Unhandled Task ERROR: EOFError: read end of file and Unhandled Task ERROR: IOError: read: connection reset by peer (ECONNRESET).

It does look like some stack trace tests also need to be updated. I'll do so shortly.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 12, 2024

Yeah, notably looks like a StackOverflowError in some place, preventing accurately getting a stacktrace though

@tecosaur tecosaur added the backport 1.11 Change should be backported to release-1.11 label Feb 16, 2024
@fingolfin fingolfin closed this Feb 23, 2024
@fingolfin fingolfin reopened this Feb 23, 2024
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 23, 2024

Removing merge-me because the error seems related:

MethodError(f=Base.stacktrace_path, args=(".\\error.jl", 35), world=0x000066c1)
2024-02-15 05:36:21 UTC	jl_method_error_bare at C:/workdir/src\gf.c:2251
2024-02-15 05:36:21 UTC	jl_method_error at C:/workdir/src\gf.c:2269
2024-02-15 05:36:21 UTC	jl_lookup_generic_ at C:/workdir/src\gf.c:3099
2024-02-15 05:36:21 UTC	ijl_apply_generic at C:/workdir/src\gf.c:3114
2024-02-15 05:36:21 UTC	#print_module_path_file#1036 at .\errorshow.jl:811
2024-02-15 05:36:21 UTC	print_module_path_file at .\errorshow.jl:805

@vtjnash vtjnash removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 23, 2024
@tecosaur tecosaur requested a review from a team as a code owner March 4, 2024 15:56
This allows for the faces used (e.g. the file path) to be
user-customised, which provides an escape from themes that make bright
black invisible.
@tecosaur
Copy link
Contributor Author

tecosaur commented Mar 4, 2024

Ah, seems like I had an ::Int64 where I should have had ::Int. Let's see if that makes CI happy.

I've also rebased to the current HEAD for fun, ignore the dramatic-looking push that GH shows above, it's just the two commits you've seen before with two lines tweaked.

@KristofferC KristofferC mentioned this pull request Mar 6, 2024
60 tasks
@fingolfin fingolfin closed this Mar 8, 2024
@fingolfin fingolfin reopened this Mar 8, 2024
@fingolfin
Copy link
Contributor

Lots of CI errors -- but perhaps unrelated to this PR?!

@staticfloat
Copy link
Sponsor Member

MethodError(f=Core.kwcall, args=((modulecolor=:default, digit_align_width=3), typeof(Base.print_module_path_file)(), Base.IOContext{Base.PipeEndpoint}(io=Base.PipeEndpoint(handle=0x07baf8b0, status=3, buffer=Base.GenericIOBuffer{GenericMemory{:not_atomic, UInt8, Core.AddrSpace{Core}(0x00)}}(data=Memory{UInt8}(0, 62d389d0)[], reinit=false, readable=true, writable=true, seekable=false, append=true, size=0, maxsize=2147483647, ptr=1, offset=0, mark=-1), cond=Base.GenericCondition{Base.Threads.SpinLock}(waitq=Base.IntrusiveLinkedList{Task}(head=nothing, tail=nothing), lock=Base.Threads.SpinLock(owned=0)), readerror=nothing, sendbuf=nothing, lock=Base.ReentrantLock(locked_by=nothing, reentrancy_cnt=0x00000000, havelock=0x00, cond_wait=Base.GenericCondition{Base.Threads.SpinLock}(waitq=Base.IntrusiveLinkedList{Task}(head=nothing, tail=nothing), lock=Base.Threads.SpinLock(owned=0)), _=(0, 0)), throttle=10485760), dict=Base.ImmutableDict{Symbol, Any}(parent=Base.ImmutableDict{Symbol, Any}(parent=#<null>, key=#<null>, value=#<null>), key=:limit, value=true)), nothing, "none", 1), world=0x00006727)

This does look related.

@tecosaur
Copy link
Contributor Author

tecosaur commented Mar 9, 2024

Yea, I need to get to going through these CI errors, I'll see if I can do so this weekend.

KristofferC added a commit that referenced this pull request Mar 17, 2024
Backported PRs:
- [x] #39071 <!-- Add a lazy `logrange` function and `LogRange` type -->
- [x] #51802 <!-- Allow AnnotatedStrings in log messages -->
- [x] #53369 <!-- Orthogonalize re-indexing for FastSubArrays -->
- [x] #48050 <!-- improve `--heap-size-hint` arg handling -->
- [x] #53482 <!-- add IR encoding for EnterNode -->
- [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t
-->
- [x] #53507 <!-- update staled `Core.Compiler.Effects` documentation
-->
- [x] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [x] #53523 <!-- add back an alias for `check_top_bit` -->
- [x] #53377 <!-- add _readdirx for returning more object info gathered
during dir scan -->
- [x] #53525 <!-- fix InteractiveUtils call in Base.runtests on failure
-->
- [x] #53540 <!-- use more efficient `_readdirx` for tab completion -->
- [x] #53545 <!-- use `_readdirx` for `walkdir` -->
- [x] #53551 <!-- revert "Add @create_log_macro for making custom styled
logging macros (#52196)" -->
- [x] #53554 <!-- Always return a value in 1-d circshift! of
abstractarray.jl -->
- [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [x] #53571 <!-- Update Documenter to v1.3 for inventory writing -->
- [x] #53403 <!-- Move parallel precompilation to Base -->
- [x] #53589 <!-- add back `unsafe_convert` to pointer for arrays -->
- [x] #53596 <!-- build: remove extra .a file -->
- [x] #53606 <!-- fix error path in `precompilepkgs` -->
- [x] #53004 <!-- Unexport with, at_with, and ScopedValue from Base -->
- [x] #53629 <!-- typo fix in scoped values docs -->
- [x] #53630 <!-- sroa: Fix incorrect scope counting -->
- [x] #53598 <!-- Use Base parallel precompilation to build stdlibs -->
- [x] #53649 <!-- precompilepkgs: package in boths deps and weakdeps are
in fact only weak -->
- [x] #53671 <!-- Fix bootstrap Base precompile in cross compile
configuration -->
- [x] #52125 <!-- Load Pkg if not already to reinstate missing package
add prompt -->
- [x] #53602 <!-- Handle zero on arrays of unions of number types and
missings -->
- [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created -->
- [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug -->
- [x] #53679 <!-- move precompile workload back from Base -->
- [x] #53663 <!-- add isassigned methods for reinterpretarray -->
- [x] #53662 <!-- [REPL] fix incorrectly cleared line after completions
accepted -->
- [x] #53611 <!-- Linalg: matprod_dest for Diagonal and adjvec -->
- [x] #53659 <!-- fix #52025, re-allow all implicit pointer casts in
cconvert for Array -->
- [x] #53631 <!-- LAPACK: validate input parameters to throw informative
errors -->
- [x] #53628 <!-- Make some improvements to the Scoped Values
documentation. -->
- [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value  -->
- [x] #53391 <!-- Default to the medium code model in x86 linux -->
- [x] #53699 <!-- Move `isexecutable, isreadable, iswritable` to
`filesystem.jl` -->
- [x] #41232 <!-- Fix linear indexing for ReshapedArray if the parent
has offset axes -->
- [x] #53527 <!-- Enable analyzegc checks for try catch and fix found
issues -->
- [x] #52092 
- [x] #53682 <!-- Increase build precompilation -->
- [x] #53720 
- [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by
innervars. -->

Contains multiple commits, manual intervention needed:
- [ ] #53305 <!-- Propagate inbounds in isassigned with CartesianIndex
indices -->

Non-merged PRs with backport label:
- [ ] #53736 <!-- fix literal-pow to return the right type when the base
is -1 -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53696 <!-- add invokelatest to on_done callback in bracketed
paste -->
- [ ] #53660 <!-- put Logging back in default sysimage -->
- [ ] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51928 <!-- Styled markdown, with a few tweaks -->
- [ ] #51816 <!-- User-themable stacktraces -->
- [ ] #51811 <!-- Make banner size depend on terminal size -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC mentioned this pull request Mar 20, 2024
41 tasks
@tecosaur
Copy link
Contributor Author

tecosaur commented Apr 2, 2024

Considering this is a straightforward new feature, I'm inclined to think at this point in the 1.11 release process we might as well give up on having it in 1.11 and just have it in 1.12 once CI is made happy.

@tecosaur tecosaur removed the backport 1.11 Change should be backported to release-1.11 label Apr 2, 2024
@DilumAluthge DilumAluthge removed the request for review from a team April 2, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve visibilty of line location in stacktrace e.g. Ability to override/disable muted colour in stacktraces
4 participants