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

Loading external StyledString package removes markdown highlighting #91

Open
KristofferC opened this issue Sep 19, 2024 · 4 comments
Open
Labels
bug Something isn't working

Comments

@KristofferC
Copy link
Member

image

This is when having this package as the active project and loading it.

@topolarity
Copy link
Member

Do we have a root cause for this yet?

@tecosaur
Copy link
Collaborator

I don't, but I have noticed that with StyledStrings as the active project you can see this in a Julia REPL session:

help?> thing
# Observe working syntax highlighting

julia> using StyledStrings

help?> thing
# Syntax highlighting breaks

julia> using Markdown

help?> thing
# Syntax highlighting works again

Checking Base.loaded_modules_order it seems like two versions of StyledStrings, JuliaSyntaxHighlighting, and Markdown are loaded by doing this:

julia> Base.loaded_modules_order
22-element Vector{Module}:
 Core
 Base
 Main
 FileWatching
 Libdl
 Artifacts
 SHA
 Sockets
 LinearAlgebra
 OpenBLAS_jll
 libblastrampoline_jll
 Random
 Base64
 StyledStrings
 JuliaSyntaxHighlighting
 Markdown
 InteractiveUtils
 Unicode
 REPL
 StyledStrings
 JuliaSyntaxHighlighting
 Markdown
 
julia> length(unique(Base.loaded_modules_order))
22

@topolarity
Copy link
Member

topolarity commented Sep 27, 2024

If I had to guess, StyledStrings has some code that is embedding Face types:

face = Expr(:call, Face, kwargs...)

but then getface here is effectively doing a isa Face check:

face = getface(styles)

which runs into an issue almost exactly like the one I accidentally created with TOML recently (JuliaLang/Pkg.jl#4017 (comment))

Basically require_stdlib means that multiple StyledStrings can exist, which means that the _ansi_writer code has to support the Face type for all of the loaded StyledStrings.

@topolarity
Copy link
Member

I think I found the issue - The second copy of StyledStrings contains a default "FACES" dictionary (w/o the Markdown faces) and then overrides Base.write(::IO, ::Base.AnnotatedString) so that any old AnnotatedStrings floating about start seeing the "FACES" only from the second loaded copy of StyledStrings. In particular, the AnnotatedString's created by Markdown are suddenly missing their Face definitions.

Mis-behavior like this is really easy to get with require_stdlib + type-piracy, so I've started to document the rules of the game in JuliaLang/julia#56005

The part that matters is:

A stdlib must not access any global state that may differ between stdlib copies in type-pirated methods

Given that guideline, I think we only have a few options here:

  • Eliminate the type piracy: Make AnnotatedString a different type for each of the StyledStrings copies (move it out of Base)
  • Avoid FACES duplication: StyledStrings needs to move into the sysimage, or require_stdlib needs to be banned
  • Include a unique, type-based identifier for Annotations so that StyledStrings can resolve FACES in a non-type-pirated method
  • Maintain a reference to the relevant FACES dict in the AnnotatedString

The basic problem is that a type-pirated method must be simultaneously correct for all copies of a stdlib.

@tecosaur tecosaur added the bug Something isn't working label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants