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

Type piracy in package #61

Open
KristofferC opened this issue May 23, 2024 · 8 comments
Open

Type piracy in package #61

KristofferC opened this issue May 23, 2024 · 8 comments

Comments

@KristofferC
Copy link
Member

All of this stuff:

StyledStrings.jl/src/io.jl

Lines 252 to 297 in 6901610

write(io::IO, s::Union{<:AnnotatedString, SubString{<:AnnotatedString}}) =
_ansi_writer(io, s, write)::Int
print(io::IO, s::Union{<:AnnotatedString, SubString{<:AnnotatedString}}) =
(write(io, s); nothing)
escape_string(io::IO, s::Union{<:AnnotatedString, SubString{<:AnnotatedString}},
esc = ""; keep = ()) =
(_ansi_writer(io, s, (io, s) -> escape_string(io, s, esc; keep)); nothing)
function write(io::IO, c::AnnotatedChar)
if get(io, :color, false) == true
termstyle(io, getface(c), getface())
bytes = write(io, c.char)
termstyle(io, getface(), getface(c))
bytes
else
write(io, c.char)
end
end
print(io::IO, c::AnnotatedChar) = (write(io, c); nothing)
function show(io::IO, c::AnnotatedChar)
if get(io, :color, false) == true
out = IOBuffer()
show(out, c.char)
print(io, ''', AnnotatedString(String(take!(out)[2:end-1]), map(a -> (1:ncodeunits(c), a), c.annotations)), ''')
else
show(io, c.char)
end
end
function write(io::IO, aio::Base.AnnotatedIOBuffer)
if get(io, :color, false) == true
# This does introduce an overhead that technically
# could be avoided, but I'm not sure that it's currently
# worth the effort to implement an efficient version of
# writing from a AnnotatedIOBuffer with style.
# In the meantime, by converting to an `AnnotatedString` we can just
# reuse all the work done to make that work.
write(io, read(aio, AnnotatedString))
else
write(io, aio.io)
end
end

looks to be a pretty bad type piracy and should probably be moved to Base.

@kimikage
Copy link

kimikage commented May 27, 2024

Somewhat related to #57, regarding the justification for this library to do the piracy.

@tecosaur
Copy link
Collaborator

Loosely at best, since here we're talking about moving some functionality from this library to Base, and #57 is more about what/how functionality should be backported to the compat version.

@kimikage
Copy link

That's right. However, isn't the "result" of that the same?
That is, when they are transferred to Base, julia1-compat pirates them anew.

@tecosaur
Copy link
Collaborator

I can't say I follow. In Julia >1.11 the julia1-compat version shouldn't be used.

@kimikage
Copy link

kimikage commented May 27, 2024

I am just referring to julia1-compat, i.e., in Julia <1.11.

The above piracy could be eliminated from StyledString as stdlib. As a result, the piracy left in julia1-compat will go from "mere backporting" to "pure piracy".

In other words, I am talking about the justification for piracy here, not how the code behaves.

@ronisbr
Copy link
Member

ronisbr commented Oct 11, 2024

Hi!

Just for the records, it seems that this issue is causing a huge precompilation overhead in PrettyTables.jl when using Julia 1.11. For more information, see JuliaLang/julia#56080.

@tecosaur
Copy link
Collaborator

Good to have, and FWIW you have some sympathy for the effort in tracking this down.

I don't think the core issue has actually been stated here, so it would probably be worth me doing so for the record.

During the series of triage discussions that occurred around StyledStrings, it was decided that an initial proposal to add the functionality to Base would be split into a Base part and a new stdlib part (StyledStrings). This was done to prioritise keeping Base slim, but had the side-effect of creating the invalidations giving us headaches now.

There's ongoing discussion about how this should best be resolved, ranging from just evicting everything currently in Base (and the convenience that comes with it), to defining a "stub" print method in Base to control invalidations that's (somehow) substituted for the "fully-fledged" print defined in this library, or even brining all the code required for printing an AnnotatedString with color into Base.

Cody also had an interesting idea I half-remember about making a more central, pluggable, print method to solve this issue and make it possible to customise printing potentially.

This is an issue that's cared about, without an obvious fix, but with a few ideas that need exploring.

@topolarity
Copy link
Member

Cody also had an interesting idea I half-remember about making a more central, pluggable, print method to solve this issue and make it possible to customise printing potentially.

Yeah the idea is to split the behavior into the AnnotatedString vs. Face portions, so that the AnnotatedString portion (which iterates annotations) can live in Base and the Face portion (which updates the actual printer state for display, resolves colors, etc.) can be separate

The additional wrinkle right now is that an AnnotatedString is just a set of Symbols mapped to regions, so we don't know which copy of StyledStrings in memory to ask to generate/display Faces for them (see #91). We might need to associate an AnnotatedString with some kind of library-specific type that allows us to dispatch the resolution / display of its annotations to the correct library.

You wouldn't be allowed to "mix" annotations from different libraries in the same string - but that also dramatically simplifies the display logic since you only need to worry about "your own" display state on the library side. The key thing to get rid of is the hard-coded behavior where we assume there is one special library in memory that knows how to display annotations in an AnnotatedString (and we invalidate on it)

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

No branches or pull requests

5 participants