Improved terminfo parser

[tecosaur]

While the initial terminfo PR turned out fairly well (in my biased opinion anyway 😛), it could do with a few improvements. This is a draft PR to collect the work done so far and discuss what else should be done.

So far this fixes #51190 but not (yet) #51110. Across the 2873 terminfo files on my system, I currently see seven errors raised, all in the style of #51110 (for various screen.X terms).

The TermInfo struct has also been changed from

struct TermInfo
    names::Vector{String}
    flags::Int
    numbers::BitVector
    strings::BitVector
    extensions::Vector{Symbol}
    capabilities::Dict{Symbol, Union{Bool, Int, String}}
end

to

struct TermInfo
    names::Vector{String}
    flags::Dict{Symbol, Bool}
    numbers::Dict{Symbol, Int}
    strings::Dict{Symbol, String}
    extensions::Union{Nothing, Set{Symbol}}
end

So it's now not possible for get(::TermInfo, ::Symbol, default) to get a surprising type, with the following dedicated flag/number/string get methods.

This does slightly complicate haskey, keys, and getindex but I feel might be worth it overall? I'm interested in second opinions.

It would probably be nice to also add some more targeted test cases. We could probably use tic -x -o . for this?

[tecosaur]

Oh, for this too, please don't squash-merge this. I put effort into commit messages, and it makes me sad when github stomps on it and mangles some more of the commit data while it's at it.

[tecosaur]

Oh also, it would be good if anyone knew of any other terminfo parsers that we could easily test against, I've found it hard to locate anything easy to use.

[vtjnash]

Converted this to a draft, since you described it as such. Comment when this is ready or convert it back to non-draft state.

[tecosaur]

Took the need to rebase as an opportunity to remove more broadcasting, after it was raised as an annoyance in #51399 (comment).

[tecosaur]

I haven't been able to figure out the linked bug, but it seems like this refactor fixes some problems people ran into on Slack, so maybe we should merge this for now as an improvement?

[christiangnrd]

If #51110 is indeed fixed by #51399, this pull request can definitely move forward.

[tecosaur]

I haven't been able to figure out the linked bug

I don't think #51399 has any bearing on the screen.xterm-256color bug, however as far as I am aware:

• This PR improves the state of the terminfo parser, and resolves other issues people have raised with me on slack
• This PR does not introduce any regressions

So I'm thinking we should merge this and keep on trying to work out exactly what is/should be going on with screen.xterm-256color. Ideally, generate a more comprehensive test set as well.

[christiangnrd]

Starting from 2defa57, I am no longer able to reproduce #51110, but it reproduced in a127ab7, hence why I assumed it was fixed by #51399.

[tecosaur]

If someone would be willing to review+merge this, or say why they think it shouldn't be yet, that would be much appreciated 🙏.

[PallHaraldsson]

I don't know a lot about terminfo, is this mostly useful for the REPL (and non-Windows)? Can all of the terminfo functionality be excluded from the sysimage, until you do using REPL (it's now a stdlib)?

[tecosaur]

I don't know a lot about terminfo, is this mostly useful for the REPL (and non-Windows)?

Pretty much. We want this for TTY output.

Can all of the terminfo functionality be excluded from the sysimage, until you do using REPL (it's now a stdlib)?

We'd actually need client.jl to become a stdlib (see #51350) and then this to become a stdlib (see #51618), but then I don't see why not.

I think this would be best discussed in a separate issue/PR though.

For now, this PR is just improving the current implementation. There are more (minor) improvements that I'd like to see made, but I don't want to let perfect be the enemy of the good (or here: better) which is why I'd like to see this "just merged".

[christiangnrd]

Bump

[tecosaur]

This PR is still waiting for attention 🙂